Re: [Xen-devel] [PATCH V2] libxl: Set path to console on domain startup.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Dec 16, 2014 at 09:30:28AM +0000, Ian Campbell wrote:
> On Mon, 2014-12-15 at 17:07 +0000, Anthony PERARD wrote:
> > On Thu, Dec 11, 2014 at 04:23:15PM +0000, Ian Campbell wrote:
> > > On Thu, 2014-12-11 at 16:16 +0000, Anthony PERARD wrote:
> > > > On Tue, Dec 09, 2014 at 03:56:02PM +0000, Ian Campbell wrote:
> > > > > On Tue, 2014-12-09 at 15:39 +0000, Anthony PERARD wrote:
> > > > > > The path to the pty of a Xen PV console is set only in
> > > > > > virDomainOpenConsole. But this is done too late. A call to
> > > > > > virDomainGetXMLDesc done before OpenConsole will not have the path 
> > > > > > to
> > > > > > the pty, but a call after OpenConsole will.
> > > > > > 
> > > > > > e.g. of the current issue.
> > > > > > Starting a domain with '<console type="pty"/>'
> > > > > > Then:
> > > > > > virDomainGetXMLDesc():
> > > > > >   <devices>
> > > > > >     <console type='pty'>
> > > > > >       <target type='xen' port='0'/>
> > > > > >     </console>
> > > > > >   </devices>
> > > > > > virDomainOpenConsole()
> > > > > > virDomainGetXMLDesc():
> > > > > >   <devices>
> > > > > >     <console type='pty' tty='/dev/pts/30'>
> > > > > >       <source path='/dev/pts/30'/>
> > > > > >       <target type='xen' port='0'/>
> > > > > >     </console>
> > > > > >   </devices>
> > > > > > 
> > > > > > The patch intend to have the TTY path on the first call of 
> > > > > > GetXMLDesc.
> > > > > > This is done by setting up the path at domain start up instead of in
> > > > > > OpenConsole.
> > > > > > 
> > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1170743
> > > > > > 
> > > > > > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > > > > > 
> > > > > > ---
> > > > > > Change in V2:
> > > > > >   Adding bug report link.
> > > > > >   Reword the last part of the patch description.
> > > > > >   Cleanup the code.
> > > > > >   Use VIR_FREE before VIR_STRDUP.
> > > > > >   Remove the code from OpenConsole as it is now a duplicate.
> > > > > > ---
> > > > > >  src/libxl/libxl_domain.c | 20 ++++++++++++++++++++
> > > > > >  src/libxl/libxl_driver.c | 15 ---------------
> > > > > >  2 files changed, 20 insertions(+), 15 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > > > > > index 9c62291..325de79 100644
> > > > > > --- a/src/libxl/libxl_domain.c
> > > > > > +++ b/src/libxl/libxl_domain.c
> > > > > > @@ -1290,6 +1290,26 @@ libxlDomainStart(libxlDriverPrivatePtr 
> > > > > > driver, virDomainObjPtr vm,
> > > > > >      if (libxlDomainSetVcpuAffinities(driver, vm) < 0)
> > > > > >          goto cleanup_dom;
> > > > > >  
> > > > > > +    if (vm->def->nconsoles) {
> > > > > > +        virDomainChrDefPtr chr = vm->def->consoles[0];
> > > > > 
> > > > > Given vm->def->nconsoles should we loop and do them all?
> > > > 
> > > > Maybe. libvirt it self will not be able to access any console that is
> > > > not the first one (virDomainOpenConsole only provide access to console
> > > > 0), but a consumer of libvirt could.
> > > > 
> > > > > Also, and I really should know the answer to this (and sorry for not
> > > > > thinking of it earlier), but:
> > > > > 
> > > > > > +            ret = libxl_console_get_tty(priv->ctx, vm->def->id,
> > > > > > +                                        chr->target.port, 
> > > > > > console_type,
> > > > > > +                                        &console);
> > > > > 
> > > > > Might this race against xenconsoled writing the node to xenstore and
> > > > > therefore be prone to failing when starting multiple guests all at 
> > > > > once?
> > > > 
> > > > I've look through out libxl, xenconsoled and libvirt, and I could not
> > > > find any synchronisation point. So I guest it's possible.
> > > > 
> > > > > Is there a hook which is called on virsh dumpxml which could update
> > > > > things on the fly (i.e. lookup the console iff it isn't already set)?
> > > > 
> > > > I guest we could modify libxlDomainGetXMLDesc and libxlDomainOpenConsole
> > > > to do the check, or having a xenstore watch on the path (if that is
> > > > possible).
> > > 
> > > The aop_console_how option to libxl_domain_create_new and
> > > libxl_domain_create_restore is documented as:
> > > 
> > >   /* A progress report will be made via ao_console_how, of type
> > >    * domain_create_console_available, when the domain's primary
> > >    * console is available and can be connected to.
> > >    */
> > > 
> > > Which sounds like exactly what is needed?
> > 
> > It looks like the progress is reported before the main function finish,
> > from the description of the type libxl_asyncprogress_how (in libxl.h).
> 
> Correct.
> 
> > Also, I tryied to use it, it works if xenconsoled is running. But if
> > xenconsoled is not running, then the callback is also called and
> > libxl_console_get_tty() return an empty string.
> 
> I'm not sure xenconsoled not running is a configuration we need to worry
> about, or at least it could be expected not to get a console in that
> case.
> 
> Unless by "not running" you meant bottlenecked or not keeping up
> perhaps.

Well, I did meant no xenconsoled process. But after, I also tried `kill
-STOP`, but the same things is happening.

> > Or, maybe my test case is not relevant, so another question: Will
> > libxl wait for xenconsoled to process the new domain before calling the
> > callback?
> 
> I don't see an explicit wait in the code, but I don't know if it has
> arranged the sequencing some other way.
> 
> > Or, should I set the callback to NULL and have the
> > domain_create_console_available event sent through
> > the callback set by libxl_event_register_callbacks()?
> 
> That would make sense, except I don't see libxl_evdisable_foo for these
> events, so I'm not sure it is possible.

Well, the domain_create_console_available event is report by
libxl__ao_progress_report which will either callback() or call
libxl__event_occurred(). So does not seams better to set callback to
NULL.

So, from this, I think I'm going to stick with the original patch and
add some hooks in GetXMLDesc and OpenConsole.

-- 
Anthony PERARD

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