| Message ID | 1475498477-2695-16-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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. [...]
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; }
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(-)