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

[Leon Alrae <leon.alrae@imgtec.com>]

Hi Matthew,

On 01/03/2015 22:17, Matthew Fortune wrote:
> 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.

Indeed, indicating an error by returning -1 would be better. Thanks for
spotting that.

>> +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).

I don't think we should use p as a format string directly as it might
contain more/different format specifiers. Presumably we would need
additional logic for checking this and returning an error for such cases
-- and this new implementation wouldn't be much simpler than current I
believe.

Current implementation allows any string given by the guest to be
printed out. If there are multiple format specifiers then only the first
occurrence of %d will be replaced with the integer.

Leon