| Message ID | 20190715102326.2805-1-xieyongji@baidu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed | expand |
On Mon, Jul 15, 2019 at 06:23:25PM +0800, elohimes@gmail.com wrote: > From: Xie Yongji <xieyongji@baidu.com> > > This avoids memory leak when device hotplug is failed. > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > --- > hw/scsi/vhost-scsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 4090f99ee4..db4a090576 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > if (err) { > error_propagate(errp, err); > error_free(vsc->migration_blocker); > - goto close_fd; > + goto free_virtio; > } > } > > @@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > migrate_del_blocker(vsc->migration_blocker); > } > g_free(vsc->dev.vqs); > + free_virtio: > + virtio_scsi_common_unrealize(dev, errp); error_set*() requires that *errp == NULL: static void error_setv(Error **errp, ... ... assert(*errp == NULL); Today virtio_scsi_common_unrealize() doesn't use the errp argument but if it ever uses it then QEMU will hit an assertion failure. Please do this instead: virtio_scsi_common_unrealize(dev, &error_abort); If virtio_scsi_common_unrealize() ever produces an error there will be an message explaining that errors are unexpected. This also applies to Patch 2. Alternatively you could do this to handle all cases and propagate the error: Error *local_err = NULL; virtio_scsi_common_unrealize(dev, &local_err); error_propagate(errp, local_err);
On Tue, 16 Jul 2019 at 22:42, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Mon, Jul 15, 2019 at 06:23:25PM +0800, elohimes@gmail.com wrote: > > From: Xie Yongji <xieyongji@baidu.com> > > > > This avoids memory leak when device hotplug is failed. > > > > Signed-off-by: Xie Yongji <xieyongji@baidu.com> > > --- > > hw/scsi/vhost-scsi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > > index 4090f99ee4..db4a090576 100644 > > --- a/hw/scsi/vhost-scsi.c > > +++ b/hw/scsi/vhost-scsi.c > > @@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > if (err) { > > error_propagate(errp, err); > > error_free(vsc->migration_blocker); > > - goto close_fd; > > + goto free_virtio; > > } > > } > > > > @@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > migrate_del_blocker(vsc->migration_blocker); > > } > > g_free(vsc->dev.vqs); > > + free_virtio: > > + virtio_scsi_common_unrealize(dev, errp); > > error_set*() requires that *errp == NULL: > > static void error_setv(Error **errp, ... > ... > assert(*errp == NULL); > > Today virtio_scsi_common_unrealize() doesn't use the errp argument but > if it ever uses it then QEMU will hit an assertion failure. > > Please do this instead: > > virtio_scsi_common_unrealize(dev, &error_abort); > > If virtio_scsi_common_unrealize() ever produces an error there will be > an message explaining that errors are unexpected. > > This also applies to Patch 2. > > Alternatively you could do this to handle all cases and propagate the > error: > > Error *local_err = NULL; > virtio_scsi_common_unrealize(dev, &local_err); > error_propagate(errp, local_err); Will fix it in v2. Thank you. Thanks, Yongji
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 4090f99ee4..db4a090576 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) if (err) { error_propagate(errp, err); error_free(vsc->migration_blocker); - goto close_fd; + goto free_virtio; } } @@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) migrate_del_blocker(vsc->migration_blocker); } g_free(vsc->dev.vqs); + free_virtio: + virtio_scsi_common_unrealize(dev, errp); close_fd: close(vhostfd); return;