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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-09-06 at 17:40 +0100, Sander Eikelenboom wrote:
> Thursday, September 6, 2012, 6:32:17 PM, you wrote:
> 
> > 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@xxxxxxxxxxxxxx>
> >> 
> >> > 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.
> 
> I was quite puzzled that global variables were used, took me quite a while 
> searching how domid could be available without being explicitly set.
> I always thought of global variables as being something pretty undesired...

Me too, and here we see why ;-)

> Is naming it "xdomid" ok ? Or would be naming it uint32_t domain_id be better 
> ?

There's not much in the way of precedent. I see a tdomid but I'm not
sure what the t is for.

This'll all have to get reworked when we come to enable Wshadow anyway
so I don't think the name here matters too much.

The other option would be to remove the domid parameter altogether and
do domid = info[i].domid in the caller. That's pretty nasty though!

Ian.



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