From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42739) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJukr-0006Bg-Te for qemu-devel@nongnu.org; Thu, 14 Jan 2016 22:08:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJuko-0005e3-LC for qemu-devel@nongnu.org; Thu, 14 Jan 2016 22:08:49 -0500 Received: from [59.151.112.132] (port=8002 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJuko-0005dn-96 for qemu-devel@nongnu.org; Thu, 14 Jan 2016 22:08:46 -0500 References: <1452689507-8188-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452689507-8188-3-git-send-email-caoj.fnst@cn.fujitsu.com> <5698214D.1080503@redhat.com> From: Cao jin Message-ID: <5698636C.9020102@cn.fujitsu.com> Date: Fri, 15 Jan 2016 11:11:40 +0800 MIME-Version: 1.0 In-Reply-To: <5698214D.1080503@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 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: stefano.stabellini@eu.citrix.com On 01/15/2016 06:29 AM, Eric Blake wrote: > On 01/13/2016 05:51 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 | 142 +++++++++++++++++++++---------------------- >> hw/xen/xen-host-pci-device.h | 5 +- >> hw/xen/xen_pt.c | 13 ++-- >> 3 files changed, 80 insertions(+), 80 deletions(-) >> >> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c >> index 351b61a..3e22de8 100644 >> --- a/hw/xen/xen-host-pci-device.c >> +++ b/hw/xen/xen-host-pci-device.c >> @@ -31,25 +31,20 @@ >> #define IORESOURCE_PREFETCH 0x00001000 /* No side effects */ >> #define IORESOURCE_MEM_64 0x00100000 >> >> -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d, >> - const char *name, char *buf, ssize_t size) >> +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d, >> + const char *name, char *buf, ssize_t size) > > Changing xen_host_pci_sysfs_path() to return void, by assert()ing on > caller error, is not mentioned in the commit message; and if I were > doing the series, I probably would have done it as a separate commit. > Thanks for the suggestion, will split it out. >> /* This size should be enough to read a long from a file */ >> #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22 >> -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, >> - unsigned int *pvalue, int base) >> +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, >> + unsigned int *pvalue, int base, Error **errp) >> { > >> buf[rc] = 0; >> - rc = qemu_strtoul(buf, &endptr, base, &value); >> - if (!rc) { >> - *pvalue = value; >> + rc = qemu_strtoul(buf, &endptr, base, (unsigned long *)pvalue); > > Ouch. Casting unsigned int * to unsigned long * and then dereferencing > it is bogus (you end up having qemu_strtoul() write beyond bounds on > platforms where long is larger than int). Yes, I considered this issue a little. Because the current condition is: the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So I guess even if on x86_64(length of int != long), it won`t break things. So, compared with following, which style do you prefer? > You'll need to revert this > part of the patch, and stick with *pvalue = value (and possibly even add > a bounds check that value <= UINT_MAX). > > Otherwise looks okay. > -- Yours Sincerely, Cao jin