Re: [Qemu-devel] [PATCH v5 2/5] Add Error **errp for xen_host_pci_device_get()

[Cao jin]

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 <address@hidden>
> > ---
> >   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