From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57125) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSUC5-0007iY-Lu for qemu-devel@nongnu.org; Mon, 02 Mar 2015 12:31:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSUC0-0002zx-Hi for qemu-devel@nongnu.org; Mon, 02 Mar 2015 12:31:49 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:9819) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSUC0-0002zt-AV for qemu-devel@nongnu.org; Mon, 02 Mar 2015 12:31:44 -0500 Message-ID: <54F49E7B.5020809@imgtec.com> Date: Mon, 2 Mar 2015 17:31:39 +0000 From: Leon Alrae MIME-Version: 1.0 References: <1425056454-27028-1-git-send-email-leon.alrae@imgtec.com> <1425056454-27028-3-git-send-email-leon.alrae@imgtec.com> <6D39441BF12EF246A7ABCE6654B0235320FE477C@LEMAIL01.le.imgtec.org> In-Reply-To: <6D39441BF12EF246A7ABCE6654B0235320FE477C@LEMAIL01.le.imgtec.org> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/4] target-mips: add Unified Hosting Interface (UHI) support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matthew Fortune , "qemu-devel@nongnu.org" Cc: "aurelien@aurel32.net" 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