| Message ID | 1392215257-26993-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 3ac3817762d1a8b39fa45998ec8c40cabfcfc802 |
| Headers | show |
Typoed George's address... On Wed, 2014-02-12 at 14:27 +0000, Ian Campbell wrote: > ARM does not (currently) support migration, so stop offering tasty looking > treats like "xl migrate". > > Apart from the UI improvement my intention is to use this in osstest to detect > whether to attempt the save/restore/migrate tests. > > Other than the additions of the #define/#ifdef there is a tiny bit of code > motion ("dump-core" in the command list and core_dump_domain in the > implementations) which serves to put ifdeffable bits next to each other. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: goerge.dunlap@citrix.com > --- > Release: > My main motivation here is to be able to get a complete osstest run on armhf > prior to Xen 4.4 and the lack of migration support is currently blocking > that (fine) but is also blocking subsequent useful tests. This change will > allow me to make osstest skip the unsupported functionality, and in a way where > it will automatically start trying to test it as soon as it is implemented. > --- > tools/libxl/libxl.h | 14 ++++++++++++++ > tools/libxl/xl.h | 4 ++++ > tools/libxl/xl_cmdimpl.c | 20 ++++++++++++-------- > tools/libxl/xl_cmdtable.c | 15 +++++++++------ > 4 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 0b992d1..06bbca6 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -431,6 +431,20 @@ > */ > #define LIBXL_HAVE_SIGCHLD_SHARING 1 > > +/* > + * LIBXL_HAVE_NO_SUSPEND_RESUME > + * > + * Is this is defined then the platform has no support for saving, > + * restoring or migrating a domain. In this case the related functions > + * should be expected to return failure. That is: > + * - libxl_domain_suspend > + * - libxl_domain_resume > + * - libxl_domain_remus_start > + */ > +#if defined(__arm__) || defined(__aarch64__) > +#define LIBXL_HAVE_NO_SUSPEND_RESUME 1 > +#endif > + > /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be > * called from within libxl itself. Callers outside libxl, who > * do not #include libxl_internal.h, are fine. */ > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h > index c876a33..f188708 100644 > --- a/tools/libxl/xl.h > +++ b/tools/libxl/xl.h > @@ -43,10 +43,12 @@ int main_pciattach(int argc, char **argv); > int main_pciassignable_add(int argc, char **argv); > int main_pciassignable_remove(int argc, char **argv); > int main_pciassignable_list(int argc, char **argv); > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > int main_restore(int argc, char **argv); > int main_migrate_receive(int argc, char **argv); > int main_save(int argc, char **argv); > int main_migrate(int argc, char **argv); > +#endif > int main_dump_core(int argc, char **argv); > int main_pause(int argc, char **argv); > int main_unpause(int argc, char **argv); > @@ -104,7 +106,9 @@ int main_cpupoolnumasplit(int argc, char **argv); > int main_getenforce(int argc, char **argv); > int main_setenforce(int argc, char **argv); > int main_loadpolicy(int argc, char **argv); > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > int main_remus(int argc, char **argv); > +#endif > int main_devd(int argc, char **argv); > > void help(const char *command); > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index aff6f90..4fc46eb 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -3384,6 +3384,15 @@ static void list_vm(void) > libxl_vminfo_list_free(info, nb_vm); > } > > +static void core_dump_domain(uint32_t domid, const char *filename) > +{ > + int rc; > + > + rc=libxl_domain_core_dump(ctx, domid, filename, NULL); > + if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); } > +} > + > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > static void save_domain_core_begin(uint32_t domid, > const char *override_config_file, > uint8_t **config_data_r, > @@ -3775,14 +3784,6 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug, > exit(-ERROR_BADFAIL); > } > > -static void core_dump_domain(uint32_t domid, const char *filename) > -{ > - int rc; > - > - rc=libxl_domain_core_dump(ctx, domid, filename, NULL); > - if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); } > -} > - > static void migrate_receive(int debug, int daemonize, int monitor, > int send_fd, int recv_fd, int remus) > { > @@ -4102,6 +4103,7 @@ int main_migrate(int argc, char **argv) > migrate_domain(domid, rune, debug, config_filename); > return 0; > } > +#endif > > int main_dump_core(int argc, char **argv) > { > @@ -7248,6 +7250,7 @@ done: > return ret; > } > > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > int main_remus(int argc, char **argv) > { > uint32_t domid; > @@ -7341,6 +7344,7 @@ int main_remus(int argc, char **argv) > close(send_fd); > return -ERROR_FAIL; > } > +#endif > > int main_devd(int argc, char **argv) > { > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c > index ebe0220..e8ab93a 100644 > --- a/tools/libxl/xl_cmdtable.c > +++ b/tools/libxl/xl_cmdtable.c > @@ -137,6 +137,7 @@ struct cmd_spec cmd_table[] = { > " -autopass\n" > "--vncviewer-autopass (consistency alias for --autopass)" > }, > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > { "save", > &main_save, 0, 1, > "Save a domain state to restore later", > @@ -158,11 +159,6 @@ struct cmd_spec cmd_table[] = { > " of the domain.\n" > "--debug Print huge (!) amount of debug during the migration process." > }, > - { "dump-core", > - &main_dump_core, 0, 1, > - "Core dump a domain", > - "<Domain> <filename>" > - }, > { "restore", > &main_restore, 0, 1, > "Restore a domain from a saved state", > @@ -179,6 +175,12 @@ struct cmd_spec cmd_table[] = { > "Restore a domain from a saved state", > "- for internal use only", > }, > +#endif > + { "dump-core", > + &main_dump_core, 0, 1, > + "Core dump a domain", > + "<Domain> <filename>" > + }, > { "cd-insert", > &main_cd_insert, 1, 1, > "Insert a cdrom into a guest's cd drive", > @@ -474,6 +476,7 @@ struct cmd_spec cmd_table[] = { > "Loads a new policy int the Flask Xen security module", > "<policy file>", > }, > +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME > { "remus", > &main_remus, 0, 1, > "Enable Remus HA for domain", > @@ -486,8 +489,8 @@ struct cmd_spec cmd_table[] = { > " ssh <host> xl migrate-receive -r [-e]\n" > "-e Do not wait in the background (on <host>) for the death\n" > " of the domain." > - > }, > +#endif > { "devd", > &main_devd, 0, 1, > "Daemon that listens for devices and launches backends",
On Wed, 2014-02-12 at 16:21 +0100, Olaf Hering wrote: > On Wed, Feb 12, Ian Campbell wrote: > > > #define LIBXL_HAVE_SIGCHLD_SHARING 1 > > > > +/* > > + * LIBXL_HAVE_NO_SUSPEND_RESUME > > Think positive? > Make that HAVE_FEATURE and define it on x86. Could do -- anyone got any strong feeling one way or the other?
On Wed, 2014-02-12 at 16:49 +0000, George Dunlap wrote: > On 02/12/2014 03:57 PM, Ian Campbell wrote: > > On Wed, 2014-02-12 at 16:21 +0100, Olaf Hering wrote: > >> On Wed, Feb 12, Ian Campbell wrote: > >> > >>> #define LIBXL_HAVE_SIGCHLD_SHARING 1 > >>> > >>> +/* > >>> + * LIBXL_HAVE_NO_SUSPEND_RESUME > >> Think positive? > >> Make that HAVE_FEATURE and define it on x86. > > Could do -- anyone got any strong feeling one way or the other? > > I'm a fan of consistency, so "HAVE_SUSPEND" sounds a little bit better > to me. But either way is fine. Right. I think I'll flip it then, it does make more logical sense that way. Ian.
On Wed, 2014-02-12 at 16:54 +0000, Ian Campbell wrote: > On Wed, 2014-02-12 at 16:49 +0000, George Dunlap wrote: > > On 02/12/2014 03:57 PM, Ian Campbell wrote: > > > On Wed, 2014-02-12 at 16:21 +0100, Olaf Hering wrote: > > >> On Wed, Feb 12, Ian Campbell wrote: > > >> > > >>> #define LIBXL_HAVE_SIGCHLD_SHARING 1 > > >>> > > >>> +/* > > >>> + * LIBXL_HAVE_NO_SUSPEND_RESUME > > >> Think positive? > > >> Make that HAVE_FEATURE and define it on x86. > > > Could do -- anyone got any strong feeling one way or the other? > > > > I'm a fan of consistency, so "HAVE_SUSPEND" sounds a little bit better > > to me. But either way is fine. > > Right. I think I'll flip it then, it does make more logical sense that > way. Actually, no I wont. Anyone who uses the positive version of this will find that migration support suddenly disappears on x86 when they build against Xen 4.3 or earlier, unless they are careful to use #if defined(LIBL_HAVE_NO_SUSPEND_RESUME) || defined(__i386__) || defined(__x86_64__) which is worse than a -ve feature flag IMHO. I will stick with LIBXL_HAVE_NO_SUSPEND_RESUME then. Ian.
On Wed, 2014-02-12 at 14:54 +0000, Ian Jackson wrote: > Ian Campbell writes ("Re: [PATCH] xl: suppress suspend/resume functions on platforms which do not support it."): > > On Wed, 2014-02-12 at 14:27 +0000, Ian Campbell wrote: > > > ARM does not (currently) support migration, so stop offering tasty looking > > > treats like "xl migrate". > > > > Other than the additions of the #define/#ifdef there is a tiny bit of code > > > motion ("dump-core" in the command list and core_dump_domain in the > > > implementations) which serves to put ifdeffable bits next to each other. > > I'm not a huge fan of #ifdef but this is tolerable, I think. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > I think this should go into 4.4. It is essential that we start > advertising lack-of-resume in 4.4 as otherwise in 4.5 we'll have to > invent a new HAVE_HAVE_NO_SUSPEND_RESUME which tells you whether the > lack of HAVE_NO_SUSPEND_RESUME means that you can definitely > suspend/resume. George release acked it and so I have pushed. I'll coordinate with you re the osstest push once this passes the xen gate. Ian.
On Fri, 2014-02-14 at 13:42 +0800, Lai Jiangshan wrote: > On 02/12/2014 10:54 PM, Ian Jackson wrote: > > Ian Campbell writes ("Re: [PATCH] xl: suppress suspend/resume functions on platforms which do not support it."): > >> On Wed, 2014-02-12 at 14:27 +0000, Ian Campbell wrote: > >>> ARM does not (currently) support migration, so stop offering tasty looking > >>> treats like "xl migrate". > > > >>> Other than the additions of the #define/#ifdef there is a tiny bit of code > >>> motion ("dump-core" in the command list and core_dump_domain in the > >>> implementations) which serves to put ifdeffable bits next to each other. > > > > I'm not a huge fan of #ifdef but this is tolerable, I think. > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > Also > > Acked-by: Lai Jiangshan <laijs@cn.fujitsu.com> Thanks. This patch was already committed. Ian.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 0b992d1..06bbca6 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -431,6 +431,20 @@ */ #define LIBXL_HAVE_SIGCHLD_SHARING 1 +/* + * LIBXL_HAVE_NO_SUSPEND_RESUME + * + * Is this is defined then the platform has no support for saving, + * restoring or migrating a domain. In this case the related functions + * should be expected to return failure. That is: + * - libxl_domain_suspend + * - libxl_domain_resume + * - libxl_domain_remus_start + */ +#if defined(__arm__) || defined(__aarch64__) +#define LIBXL_HAVE_NO_SUSPEND_RESUME 1 +#endif + /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be * called from within libxl itself. Callers outside libxl, who * do not #include libxl_internal.h, are fine. */ diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h index c876a33..f188708 100644 --- a/tools/libxl/xl.h +++ b/tools/libxl/xl.h @@ -43,10 +43,12 @@ int main_pciattach(int argc, char **argv); int main_pciassignable_add(int argc, char **argv); int main_pciassignable_remove(int argc, char **argv); int main_pciassignable_list(int argc, char **argv); +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME int main_restore(int argc, char **argv); int main_migrate_receive(int argc, char **argv); int main_save(int argc, char **argv); int main_migrate(int argc, char **argv); +#endif int main_dump_core(int argc, char **argv); int main_pause(int argc, char **argv); int main_unpause(int argc, char **argv); @@ -104,7 +106,9 @@ int main_cpupoolnumasplit(int argc, char **argv); int main_getenforce(int argc, char **argv); int main_setenforce(int argc, char **argv); int main_loadpolicy(int argc, char **argv); +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME int main_remus(int argc, char **argv); +#endif int main_devd(int argc, char **argv); void help(const char *command); diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index aff6f90..4fc46eb 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -3384,6 +3384,15 @@ static void list_vm(void) libxl_vminfo_list_free(info, nb_vm); } +static void core_dump_domain(uint32_t domid, const char *filename) +{ + int rc; + + rc=libxl_domain_core_dump(ctx, domid, filename, NULL); + if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); } +} + +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME static void save_domain_core_begin(uint32_t domid, const char *override_config_file, uint8_t **config_data_r, @@ -3775,14 +3784,6 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug, exit(-ERROR_BADFAIL); } -static void core_dump_domain(uint32_t domid, const char *filename) -{ - int rc; - - rc=libxl_domain_core_dump(ctx, domid, filename, NULL); - if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); } -} - static void migrate_receive(int debug, int daemonize, int monitor, int send_fd, int recv_fd, int remus) { @@ -4102,6 +4103,7 @@ int main_migrate(int argc, char **argv) migrate_domain(domid, rune, debug, config_filename); return 0; } +#endif int main_dump_core(int argc, char **argv) { @@ -7248,6 +7250,7 @@ done: return ret; } +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME int main_remus(int argc, char **argv) { uint32_t domid; @@ -7341,6 +7344,7 @@ int main_remus(int argc, char **argv) close(send_fd); return -ERROR_FAIL; } +#endif int main_devd(int argc, char **argv) { diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c index ebe0220..e8ab93a 100644 --- a/tools/libxl/xl_cmdtable.c +++ b/tools/libxl/xl_cmdtable.c @@ -137,6 +137,7 @@ struct cmd_spec cmd_table[] = { " -autopass\n" "--vncviewer-autopass (consistency alias for --autopass)" }, +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME { "save", &main_save, 0, 1, "Save a domain state to restore later", @@ -158,11 +159,6 @@ struct cmd_spec cmd_table[] = { " of the domain.\n" "--debug Print huge (!) amount of debug during the migration process." }, - { "dump-core", - &main_dump_core, 0, 1, - "Core dump a domain", - "<Domain> <filename>" - }, { "restore", &main_restore, 0, 1, "Restore a domain from a saved state", @@ -179,6 +175,12 @@ struct cmd_spec cmd_table[] = { "Restore a domain from a saved state", "- for internal use only", }, +#endif + { "dump-core", + &main_dump_core, 0, 1, + "Core dump a domain", + "<Domain> <filename>" + }, { "cd-insert", &main_cd_insert, 1, 1, "Insert a cdrom into a guest's cd drive", @@ -474,6 +476,7 @@ struct cmd_spec cmd_table[] = { "Loads a new policy int the Flask Xen security module", "<policy file>", }, +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME { "remus", &main_remus, 0, 1, "Enable Remus HA for domain", @@ -486,8 +489,8 @@ struct cmd_spec cmd_table[] = { " ssh <host> xl migrate-receive -r [-e]\n" "-e Do not wait in the background (on <host>) for the death\n" " of the domain." - }, +#endif { "devd", &main_devd, 0, 1, "Daemon that listens for devices and launches backends",
ARM does not (currently) support migration, so stop offering tasty looking treats like "xl migrate". Apart from the UI improvement my intention is to use this in osstest to detect whether to attempt the save/restore/migrate tests. Other than the additions of the #define/#ifdef there is a tiny bit of code motion ("dump-core" in the command list and core_dump_domain in the implementations) which serves to put ifdeffable bits next to each other. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: goerge.dunlap@citrix.com --- Release: My main motivation here is to be able to get a complete osstest run on armhf prior to Xen 4.4 and the lack of migration support is currently blocking that (fine) but is also blocking subsequent useful tests. This change will allow me to make osstest skip the unsupported functionality, and in a way where it will automatically start trying to test it as soon as it is implemented. --- tools/libxl/libxl.h | 14 ++++++++++++++ tools/libxl/xl.h | 4 ++++ tools/libxl/xl_cmdimpl.c | 20 ++++++++++++-------- tools/libxl/xl_cmdtable.c | 15 +++++++++------ 4 files changed, 39 insertions(+), 14 deletions(-)