[v4,15/17] vfio/pci: Remove vfio_msix_early_setup returned value
diff mbox

Message ID 1475498477-2695-16-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Oct. 3, 2016, 12:41 p.m. UTC
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 <eric.auger@redhat.com>

---

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.
---
 hw/vfio/pci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Markus Armbruster Oct. 4, 2016, 1:05 p.m. UTC | #1
Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>
> ---
>
> 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.
Auger Eric Oct. 6, 2016, 4:09 p.m. UTC | #2
Hi Markus,

On 04/10/2016 15:05, Markus Armbruster wrote:
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>
>> ---
>>
>> 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.

The risk is that if a programmer returns an error value without setting
the errp he will get a sigsev on subsequent error_prepend().
> 
>> ---
>>  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.
I removed one flip at least

Thanks

Eric
>
Markus Armbruster Oct. 7, 2016, 7:15 a.m. UTC | #3
Auger Eric <eric.auger@redhat.com> writes:

> Hi Markus,
>
> On 04/10/2016 15:05, Markus Armbruster wrote:
>> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> 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.
>
> The risk is that if a programmer returns an error value without setting
> the errp he will get a sigsev on subsequent error_prepend().

Yes, the function contract becomes more complex, giving the programmer
another way to screw up.  Besides crashing, callers might also detect
success as failure, failure as success, and leak error objects.

I don't particularly like setting such traps, but:

1. the risk already exists for functions returning a distinct value on
failure, e.g. null on failure and non-null on success, and

2. I've gotten really tired of the extra error-checking boiler-plate
necessary for functions returning void.

[...]

Patch
diff mbox

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;
     }