From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56043) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHr2f-0004lE-VB for qemu-devel@nongnu.org; Sat, 09 Jan 2016 05:46:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHr2c-0007aW-Pj for qemu-devel@nongnu.org; Sat, 09 Jan 2016 05:46:41 -0500 Received: from [59.151.112.132] (port=9882 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHr2b-0007Vq-Ta for qemu-devel@nongnu.org; Sat, 09 Jan 2016 05:46:38 -0500 References: <1452242274-8345-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452242274-8345-3-git-send-email-caoj.fnst@cn.fujitsu.com> <56903D34.2020204@redhat.com> From: Cao jin Message-ID: <5690E5B7.6050900@cn.fujitsu.com> Date: Sat, 9 Jan 2016 18:49:27 +0800 MIME-Version: 1.0 In-Reply-To: <56903D34.2020204@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Markus Armbruster , stefano.stabellini@eu.citrix.com On 01/09/2016 06:50 AM, Eric Blake wrote: > On 01/08/2016 01:37 AM, Cao jin wrote: >> To catch the error msg. Also modify the caller >> >> Signed-off-by: Cao jin >> --- >> hw/xen/xen-host-pci-device.c | 134 ++++++++++++++++++++++--------------------- >> hw/xen/xen-host-pci-device.h | 5 +- >> hw/xen/xen_pt.c | 13 +++-- >> 3 files changed, 81 insertions(+), 71 deletions(-) >> > >> @@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d, >> d->domain, d->bus, d->dev, d->func, name); >> >> if (rc >= size || rc < 0) { >> - /* The output is truncated, or some other error was encountered */ >> - return -ENODEV; >> + /* The output is truncated, or some other error was encountered. >> + * Assert here since user can do nothing in case of failure */ >> + assert(0); >> } > > Might be shorter to drop the 'if' block, and just write: > > assert(rc >= 0 && rc < size); > > where you then don't need a comment, because the body of the assert() is > then more specific on the caller's responsibility for passing in a > decent size argument. > Yes, seems better > >> buf[rc] = 0; >> rc = qemu_strtoul(buf, &endptr, base, &value); > > Do you still need a local 'value' variable, or can you just reuse pvalue > here? > >> if (!rc) { >> *pvalue = value; >> + } else if (rc == -EINVAL) { >> + error_setg(errp, "strtoul: Invalid argument"); >> + } else { >> + error_setg_errno(errp, errno, "strtoul err"); > > Still not quite right - you are not guaranteed that 'errno' is sane > after qemu_strtoul(), only that -rc is sane. And feels repetitive. > Better might be: > > rc = qemu_strtoul(buf, &endptr, base, pvalue); > if (rc) { > error_setg_errno(errp, -rc, "failed to parse value '%s'", buf); > } > yes, shorter and better. But you said before: the only correct way to safely check errno after strtol() is to first prime it to 0 prior to calling strtol. Seems qemu_strtoul() did it as you said(first prime it to 0), so the errno is sane? Then I guess my patch can achieve the same result as yours? because return value of qemu_strtoul() is as following: success: 0 failure: -EINVAL or -errno [...] >> - if (rc) { >> - XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", rc); >> + xen_host_pci_device_get(&s->real_device, >> + s->hostaddr.domain, s->hostaddr.bus, >> + s->hostaddr.slot, s->hostaddr.function, >> + &err); >> + if (err) { >> + error_append_hint(&err, "Failed to \"open\" the real pci device"); > > Markus may have an opinion on whether his new error_prepend code is a > better fit (error_append_hint lists _after_ the original failure, but it > sounds like you want "failed to open the real pci device: low level > details"). > > But looks like you're getting closer. > greped the code, seems error_prepend doesn`t exist in upstream? So I guess maybe we can use append hint for now? -- Yours Sincerely, Cao jin