[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] target-mips: add Unified Hosting Interface
From: |
Matthew Fortune |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] target-mips: add Unified Hosting Interface (UHI) support |
Date: |
Sun, 1 Mar 2015 22:17:33 +0000 |
Hi Leon,
Many thanks for implementing this interface in QEMU. I haven't reviewed
in great detail as I am not familiar enough with QEMU internals to do
so. Overall it seems to match the UHI spec. The one potential issue is
translation of errno values, I suspect some do not map 1:1.
A couple of minor review comments below:
> + * Copyright (c) 2014 Imagination Technologies
Dates need updating.
> +static int write_to_file(CPUMIPSState *env, target_ulong fd,
> target_ulong vaddr,
> + target_ulong len, target_ulong offset) {
> + int num_of_bytes;
> + void *dst = lock_user(VERIFY_READ, vaddr, len, 1);
> + if (!dst) {
> + return 0;
> + }
Ideally I think this this should return -1 and fake an errno but I
may not understand what case this code is dealing with.
> +static int read_from_file(CPUMIPSState *env, target_ulong fd,
> + target_ulong vaddr, target_ulong len,
> + target_ulong offset) {
> + int num_of_bytes;
> + void *dst = lock_user(VERIFY_WRITE, vaddr, len, 0);
> + if (!dst) {
> + return 0;
> + }
Likewise.
> + case UHI_plog:
> + GET_TARGET_STRING(p, gpr[4]);
> + p2 = strstr(p, "%d");
> + if (p2) {
> + int char_num = p2 - p;
> + char *buf = g_malloc(char_num + 1);
> + strncpy(buf, p, char_num);
> + buf[char_num] = '\0';
> + gpr[2] = printf("%s%d%s", buf, (int)gpr[5], p2 + 2);
> + g_free(buf);
> + } else {
> + gpr[2] = printf("%s", p);
> + }
Is all this necessary vs just: printf(p, (int)gpr[5])? I guess you
may want to do the scan for %d and choose between that and just
printf(p).
Thanks,
Matthew
- Re: [Qemu-devel] [PATCH 2/4] target-mips: add Unified Hosting Interface (UHI) support,
Matthew Fortune <=