| Message ID | 1539926412-21831-8-git-send-email-liq3ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: some trivial fixes | expand |
Hi Li, On 10/19/18 7:20 AM, Li Qiang wrote: > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > hw/vfio/platform.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > index ba19143..e9d9e80 100644 > --- a/hw/vfio/platform.c > +++ b/hw/vfio/platform.c > @@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) > error_setg(errp, "%s", gerr->message); > g_error_free(gerr); > g_free(path); > - return; > + goto out; You must set ret to != 0 otherwise the qemu_mutex_destroy will not be reached I think. Also this will fix the fact we are not prepending the vfio error prefix in that case, as we should. Besides I am unsure about the cleanup strategy in case or error in vfio_platform_realize(). The qemu process should always exit in case of failure in vfio_platform_realize(). Platform devices can only be cold-plugged through the qemu CLI. Cleaning all the allocated resources may add a substantial amount of code. For instance resources allocated in vfio_base_device_init() are not freed either. Comprehensive free in realize() functions may only be needed in case of hotplug I think. Thanks Eric > } > g_free(path); > vdev->compat = contents; > @@ -691,6 +691,8 @@ out: > return; > } > > + qemu_mutex_destroy(&vdev->intp_mutex); > + > if (vdev->vbasedev.name) { > error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name); > } else { >
Hello Auger, Auger Eric <eric.auger@redhat.com> 于2018年10月20日周六 上午12:41写道： > Hi Li, > > On 10/19/18 7:20 AM, Li Qiang wrote: > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > hw/vfio/platform.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > > index ba19143..e9d9e80 100644 > > --- a/hw/vfio/platform.c > > +++ b/hw/vfio/platform.c > > @@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev, > Error **errp) > > error_setg(errp, "%s", gerr->message); > > g_error_free(gerr); > > g_free(path); > > - return; > > + goto out; > You must set ret to != 0 otherwise the qemu_mutex_destroy will not be > reached I think. Also this will fix the fact we are not prepending the > vfio error prefix in that case, as we should. > > Besides I am unsure about the cleanup strategy in case or error in > vfio_platform_realize(). The qemu process should always exit in case of > failure in vfio_platform_realize(). Platform devices can only be > cold-plugged through the qemu CLI. Got this. > Cleaning all the allocated resources > may add a substantial amount of code. Agree. Thanks, Li Qiang > For instance resources allocated > in vfio_base_device_init() are not freed either. Comprehensive free in > realize() functions may only be needed in case of hotplug I think. > > Thanks > > Eric > > } > > g_free(path); > > vdev->compat = contents; > > @@ -691,6 +691,8 @@ out: > > return; > > } > > > > + qemu_mutex_destroy(&vdev->intp_mutex); > > + > > if (vdev->vbasedev.name) { > > error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name); > > } else { > > >
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index ba19143..e9d9e80 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -668,7 +668,7 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp) error_setg(errp, "%s", gerr->message); g_error_free(gerr); g_free(path); - return; + goto out; } g_free(path); vdev->compat = contents; @@ -691,6 +691,8 @@ out: return; } + qemu_mutex_destroy(&vdev->intp_mutex); + if (vdev->vbasedev.name) { error_prepend(errp, ERR_PREFIX, vdev->vbasedev.name); } else {
Signed-off-by: Li Qiang <liq3ea@gmail.com> --- hw/vfio/platform.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)