From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ4b5-0000Uk-HX for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:22:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJ4b2-0004dh-7d for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:22:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33846) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJ4b2-0004dX-0A for qemu-devel@nongnu.org; Wed, 04 Feb 2015 13:22:40 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t14IMdYv031762 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Feb 2015 13:22:39 -0500 Message-ID: <1423074157.22865.475.camel@redhat.com> From: Alex Williamson Date: Wed, 04 Feb 2015 11:22:37 -0700 In-Reply-To: <1423051870-26473-2-git-send-email-pbonzini@redhat.com> References: <1423051870-26473-1-git-send-email-pbonzini@redhat.com> <1423051870-26473-2-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] vfio: cleanup vfio_get_device error path, remove vfio_populate_device callback List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On Wed, 2015-02-04 at 13:11 +0100, Paolo Bonzini wrote: > With the next patch vfio_put_base_device will be called unconditionally at > instance_finalize time, which will mean calling it twice if vfio_populate_device > fails. This works, but it is slightly harder to follow. > > Change vfio_get_device to not touch the vbasedev struct until it will > definitely succeed, moving the vfio_populate_device call back to vfio-pci. > This way, vfio_put_base_device will only be called once and only on > non-error paths. > > Cc: Alex Williamson > Signed-off-by: Paolo Bonzini > --- > hw/vfio/common.c | 31 ++++++++++++------------------- > hw/vfio/pci.c | 11 +++++++---- > include/hw/vfio/vfio-common.h | 1 - > 3 files changed, 19 insertions(+), 24 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index cf483ff..242b71d 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -867,27 +867,28 @@ int vfio_get_device(VFIOGroup *group, const char *name, > VFIODevice *vbasedev) > { > struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) }; > - int ret; > + int ret, fd; > > - ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); > - if (ret < 0) { > + fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); > + if (fd < 0) { > error_report("vfio: error getting device %s from group %d: %m", > name, group->groupid); > error_printf("Verify all devices in group %d are bound to vfio- " > "or pci-stub and not already in use\n", group->groupid); > - return ret; > + return fd; > } > > - vbasedev->fd = ret; > - vbasedev->group = group; > - QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); > - > - ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_INFO, &dev_info); > + ret = ioctl(fd, VFIO_DEVICE_GET_INFO, &dev_info); > if (ret) { > error_report("vfio: error getting device info: %m"); > - goto error; > + close(fd); > + return ret; > } > > + vbasedev->fd = fd; > + vbasedev->group = group; > + QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); > + > vbasedev->num_irqs = dev_info.num_irqs; > vbasedev->num_regions = dev_info.num_regions; > vbasedev->flags = dev_info.flags; > @@ -896,20 +897,12 @@ int vfio_get_device(VFIOGroup *group, const char *name, > dev_info.num_irqs); > > vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET); > - > - ret = vbasedev->ops->vfio_populate_device(vbasedev); > - > -error: > - if (ret) { > - vfio_put_base_device(vbasedev); > - } > - return ret; > + return 0; > } > > void vfio_put_base_device(VFIODevice *vbasedev) > { > QLIST_REMOVE(vbasedev, next); > - vbasedev->group = NULL; I can't figure out why this is necessary. If we don't instantiate a vfio device, then group will be NULL, which is what's used in the next patch to filter certain code paths, including this one. It would be just as incorrect to call those code paths on a finalized device, so why do we not clear this? Otherwise the series appears reasonable to me. Thanks, Alex > trace_vfio_put_base_device(vbasedev->fd); > close(vbasedev->fd); > }