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

On Thu, 2012-09-06 at 11:07 +0100, Sander Eikelenboom
wrote:
> docs/man/xl.pod.1         |   6 +++++-
>  tools/libxl/xl_cmdimpl.c  |  39 ++++++++++++++++++++++++++++++++++++---
>  tools/libxl/xl_cmdtable.c |   3 ++-
>  3 files changed, 43 insertions(+), 5 deletions(-)
> 
> 
> xl: Introduce shutdown xm compatibility option -a to shutdown all domains
> 
> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
Thanks Sander, however I''m afraid this comes too late for 4.2.0 (it was
always going to be tight). We should take it for 4.2.1 though.
> diff -r 9dc729b75595 -r 67f9ef649937 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 12:04:12 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<domain-id>]
Perhaps write this as "I<-a|domain-id>" to make it clear one or
the
other is needed?
>  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 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 67f9ef649937 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 12:04:12 2012 +0200
> @@ -3670,14 +3670,20 @@ int main_destroy(int argc, char **argv)
>  
>  int main_shutdown(int argc, char **argv)
>  {
> -    int opt;
> +    libxl_dominfo *dominfo;
> +    char *domname;
> +    int opt, i, nb_domain;
> +    int all = 0;
>      int wait = 0;
>      int fallback_trigger = 0;
>  
> -    while ((opt = def_getopt(argc, argv, "wF",
"shutdown", 1)) != -1) {
> +    while ((opt = def_getopt(argc, argv, "awF",
"shutdown", 0)) != -1) {
>          switch (opt) {
>          case 0: case 2:
>              return opt;
> +        case ''a'':
> +            all = 1;
> +            break;
>          case ''w'':
>              wait = 1;
>              break;
> @@ -3687,7 +3693,34 @@ int main_shutdown(int argc, char **argv)
>          }
>      }
>  
> -    shutdown_domain(argv[optind], wait, fallback_trigger);
> +    if (!argv[optind] && !all) {
> +        fprintf(stderr, "You must specify -a or a domain
id.\n\n");
> +        return opt;
> +    }
> +
> +    if (all) {
> +        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
> +            fprintf(stderr, "libxl_list_domain failed.\n");
> +            goto main_shutdown_out;
return -1 here, main_shutdown_out reurns 0 AKA success.
> +        }
> +
> +        for (i = 0; i<nb_domain; i++) {
> +            if (dominfo[i].domid == 0)
> +                continue;
> +
> +            domname = libxl_domid_to_name(ctx, dominfo[i].domid);
> +            if (domname)
> +                shutdown_domain(domname, wait, fallback_trigger);
> +
> +	    free(domname);
> +        }
> +
> +        libxl_dominfo_list_free(dominfo, nb_domain);
> +    } else {
> +        shutdown_domain(argv[optind], wait, fallback_trigger);
> +    }
Can you make shutdown_domain take a domid instead, which avoids the
needless libxl_domid_to_name->libxl_name_to_domid laundering in the -a
case.

Otherwise the patch looks good, thanks!

Thanks,
Ian.

Thursday, September 6, 2012, 11:27:08 AM, you wrote:
> On Thu, 2012-09-06 at 11:07 +0100, Sander Eikelenboom wrote:
>> docs/man/xl.pod.1         |   6 +++++-
>>  tools/libxl/xl_cmdimpl.c  |  39
++++++++++++++++++++++++++++++++++++---
>>  tools/libxl/xl_cmdtable.c |   3 ++-
>>  3 files changed, 43 insertions(+), 5 deletions(-)
>> 
>> 
>> xl: Introduce shutdown xm compatibility option -a to shutdown all
domains
>> 
>> Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>
> Thanks Sander, however I''m afraid this comes too late for 4.2.0
(it was
> always going to be tight). We should take it for 4.2.1 though.
Ok, although init.d/xendomains in combination with the default
sysconfig-xendomains scripts use this (needs another patch to use the short -a
instead of --all
Will respin with the changes below !

>> diff -r 9dc729b75595 -r 67f9ef649937 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 12:04:12 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<domain-id>]
> Perhaps write this as "I<-a|domain-id>" to make it clear
one or the
> other is needed?
>>  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 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 67f9ef649937 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 12:04:12 2012 +0200
>> @@ -3670,14 +3670,20 @@ int main_destroy(int argc, char **argv)
>>  
>>  int main_shutdown(int argc, char **argv)
>>  {
>> -    int opt;
>> +    libxl_dominfo *dominfo;
>> +    char *domname;
>> +    int opt, i, nb_domain;
>> +    int all = 0;
>>      int wait = 0;
>>      int fallback_trigger = 0;
>>  
>> -    while ((opt = def_getopt(argc, argv, "wF",
"shutdown", 1)) != -1) {
>> +    while ((opt = def_getopt(argc, argv, "awF",
"shutdown", 0)) != -1) {
>>          switch (opt) {
>>          case 0: case 2:
>>              return opt;
>> +        case ''a'':
>> +            all = 1;
>> +            break;
>>          case ''w'':
>>              wait = 1;
>>              break;
>> @@ -3687,7 +3693,34 @@ int main_shutdown(int argc, char **argv)
>>          }
>>      }
>>  
>> -    shutdown_domain(argv[optind], wait, fallback_trigger);
>> +    if (!argv[optind] && !all) {
>> +        fprintf(stderr, "You must specify -a or a domain
id.\n\n");
>> +        return opt;
>> +    }
>> +
>> +    if (all) {
>> +        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
>> +            fprintf(stderr, "libxl_list_domain failed.\n");
>> +            goto main_shutdown_out;
> return -1 here, main_shutdown_out reurns 0 AKA success.
>> +        }
>> +
>> +        for (i = 0; i<nb_domain; i++) {
>> +            if (dominfo[i].domid == 0)
>> +                continue;
>> +
>> +            domname = libxl_domid_to_name(ctx, dominfo[i].domid);
>> +            if (domname)
>> +                shutdown_domain(domname, wait, fallback_trigger);
>> +
>> +         free(domname);
>> +        }
>> +
>> +        libxl_dominfo_list_free(dominfo, nb_domain);
>> +    } else {
>> +        shutdown_domain(argv[optind], wait, fallback_trigger);
>> +    }
> Can you make shutdown_domain take a domid instead, which avoids the
> needless libxl_domid_to_name->libxl_name_to_domid laundering in the -a
> case.
> Otherwise the patch looks good, thanks!
> Thanks,
> Ian.

docs/man/xl.pod.1         |   6 +++++-
 tools/libxl/xl_cmdimpl.c  |  39 ++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl_cmdtable.c |   3 ++-
 3 files changed, 43 insertions(+), 5 deletions(-)


xl: Introduce shutdown xm compatibility option -a to shutdown all domains

Signed-off-by: Sander Eikelenboom <linux@eikelenboom.it>

diff -r 9dc729b75595 -r 67f9ef649937 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 12:04:12 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<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 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 67f9ef649937 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 12:04:12 2012 +0200
@@ -3670,14 +3670,20 @@ int main_destroy(int argc, char **argv)
 
 int main_shutdown(int argc, char **argv)
 {
-    int opt;
+    libxl_dominfo *dominfo;
+    char *domname;
+    int opt, i, nb_domain;
+    int all = 0;
     int wait = 0;
     int fallback_trigger = 0;
 
-    while ((opt = def_getopt(argc, argv, "wF", "shutdown",
1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "awF", "shutdown",
0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case ''a'':
+            all = 1;
+            break;
         case ''w'':
             wait = 1;
             break;
@@ -3687,7 +3693,34 @@ int main_shutdown(int argc, char **argv)
         }
     }
 
-    shutdown_domain(argv[optind], wait, fallback_trigger);
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
+            fprintf(stderr, "libxl_list_domain failed.\n");
+            goto main_shutdown_out;
+        }
+
+        for (i = 0; i<nb_domain; i++) {
+            if (dominfo[i].domid == 0)
+                continue;
+
+            domname = libxl_domid_to_name(ctx, dominfo[i].domid);
+            if (domname)
+                shutdown_domain(domname, wait, fallback_trigger);
+
+	    free(domname);
+        }
+
+        libxl_dominfo_list_free(dominfo, nb_domain);
+    } else {
+        shutdown_domain(argv[optind], wait, fallback_trigger);
+    }
+
+main_shutdown_out:
     return 0;
 }
 
diff -r 9dc729b75595 -r 67f9ef649937 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Mon Sep 03 11:22:02 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Sep 06 12:04:12 2012 +0200
@@ -60,7 +60,8 @@ struct cmd_spec cmd_table[] = {
     { "shutdown",
       &main_shutdown, 0, 1,
       "Issue a shutdown signal to a domain",
-      "[options] <Domain>",
+      "[options] [Domain]",
+      "-a                      Shutdown all domains.\n"
       "-h                      Print this help.\n"
       "-F                      Fallback to ACPI power event for HVM guests
with\n"
       "                        no PV drivers.\n"

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) {
@@ -3670,14 +3669,19 @@ int main_destroy(int argc, char **argv)
 
 int main_shutdown(int argc, char **argv)
 {
-    int opt;
+    libxl_dominfo *dominfo;
+    int opt, i, nb_domain;
+    int all = 0;
     int wait = 0;
     int fallback_trigger = 0;
 
-    while ((opt = def_getopt(argc, argv, "wF", "shutdown",
1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "awF", "shutdown",
0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
+        case ''a'':
+            all = 1;
+            break;
         case ''w'':
             wait = 1;
             break;
@@ -3687,7 +3691,30 @@ int main_shutdown(int argc, char **argv)
         }
     }
 
-    shutdown_domain(argv[optind], wait, fallback_trigger);
+    if (!argv[optind] && !all) {
+        fprintf(stderr, "You must specify -a or a domain id.\n\n");
+        return opt;
+    }
+
+    if (all) {
+        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
+            fprintf(stderr, "libxl_list_domain failed.\n");
+            return -1;
+        }
+
+        for (i = 0; i<nb_domain; i++) {
+            if (dominfo[i].domid == 0)
+                continue;
+
+            shutdown_domain(dominfo[i].domid, wait, fallback_trigger);
+        }
+
+        libxl_dominfo_list_free(dominfo, nb_domain);
+    } else {
+        find_domain(argv[optind]);
+        shutdown_domain(domid, wait, fallback_trigger);
+    }
+
     return 0;
 }
 
diff -r 9dc729b75595 -r c6d5f62c345b tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Mon Sep 03 11:22:02 2012 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Sep 06 16:35:04 2012 +0200
@@ -60,7 +60,8 @@ struct cmd_spec cmd_table[] = {
     { "shutdown",
       &main_shutdown, 0, 1,
       "Issue a shutdown signal to a domain",
-      "[options] <Domain>",
+      "[options] <-a|Domain>",
+      "-a                      Shutdown all guest domains.\n"
       "-h                      Print this help.\n"
       "-F                      Fallback to ACPI power event for HVM guests
with\n"
       "                        no PV drivers.\n"

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 ?

--
Sander

> @@ -3670,14 +3669,19 @@ int main_destroy(int argc, char **argv)
>  
>  int main_shutdown(int argc, char **argv)
>  {
> -    int opt;
> +    libxl_dominfo *dominfo;
> +    int opt, i, nb_domain;
> +    int all = 0;
>      int wait = 0;
>      int fallback_trigger = 0;
>  
> -    while ((opt = def_getopt(argc, argv, "wF",
"shutdown", 1)) != -1) {
> +    while ((opt = def_getopt(argc, argv, "awF",
"shutdown", 0)) != -1) {
>          switch (opt) {
>          case 0: case 2:
>              return opt;
> +        case ''a'':
> +            all = 1;
> +            break;
>          case ''w'':
>              wait = 1;
>              break;
> @@ -3687,7 +3691,30 @@ int main_shutdown(int argc, char **argv)
>          }
>      }
>  
> -    shutdown_domain(argv[optind], wait, fallback_trigger);
> +    if (!argv[optind] && !all) {
> +        fprintf(stderr, "You must specify -a or a domain
id.\n\n");
> +        return opt;
> +    }
> +
> +    if (all) {
> +        if (!(dominfo = libxl_list_domain(ctx, &nb_domain))) {
> +            fprintf(stderr, "libxl_list_domain failed.\n");
> +            return -1;
> +        }
> +
> +        for (i = 0; i<nb_domain; i++) {
> +            if (dominfo[i].domid == 0)
> +                continue;
> +
> +            shutdown_domain(dominfo[i].domid, wait, fallback_trigger);
> +        }
> +
> +        libxl_dominfo_list_free(dominfo, nb_domain);
> +    } else {
> +        find_domain(argv[optind]);
> +        shutdown_domain(domid, wait, fallback_trigger);
> +    }
> +
>      return 0;
>  }
>  
> diff -r 9dc729b75595 -r c6d5f62c345b tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c       Mon Sep 03 11:22:02 2012 +0100
> +++ b/tools/libxl/xl_cmdtable.c       Thu Sep 06 16:35:04 2012 +0200
> @@ -60,7 +60,8 @@ struct cmd_spec cmd_table[] = {
>      { "shutdown",
>        &main_shutdown, 0, 1,
>        "Issue a shutdown signal to a domain",
> -      "[options] <Domain>",
> +      "[options] <-a|Domain>",
> +      "-a                      Shutdown all guest domains.\n"
>        "-h                      Print this help.\n"
>        "-F                      Fallback to ACPI power event for HVM
guests with\n"
>        "                        no PV drivers.\n"

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.

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

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