* [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
@ 2012-09-06 10:07 Sander Eikelenboom
2012-09-06 9:27 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Sander Eikelenboom @ 2012-09-06 10:07 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Campbell
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"
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 10:07 [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains Sander Eikelenboom
@ 2012-09-06 9:27 ` Ian Campbell
2012-09-06 9:43 ` Sander Eikelenboom
0 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2012-09-06 9:27 UTC (permalink / raw)
To: Sander Eikelenboom; +Cc: xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 9:27 ` Ian Campbell
@ 2012-09-06 9:43 ` Sander Eikelenboom
0 siblings, 0 replies; 8+ messages in thread
From: Sander Eikelenboom @ 2012-09-06 9:43 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
@ 2012-09-06 14:36 Sander Eikelenboom
2012-09-06 16:02 ` Sander Eikelenboom
0 siblings, 1 reply; 8+ messages in thread
From: Sander Eikelenboom @ 2012-09-06 14:36 UTC (permalink / raw)
To: xen-devel; +Cc: Ian.Campbell
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"
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 14:36 Sander Eikelenboom
@ 2012-09-06 16:02 ` Sander Eikelenboom
2012-09-06 16:32 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Sander Eikelenboom @ 2012-09-06 16:02 UTC (permalink / raw)
To: Sander Eikelenboom; +Cc: xen-devel, Ian.Campbell
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"
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 16:02 ` Sander Eikelenboom
@ 2012-09-06 16:32 ` Ian Campbell
2012-09-06 16:40 ` Sander Eikelenboom
0 siblings, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2012-09-06 16:32 UTC (permalink / raw)
To: Sander Eikelenboom; +Cc: xen-devel@lists.xensource.com, Ian Jackson
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 16:32 ` Ian Campbell
@ 2012-09-06 16:40 ` Sander Eikelenboom
2012-09-06 16:49 ` Ian Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Sander Eikelenboom @ 2012-09-06 16:40 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com, Ian Jackson
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
2012-09-06 16:40 ` Sander Eikelenboom
@ 2012-09-06 16:49 ` Ian Campbell
0 siblings, 0 replies; 8+ messages in thread
From: Ian Campbell @ 2012-09-06 16:49 UTC (permalink / raw)
To: Sander Eikelenboom; +Cc: xen-devel@lists.xensource.com, Ian Jackson
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-09-06 16:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-06 10:07 [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains Sander Eikelenboom
2012-09-06 9:27 ` Ian Campbell
2012-09-06 9:43 ` Sander Eikelenboom
-- strict thread matches above, loose matches on Subject: below --
2012-09-06 14:36 Sander Eikelenboom
2012-09-06 16:02 ` Sander Eikelenboom
2012-09-06 16:32 ` Ian Campbell
2012-09-06 16:40 ` Sander Eikelenboom
2012-09-06 16:49 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).