Re: [PATCH 20/24] vhost-user: add vhost-user

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 12 Feb 2024 11:02:14 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 2/12/24 03:49, David Gibson wrote:
> > On Mon, Feb 12, 2024 at 12:19:01AM +0100, Stefano Brivio wrote:  
> >> On Fri,  2 Feb 2024 15:11:47 +0100
> >> Laurent Vivier <lvivier@redhat.com> wrote:
> >>  
> >>> add virtio and vhost-user functions to connect with QEMU.
> >>>
> >>>    $ ./passt --vhost-user
> >>>
> >>> and
> >>>
> >>>    # qemu-system-x86_64 ... -m 4G \
> >>>          -object memory-backend-memfd,id=memfd0,share=on,size=4G \
> >>>          -numa node,memdev=memfd0 \
> >>>          -chardev socket,id=chr0,path=/tmp/passt_1.socket \
> >>>          -netdev vhost-user,id=netdev0,chardev=chr0 \
> >>>          -device virtio-net,mac=9a:2b:2c:2d:2e:2f,netdev=netdev0 \
> >>>          ...
> >>>
> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>> ---
> >>>   conf.c  | 20 ++++++++++++++--
> >>>   passt.c |  7 ++++++
> >>>   passt.h |  1 +
> >>>   tap.c   | 73 ++++++++++++++++++++++++++++++++++++++++++---------------
> >>>   tcp.c   |  8 +++++--
> >>>   udp.c   |  6 +++--
> >>>   6 files changed, 90 insertions(+), 25 deletions(-)  
> >> This would need a matching change in the man page, passt.1, at least
> >> documenting the --vhost-user option and adjusting descriptions about
> >> the guest communication interface (look for "UNIX domain" there).
> >>  
> >>> diff --git a/conf.c b/conf.c
> >>> index b6a2a1f0fdc3..40aa9519f8a6 100644
> >>> --- a/conf.c
> >>> +++ b/conf.c
> >>> @@ -44,6 +44,7 @@
> >>>   #include "lineread.h"
> >>>   #include "isolation.h"
> >>>   #include "log.h"
> >>> +#include "vhost_user.h"
> >>>   
> >>>   /**
> >>>    * next_chunk - Return the next piece of a string delimited by a character
> >>> @@ -735,9 +736,12 @@ static void print_usage(const char *name, int status)
> >>>   		info(   "  -I, --ns-ifname NAME	namespace interface name");
> >>>   		info(   "    default: same interface name as external one");
> >>>   	} else {
> >>> -		info(   "  -s, --socket PATH	UNIX domain socket path");
> >>> +		info(   "  -s, --socket, --socket-path PATH	UNIX domain socket path");  
> >> I don't get the point of --socket-path. It's handled just like -s
> >> anyway, right? Why can't it just be -s / --socket?  
> > I believe the issue is that there's an expected command line interface
> > for the vhost server, which uses --socket-path for, well, the socket
> > path.  Hence adding an alias to the existing passt option.
> >  
> >>>   		info(   "    default: probe free path starting from "
> >>>   		     UNIX_SOCK_PATH, 1);
> >>> +		info(   "  --vhost-user		Enable vhost-user mode");
> >>> +		info(   "    UNIX domain socket is provided by -s option");
> >>> +		info(   "  --print-capabilities	print back-end capabilities in JSON format");  
> >> Instead of introducing a new option, couldn't we have these printed
> >> unconditionally with debug()? I guess it's debug-level stuff anyway.  
> > Likewise, I think this option is expected by the thing which starts
> > the vhost server.
>
> Yes, these parameters are defined in the vhost-user protocol specification:
> 
> https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#backend-program-conventions

So, I went through the discussion from which these option names
originated:

  https://patchwork.ozlabs.org/project/qemu-devel/cover/20180713130916.4153-1-marcandre.lureau@redhat.com/#1986839

...and I don't actually see the point for our case: libvirt and krunvm
(pending) have a bit of code to start passt, and so do Podman and
rootlesskit for pasta:

  https://gitlab.com/libvirt/libvirt/-/blob/20a5f77156bb0237269008351ddf285067065516/src/qemu/qemu_passt.c#L183
  https://github.com/containers/krunvm/pull/56/commits/5fc42aa4dfde8608320e06192a7d5ec428834bb5#diff-3cd015c5feef5904a99e9a7513340c1e656649037b7442edeed2a799d242ede3R66
  https://github.com/containers/common/blob/91e0fac33e22545f6e0d99d41d315075c02576e1/libnetwork/pasta/pasta.go#L56
  https://github.com/rootless-containers/rootlesskit/blob/efee459a225b80ace3d51ae39e9b838616c3d652/pkg/network/pasta/pasta.go#L113

where option names are happily open-coded (of course!).

That is, I find rather unfeasible that any of these users would switch
to some "unified" front-end that would ever need to start passt with a
standardised command line.

Anyway, okay, the cost isn't much as you already have the code changes,
so I guess we can indulge in a bit of... compliance cult and keep all
this.

-- 
Stefano