| Message ID | 20240708143420.16953-4-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/vfio: IOMMUFD Dirty Tracking | expand |
Hello Joao, On 7/8/24 4:34 PM, Joao Martins wrote: > In preparation to implement auto domains have the attach function > return the errno it got during domain attach instead of a bool. > > -EINVAL is tracked to track domain incompatibilities, and decide whether > to create a new IOMMU domain. Please leave the return value as a bool unless there is a very good reason not to. Thanks, C. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/vfio/iommufd.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index 9cee71659b1c..5a5993b17c2e 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -172,7 +172,7 @@ out: > return ret; > } > > -static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, > +static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, > Error **errp) > { > int iommufd = vbasedev->iommufd->fd; > @@ -187,12 +187,12 @@ static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, > error_setg_errno(errp, errno, > "[iommufd=%d] error attach %s (%d) to id=%d", > iommufd, vbasedev->name, vbasedev->fd, id); > - return false; > + return -errno; > } > > trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name, > vbasedev->fd, id); > - return true; > + return 0; > } > > static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) > @@ -216,7 +216,7 @@ static bool iommufd_cdev_attach_container(VFIODevice *vbasedev, > VFIOIOMMUFDContainer *container, > Error **errp) > { > - return iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp); > + return !iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp); > } > > static void iommufd_cdev_detach_container(VFIODevice *vbasedev,
On 08/07/2024 16:28, Cédric Le Goater wrote: > Hello Joao, > > On 7/8/24 4:34 PM, Joao Martins wrote: >> In preparation to implement auto domains have the attach function >> return the errno it got during domain attach instead of a bool. >> >> -EINVAL is tracked to track domain incompatibilities, and decide whether >> to create a new IOMMU domain. > > Please leave the return value as a bool unless there is a very > good reason not to. > Error* doesn't store the errno, and thus I can't actually test the number of errno to know when to bail to the next hwpt. Maybe the commit message wasn't clear enough there. But not sure if we have an alternative here? Or maybe Error does store errno, and I totally missed it.
On 08/07/2024 16:32, Joao Martins wrote: > On 08/07/2024 16:28, Cédric Le Goater wrote: >> Hello Joao, >> >> On 7/8/24 4:34 PM, Joao Martins wrote: >>> In preparation to implement auto domains have the attach function >>> return the errno it got during domain attach instead of a bool. >>> >>> -EINVAL is tracked to track domain incompatibilities, and decide whether >>> to create a new IOMMU domain. >> >> Please leave the return value as a bool unless there is a very >> good reason not to. >> > > Error* doesn't store the errno, and thus I can't actually test the number of > errno to know when to bail to the next hwpt. Maybe the commit message wasn't > clear enough there. But not sure if we have an alternative here? Or maybe Error > does store errno, and I totally missed it. Or I can just use 'errno' and keep the same return value as bool. But I didn't do that because we are purposedly calling error_set*(errp, errno) with the errno value that it felt the right thing to just return it. Also, considering how hard the code in util/error.c saves/restores errno in all the helpers. Joao
On 7/8/24 5:32 PM, Joao Martins wrote: > On 08/07/2024 16:28, Cédric Le Goater wrote: >> Hello Joao, >> >> On 7/8/24 4:34 PM, Joao Martins wrote: >>> In preparation to implement auto domains have the attach function >>> return the errno it got during domain attach instead of a bool. >>> >>> -EINVAL is tracked to track domain incompatibilities, and decide whether >>> to create a new IOMMU domain. >> >> Please leave the return value as a bool unless there is a very >> good reason not to. >> > > Error* doesn't store the errno, and thus I can't actually test the number of > errno to know when to bail to the next hwpt. Maybe the commit message wasn't > clear enough there. But not sure if we have an alternative here? Or maybe Error > does store errno, and I totally missed it. OK. Let's do the 'bool' -> 'int' change for iommufd_cdev_attach_ioas_hwpt(). I see how it is used later on in the series. Thanks, C.
On 09/07/2024 07:20, Cédric Le Goater wrote: > On 7/8/24 5:32 PM, Joao Martins wrote: >> On 08/07/2024 16:28, Cédric Le Goater wrote: >>> Hello Joao, >>> >>> On 7/8/24 4:34 PM, Joao Martins wrote: >>>> In preparation to implement auto domains have the attach function >>>> return the errno it got during domain attach instead of a bool. >>>> >>>> -EINVAL is tracked to track domain incompatibilities, and decide whether >>>> to create a new IOMMU domain. >>> >>> Please leave the return value as a bool unless there is a very >>> good reason not to. >>> >> >> Error* doesn't store the errno, and thus I can't actually test the number of >> errno to know when to bail to the next hwpt. Maybe the commit message wasn't >> clear enough there. But not sure if we have an alternative here? Or maybe Error >> does store errno, and I totally missed it. > > OK. Let's do the 'bool' -> 'int' change for iommufd_cdev_attach_ioas_hwpt(). > I see how it is used later on in the series. Got it
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index 9cee71659b1c..5a5993b17c2e 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -172,7 +172,7 @@ out: return ret; } -static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, +static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, Error **errp) { int iommufd = vbasedev->iommufd->fd; @@ -187,12 +187,12 @@ static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id, error_setg_errno(errp, errno, "[iommufd=%d] error attach %s (%d) to id=%d", iommufd, vbasedev->name, vbasedev->fd, id); - return false; + return -errno; } trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name, vbasedev->fd, id); - return true; + return 0; } static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp) @@ -216,7 +216,7 @@ static bool iommufd_cdev_attach_container(VFIODevice *vbasedev, VFIOIOMMUFDContainer *container, Error **errp) { - return iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp); + return !iommufd_cdev_attach_ioas_hwpt(vbasedev, container->ioas_id, errp); } static void iommufd_cdev_detach_container(VFIODevice *vbasedev,
In preparation to implement auto domains have the attach function return the errno it got during domain attach instead of a bool. -EINVAL is tracked to track domain incompatibilities, and decide whether to create a new IOMMU domain. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- hw/vfio/iommufd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)