Ian Campbell
2012-Sep-06 09:27 UTC
Re: [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.
Sander Eikelenboom
2012-Sep-06 09:43 UTC
Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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.
Sander Eikelenboom
2012-Sep-06 10:07 UTC
[PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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"
Sander Eikelenboom
2012-Sep-06 14:36 UTC
[PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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"
Sander Eikelenboom
2012-Sep-06 16:02 UTC
Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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"
Ian Campbell
2012-Sep-06 16:32 UTC
Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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.
Sander Eikelenboom
2012-Sep-06 16:40 UTC
Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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
Ian Campbell
2012-Sep-06 16:49 UTC
Re: [PATCH] xl: Introduce shutdown xm compatibility option -a to shutdown all domains
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.
Maybe Matching Threads
- [PATCH] [Backport 4.2.x] Fix issue with 'xl list -l' showing domids as -1 when using SXP
- [PATCH 00 of 16] libxl: autogenerate type definitions and destructor functions
- Xen 4.1 rc1 test report
- [PATCH v2] libxl: prevent xl from running if xend is running.
- [PATCH] tools: xl: on create, if debug && !daemonize, wait for domain destroy in the foreground