Hi ----- Original Message ----- > On 21.07.2016 11:57, Marc-André Lureau wrote: > > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > vhost_net_init() calls vhost_dev_init() and in case of failure, calls > > vhost_dev_cleanup() directly. However, the structure is already > > partially cleaned on error. Calling vhost_dev_cleanup() again will call > > vhost_virtqueue_cleanup() on already clean queues, and causing potential > > double-close. Instead, adjust dev->nvqs and simplify vhost_dev_init() > > code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanup() > > instead. > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > --- > > hw/virtio/vhost.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 9400b47..c61302a 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1047,7 +1047,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void > > *opaque, > > for (i = 0; i < hdev->nvqs; ++i) { > > r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); > > if (r < 0) { > > - goto fail_vq; > > + hdev->nvqs = i; > > + goto fail; > > } > > } > > > > @@ -1104,19 +1105,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void > > *opaque, > > memory_listener_register(&hdev->memory_listener, > > &address_space_memory); > > QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); > > return 0; > > + > > fail_busyloop: > > while (--i >= 0) { > > vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0); > > } > > - i = hdev->nvqs; > > -fail_vq: > > - while (--i >= 0) { > > - vhost_virtqueue_cleanup(hdev->vqs + i); > > - } > > fail: > > - r = -errno; > > - hdev->vhost_ops->vhost_backend_cleanup(hdev); > > - QLIST_REMOVE(hdev, entry); > > + vhost_dev_cleanup(hdev); > > return r; > > } > > > > > > This patch introduces closing of zero fd on backend init failure or any > other error before virtqueue_init loop because of calling > 'vhost_virtqueue_cleanup()' on not initialized virtqueues. > > I'm suggesting following fixup: > > ---------------------------------------------------------------------- > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 6175d8b..d7428c5 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void > *opaque, > VhostBackendType backend_type, uint32_t busyloop_timeout) > { > uint64_t features; > - int i, r; > + int i, r, n_initialized_vqs; > > + n_initialized_vqs = 0; > hdev->migration_blocker = NULL; > > r = vhost_set_backend_type(hdev, backend_type); > > @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void > *opaque, > goto fail; > } > > - for (i = 0; i < hdev->nvqs; ++i) { > + for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) { > r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); > if (r < 0) { > - hdev->nvqs = i;
Isn't that assignment doing the same thing? btw, thanks for the review > goto fail; > } > } > @@ -1136,6 +1137,7 @@ fail_busyloop: > vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0); > } > fail: > + hdev->nvqs = n_initialized_vqs; > vhost_dev_cleanup(hdev); > return r; > } > ---------------------------------------------------------------------- > > Best regards, Ilya Maximets. >