From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33117) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMN00-0000jQ-UH for qemu-devel@nongnu.org; Tue, 26 Aug 2014 16:05:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMMzo-00037w-P0 for qemu-devel@nongnu.org; Tue, 26 Aug 2014 16:05:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20790) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMMzo-00037c-FX for qemu-devel@nongnu.org; Tue, 26 Aug 2014 16:05:36 -0400 Message-ID: <1409083531.2906.142.camel@ul30vt.home> From: Alex Williamson Date: Tue, 26 Aug 2014 14:05:31 -0600 In-Reply-To: <53FC6113.5030909@ozlabs.ru> References: <1408528328-11560-1-git-send-email-gwshan@linux.vnet.ibm.com> <1408528328-11560-2-git-send-email-gwshan@linux.vnet.ibm.com> <53FC6113.5030909@ozlabs.ru> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 1/2] VFIO: Drop vfio_container_do_ioctl() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Gavin Shan , qemu-devel@nongnu.org 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 > > --- > > 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