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
