| Message ID | 20240522044015.412951-21-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | VFIO: misc cleanups part2 | expand |
On 5/22/24 06:40, Zhenzhong Duan wrote: > When get name failed, we should call unrealize() so that > vfio_ccw_realize() is self contained. > > Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle") > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> If the realize handler fails, the unrealize handler should be called. See device_set_realized(). We should be fine without IMO. Thanks, C. > --- > hw/vfio/ccw.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 168c9e5973..161704cd7b 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > } > > if (!vfio_device_get_name(vbasedev, errp)) { > - return; > + goto out_unrealize; > } > > if (!vfio_attach_device(cdev->mdevid, vbasedev, > @@ -633,6 +633,7 @@ out_region_err: > vfio_detach_device(vbasedev); > out_attach_dev_err: > g_free(vbasedev->name); > +out_unrealize: > if (cdc->unrealize) { > cdc->unrealize(cdev); > }
Hi Cédric, >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Wednesday, May 22, 2024 3:52 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org >Cc: alex.williamson@redhat.com; eric.auger@redhat.com; Peng, Chao P ><chao.p.peng@intel.com>; Eric Farman <farman@linux.ibm.com>; Matthew >Rosato <mjrosato@linux.ibm.com>; Thomas Huth <thuth@redhat.com>; >open list:vfio-ccw <qemu-s390x@nongnu.org> >Subject: Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in >error path > >On 5/22/24 06:40, Zhenzhong Duan wrote: >> When get name failed, we should call unrealize() so that >> vfio_ccw_realize() is self contained. >> >> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a >file handle") >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >If the realize handler fails, the unrealize handler should be called. >See device_set_realized(). We should be fine without IMO. Do you mean when vfio_ccw_realize() fails, vfio_ccw_unrealize() will be called? Looked into device_set_realized(), I didn't see where vfio_ccw_unrealize() was called. Do I misunderstand? Thanks Zhenzhong > > >Thanks, > >C. > > > >> --- >> hw/vfio/ccw.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 168c9e5973..161704cd7b 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, >Error **errp) >> } >> >> if (!vfio_device_get_name(vbasedev, errp)) { >> - return; >> + goto out_unrealize; >> } >> >> if (!vfio_attach_device(cdev->mdevid, vbasedev, >> @@ -633,6 +633,7 @@ out_region_err: >> vfio_detach_device(vbasedev); >> out_attach_dev_err: >> g_free(vbasedev->name); >> +out_unrealize: >> if (cdc->unrealize) { >> cdc->unrealize(cdev); >> }
On 5/22/24 10:05, Duan, Zhenzhong wrote: > Hi Cédric, > >> -----Original Message----- >> From: Cédric Le Goater <clg@redhat.com> >> Sent: Wednesday, May 22, 2024 3:52 PM >> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >> devel@nongnu.org >> Cc: alex.williamson@redhat.com; eric.auger@redhat.com; Peng, Chao P >> <chao.p.peng@intel.com>; Eric Farman <farman@linux.ibm.com>; Matthew >> Rosato <mjrosato@linux.ibm.com>; Thomas Huth <thuth@redhat.com>; >> open list:vfio-ccw <qemu-s390x@nongnu.org> >> Subject: Re: [PATCH v2 20/20] vfio/ccw: Fix the missed unrealize() call in >> error path >> >> On 5/22/24 06:40, Zhenzhong Duan wrote: >>> When get name failed, we should call unrealize() so that >>> vfio_ccw_realize() is self contained. >>> >>> Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a >> file handle") >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> If the realize handler fails, the unrealize handler should be called. >> See device_set_realized(). We should be fine without IMO. > > Do you mean when vfio_ccw_realize() fails, vfio_ccw_unrealize() will be called? > Looked into device_set_realized(), I didn't see where vfio_ccw_unrealize() was called. > Do I misunderstand? no, it's me. I thought it was called in the failure path :/ Anyhow, let's keep this patch for a ccw series. Thanks, C. > > Thanks > Zhenzhong > >> >> >> Thanks, >> >> C. >> >> >> >>> --- >>> hw/vfio/ccw.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >>> index 168c9e5973..161704cd7b 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >>> @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, >> Error **errp) >>> } >>> >>> if (!vfio_device_get_name(vbasedev, errp)) { >>> - return; >>> + goto out_unrealize; >>> } >>> >>> if (!vfio_attach_device(cdev->mdevid, vbasedev, >>> @@ -633,6 +633,7 @@ out_region_err: >>> vfio_detach_device(vbasedev); >>> out_attach_dev_err: >>> g_free(vbasedev->name); >>> +out_unrealize: >>> if (cdc->unrealize) { >>> cdc->unrealize(cdev); >>> } >
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 168c9e5973..161704cd7b 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -589,7 +589,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) } if (!vfio_device_get_name(vbasedev, errp)) { - return; + goto out_unrealize; } if (!vfio_attach_device(cdev->mdevid, vbasedev, @@ -633,6 +633,7 @@ out_region_err: vfio_detach_device(vbasedev); out_attach_dev_err: g_free(vbasedev->name); +out_unrealize: if (cdc->unrealize) { cdc->unrealize(cdev); }
When get name failed, we should call unrealize() so that vfio_ccw_realize() is self contained. Fixes: 909a6254eda ("vfio/ccw: Make vfio cdev pre-openable by passing a file handle") Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/ccw.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)