Re: [PATCH v4 0/5] vhost-user block device backend implementation

[Daniel P . Berrangé]

On Thu, Feb 27, 2020 at 12:19:58PM +0100, Kevin Wolf wrote:
> Am 27.02.2020 um 12:07 hat Marc-André Lureau geschrieben:
> > On Thu, Feb 27, 2020 at 11:55 AM Kevin Wolf <address@hidden> wrote:
> > > Am 27.02.2020 um 11:28 hat Coiby Xu geschrieben:
> > > > > > we still need customized vu_message_read because libvhost-user 
> > > > > > assumes
> > > > > > we will always get a full-size VhostUserMsg and hasn't taken care of
> > > > > > this short read case. I will improve libvhost-user's vu_message_read
> > > > > > by making it keep reading from socket util getting enough bytes. I
> > > > > > assume short read is a rare case thus introduced performance penalty
> > > > > > would be negligible.
> > > >
> > > > > In any case, please make sure that we use the QIOChannel functions
> > > > > called from a coroutine in QEMU so that it will never block, but the
> > > > > coroutine can just yield while it's waiting for more bytes.
> > > >
> > > > But if I am not wrong, libvhost-user is supposed to be indepdent from
> > > > the main QEMU code. So it can't use the QIOChannel functions if we
> > > > simply modify exiting vu_message_read to address the short read issue.
> > > > In v3 & v4, I extended libvhost-user to allow vu_message_read to be
> > > > replaced by one which will depend on the main QEMU code. I'm not sure
> > > > which way is better.
> > >
> > > The way your latest patches have it, with a separate read function,
> > > works for me.
> > 
> > Done right, I am not against it, fwiw
> > 
> > > You could probably change libvhost-user to reimplement the same
> > > functionality, and it might be an improvement for other users of the
> > > library, but it's also code duplication and doesn't provide more value
> > > in the context of the vhost-user export in QEMU.
> > >
> > > The point that's really important to me is just that we never block when
> > > we run inside QEMU because that would actually stall the guest. This
> > > means busy waiting in a tight loop until read() returns enough bytes is
> > > not acceptable in QEMU.
> > 
> > In the context of vhost-user, local unix sockets with short messages
> > (do we have >1k messages?), I am not sure if this is really a problem.
> 
> I'm not sure how much of a problem it is in practice, and whether we
> can consider the vhost-user client trusted. But using QIOChannel from
> within a coroutine just avoids the problem completely, so it feels like
> a natural choice to just do that.

It isn't clear to me that we have a consitent plan for how we intend
libvhost-user to develop & what it is permitted to use.  What information
I see in the source code and in this thread are contradictory.

For example, in the text quoted above:

  "libvhost-user is supposed to be indepdent from the main QEMU code."

which did match my overall understanding too. At the top of libvhost-user.c
there is a comment

   /* this code avoids GLib dependency */

but a few lines later it does

  #include "qemu/atomic.h"
  #include "qemu/osdep.h"
  #include "qemu/memfd.h"

and in the Makefile we link it to much of QEMU util code:

  libvhost-user.a: $(libvhost-user-obj-y) $(util-obj-y) $(stub-obj-y)

this in turn pulls in GLib code, and looking at symbols we can see
over 100 GLib functions used:

  $ nm ./libvhost-user.a | grep 'U g_' | sort | uniq | wc -l
  128

And over 200 QEMU source object files included:

  $ nm ./libvhost-user.a | grep '.o:' | sort | wc -l
  224

So unless I'm missing something, we have lost any independance from
both QEMU and GLib code that we might have had in the past.

Note this also has licensing implications, as I expect this means that
via the QEMU source objects it pulls in, libvhost-user.a has become
a GPLv2-only combined work, not a GPLv2-or-later combined work.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|