Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get()

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 01/06/2016 11:51 PM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, 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 | 106 +++++++++++++++++++++++++------------------
>>   hw/xen/xen-host-pci-device.h |   5 +-
>>   hw/xen/xen_pt.c              |  12 +++--
>>   3 files changed, 71 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 7d8a023..952cae0 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
>>
>>   /* This size should be enough to read the first 7 lines of a resource file */
>>   #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
>> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
>>   {
>>       int i, rc, fd;
>>       char path[PATH_MAX];
>> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>>
>>       rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>
> Are you sure that errno is relevant?  And "snprintf err" doesn't seem to
> be the correct message, as there is no snprintf in the line above.
>

snprintf() is called in xen_host_pci_sysfs_path() above, and is the only 
possible error source, so I guess the errno is relevant?

Or, replace the error_setg_errno() to assert(0)? because if snprintf 
goes wrong, user seems can do nothing.

>> +        return;
>>       }
>> +
>>       fd = open(path, O_RDONLY);
>>       if (fd == -1) {
>> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> -        return -errno;
>> +        error_setg_errno(errp, errno, "open %s err", path);
>
> Please use error_setg_file_open() for reporting open() failures.
>
>> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>>               d->rom.bus_flags = flags & IORESOURCE_BITS;
>>           }
>>       }
>> +
>>       if (i != PCI_NUM_REGIONS) {
>>           /* Invalid format or input to short */
>> -        rc = -ENODEV;
>> +        error_setg(errp, "Invalid format or input to short: %s", buf);
>
> s/to/too/ (and might as well fix the same typo in the comment while at it)
>
>
>> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
>>
>>       rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>> +        return;
>>       }
>> +
>>       fd = open(path, O_RDONLY);
>>       if (fd == -1) {
>> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, strerror(errno));
>> -        return -errno;
>> +        error_setg_errno(errp, errno, "open %s err", path);
>
> Same comments as above.
>
>> +        return;
>>       }
>> +
>>       do {
>>           rc = read(fd, &buf, sizeof (buf) - 1);
>>           if (rc < 0 && errno != EINTR) {
>> -            rc = -errno;
>> +            error_setg_errno(errp, errno, "read err");
>>               goto out;
>>           }
>>       } while (rc < 0);
>> +
>>       buf[rc] = 0;
>>       value = strtol(buf, &endptr, base);
>>       if (endptr == buf || *endptr != '\n') {
>> -        rc = -1;
>> +        error_setg(errp, "format invalid: %s", buf);
>>       } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
>> -        rc = -errno;
>> +        error_setg_errno(errp, errno, "strtol err");
>
> This is pre-existing invalid use of strtol (the value of errno is not
> guaranteed to be ERANGE on overflow; and the only correct way to safely
> check errno after strtol() is to first prime it to 0 prior to calling
> strtol).  Better would be to use qemu_strtol() (preferably as a separate
> patch), so that you don't even have to worry about using strtol()
> incorrectly.
>

Ok, will split it into a separate patch

>> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
>>   {
>>       char path[PATH_MAX];
>>       int rc;
>>
>>       rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
>
> May want to delete the space before ( while touching the code in this
> vicinity.
>
>>       if (rc) {
>> -        return rc;
>> +        error_setg_errno(errp, errno, "snprintf err");
>
> Another suspect message.
>
>
>> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>> +                            uint8_t bus, uint8_t dev, uint8_t func,
>> +                            Error **errp)
>>   {
>>       unsigned int v;
>> -    int rc = 0;
>> +    Error *local_err = NULL;
>
> These days, naming the local variable 'err' is more common than 'local_err'.
>

agree. I guess maybe "local_err" is a better mnemonic for newbie. and 
when I am gradually familiar with error report, I also prefer "err". 
Actually I considered to change this name, I just don`t want to bother 
the reviewer to review it again, especially when the patch got a 
Review-by and new version just changes some names. Will fix it.

>> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
>>                  s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>>                  s->dev.devfn);
>>
>> -    rc = xen_host_pci_device_get(&s->real_device,
>> -                                 s->hostaddr.domain, s->hostaddr.bus,
>> -                                 s->hostaddr.slot, s->hostaddr.function);
>> -    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,
>> +                            &local_err);
>> +    if (local_err) {
>> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>
> Leaks local_err.
>

Yes, but this will be fixed with error_propagate when patch "4/4 convert 
to realize" comes, so is it ok to let it be here?

I originally want to make these patch independent from each other with 
the most necessary modification.

-- 
Yours Sincerely,

Cao jin