Re: [Qemu-devel] [PATCH v4 15/17] vfio/pci: Remove vfio_msix_early_setup returned value

[Markus Armbruster]

Eric Auger <address@hidden> writes:

> The returned value is not used anymore by the caller, vfio_realize,
> since the error now is stored in the error object. So let's remove it.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> Logically we could do that job for all the functions now getting an
> Error object passed as a parameter to avoid duplicate information
> between the error content and the returned value. This requires to use
> a local error object in vfio_realize. So I am not sure this is worth
> the candle.

Matter of taste, yours is fine.

We used to recommend returing void instead of an error code when the
function sets and error.  More parsimonious in theory, more boiler-plate
in practice, so we accept either now.  Perhaps we should even recommend
returning an error code, but such a recommendation needs to come with
patches converting existing code to it.

> ---
>  hw/vfio/pci.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index b316e13..cea4d48 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1290,7 +1290,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice 
> *vdev)
>   * need to first look for where the MSI-X table lives.  So we
>   * unfortunately split MSI-X setup across two functions.
>   */
> -static int vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>  {
>      uint8_t pos;
>      uint16_t ctrl;
> @@ -1300,25 +1300,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, 
> Error **errp)
>  
>      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> -        return 0;
> +        return;
>      }
>  
>      if (pread(fd, &ctrl, sizeof(ctrl),
>                vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) {
>          error_setg_errno(errp, errno, "failed to read PCI MSIX FLAGS");
> -        return -errno;
> +        return;
>      }
>  
>      if (pread(fd, &table, sizeof(table),
>                vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) {
>          error_setg_errno(errp, errno, "failed to read PCI MSIX TABLE");
> -        return -errno;
> +        return;
>      }
>  
>      if (pread(fd, &pba, sizeof(pba),
>                vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
>          error_setg_errno(errp, errno, "failed to read PCI MSIX PBA");
> -        return -errno;
> +        return;
>      }
>  
>      ctrl = le16_to_cpu(ctrl);
> @@ -1351,7 +1351,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, 
> Error **errp)
>              error_setg(errp, "hardware reports invalid configuration, "
>                         "MSIX PBA outside of specified BAR");
>              g_free(msix);
> -            return -EINVAL;
> +            return;
>          }
>      }
>  
> @@ -1360,8 +1360,6 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev, 
> Error **errp)
>      vdev->msix = msix;
>  
>      vfio_pci_fixup_msix_region(vdev);
> -
> -    return 0;
>  }
>  
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> @@ -2519,6 +2517,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;
> @@ -2670,8 +2669,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>  
>      vfio_pci_size_rom(vdev);
>  
> -    ret = vfio_msix_early_setup(vdev, errp);
> -    if (ret) {
> +    vfio_msix_early_setup(vdev, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          goto error;
>      }

PATCH 04 checks err, PATCH 13 flips to ret, and this one flips back.
Have you considered dropping both flips?  Your choice.