Message ID | 1408528328-11560-2-git-send-email-gwshan@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On 08/20/2014 07:52 PM, Gavin Shan wrote: > The patch drops vfio_container_do_ioctl() and merges its logic to > parent function call vfio_container_ioctl() so that the subsequent > patches can reused the found VFIO group in vfio_container_ioctl(). > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > hw/misc/vfio.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index 7d5f1bb..1a3e7eb 100644 > --- a/hw/misc/vfio.c > +++ b/hw/misc/vfio.c > @@ -4419,8 +4419,8 @@ static void register_vfio_pci_dev_type(void) > > type_init(register_vfio_pci_dev_type) > > -static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, > - int req, void *param) > +int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > + int req, void *param) > { > VFIOGroup *group; > VFIOContainer *container; > @@ -4433,22 +4433,11 @@ static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, > } > > container = group->container; > - if (group->container) { > - ret = ioctl(container->fd, req, param); > - if (ret < 0) { > - error_report("vfio: failed to ioctl container: ret=%d, %s", > - ret, strerror(errno)); > - } > + if (!container) { > + error_report("vfio: no container for group %d\n", groupid); > + goto out; > } > > - vfio_put_group(group); > - > - return ret; > -} > - > -int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > - int req, void *param) > -{ > /* We allow only certain ioctls to the container */ > switch (req) { > case VFIO_CHECK_EXTENSION: > @@ -4458,8 +4447,16 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > default: > /* Return an error on unknown requests */ > error_report("vfio: unsupported ioctl %X", req); > - return -1; > + goto out; > } > > - return vfio_container_do_ioctl(as, groupid, req, param); > + ret = ioctl(container->fd, req, param); > + if (ret < 0) { > + error_report("vfio: failed to ioctl container: ret=%d, %s", > + ret, strerror(errno)); > + } > + > +out: > + vfio_put_group(group); > + return ret; Nack. We specifically separated an ioctl-filtering function and actual code which does ioctl. I understand that you want to save on vfio_get_group() calls for the next EEH patch but this is not a hot path anyway. > } >
On Tue, 2014-08-26 at 20:27 +1000, Alexey Kardashevskiy wrote: > On 08/20/2014 07:52 PM, Gavin Shan wrote: > > The patch drops vfio_container_do_ioctl() and merges its logic to > > parent function call vfio_container_ioctl() so that the subsequent > > patches can reused the found VFIO group in vfio_container_ioctl(). > > > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > > --- > > hw/misc/vfio.c | 33 +++++++++++++++------------------ > > 1 file changed, 15 insertions(+), 18 deletions(-) > > > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > > index 7d5f1bb..1a3e7eb 100644 > > --- a/hw/misc/vfio.c > > +++ b/hw/misc/vfio.c > > @@ -4419,8 +4419,8 @@ static void register_vfio_pci_dev_type(void) > > > > type_init(register_vfio_pci_dev_type) > > > > -static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, > > - int req, void *param) > > +int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > > + int req, void *param) > > { > > VFIOGroup *group; > > VFIOContainer *container; > > @@ -4433,22 +4433,11 @@ static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, > > } > > > > container = group->container; > > - if (group->container) { > > - ret = ioctl(container->fd, req, param); > > - if (ret < 0) { > > - error_report("vfio: failed to ioctl container: ret=%d, %s", > > - ret, strerror(errno)); > > - } > > + if (!container) { > > + error_report("vfio: no container for group %d\n", groupid); > > + goto out; > > } > > > > - vfio_put_group(group); > > - > > - return ret; > > -} > > - > > -int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > > - int req, void *param) > > -{ > > /* We allow only certain ioctls to the container */ > > switch (req) { > > case VFIO_CHECK_EXTENSION: > > @@ -4458,8 +4447,16 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, > > default: > > /* Return an error on unknown requests */ > > error_report("vfio: unsupported ioctl %X", req); > > - return -1; > > + goto out; > > } > > > > - return vfio_container_do_ioctl(as, groupid, req, param); > > + ret = ioctl(container->fd, req, param); > > + if (ret < 0) { > > + error_report("vfio: failed to ioctl container: ret=%d, %s", > > + ret, strerror(errno)); > > + } > > + > > +out: > > + vfio_put_group(group); > > + return ret; > > > Nack. We specifically separated an ioctl-filtering function and actual code > which does ioctl. I understand that you want to save on vfio_get_group() > calls for the next EEH patch but this is not a hot path anyway. I think the split was a result of a comment from me suggesting that a wrapper function might make the code flow better. As implemented, I don't think that result was every actually realized, but the code was functional, so I didn't push it any further. I don't have any issue with recombining it. Thanks, Alex
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c index 7d5f1bb..1a3e7eb 100644 --- a/hw/misc/vfio.c +++ b/hw/misc/vfio.c @@ -4419,8 +4419,8 @@ static void register_vfio_pci_dev_type(void) type_init(register_vfio_pci_dev_type) -static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, - int req, void *param) +int vfio_container_ioctl(AddressSpace *as, int32_t groupid, + int req, void *param) { VFIOGroup *group; VFIOContainer *container; @@ -4433,22 +4433,11 @@ static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid, } container = group->container; - if (group->container) { - ret = ioctl(container->fd, req, param); - if (ret < 0) { - error_report("vfio: failed to ioctl container: ret=%d, %s", - ret, strerror(errno)); - } + if (!container) { + error_report("vfio: no container for group %d\n", groupid); + goto out; } - vfio_put_group(group); - - return ret; -} - -int vfio_container_ioctl(AddressSpace *as, int32_t groupid, - int req, void *param) -{ /* We allow only certain ioctls to the container */ switch (req) { case VFIO_CHECK_EXTENSION: @@ -4458,8 +4447,16 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid, default: /* Return an error on unknown requests */ error_report("vfio: unsupported ioctl %X", req); - return -1; + goto out; } - return vfio_container_do_ioctl(as, groupid, req, param); + ret = ioctl(container->fd, req, param); + if (ret < 0) { + error_report("vfio: failed to ioctl container: ret=%d, %s", + ret, strerror(errno)); + } + +out: + vfio_put_group(group); + return ret; }
The patch drops vfio_container_do_ioctl() and merges its logic to parent function call vfio_container_ioctl() so that the subsequent patches can reused the found VFIO group in vfio_container_ioctl(). Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- hw/misc/vfio.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)