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;
         }
         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 {
-- 
1.8.3.1

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 {
> >
>