Re: [PATCH 3/9] conf: Detect vhost-user mode earlier

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]

On Tue, Mar 11, 2025 at 11:45:03PM +0100, Stefano Brivio wrote:
> On Tue, 11 Mar 2025 17:03:12 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We detect our operating mode in conf_mode(), unless we're using vhost-user
> > mode, in which case we change it later when we parse the --vhost-user
> > option.  That means we need to delay parsing the --repair-path option (for
> > vhost-user only) until still later.
> > 
> > However, there are many other places in the main option parsing loop which
> > also rely on mode.  We get away with those, because they happen to be able
> > to treat passt and vhost-user modes identically.  This is potentially
> > confusing, though.  So, move setting of MODE_VU into conf_mode() so
> > c->mode always has its final value from that point onwards.
> > 
> > To match, we move the parsing of --repair-path back into the main option
> > parsing loop.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c | 43 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 2022ea1d..b58e2a6e 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -998,10 +998,23 @@ pasta_opts:
> >   *
> >   * Return: mode to operate in, PASTA or PASST
> >   */
> > -/* cppcheck-suppress constParameter */
> >  enum passt_modes conf_mode(int argc, char *argv[])
> >  {
> > +	int vhost_user = 0;
> > +	const struct option optvu[] = {
> > +		{"vhost-user",	no_argument,		&vhost_user,	1 },
> > +		{ 0 },
> > +	};
> >  	char argv0[PATH_MAX], *basearg0;
> > +	int name;
> > +
> > +	optind = 0;
> > +	do {
> > +		name = getopt_long(argc, argv, "-:", optvu, NULL);
> > +	} while (name != -1);
> > +
> > +	if (vhost_user)
> > +		return MODE_VU;
> >  
> >  	if (argc < 1)
> >  		die("Cannot determine argv[0]");
> > @@ -1604,9 +1617,8 @@ void conf(struct ctx *c, int argc, char **argv)
> >  
> >  			die("Invalid host nameserver address: %s", optarg);
> >  		case 25:
> > -			if (c->mode == MODE_PASTA)
> > -				die("--vhost-user is for passt mode only");
> 
> This check should now be moved to conf_mode() instead of being dropped,
> otherwise you can do:
> 
> $ ./pasta -f --vhost-user
> 
> and at this point, the mode is MODE_VU, so it's all fine, but I don't
> think it's intended (...or is it?).

It's more or less intended.  To me it seemed simpler to treat
"vhost-user mode" as co-equal with "/dev/net/tun mode" (pasta) or
"qemu -net stream mode" (passt), rather than having vu be sort of a
sub-mode of passt.  It's true that vu mode has slightly more in common
with passt mode than pasta at the moment, but I don't see that as
really inherent.

I also saw this as a precursor to a "--mode whatever" option which
would override the mode regardless of argv[0], in case there are
circumstances where manipulating argv[0] is inconvenient.

But if you'd really prefer I can reinstate the check.

> > -			c->mode = MODE_VU;
> > +			/* Already handled in conf_mode() */
> > +			ASSERT(c->mode == MODE_VU);
> >  			break;
> >  		case 26:
> >  			vu_print_capabilities();
> 
> Pre-existing, but now we can fix this: case 26 (--print-capabilities)
> should only be accepted if (c->mode == MODE_VU).

I was unsure about this, because I wasn't certain if --vhost-user was
passed when we were invoked just to probe capabilities.

> It can also be done in another patch I would say, if you don't want to
> re-spin this.
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]