Hi Markus,
On 12/09/2016 14:51, Markus Armbruster wrote:
> Eric Auger <eric.au...@redhat.com> writes:
>
>> Let's expand the usage of QEMU Error objects to vfio_populate_device.
>>
>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> ---
>> hw/vfio/pci.c | 45 ++++++++++++++++++++-------------------------
>> 1 file changed, 20 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ae1967c..f7768e9 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev)
>> return 0;
>> }
>>
>> -static int vfio_populate_device(VFIOPCIDevice *vdev)
>> +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>> {
>> VFIODevice *vbasedev = &vdev->vbasedev;
>> struct vfio_region_info *reg_info;
> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
> int i, ret = -1;
>> @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>>
>> /* Sanity check device */
>> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
>> - error_report("vfio: Um, this isn't a PCI device");
>> - goto error;
>> + error_setg(errp, "this isn't a PCI device");
>> + return;
>
> This is actually a bug fix :)
>
> Before your series, vfio_populate_device() returns negative errno on
> some errors, and -1 on others. Its caller expects the former.
Sorry but I don't get your comment. Who is the caller you refer to?
>
> Please mention the fix in the commit message. Fixing it in a separate
> commit would also be fine, and possibly clearer.
>
>> }
>>
>> if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
>> - error_report("vfio: unexpected number of io regions %u",
>> - vbasedev->num_regions);
>> - goto error;
>> + error_setg(errp, "unexpected number of io regions %u",
>> + vbasedev->num_regions);
>> + return;
>> }
>>
>> if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
>> - error_report("vfio: unexpected number of irqs %u",
>> vbasedev->num_irqs);
>> - goto error;
>> + error_setg(errp, "unexpected number of irqs %u",
>> vbasedev->num_irqs);
>> + return;
>> }
>>
>> for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX;
>> i++) {
>> @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> g_free(name);
>>
>> if (ret) {
>> - error_report("vfio: Error getting region %d info: %m", i);
>> - goto error;
>> + error_setg_errno(errp, ret, "failed to get region %d info", i);
>> + return;
>> }
>>
>> QLIST_INIT(&vdev->bars[i].quirks);
>> @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> ret = vfio_get_region_info(vbasedev,
>> VFIO_PCI_CONFIG_REGION_INDEX, ®_info);
>> if (ret) {
>> - error_report("vfio: Error getting config info: %m");
>> - goto error;
>> + error_setg_errno(errp, ret, "failed to get config info");
>> + return;
>> }
>>
>> trace_vfio_populate_device_config(vdev->vbasedev.name,
>> @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
>> ret = vfio_populate_vga(vdev);
>> if (ret) {
>> - error_report(
>> - "vfio: Device does not support requested feature x-vga");
>> - goto error;
>> + error_setg_errno(errp, ret,
>> + "device does not support requested feature x-vga");
>> + return;
>> }
>> }
>>
>> @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev)
>> if (ret) {
>> /* This can fail for an old kernel or legacy PCI dev */
>> trace_vfio_populate_device_get_irq_info_failure();
>> - ret = 0;
>> } else if (irq_info.count == 1) {
>> vdev->pci_aer = true;
>> } else {
>> - error_report("vfio: %s "
>> - "Could not enable error recovery for the device",
>> - vbasedev->name);
>> + error_setg_errno(errp, ret, "could not enable error recovery");
>
> This isn't an error before this patch (ret stays zero). Your patch
> turns it into one. Intentional?
Hum no thank you for spotting this bug!
Thanks
Eric
>
>> }
>> -
>> -error:
>> - return ret;
>> }
>>
>> static void vfio_put_device(VFIOPCIDevice *vdev)
>> @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>> VFIODevice *vbasedev_iter;
>> VFIOGroup *group;
>> char *tmp, group_path[PATH_MAX], *group_name;
>> + Error *err = NULL;
>> ssize_t len;
>> struct stat st;
>> int groupid;
>> @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>> goto error;
>> }
>>
>> - ret = vfio_populate_device(vdev);
>> - if (ret) {
>> - error_setg_errno(errp, ret, "failed to populate the device");
>> + vfio_populate_device(vdev, &err);
>> + if (err) {
>> + error_propagate(errp, err);
>> goto error;
>> }
>
