Re: [PATCH] libs/light: pass some infos to qemu

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Jan 16, 2021 at 12:25:02PM +0100, Manuel Bouyer wrote:
> On Sat, Jan 16, 2021 at 11:16:06AM +0100, Roger Pau Monné wrote:
> > On Tue, Jan 12, 2021 at 07:12:37PM +0100, Manuel Bouyer wrote:
> > > From: Manuel Bouyer <bouyer@xxxxxxxxxx>
> > > 
> > > Pass bridge name to qemu as command line option
> > > When starting qemu, set an environnement variable XEN_DOMAIN_ID,
> > > to be used by qemu helper scripts
> > > 
> > > Signed-off-by: Manuel Bouyer <bouyer@xxxxxxxxxx>
> > > ---
> > >  tools/libs/light/libxl_dm.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> > > index 3da83259c0..8866c3f5ad 100644
> > > --- a/tools/libs/light/libxl_dm.c
> > > +++ b/tools/libs/light/libxl_dm.c
> > > @@ -761,6 +761,8 @@ static int 
> > > libxl__build_device_model_args_old(libxl__gc *gc,
> > >          int nr_set_cpus = 0;
> > >          char *s;
> > >  
> > > +        flexarray_append_pair(dm_envs, "XEN_DOMAIN_ID", GCSPRINTF("%d", 
> > > domid));
> > > +
> > >          if (b_info->kernel) {
> > >              LOGD(ERROR, domid, "HVM direct kernel boot is not supported 
> > > by "
> > >                   "qemu-xen-traditional");
> > > @@ -1547,8 +1549,10 @@ static int 
> > > libxl__build_device_model_args_new(libxl__gc *gc,
> > >                  flexarray_append(dm_args, "-netdev");
> > >                  flexarray_append(dm_args,
> > >                                   GCSPRINTF("type=tap,id=net%d,ifname=%s,"
> > > +                                    "br=%s,"
> > >                                             "script=%s,downscript=%s",
> > >                                             nics[i].devid, ifname,
> > > +                                    nics[i].bridge,
> > 
> > You have some hard tabs in there.
> 
> Yes. What's the problem ?

This file (and libxenlight) uses only spaces for indentation, it
breaks the coding style.

The line you added above uses spaces and it's fine.

> > 
> > Also looking at the manual the br= option seems to only be available
> > for the bridge networking mode, while here Xen is using tap instead?
> 
> Unless I missed something, the bridge networking mode is using the
> tap interface, to connect qemu to the bridge. And indeed, the qemu-ifup
> script is doing
> exec /sbin/brconfig $2 add $1
> 
> (the script is called with: qemu-ifup <tap if> <bridge if>)
> 
> This is a problem that hit me when I converted NetBSD to qemu-xen:
> qemu-traditional does call the qemu-ifup script with the 2 parameters,
> while qemu-xen calls it only with the tap if. So the qemu-ifup script can't
> know to which bridge the tap interface should be attached to.

OK, so the only functional difference of adding the br parameter is
that it gets passed to the script. I would add that to the commit
message:

"The only functional difference of using the br parameter is that the
bridge name gets passed to the QEMU script."

Note also that there are networking modes that don't use a bridge, so
you could likely end up with nics[i].bridge == NULL?

I also wonder why NetBSD needs to add the tap interface to the bridge
in the QEMU script instead of doing it from the hotplug script called
by libxl, like Linux and FreeBSD do.

Thanks, Roger.