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

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

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...

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

--
Sander




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