Hi,
On Thu, Jun 06, 2019 at 03:30:29PM +0200, Marc-André Lureau wrote:
> Hi
>
> On Fri, Apr 26, 2019 at 8:32 AM Tiwei Bie <tiwei....@intel.com> wrote:
> >
> > We need to destroy the host notifiers when cleaning up
> > the backend. Otherwise, some resources are not released
> > after the connection is closed, and it may prevent the
> > external backend from reopening them (e.g. VFIO files)
> > during restart.
> >
> > Fixes: 44866521bd6e ("vhost-user: support registering external host
> > notifiers")
> > Cc: qemu-sta...@nongnu.org
> >
> > Signed-off-by: Tiwei Bie <tiwei....@intel.com>
> > ---
> > hw/virtio/vhost-user.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 553319c7ac..56656629c0 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1454,10 +1454,24 @@ static int vhost_user_backend_init(struct vhost_dev
> > *dev, void *opaque)
> > static int vhost_user_backend_cleanup(struct vhost_dev *dev)
> > {
> > struct vhost_user *u;
> > + VhostUserState *user;
> > + int i;
> >
> > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >
> > u = dev->opaque;
> > +
> > + if (dev->vq_index == 0) {
> > + user = u->user;
> > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > + if (user->notifier[i].addr) {
> > + object_unparent(OBJECT(&user->notifier[i].mr));
> > + munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > + user->notifier[i].addr = NULL;
> > + }
> > + }
> > + }
>
> Why not call vhost_user_cleanup() ? Alternatively, factor the notifier
> code in a seperate vhost_user_notifiers_cleanup() ?
I like the idea to factor the notifier code in a seperate
vhost_user_notifiers_cleanup(). I can do it. Thanks!
>
> > +
> > if (u->postcopy_notifier.notify) {
> > postcopy_remove_notifier(&u->postcopy_notifier);
> > u->postcopy_notifier.notify = NULL;
> > @@ -1881,6 +1895,8 @@ bool vhost_user_init(VhostUserState *user,
> > CharBackend *chr, Error **errp)
> > error_setg(errp, "Cannot initialize vhost-user state");
> > return false;
> > }
> > +
> > + memset(user, 0, sizeof(*user));
>
> This looks superflous. Is it really needed?
I think you are right. We already checked whether user->chr
is zero. The caller should make sure that the VhostUserState
will be zero initialized.
>
> I wish there would be some basic tests for external host notifiers. Is
> it too much to ask to add it in vhost-user-test.c ?
Sounds good to me. Besides, there are already some basic
external host notifier supports in tests/vhost-user-bridge.c
that can be enabled with -H. I'm not sure whether that already
met what you want. If adding some basic tests in vhost-user-test.c
would help, I'd like to do it.
Thanks for the review!
Tiwei
>
>
> > user->chr = chr;
> > return true;
> > }
> > --
> > 2.17.1
> >
> >
>
>
> --
> Marc-André Lureau
