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

[Kevin Wolf]

Am 24.06.2020 um 05:36 hat Coiby Xu geschrieben:
> 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.

I think I would rename it permanently and keep the new name for the
actual submission. The reason is that if someone else is working on a
series adding new references, the compiler would catch it there, too.

vu_message_read_default() sounds good to me.

> > > @@ -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,

Yes, I saw the reason why you need it in later patches.

If you can remove it completely, that is even better, but otherwise I
would make the addition only later (either in the patch that actually
needs it or in a new separate patch) because it's not really part of
"allowing vu_message_read to be replaced", as the commit message says.

Kevin