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

[Cao jin <caoj.fnst@cn.fujitsu.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 <caoj.fnst@cn.fujitsu.com>
>> ---
>>   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