Re: [Qemu-devel] [PATCH 2/4] hw/pci-host/piix: QOM'ify the IGD Passthrough host bridge

[Philippe Mathieu-Daudé]

[...]
>>>   -static int host_pci_config_read(int pos, int len, uint32_t *val)
>>> +static void host_pci_config_read(int pos, int len, uint32_t *val,
>>> Error **errp)
>>>   {
>>>       char path[PATH_MAX];
>>>       int config_fd;
>>> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>>       /* Access real host bridge. */
>>>       int rc = snprintf(path, size,
>>> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>>>                         0, 0, 0, 0, "config");
>>> -    int ret = 0;
>>>         if (rc >= size || rc < 0) {
>>> -        return -ENODEV;
>>> +        error_setg(&error_abort, "Invalid PCI device");
>>
>> I think a return statement is missing here.
>
> Well, error_setg(&error_abort) will never return, however checking the
> documentation again I noticed I forgot these recommendations:
>
>  * Please don't error_setg(&error_fatal, ...), use error_report() and
>  * exit(), because that's more obvious.
>  * Likewise, don't error_setg(&error_abort, ...), use assert().
>
> abort() might be OK but a bit harsh, so I'll use error_report() + exit()
> here.

Actually g_strdup_printf() is cleaner.

>>>       }
>>>         config_fd = open(path, O_RDWR);
>>>       if (config_fd < 0) {
>>> -        return -ENODEV;
>>> +        error_setg_errno(errp, errno, "Failed to open: %s", path);
>>> +        return;
>>>       }
>>>         if (lseek(config_fd, pos, SEEK_SET) != pos) {
>>> -        ret = -errno;
>>> +        error_setg_errno(errp, errno, "Failed to seek: %s", path);
>>>           goto out;
>>>       }
>>>   @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>>           rc = read(config_fd, (uint8_t *)val, len);
>>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>>       if (rc != len) {
>>> -        ret = -errno;
>>> +        error_setg_errno(errp, errno, "Failed to read: %s", path);
>>>       }
>>>     out:
>>>       close(config_fd);
>>> -    return ret;
>>>   }
[...]