On Sat, 07 Apr 2018 16:44:01 +0200 Greg Kurz <gr...@kaod.org> wrote: > A recent patch fixed leaks of the dynamically allocated vcdev->vdev.name > field in vfio_ccw_realize(), but we now have three freeing sites for it. > This is unfortunate and seems to indicate something is wrong with its > life cycle. > > The root issue is that vcdev->vdev.name is set before vfio_get_device() > is called, which theoretically prevents to call vfio_put_device() to > do the freeing. Well actually, we could call it anyway because > vfio_put_base_device() is a nop if the device isn't attached, but this > would be confusing. > > This patch hence moves all the logic of attaching the device, including > the "already attached" check, to a separate vfio_ccw_get_device() function, > counterpart of vfio_put_device(). While here, vfio_put_device() is renamed > to vfio_ccw_put_device() for consistency. > > Signed-off-by: Greg Kurz <gr...@kaod.org> > --- > hw/vfio/ccw.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 36 insertions(+), 20 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index fe34b507699f..49ae986d288d 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -292,12 +292,44 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > g_free(vcdev->io_region); > } > > -static void vfio_put_device(VFIOCCWDevice *vcdev) > +static void vfio_ccw_put_device(VFIOCCWDevice *vcdev) > { > g_free(vcdev->vdev.name); > vfio_put_base_device(&vcdev->vdev); > } > > +static int vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, > + Error **errp)
Make this one void, as you already pass an error object? > +{ > + char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, > + vcdev->cdev.hostid.ssid, > + vcdev->cdev.hostid.devid); > + VFIODevice *vbasedev; > + > + QLIST_FOREACH(vbasedev, &group->device_list, next) { > + if (strcmp(vbasedev->name, name) == 0) { > + error_setg(errp, "vfio: subchannel %s has already been attached", > + name); > + goto out_err; > + } > + } > + > + if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) { > + goto out_err; > + } > + > + vcdev->vdev.ops = &vfio_ccw_ops; > + vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW; > + vcdev->vdev.name = name; > + vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj; > + > + return 0; > + > +out_err: > + g_free(name); > + return -1; That -1 return code looks artificial, so I really think this should be void :) The only caller already defines a local error object anyway, so it can just check for err. > +} > + > static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) > { > char *tmp, group_path[PATH_MAX];