Re: [Qemu-devel] [PATCH 2/4] target-mips: add Unified Hosting Interface (UHI) support

[Matthew Fortune]

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