Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2012-09-06 at 17:02 +0100, Sander Eikelenboom wrote:
> Thursday, September 6, 2012, 4:36:27 PM, you wrote:
> 
> > xl: Introduce shutdown xm compatibility option -a to shutdown all domains
> 
> > v2: address review comments.
> >     - Change shutdown_domain to take domid instead of domname
> >     - Docs: Make it more clear -a only shuts down GUEST domains
> 
> > Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
> 
> > diff -r 9dc729b75595 -r c6d5f62c345b docs/man/xl.pod.1
> > --- a/docs/man/xl.pod.1       Mon Sep 03 11:22:02 2012 +0100
> > +++ b/docs/man/xl.pod.1       Thu Sep 06 16:35:04 2012 +0200
> > @@ -527,7 +527,7 @@ List specifically for that domain. Other
> >  
> >  =back
> >  
> > -=item B<shutdown> [I<OPTIONS>] I<domain-id>
> > +=item B<shutdown> [I<OPTIONS>] I<-a|domain-id>
> >  
> >  Gracefully shuts down a domain.  This coordinates with the domain OS
> >  to perform graceful shutdown, so there is no guarantee that it will
> > @@ -550,6 +550,10 @@ B<OPTIONS>
> >  
> >  =over 4
> >  
> > +=item B<-a>
> > +
> > +-a  Shutdown all guest domains.  Often used when doing a complete shutdown of a Xen system.
> > +
> >  =item B<-w>
> >  
> >  Wait for the domain to complete shutdown before returning.
> > diff -r 9dc729b75595 -r c6d5f62c345b tools/libxl/xl_cmdimpl.c
> > --- a/tools/libxl/xl_cmdimpl.c  Mon Sep 03 11:22:02 2012 +0100
> > +++ b/tools/libxl/xl_cmdimpl.c  Thu Sep 06 16:35:04 2012 +0200
> > @@ -2683,12 +2683,11 @@ static void destroy_domain(const char *p
> >      if (rc) { fprintf(stderr,"destroy failed (rc=%d)\n",rc); exit(-1); }
> >  }
> >  
> > -static void shutdown_domain(const char *p, int wait, int fallback_trigger)
> > +static void shutdown_domain(uint32_t domid, int wait, int fallback_trigger)
> >  {
> >      int rc;
> >      libxl_event *event;
> >  
> > -    find_domain(p);
> >      rc=libxl_domain_shutdown(ctx, domid);
> >      if (rc == ERROR_NOPARAVIRT) {
> >          if (fallback_trigger) {
> 
> Hi Ian,
> 
> Done some more testing and this seems to lead to the following error when issueing both -a and -w:
> 
> xentest:/usr/src/xen-unstable.hg# xl shutdown -a -w
> libxl: error: libxl_json.c:773:libxl__object_to_json: unable to convert libxl_event to JSON representation. YAJL error code 1: keys must be strings
> INTERNAL PROBLEM - ignoring unexpected event for domain 11 (expected -1): event=(null)
> libxl: error: libxl_json.c:773:libxl__object_to_json: unable to convert libxl_event to JSON representation. YAJL error code 1: keys must be strings
> INTERNAL PROBLEM - ignoring unexpected event for domain 11 (expected -1): event=(null)
> 
> If i only use -w and specify a specific domain, it works without a problem.
> 
> Any ideas ?

Just a guess but we have some issues in xl with local variables
shadowing global ones (which happens with domid in particular). This is
something we plan to look at in 4.3 (by enable -Wshadow and cleaning up
the mess).

I think what is happening is that shutdown_domain now takes a uint32_t
domid, which shadows the global domid but then we call domain_wait_event
which uses the global one. So when you use -a you never set the global
one because you don't need to call find_domain. Quite how this results
in the message above I'm not too sure (Ian J may have some insight to
the events subsystem)

It's all rather horrid, I think for now the best thing might be for
shutdown_domain to look like:

static void shutdown_domain(uint32_t xdomid, int wait, int fallback_trigger)
{
    [... vars...]

    domid = xdomid

    ...

Yes, this is horrible...

Ian.