| Message ID | 20240515082041.556571-11-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | VFIO: misc cleanups part2 | expand |
On 5/15/24 10:20, Zhenzhong Duan wrote: > Since vfio_populate_device() takes an 'Error **' argument, > best practices suggest to return a bool. See the qapi/error.h > Rules section. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/pci.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 379cbad757..c091d21adf 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) > return 0; > } > > -static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > +static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > { > VFIODevice *vbasedev = &vdev->vbasedev; > struct vfio_region_info *reg_info; > @@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > /* Sanity check device */ > if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { > error_setg(errp, "this isn't a PCI device"); > - return; > + return false; > } > > if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { > error_setg(errp, "unexpected number of io regions %u", > vbasedev->num_regions); > - return; > + return false; > } > > if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { > error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); > - return; > + return false; > } > > for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { > @@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > > if (ret) { > error_setg_errno(errp, -ret, "failed to get region %d info", i); > - return; > + return false; > } > > QLIST_INIT(&vdev->bars[i].quirks); > @@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > VFIO_PCI_CONFIG_REGION_INDEX, ®_info); > if (ret) { > error_setg_errno(errp, -ret, "failed to get config info"); > - return; > + return false; > } > > trace_vfio_populate_device_config(vdev->vbasedev.name, > @@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > if (ret) { > error_append_hint(errp, "device does not support " > "requested feature x-vga\n"); > - return; > + return false; > } > } > > @@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > "Could not enable error recovery for the device", > vbasedev->name); > } > + > + return true; > } > > static void vfio_pci_put_device(VFIOPCIDevice *vdev) > @@ -3036,8 +3038,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > goto error; > } > > - vfio_populate_device(vdev, &err); > - if (err) { > + if (!vfio_populate_device(vdev, &err)) { why not pass errp directly and avoid error_propagate() ? Thanks, C. > error_propagate(errp, err); > goto error; > }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 379cbad757..c091d21adf 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2740,7 +2740,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) return 0; } -static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) +static bool vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info; @@ -2750,18 +2750,18 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) /* Sanity check device */ if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { error_setg(errp, "this isn't a PCI device"); - return; + return false; } if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { error_setg(errp, "unexpected number of io regions %u", vbasedev->num_regions); - return; + return false; } if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); - return; + return false; } for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { @@ -2773,7 +2773,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) if (ret) { error_setg_errno(errp, -ret, "failed to get region %d info", i); - return; + return false; } QLIST_INIT(&vdev->bars[i].quirks); @@ -2783,7 +2783,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) VFIO_PCI_CONFIG_REGION_INDEX, ®_info); if (ret) { error_setg_errno(errp, -ret, "failed to get config info"); - return; + return false; } trace_vfio_populate_device_config(vdev->vbasedev.name, @@ -2804,7 +2804,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) if (ret) { error_append_hint(errp, "device does not support " "requested feature x-vga\n"); - return; + return false; } } @@ -2821,6 +2821,8 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) "Could not enable error recovery for the device", vbasedev->name); } + + return true; } static void vfio_pci_put_device(VFIOPCIDevice *vdev) @@ -3036,8 +3038,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto error; } - vfio_populate_device(vdev, &err); - if (err) { + if (!vfio_populate_device(vdev, &err)) { error_propagate(errp, err); goto error; }
Since vfio_populate_device() takes an 'Error **' argument, best practices suggest to return a bool. See the qapi/error.h Rules section. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/pci.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)