Re: [PATCH v9 1/5] Allow vu_message_read to be replaced

[Coiby Xu]

On Thu, Jun 18, 2020 at 12:43:47PM +0200, Kevin Wolf wrote:
> Am 14.06.2020 um 20:39 hat Coiby Xu geschrieben:
> > Allow vu_message_read to be replaced by one which will make use of the
> > QIOChannel functions. Thus reading vhost-user message won't stall the
> > guest.
> >
> > Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>
> _vu_queue_notify() still has a direct call of vu_message_read() instead
> of using the pointer. Is this intentional?

This is a mistake. Thank you for reminding me!

> Renaming the function would make sure that such semantic merge conflicts
> don't stay unnoticed.

Thank you for this tip! Do you suggest renaming the function only for
the purpose of testing or should I adopt a name when submitting the
patch? For the latter case, I will change it to vu_message_read_default
then.

> > @@ -1704,6 +1702,7 @@ vu_deinit(VuDev *dev)
> >          }
> >
> >          if (vq->kick_fd != -1) {
> > +            dev->remove_watch(dev, vq->kick_fd);
> >              close(vq->kick_fd);
> >              vq->kick_fd = -1;
> >          }
>
> This hunk looks unrelated.

In v4, I made the comment to explain why it's needed. But libvhost-user
is supposed to be independent from QEMU, so Stefan suggested to remove it,

> > @@ -1627,6 +1647,12 @@ vu_deinit(VuDev *dev)
> >          }
> >
> >          if (vq->kick_fd != -1) {
> > +            /* remove watch for kick_fd
> > +             * When client process is running in gdb and
> > +             * quit command is run in gdb, QEMU will still dispatch the event
> > +             * which will cause segment fault in the callback function
> > +             */
>
> Code and comments in libvhost-user should not refer to QEMU specifics.
> Removing the watch is a good idea regardless of the application or event
> loop implementation.  No comment is needed here.
> +            /* remove watch for kick_fd
> +             * When client process is running in gdb and
> +             * quit command is run in gdb, QEMU will still dispatch the event
> +             * which will cause segment fault in the callback function
> +             */
> +            dev->remove_watch(dev, vq->kick_fd);

--
Best regards,
Coiby