Re: [Xen-devel] [PATCH] libxl: default to xenconsoled for pv guests, even if qemu is running

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 25 Apr 2012, Ian Campbell wrote:
> On Wed, 2012-04-25 at 11:36 +0100, Stefano Stabellini wrote:
> > On Wed, 25 Apr 2012, Ian Campbell wrote:
> > > # HG changeset patch
> > > # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > # Date 1335348624 -3600
> > > # Node ID c8486295429011e9a220db1b6ed9f34ba690e729
> > > # Parent  6f740f2f6e3e080e4bba9df59c826947885f6bd7
> > > libxl: default to xenconsoled for pv guests, even if qemu is running.
> > > 
> > > Currently we prefer to use qemu for the disk backend if we are starting 
> > > qemu
> > > anyway (e.g. to service a disk).
> > > 
> > > Unfortunately qemu doesn't log the console, which xenconsoled can do via
> > > XENCONSOLED_TRACE=guest. Since xenconsoled is also running anyway it 
> > > seems like
> > > there is no particular reason to prefer qemu just because it happens to be
> > > running.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > QEMU's console backend supports multiple PV consoles while xenconsoled
> > does not.
> > I think you should just change the default only in case a single PV
> > console is configured.
> > 
> > 
> > > I'm not sure if this is 4.2 material, perhaps too late to be making this 
> > > sort
> > > of change?
> > > 
> > > diff -r 6f740f2f6e3e -r c84862954290 tools/libxl/libxl_create.c
> > > --- a/tools/libxl/libxl_create.c  Wed Apr 25 11:05:05 2012 +0100
> > > +++ b/tools/libxl/libxl_create.c  Wed Apr 25 11:10:24 2012 +0100
> > > @@ -682,8 +682,7 @@ static int do_domain_create(libxl__gc *g
> > >                  d_config->num_vfbs, d_config->vfbs,
> > >                  d_config->num_disks, &d_config->disks[0]);
> > >  
> > > -        if (need_qemu)
> > > -             console.consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> > > +        console.consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> > >  
> > >          libxl__device_console_add(gc, domid, &console, &state);
> > >          libxl__device_console_dispose(&console);
> > 
> > I would change the if into:
> > 
> > if (need_qemu and nr_console > 1)
> > 
> > even though I am aware that at the moment nr_console is always 1.
> 
> There isn't actually a nr_console here, just a hardcoded 1... I could
> add a nr_console just for this but perhaps it makes more sense for
> libxl__need_xenpv_qemu to enforce this by updating consback for all
> consoles iff there are > 1 of them and then returning need_qemu as
> appropriate?
> 
> I'm a little unhappy with having need_qemu have side-effects like this,
> but it seems better than splitting the logic into two places...

I would be OK with that, maybe it is worth adding a comment on
libxl__need_xenpv_qemu to document the behavior.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel