Re: [Qemu-devel] [RFC PATCH 1/2] VFIO: Drop vfio_container_do_ioctl()

[Alex Williamson]

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 <address@hidden>
> > ---
> >  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