Skip to contentSkip to conversation

xl shutdown compatibility with xm

PATCH In review Ian Campbell 3 participants 31 messages Oct 15, 2012 – Oct 25, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xl shutdown compatibility with xm

The following implements xm compatibility for the xl shutdown command and a prerequisite bug fix.

In particular add --all and --wait which are used by the xendomains initscript. It also adds the same options to xl reboot.

Lastly it cleans up and simplifies option parsing.

The xl shutdown/reboot patches should go into 4.2.1, the option parsing cleanups should not.

#1 lore raw

PATCH 1/1 libxl: propagate user supplied values into event for_user field

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1350290590 -3600
# Node ID 65e301c71ca41af8cf0aaea653cd9e8d08bca2f7
# Parent de18f0577d3ff521822dd3760fbfe5e08a771f9d
libxl: propagate user supplied values into event for_user field.

This was ommited in the majority of cases. Add as a parameter to libxl__event_new and the NEW_EVENT wrapper to help prevent it being forgotten in the future.

---
This should be backported to 4.2.

tools/libxl/libxl.c +5 -4
diff -r de18f0577d3f -r 65e301c71ca4 tools/libxl/libxl.c
@@ -955,7 +955,7 @@ static void domain_death_occurred(libxl_
     libxl_evgen_domain_death *evg_next = LIBXL_TAILQ_NEXT(evg, entry);
     *evg_upd = evg_next;
 
-    libxl_event *ev = NEW_EVENT(egc, DOMAIN_DEATH, evg->domid);
+    libxl_event *ev = NEW_EVENT(egc, DOMAIN_DEATH, evg->domid, evg->user);
 
     libxl__event_occurred(egc, ev);
 
@@ -1041,8 +1041,9 @@ static void domain_death_xswatch_callbac
 
             if (!evg->shutdown_reported &&
                 (got->flags & XEN_DOMINF_shutdown)) {
-                libxl_event *ev = NEW_EVENT(egc, DOMAIN_SHUTDOWN, got->domain);
-                
+                libxl_event *ev = NEW_EVENT(egc, DOMAIN_SHUTDOWN,
+                                            got->domain, evg->user);
+
                 LIBXL__LOG(CTX, LIBXL__LOG_DEBUG, " shutdown reporting");
 
                 ev->u.domain_shutdown.shutdown_reason =
@@ -1141,7 +1142,7 @@ static void disk_eject_xswatch_callback(
         return;
     }
 
-    libxl_event *ev = NEW_EVENT(egc, DISK_EJECT, evg->domid);
+    libxl_event *ev = NEW_EVENT(egc, DISK_EJECT, evg->domid, evg->user);
     libxl_device_disk *disk = &ev->u.disk_eject.disk;
     
     backend = libxl__xs_read(gc, XBT_NULL,
tools/libxl/libxl_create.c +2 -1
diff -r de18f0577d3f -r 65e301c71ca4 tools/libxl/libxl_create.c
@@ -705,7 +705,8 @@ static void domcreate_console_available(
                                         libxl__domain_create_state *dcs) {
     libxl__ao_progress_report(egc, dcs->ao, &dcs->aop_console_how,
                               NEW_EVENT(egc, DOMAIN_CREATE_CONSOLE_AVAILABLE,
-                                        dcs->guest_domid));
+                                        dcs->guest_domid,
+                                        dcs->aop_console_how.for_event));
 }
 
 static void domcreate_bootloader_done(libxl__egc *egc,
tools/libxl/libxl_event.c +4 -4
diff -r de18f0577d3f -r 65e301c71ca4 tools/libxl/libxl_event.c
@@ -1157,7 +1157,8 @@ void libxl_event_free(libxl_ctx *ctx, li
 }
 
 libxl_event *libxl__event_new(libxl__egc *egc,
-                              libxl_event_type type, uint32_t domid)
+                              libxl_event_type type, uint32_t domid,
+                              libxl_ev_user for_user)
 {
     EGC_GC;
     libxl_event *ev;
@@ -1168,6 +1169,7 @@ libxl_event *libxl__event_new(libxl__egc
     libxl_event_init_type(ev, type);
 
     ev->domid = domid;
+    ev->for_user = for_user;
 
     return ev;
 }
@@ -1528,9 +1530,8 @@ void libxl__ao_complete_check_progress_r
         LIBXL_TAILQ_INSERT_TAIL(&egc->aos_for_callback, ao, entry_for_callback);
     } else {
         libxl_event *ev;
-        ev = NEW_EVENT(egc, OPERATION_COMPLETE, ao->domid);
+        ev = NEW_EVENT(egc, OPERATION_COMPLETE, ao->domid, ao->how.u.for_event);
         if (ev) {
-            ev->for_user = ao->how.u.for_event;
             ev->u.operation_complete.rc = ao->rc;
             libxl__event_occurred(egc, ev);
         }
@@ -1662,7 +1663,6 @@ void libxl__ao_progress_report(libxl__eg
         const libxl_asyncprogress_how *how, libxl_event *ev)
 {
     AO_GC;
-    ev->for_user = how->for_event;
     if (how->callback == dummy_asyncprogress_callback_ignore) {
         LOG(DEBUG,"ao %p: progress report: ignored",ao);
         libxl_event_free(CTX,ev);
tools/libxl/libxl_internal.h +4 -3
diff -r de18f0577d3f -r 65e301c71ca4 tools/libxl/libxl_internal.h
@@ -783,13 +783,14 @@ _hidden void libxl__event_occurred(libxl
    * event should be suitable for passing to libxl_event_free. */
 
 _hidden libxl_event *libxl__event_new(libxl__egc*, libxl_event_type,
-                                      uint32_t domid);
+                                      uint32_t domid,
+                                      libxl_ev_user for_user);
   /* Convenience function.
    * Allocates a new libxl_event, fills in domid and type.
    * Cannot fail. */
 
-#define NEW_EVENT(egc, type, domid)                              \
-    libxl__event_new((egc), LIBXL_EVENT_TYPE_##type, (domid))
+#define NEW_EVENT(egc, type, domid, user)                        \
+    libxl__event_new((egc), LIBXL_EVENT_TYPE_##type, (domid), (user))
     /* Convenience macro. */
 
 /*
Acked-by on v1 1/1
#2 lore raw

Re: [PATCH 1 of 5] libxl: propagate user supplied values into event for_user field

Ian Campbell writes ("[PATCH 1 of 5] libxl: propagate user supplied values into event for_user field"):

Quoted 5 lines

libxl: propagate user supplied values into event for_user field.

This was ommited in the majority of cases. Add as a parameter to libxl__event_new and the NEW_EVENT wrapper to help prevent it being forgotten in the future.

Oops.

#3 lore raw

PATCH 1/1 xl: Introduce xl shutdown --all support for xm compatibility

In reply to Ian Campbell

# HG changeset patch
# User Sander Eikelenboom <[email protected]>
# Date 1346960487 -7200
# Node ID 2329dca4ef449979b1403c4bb002fcb70c578b35 # Parent 65e301c71ca41af8cf0aaea653cd9e8d08bca2f7
xl: Introduce xl shutdown --all support for xm compatibility

Based on a patch by Sander Eikelenboom

---

Changed since v3: (ijc)
* Rebased. -w already implemented.
* Shutdown domains in //.

Changed since v2:
* fix error occuring when using both -a and -w options
* Due to mixing local and global domid variable

Changed since v1:
* address review comments.
* Change shutdown_domain to take domid instead of domname
* Docs: Make it more clear -a only shuts down GUEST domains

docs/man/xl.pod.1 +6 -1
diff -r 65e301c71ca4 -r 2329dca4ef44 docs/man/xl.pod.1
@@ -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,11 @@ B<OPTIONS>
 
 =over 4
 
+=item B<-a>, B<--all>
+
+Shutdown all guest domains.  Often used when doing a complete shutdown
+of a Xen system.
+
 =item B<-w>, B<--wait>
 
 Wait for the domain to complete shutdown before returning.
tools/libxl/xl_cmdimpl.c +86 -37

167 lines not on this page · raw message

tools/libxl/xl_cmdtable.c +3 -2
diff -r 65e301c71ca4 -r 2329dca4ef44 tools/libxl/xl_cmdtable.c
@@ -60,11 +60,12 @@ struct cmd_spec cmd_table[] = {
     { "shutdown",
       &main_shutdown, 0, 1,
       "Issue a shutdown signal to a domain",
-      "[options] <Domain>",
+      "[options] <-a|Domain>",
+      "-a, --all               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"
-      "-w, --wait              Wait for guest to shutdown.\n"
+      "-w, --wait              Wait for guest(s) to shutdown.\n"
     },
     { "reboot",
       &main_reboot, 0, 1,
Acked-by on v1 1/1
#4 lore raw

Re: [PATCH 2 of 5] xl: Introduce xl shutdown --all support for xm compatibility

Ian Campbell writes ("[PATCH 2 of 5] xl: Introduce xl shutdown --all support for xm compatibility"):

Quoted 1 line

xl: Introduce xl shutdown --all support for xm compatibility

...>

Quoted 1 line

Based on a patch by Sander Eikelenboom

...

Quoted 1 line

int main_shutdown(int argc, char **argv)

...

tools/libxl/xl_cmdimpl.c int main_shutdown(int argc, char **argv) · line 3756

Quoted 2 lines
tools/libxl/xl_cmdimpl.c +2 -0
+        if (wait_for_it)
+            deathws = calloc(nb_domain, sizeof(deathws));

This should read sizeof(*deathws). It's a shame we don't have a macro to avoid this broken pattern in xl. In libxl we have GCNEW_ARRAY of course. (In fact in this particular case the two things are both pointers so the sizeof() is the same and the bug is theoretical.)

Apart from that,

Ian.

#5 lore raw

Re: [PATCH 2 of 5] xl: Introduce xl shutdown --all support for xm compatibility

Quoted 6 lines · Ian Jackson

Ian Campbell writes ("[PATCH 2 of 5] xl: Introduce xl shutdown --all support for xm compatibility"):

Quoted 1 line

xl: Introduce xl shutdown --all support for xm compatibility

...>

Quoted 1 line

Based on a patch by Sander Eikelenboom

...

Quoted 1 line

int main_shutdown(int argc, char **argv)

Quoted 8 lines

...

tools/libxl/xl_cmdimpl.c int main_shutdown(int argc, char **argv) · line 3756

Quoted 2 lines
tools/libxl/xl_cmdimpl.c +2 -0
+        if (wait_for_it)
+            deathws = calloc(nb_domain, sizeof(deathws));

This should read sizeof(*deathws). It's a shame we don't have a macro to avoid this broken pattern in xl. In libxl we have GCNEW_ARRAY of course. (In fact in this particular case the two things are both pointers so the sizeof() is the same and the bug is theoretical.)

Oops, will fix upon commit, thanks.

Quoted 5 lines

Apart from that,

Ian.

#6 lore raw

PATCH 1/1 xl: Add --wait and --all to xl reboot

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1350295718 -3600
# Node ID 5a24315f39c035e5e5b773ebca676d5248a41252 # Parent 2329dca4ef449979b1403c4bb002fcb70c578b35
xl: Add --wait and --all to xl reboot.

Inspired by a patch by Sander Eikelenboom.

tools/libxl/xl_cmdimpl.c +30 -29
diff -r 2329dca4ef44 -r 5a24315f39c0 tools/libxl/xl_cmdimpl.c
@@ -2716,7 +2716,7 @@ static void destroy_domain(uint32_t domi
 static void wait_for_domain_deaths(libxl_evgen_domain_death **deathws, int nr)
 {
     int rc, count = 0;
-    LOG("Waiting for %d domains to shutdown", nr);
+    LOG("Waiting for %d domains", nr);
     while(1 && count < nr) {
         libxl_event *event;
         rc = libxl_event_wait(ctx, &event, LIBXL_EVENTMASK_ALL, 0,0);
@@ -2776,15 +2776,15 @@ static void shutdown_domain(uint32_t dom
             fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
             exit(-1);
         }
-        printf("Waiting for domain %d death %p %"PRIx64"\n",
-               domid, *deathw, for_user);
-    }
-}
-
-static void reboot_domain(uint32_t domid, int fallback_trigger)
+    }
+}
+
+static void reboot_domain(uint32_t domid, libxl_evgen_domain_death **deathw,
+                          libxl_ev_user for_user, int fallback_trigger)
 {
     int rc;
 
+    fprintf(stderr, "Rebooting domain %d\n", domid);
     rc=libxl_domain_reboot(ctx, domid);
     if (rc == ERROR_NOPARAVIRT) {
         if (fallback_trigger) {
@@ -2800,6 +2800,14 @@ static void reboot_domain(uint32_t domid
     if (rc) {
         fprintf(stderr,"reboot failed (rc=%d)\n",rc);exit(-1);
     }
+
+    if (deathw) {
+        rc = libxl_evenable_domain_death(ctx, domid, for_user, deathw);
+        if (rc) {
+            fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",rc);
+            exit(-1);
+        }
+    }
 }
 
 static void list_domains_details(const libxl_dominfo *info, int nb_domain)
@@ -3713,8 +3721,11 @@ int main_destroy(int argc, char **argv)
     return 0;
 }
 
-int main_shutdown(int argc, char **argv)
-{
+static int main_shutdown_or_reboot(int reboot, int argc, char **argv)
+{
+    void (*fn)(uint32_t domid,
+               libxl_evgen_domain_death **, libxl_ev_user, int) =
+        reboot ? &reboot_domain : &shutdown_domain;
     int opt, i, nb_domain;
     int wait_for_it = 0, all =0;
     int fallback_trigger = 0;
@@ -3730,6 +3741,7 @@ int main_shutdown(int argc, char **argv)
             return opt;
         case 'a':
             all = 1;
+            break;
         case 'w':
             wait_for_it = 1;
             break;
@@ -3758,9 +3770,8 @@ int main_shutdown(int argc, char **argv)
         for (i = 0; i<nb_domain; i++) {
             if (dominfo[i].domid == 0)
                 continue;
-            shutdown_domain(dominfo[i].domid,
-                            deathws ? &deathws[i] : NULL, i,
-                            fallback_trigger);
+            fn(dominfo[i].domid, deathws ? &deathws[i] : NULL, i,
+               fallback_trigger);
         }
 
         wait_for_domain_deaths(deathws, nb_domain - 1 /* not dom 0 */);
@@ -3769,8 +3780,7 @@ int main_shutdown(int argc, char **argv)
         libxl_evgen_domain_death *deathw = NULL;
         uint32_t domid = find_domain(argv[optind]);
 
-        shutdown_domain(domid, wait_for_it ? &deathw : NULL, 0,
-                        fallback_trigger);
+        fn(domid, wait_for_it ? &deathw : NULL, 0, fallback_trigger);
 
         if (wait_for_it)
             wait_for_domain_deaths(&deathw, 1);
@@ -3780,23 +3790,14 @@ int main_shutdown(int argc, char **argv)
     return 0;
 }
 
+int main_shutdown(int argc, char **argv)
+{
+    return main_shutdown_or_reboot(0, argc, argv);
+}
+
 int main_reboot(int argc, char **argv)
 {
-    int opt;
-    int fallback_trigger = 0;
-
-    while ((opt = def_getopt(argc, argv, "F", "reboot", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'F':
-            fallback_trigger = 1;
-            break;
-        }
-    }
-
-    reboot_domain(find_domain(argv[optind]), fallback_trigger);
-    return 0;
+    return main_shutdown_or_reboot(1, argc, argv);
 }
 
 int main_list(int argc, char **argv)
tools/libxl/xl_cmdtable.c +3 -1
diff -r 2329dca4ef44 -r 5a24315f39c0 tools/libxl/xl_cmdtable.c
@@ -70,10 +70,12 @@ struct cmd_spec cmd_table[] = {
     { "reboot",
       &main_reboot, 0, 1,
       "Issue a reboot signal to a domain",
-      "[options] <Domain>",
+      "[options] <-a|Domain>",
+      "-a, --all               Shutdown all guest domains.\n"
       "-h                      Print this help.\n"
       "-F                      Fallback to ACPI reset event for HVM guests with\n"
       "                        no PV drivers.\n"
+      "-w, --wait              Wait for guest(s) to reboot.\n"
     },
     { "pci-attach",
       &main_pciattach, 0, 1,
Acked-by on v1 1/1
#7 lore raw

Re: [PATCH 3 of 5] xl: Add --wait and --all to xl reboot

Ian Campbell writes ("[PATCH 3 of 5] xl: Add --wait and --all to xl reboot"):

Quoted 1 line

xl: Add --wait and --all to xl reboot.

#8 lore raw

PATCH 1/1 xl: allow def_getopt to handle long options

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1350295719 -3600
# Node ID bbb839ceb966cc265e5d2ca530ba0f2bf823176a # Parent 5a24315f39c035e5e5b773ebca676d5248a41252
xl: allow def_getopt to handle long options

Improves consistency of option parsing and error handling.

Consistently support --help for all options.

Many users of getopt_long were needlessly passing an option_index pointer which was not used.

tools/libxl/xl_cmdimpl.c +127 -200
diff -r 5a24315f39c0 -r bbb839ceb966 tools/libxl/xl_cmdimpl.c
@@ -2241,19 +2241,34 @@ static int64_t parse_mem_size_kb(const c
     return kbytes;
 }
 
-static int def_getopt(int argc, char * const argv[], const char *optstring,
+#define COMMON_LONG_OPTS {"help", 0, 0, 'h'}
+
+static int def_getopt(int argc, char * const argv[],
+                      const char *optstring,
+                      const struct option *longopts,
                       const char* helpstr, int reqargs)
 {
     int opt;
+    const struct option def_options[] = {
+        COMMON_LONG_OPTS,
+        {0, 0, 0, 0}
+    };
+
+    if (!longopts)
+        longopts = def_options;
 
     opterr = 0;
-    while ((opt = getopt(argc, argv, optstring)) == '?') {
+    while ((opt = getopt_long(argc, argv, optstring, longopts, NULL)) == '?') {
         if (optopt == 'h') {
             help(helpstr);
             return 0;
         }
         fprintf(stderr, "option `%c' not supported.\n", optopt);
     }
+    if (opt == 'h') {
+        help(helpstr);
+        return 0;
+    }
     if (opt != -1)
         return opt;
 
@@ -2289,7 +2304,7 @@ int main_memmax(int argc, char **argv)
     char *mem;
     int rc;
 
-    if ((opt = def_getopt(argc, argv, "", "mem-max", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2323,7 +2338,7 @@ int main_memset(int argc, char **argv)
     int opt = 0;
     const char *mem;
 
-    if ((opt = def_getopt(argc, argv, "", "mem-set", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "mem-set", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2362,7 +2377,7 @@ int main_cd_eject(int argc, char **argv)
     int opt = 0;
     const char *virtdev;
 
-    if ((opt = def_getopt(argc, argv, "", "cd-eject", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cd-eject", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2379,7 +2394,7 @@ int main_cd_insert(int argc, char **argv
     const char *virtdev;
     char *file = NULL; /* modified by cd_insert tokenising it */
 
-    if ((opt = def_getopt(argc, argv, "", "cd-insert", 3)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cd-insert", 3)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2396,7 +2411,7 @@ int main_console(int argc, char **argv)
     int opt = 0, num = 0;
     libxl_console_type type = 0;
 
-    while ((opt = def_getopt(argc, argv, "n:t:", "console", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "n:t:", NULL, "console", 1)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -2427,36 +2442,23 @@ int main_console(int argc, char **argv)
 
 int main_vncviewer(int argc, char **argv)
 {
-    static const struct option long_options[] = {
+    static const struct option opts[] = {
         {"autopass", 0, 0, 'a'},
         {"vncviewer-autopass", 0, 0, 'a'},
-        {"help", 0, 0, 'h'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
     uint32_t domid;
     int opt, autopass = 0;
 
-    while (1) {
-        opt = getopt_long(argc, argv, "ah", long_options, NULL);
-        if (opt == -1)
-            break;
-
+    while ((opt = def_getopt(argc, argv, "ah", opts, "vncviewer", 1)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             autopass = 1;
             break;
-        case 'h':
-            help("vncviewer");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-
-    if (argc - optind != 1) {
-        help("vncviewer");
-        return 2;
+        }
     }
 
     domid = find_domain(argv[optind]);
@@ -2489,7 +2491,7 @@ int main_pcilist(int argc, char **argv)
     uint32_t domid;
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "pci-list", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "pci-list", 1)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2528,7 +2530,7 @@ int main_pcidetach(int argc, char **argv
     int force = 0;
     const char *bdf = NULL;
 
-    while ((opt = def_getopt(argc, argv, "f", "pci-detach", 2)) != -1) {
+    while ((opt = def_getopt(argc, argv, "f", NULL, "pci-detach", 2)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -2570,7 +2572,7 @@ int main_pciattach(int argc, char **argv
     int opt;
     const char *bdf = NULL, *vs = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", "pci-attach", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "pci-attach", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -2604,7 +2606,7 @@ int main_pciassignable_list(int argc, ch
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "pci-assignable-list", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "pci-assignable-list", 0)) != -1)
         return opt;
 
     pciassignable_list();
@@ -2636,7 +2638,7 @@ int main_pciassignable_add(int argc, cha
     int opt;
     const char *bdf = NULL;
 
-    while ((opt = def_getopt(argc, argv, "", "pci-assignable-add", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "", NULL, "pci-assignable-add", 1)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -2675,7 +2677,7 @@ int main_pciassignable_remove(int argc, 
     const char *bdf = NULL;
     int rebind = 0;
 
-    while ((opt = def_getopt(argc, argv, "r", "pci-assignable-remove", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "r", NULL, "pci-assignable-remove", 1)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -3486,24 +3488,18 @@ int main_restore(int argc, char **argv)
     int paused = 0, debug = 0, daemonize = 1, monitor = 1,
         console_autoconnect = 0, vnc = 0, vncautopass = 0;
     int opt, rc;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    while (1) {
-        opt = getopt_long(argc, argv, "FhcpdeVA", long_options, &option_index);
-        if (opt == -1)
-            break;
-
+    while ((opt = def_getopt(argc, argv, "FhcpdeVA",
+                             opts, "restore", 1)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
-        case 'h':
-            help("restore");
-            return 2;
         case 'c':
             console_autoconnect = 1;
             break;
@@ -3563,7 +3559,7 @@ int main_migrate_receive(int argc, char 
     int debug = 0, daemonize = 1, monitor = 1, remus = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "Fedr", "migrate-receive", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "Fedr", NULL, "migrate-receive", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -3602,7 +3598,7 @@ int main_save(int argc, char **argv)
     int checkpoint = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "c", "save", 2)) != -1) {
+    while ((opt = def_getopt(argc, argv, "c", NULL, "save", 2)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -3635,7 +3631,7 @@ int main_migrate(int argc, char **argv)
     char *host;
     int opt, daemonize = 1, monitor = 1, debug = 0;
 
-    while ((opt = def_getopt(argc, argv, "FC:s:ed", "migrate", 2)) != -1) {
+    while ((opt = def_getopt(argc, argv, "FC:s:ed", NULL, "migrate", 2)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -3679,7 +3675,7 @@ int main_dump_core(int argc, char **argv
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "dump-core", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "dump-core", 2)) != -1)
         return opt;
 
     core_dump_domain(find_domain(argv[optind]), argv[optind + 1]);
@@ -3690,7 +3686,7 @@ int main_pause(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "pause", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "pause", 1)) != -1)
         return opt;
 
     pause_domain(find_domain(argv[optind]));
@@ -3702,7 +3698,7 @@ int main_unpause(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "unpause", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "unpause", 1)) != -1)
         return opt;
 
     unpause_domain(find_domain(argv[optind]));
@@ -3714,7 +3710,7 @@ int main_destroy(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "destroy", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "destroy", 1)) != -1)
         return opt;
 
     destroy_domain(find_domain(argv[optind]));
@@ -3723,19 +3719,21 @@ int main_destroy(int argc, char **argv)
 
 static int main_shutdown_or_reboot(int reboot, int argc, char **argv)
 {
+    const char *what = reboot ? "reboot" : "shutdown";
     void (*fn)(uint32_t domid,
                libxl_evgen_domain_death **, libxl_ev_user, int) =
         reboot ? &reboot_domain : &shutdown_domain;
     int opt, i, nb_domain;
     int wait_for_it = 0, all =0;
     int fallback_trigger = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"all", 0, 0, 'a'},
         {"wait", 0, 0, 'w'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    while ((opt = getopt_long(argc, argv, "awF", long_options, NULL)) != -1) {
+    while ((opt = def_getopt(argc, argv, "awF", opts, what, 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -3805,12 +3803,11 @@ int main_list(int argc, char **argv)
     int opt, verbose = 0;
     int context = 0;
     int details = 0;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"long", 0, 0, 'l'},
-        {"help", 0, 0, 'h'},
         {"verbose", 0, 0, 'v'},
         {"context", 0, 0, 'Z'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
@@ -3818,12 +3815,10 @@ int main_list(int argc, char **argv)
     libxl_dominfo *info, *info_free=0;
     int nb_domain, rc;
 
-    while (1) {
-        opt = getopt_long(argc, argv, "lvhZ", long_options, &option_index);
-        if (opt == -1)
-            break;
-
+    while ((opt = def_getopt(argc, argv, "lvhZ", opts, "list", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'l':
             details = 1;
             break;
@@ -3836,9 +3831,6 @@ int main_list(int argc, char **argv)
         case 'Z':
             context = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
@@ -3885,7 +3877,7 @@ int main_vm_list(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "vm-list", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "vm-list", 0)) != -1)
         return opt;
 
     list_vm();
@@ -3901,14 +3893,13 @@ int main_create(int argc, char **argv)
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
         quiet = 0, monitor = 1, vnc = 0, vncautopass = 0;
     int opt, rc;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"dryrun", 0, 0, 'n'},
         {"quiet", 0, 0, 'q'},
-        {"help", 0, 0, 'h'},
         {"defconfig", 1, 0, 'f'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
@@ -3917,12 +3908,10 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    while (1) {
-        opt = getopt_long(argc, argv, "Fhnqf:pcdeVA", long_options, &option_index);
-        if (opt == -1)
-            break;
-
+    while ((opt = def_getopt(argc, argv, "Fhnqf:pcdeVA", opts, "create", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'f':
             filename = optarg;
             break;
@@ -3957,9 +3946,6 @@ int main_create(int argc, char **argv)
         case 'A':
             vnc = vncautopass = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
@@ -4007,11 +3993,10 @@ int main_config_update(int argc, char **
     int config_len = 0;
     libxl_domain_config d_config;
     int opt, rc;
-    int option_index = 0;
     int debug = 0;
-    static struct option long_options[] = {
-        {"help", 0, 0, 'h'},
+    static struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
@@ -4029,24 +4014,16 @@ int main_config_update(int argc, char **
         argc--; argv++;
     }
 
-    while (1) {
-        opt = getopt_long(argc, argv, "dhqf:", long_options, &option_index);
-        if (opt == -1)
-            break;
-
+    while ((opt = def_getopt(argc, argv, "dhqf:", opts, "config_update", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'd':
             debug = 1;
             break;
         case 'f':
             filename = optarg;
             break;
-        case 'h':
-            help("create");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
@@ -4135,7 +4112,7 @@ int main_button_press(int argc, char **a
     fprintf(stderr, "WARNING: \"button-press\" is deprecated. "
             "Please use \"trigger\"\n");
 
-    if ((opt = def_getopt(argc, argv, "", "button-press", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "button-press", 2)) != -1)
         return opt;
 
     button_press(find_domain(argv[optind]), argv[optind + 1]);
@@ -4276,7 +4253,7 @@ int main_vcpulist(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "cpu-list", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpu-list", 0)) != -1)
         return opt;
 
     vcpulist(argc - optind, argv + optind);
@@ -4337,7 +4314,7 @@ int main_vcpupin(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "vcpu-pin", 3)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "vcpu-pin", 3)) != -1)
         return opt;
 
     vcpupin(find_domain(argv[optind]), argv[optind+1] , argv[optind+2]);
@@ -4373,7 +4350,7 @@ int main_vcpuset(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "vcpu-set", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "vcpu-set", 2)) != -1)
         return opt;
 
     vcpuset(find_domain(argv[optind]), argv[optind+1]);
@@ -4549,25 +4526,20 @@ static void print_info(int numa)
 int main_info(int argc, char **argv)
 {
     int opt;
-    int option_index = 0;
-    static struct option long_options[] = {
-        {"help", 0, 0, 'h'},
+    static struct option opts[] = {
         {"numa", 0, 0, 'n'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
     int numa = 0;
 
-    while ((opt = getopt_long(argc, argv, "hn", long_options, &option_index)) != -1) {
+    while ((opt = def_getopt(argc, argv, "hn", opts, "info", 0)) != -1) {
         switch (opt) {
-        case 'h':
-            help("info");
-            return 0;
+        case 0: case 2:
+            return opt;
         case 'n':
             numa = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
@@ -4602,7 +4574,7 @@ int main_sharing(int argc, char **argv)
     libxl_dominfo *info, *info_free = NULL;
     int nb_domain, rc;
 
-    if ((opt = def_getopt(argc, argv, "", "sharing", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "sharing", 0)) != -1)
         return opt;
 
     if (optind >= argc) {
@@ -4871,8 +4843,7 @@ int main_sched_credit(int argc, char **a
     int opt_s = 0;
     int tslice = 0, opt_t = 0, ratelimit = 0, opt_r = 0;
     int opt, rc;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"domain", 1, 0, 'd'},
         {"weight", 1, 0, 'w'},
         {"cap", 1, 0, 'c'},
@@ -4880,15 +4851,11 @@ int main_sched_credit(int argc, char **a
         {"tslice_ms", 1, 0, 't'},
         {"ratelimit_us", 1, 0, 'r'},
         {"cpupool", 1, 0, 'p'},
-        {"help", 0, 0, 'h'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    while (1) {
-        opt = getopt_long(argc, argv, "d:w:c:p:t:r:hs", long_options,
-                          &option_index);
-        if (opt == -1)
-            break;
+    while ((opt = def_getopt(argc, argv, "d:w:c:p:t:r:hs", opts, "sched-credit", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -4917,9 +4884,6 @@ int main_sched_credit(int argc, char **a
         case 'p':
             cpupool = optarg;
             break;
-        case 'h':
-            help("sched-credit");
-            return 0;
         }
     }
 
@@ -5002,19 +4966,15 @@ int main_sched_credit2(int argc, char **
     const char *cpupool = NULL;
     int weight = 256, opt_w = 0;
     int opt, rc;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"domain", 1, 0, 'd'},
         {"weight", 1, 0, 'w'},
         {"cpupool", 1, 0, 'p'},
-        {"help", 0, 0, 'h'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    while (1) {
-        opt = getopt_long(argc, argv, "d:w:p:h", long_options, &option_index);
-        if (opt == -1)
-            break;
+    while ((opt = def_getopt(argc, argv, "d:w:p:h", opts, "sched-credit2", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5028,9 +4988,6 @@ int main_sched_credit2(int argc, char **
         case 'p':
             cpupool = optarg;
             break;
-        case 'h':
-            help("sched-credit");
-            return 0;
         }
     }
 
@@ -5081,23 +5038,18 @@ int main_sched_sedf(int argc, char **arg
     int extra = 0, opt_e = 0;
     int weight = 0, opt_w = 0;
     int opt, rc;
-    int option_index = 0;
-    static struct option long_options[] = {
+    static struct option opts[] = {
         {"period", 1, 0, 'p'},
         {"slice", 1, 0, 's'},
         {"latency", 1, 0, 'l'},
         {"extra", 1, 0, 'e'},
         {"weight", 1, 0, 'w'},
         {"cpupool", 1, 0, 'c'},
-        {"help", 0, 0, 'h'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    while (1) {
-        opt = getopt_long(argc, argv, "d:p:s:l:e:w:c:h", long_options,
-                          &option_index);
-        if (opt == -1)
-            break;
+    while ((opt = def_getopt(argc, argv, "d:p:s:l:e:w:c:h", opts, "sched-sedf", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5127,9 +5079,6 @@ int main_sched_sedf(int argc, char **arg
         case 'c':
             cpupool = optarg;
             break;
-        case 'h':
-            help("sched-sedf");
-            return 0;
         }
     }
 
@@ -5197,7 +5146,7 @@ int main_domid(int argc, char **argv)
     int opt;
     const char *domname = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", "domid", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "domid", 1)) != -1)
         return opt;
 
     domname = argv[optind];
@@ -5219,7 +5168,7 @@ int main_domname(int argc, char **argv)
     char *domname = NULL;
     char *endptr = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", "domname", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "domname", 1)) != -1)
         return opt;
 
     domid = strtol(argv[optind], &endptr, 10);
@@ -5247,7 +5196,7 @@ int main_rename(int argc, char **argv)
     int opt;
     const char *dom, *new_name;
 
-    if ((opt = def_getopt(argc, argv, "", "rename", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "rename", 2)) != -1)
         return opt;
 
     dom = argv[optind++];
@@ -5271,7 +5220,7 @@ int main_trigger(int argc, char **argv)
     const char *trigger_name = NULL;
     libxl_trigger trigger;
 
-    if ((opt = def_getopt(argc, argv, "", "trigger", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "trigger", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind++]);
@@ -5301,7 +5250,7 @@ int main_sysrq(int argc, char **argv)
     int opt;
     const char *sysrq = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", "sysrq", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "sysrq", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind++]);
@@ -5324,7 +5273,7 @@ int main_debug_keys(int argc, char **arg
     int opt;
     char *keys;
 
-    if ((opt = def_getopt(argc, argv, "", "debug-keys", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "debug-keys", 1)) != -1)
         return opt;
 
     keys = argv[optind];
@@ -5344,7 +5293,7 @@ int main_dmesg(int argc, char **argv)
     char *line;
     int opt, ret = 1;
 
-    while ((opt = def_getopt(argc, argv, "c", "dmesg", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "c", NULL, "dmesg", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5370,7 +5319,7 @@ int main_top(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", "top", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "top", 0)) != -1)
         return opt;
 
     return system("xentop");
@@ -5387,7 +5336,7 @@ int main_networkattach(int argc, char **
     int i;
     unsigned int val;
 
-    if ((opt = def_getopt(argc, argv, "", "network-attach", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "network-attach", 1)) != -1)
         return opt;
 
     if (argc-optind > 11) {
@@ -5474,7 +5423,7 @@ int main_networklist(int argc, char **ar
     libxl_nicinfo nicinfo;
     int nb, i;
 
-    if ((opt = def_getopt(argc, argv, "", "network-list", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "network-list", 1)) != -1)
         return opt;
 
     /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
@@ -5511,7 +5460,7 @@ int main_networkdetach(int argc, char **
     int opt;
     libxl_device_nic nic;
 
-    if ((opt = def_getopt(argc, argv, "", "network-detach", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "network-detach", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -5542,7 +5491,7 @@ int main_blockattach(int argc, char **ar
     libxl_device_disk disk = { 0 };
     XLU_Config *config = 0;
 
-    if ((opt = def_getopt(argc, argv, "", "block-attach", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "block-attach", 2)) != -1)
         return opt;
 
     if (domain_qualifier_to_domid(argv[optind], &fe_domid, 0) < 0) {
@@ -5577,7 +5526,7 @@ int main_blocklist(int argc, char **argv
     libxl_device_disk *disks;
     libxl_diskinfo diskinfo;
 
-    if ((opt = def_getopt(argc, argv, "", "block-list", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "block-list", 1)) != -1)
         return opt;
 
     printf("%-5s %-3s %-6s %-5s %-6s %-8s %-30s\n",
@@ -5613,7 +5562,7 @@ int main_blockdetach(int argc, char **ar
     int opt, rc = 0;
     libxl_device_disk disk;
 
-    if ((opt = def_getopt(argc, argv, "", "block-detach", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "block-detach", 2)) != -1)
         return opt;
 
     domid = find_domain(argv[optind]);
@@ -5797,7 +5746,7 @@ int main_uptime(int argc, char **argv)
     int nb_doms = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "s", "uptime", 1)) != -1) {
+    while ((opt = def_getopt(argc, argv, "s", NULL, "uptime", 1)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5824,7 +5773,7 @@ int main_tmem_list(int argc, char **argv
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "al", "tmem-list", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "al", NULL, "tmem-list", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5865,7 +5814,7 @@ int main_tmem_freeze(int argc, char **ar
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "a", "tmem-freeze", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "a", NULL, "tmem-freeze", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5898,7 +5847,7 @@ int main_tmem_thaw(int argc, char **argv
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "a", "tmem-thaw", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "a", NULL, "tmem-thaw", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5933,7 +5882,7 @@ int main_tmem_set(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "aw:c:p:", "tmem-set", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "aw:c:p:", NULL, "tmem-set", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -5994,7 +5943,7 @@ int main_tmem_shared_auth(int argc, char
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "au:A:", "tmem-shared-auth", 0)) != -1) {
+    while ((opt = def_getopt(argc, argv, "au:A:", NULL, "tmem-shared-auth", 0)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
@@ -6044,7 +5993,7 @@ int main_tmem_freeable(int argc, char **
     int opt;
     int mb;
 
-    if ((opt = def_getopt(argc, argv, "", "tmem-freeable", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "tmem-freeable", 0)) != -1)
         return opt;
 
     mb = libxl_tmem_freeable(ctx);
@@ -6061,11 +6010,10 @@ int main_cpupoolcreate(int argc, char **
     const char *p;
     char extra_config[1024];
     int opt;
-    int option_index = 0;
-    static struct option long_options[] = {
-        {"help", 0, 0, 'h'},
+    static struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
         {"dryrun", 0, 0, 'n'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
     int ret;
@@ -6083,26 +6031,18 @@ int main_cpupoolcreate(int argc, char **
     libxl_bitmap cpumap;
     libxl_uuid uuid;
     libxl_cputopology *topology;
-    int rc = -ERROR_FAIL; 
-
-    while (1) {
-        opt = getopt_long(argc, argv, "hnf:", long_options, &option_index);
-        if (opt == -1)
-            break;
-
+    int rc = -ERROR_FAIL;
+
+    while ((opt = def_getopt(argc, argv, "hnf:", opts, "cpupool-create", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'f':
             filename = optarg;
             break;
-        case 'h':
-            help("cpupool-create");
-            return 0;
         case 'n':
             dryrun_only = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
@@ -6265,10 +6205,9 @@ out:
 int main_cpupoollist(int argc, char **argv)
 {
     int opt;
-    int option_index = 0;
-    static struct option long_options[] = {
-        {"help", 0, 0, 'h'},
+    static struct option opts[] = {
         {"cpus", 0, 0, 'c'},
+        COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
     int opt_cpus = 0;
@@ -6279,28 +6218,16 @@ int main_cpupoollist(int argc, char **ar
     char *name;
     int ret = 0;
 
-    while (1) {
-        opt = getopt_long(argc, argv, "hc", long_options, &option_index);
-        if (opt == -1)
+    while ((opt = def_getopt(argc, argv, "hc", opts, "cpupool-list", 1)) != -1) {
+        switch (opt) {
+        case 0: case 2:
             break;
-
-        switch (opt) {
-        case 'h':
-            help("cpupool-list");
-            return 0;
         case 'c':
             opt_cpus = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-
-    if ((optind + 1) < argc) {
-        help("cpupool-list");
-        return -ERROR_FAIL;
-    }
+        }
+    }
+
     if (optind < argc) {
         pool = argv[optind];
         if (libxl_name_to_cpupoolid(ctx, pool, &poolid)) {
@@ -6358,7 +6285,7 @@ int main_cpupooldestroy(int argc, char *
     const char *pool;
     uint32_t poolid;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-destroy", 1)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-destroy", 1)) != -1)
         return opt;
 
     pool = argv[optind];
@@ -6379,7 +6306,7 @@ int main_cpupoolrename(int argc, char **
     const char *new_name;
     uint32_t poolid;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-rename", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-rename", 2)) != -1)
         return opt;
 
     pool = argv[optind++];
@@ -6409,7 +6336,7 @@ int main_cpupoolcpuadd(int argc, char **
     int node;
     int n;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-cpu-add", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-cpu-add", 2)) != -1)
         return opt;
 
     pool = argv[optind++];
@@ -6453,7 +6380,7 @@ int main_cpupoolcpuremove(int argc, char
     int node;
     int n;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-cpu-remove", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-cpu-remove", 2)) != -1)
         return opt;
 
     pool = argv[optind++];
@@ -6496,7 +6423,7 @@ int main_cpupoolmigrate(int argc, char *
     const char *dom;
     uint32_t domid;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-migrate", 2)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-migrate", 2)) != -1)
         return opt;
 
     dom = argv[optind++];
@@ -6536,7 +6463,7 @@ int main_cpupoolnumasplit(int argc, char
     libxl_cputopology *topology;
     libxl_dominfo info;
 
-    if ((opt = def_getopt(argc, argv, "", "cpupool-numa-split", 0)) != -1)
+    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-numa-split", 0)) != -1)
         return opt;
     ret = 0;
 
@@ -6789,7 +6716,7 @@ int main_remus(int argc, char **argv)
     r_info.blackhole = 0;
     r_info.compression = 1;
 
-    while ((opt = def_getopt(argc, argv, "bui:s:e", "remus", 2)) != -1) {
+    while ((opt = def_getopt(argc, argv, "bui:s:e", NULL, "remus", 2)) != -1) {
         switch (opt) {
         case 0: case 2:
             return opt;
Acked-by on v1 1/1
#9 lore raw

Re: [PATCH 4 of 5] xl: allow def_getopt to handle long options

Ian Campbell writes ("[PATCH 4 of 5] xl: allow def_getopt to handle long options"):

Quoted 1 line

xl: allow def_getopt to handle long options

#10 lore raw

PATCH 1/1 xl: Introduce helper macro for option parsing

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1350295720 -3600
# Node ID 22688187afa84747de9b820cb29addbd65385042 # Parent bbb839ceb966cc265e5d2ca530ba0f2bf823176a
xl: Introduce helper macro for option parsing.

tools/libxl/xl_cmdimpl.c +448 -427
diff -r bbb839ceb966 -r 22688187afa8 tools/libxl/xl_cmdimpl.c
@@ -2281,6 +2281,11 @@ static int def_getopt(int argc, char * c
     return -1;
 }
 
+#define FOREACH_OPT(_opt, _opts, _lopts, _help, _req)           \
+    while (((_opt) = def_getopt(argc, argv, (_opts), (_lopts),  \
+                                (_help), (_req))) != -1)        \
+        switch (opt)
+
 static int set_memory_max(uint32_t domid, const char *mem)
 {
     int64_t memorykb;
@@ -2304,8 +2309,10 @@ int main_memmax(int argc, char **argv)
     char *mem;
     int rc;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
@@ -2338,8 +2345,10 @@ int main_memset(int argc, char **argv)
     int opt = 0;
     const char *mem;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "mem-set", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "mem-set", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
     mem = argv[optind + 1];
@@ -2377,8 +2386,10 @@ int main_cd_eject(int argc, char **argv)
     int opt = 0;
     const char *virtdev;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cd-eject", 2)) != -1)
-        return opt;
+    FOREACH_OPT(opt, "", NULL, "cd-eject", 2) {
+        case 0: case 2:
+            return opt;
+    }
 
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
@@ -2394,8 +2405,10 @@ int main_cd_insert(int argc, char **argv
     const char *virtdev;
     char *file = NULL; /* modified by cd_insert tokenising it */
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cd-insert", 3)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cd-insert", 3) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
@@ -2411,24 +2424,22 @@ int main_console(int argc, char **argv)
     int opt = 0, num = 0;
     libxl_console_type type = 0;
 
-    while ((opt = def_getopt(argc, argv, "n:t:", NULL, "console", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 't':
-            if (!strcmp(optarg, "pv"))
-                type = LIBXL_CONSOLE_TYPE_PV;
-            else if (!strcmp(optarg, "serial"))
-                type = LIBXL_CONSOLE_TYPE_SERIAL;
-            else {
-                fprintf(stderr, "console type supported are: pv, serial\n");
-                return 2;
-            }
-            break;
-        case 'n':
-            num = atoi(optarg);
-            break;
+    FOREACH_OPT(opt, "n:t:", NULL, "console", 1) {
+    case 0: case 2:
+        return opt;
+    case 't':
+        if (!strcmp(optarg, "pv"))
+            type = LIBXL_CONSOLE_TYPE_PV;
+        else if (!strcmp(optarg, "serial"))
+            type = LIBXL_CONSOLE_TYPE_SERIAL;
+        else {
+            fprintf(stderr, "console type supported are: pv, serial\n");
+            return 2;
         }
+        break;
+    case 'n':
+        num = atoi(optarg);
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -2451,14 +2462,12 @@ int main_vncviewer(int argc, char **argv
     uint32_t domid;
     int opt, autopass = 0;
 
-    while ((opt = def_getopt(argc, argv, "ah", opts, "vncviewer", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'a':
-            autopass = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "ah", opts, "vncviewer", 1) {
+    case 0: case 2:
+        return opt;
+    case 'a':
+        autopass = 1;
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -2491,8 +2500,10 @@ int main_pcilist(int argc, char **argv)
     uint32_t domid;
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "pci-list", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "pci-list", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
 
@@ -2530,14 +2541,12 @@ int main_pcidetach(int argc, char **argv
     int force = 0;
     const char *bdf = NULL;
 
-    while ((opt = def_getopt(argc, argv, "f", NULL, "pci-detach", 2)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'f':
-            force = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "f", NULL, "pci-detach", 2) {
+    case 0: case 2:
+        return opt;
+    case 'f':
+        force = 1;
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -2572,8 +2581,10 @@ int main_pciattach(int argc, char **argv
     int opt;
     const char *bdf = NULL, *vs = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "pci-attach", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "pci-attach", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
     bdf = argv[optind + 1];
@@ -2606,8 +2617,10 @@ int main_pciassignable_list(int argc, ch
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "pci-assignable-list", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "pci-assignable-list", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     pciassignable_list();
     return 0;
@@ -2638,11 +2651,9 @@ int main_pciassignable_add(int argc, cha
     int opt;
     const char *bdf = NULL;
 
-    while ((opt = def_getopt(argc, argv, "", NULL, "pci-assignable-add", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        }
+    FOREACH_OPT(opt, "", NULL, "pci-assignable-add", 1) {
+    case 0: case 2:
+        return opt;
     }
 
     bdf = argv[optind];
@@ -2677,14 +2688,12 @@ int main_pciassignable_remove(int argc, 
     const char *bdf = NULL;
     int rebind = 0;
 
-    while ((opt = def_getopt(argc, argv, "r", NULL, "pci-assignable-remove", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'r':
-            rebind=1;
-            break;
-        }
+    FOREACH_OPT(opt, "r", NULL, "pci-assignable-remove", 1) {
+    case 0: case 2:
+        return opt;
+    case 'r':
+        rebind=1;
+        break;
     }
 
     bdf = argv[optind];
@@ -3495,34 +3504,31 @@ int main_restore(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
-    while ((opt = def_getopt(argc, argv, "FhcpdeVA",
-                             opts, "restore", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'c':
-            console_autoconnect = 1;
-            break;
-        case 'p':
-            paused = 1;
-            break;
-        case 'd':
-            debug = 1;
-            break;
-        case 'F':
-            daemonize = 0;
-            break;
-        case 'e':
-            daemonize = 0;
-            monitor = 0;
-            break;
-        case 'V':
-            vnc = 1;
-            break;
-        case 'A':
-            vnc = vncautopass = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "FhcpdeVA", opts, "restore", 1) {
+    case 0: case 2:
+        return opt;
+    case 'c':
+        console_autoconnect = 1;
+        break;
+    case 'p':
+        paused = 1;
+        break;
+    case 'd':
+        debug = 1;
+        break;
+    case 'F':
+        daemonize = 0;
+        break;
+    case 'e':
+        daemonize = 0;
+        monitor = 0;
+        break;
+    case 'V':
+        vnc = 1;
+        break;
+    case 'A':
+        vnc = vncautopass = 1;
+        break;
     }
 
     if (argc-optind == 1) {
@@ -3559,24 +3565,22 @@ int main_migrate_receive(int argc, char 
     int debug = 0, daemonize = 1, monitor = 1, remus = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "Fedr", NULL, "migrate-receive", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'F':
-            daemonize = 0;
-            break;
-        case 'e':
-            daemonize = 0;
-            monitor = 0;
-            break;
-        case 'd':
-            debug = 1;
-            break;
-        case 'r':
-            remus = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "Fedr", NULL, "migrate-receive", 0) {
+    case 0: case 2:
+        return opt;
+    case 'F':
+        daemonize = 0;
+        break;
+    case 'e':
+        daemonize = 0;
+        monitor = 0;
+        break;
+    case 'd':
+        debug = 1;
+        break;
+    case 'r':
+        remus = 1;
+        break;
     }
 
     if (argc-optind != 0) {
@@ -3598,14 +3602,12 @@ int main_save(int argc, char **argv)
     int checkpoint = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "c", NULL, "save", 2)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'c':
-            checkpoint = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "c", NULL, "save", 2) {
+    case 0: case 2:
+        return opt;
+    case 'c':
+        checkpoint = 1;
+        break;
     }
 
     if (argc-optind > 3) {
@@ -3631,27 +3633,25 @@ int main_migrate(int argc, char **argv)
     char *host;
     int opt, daemonize = 1, monitor = 1, debug = 0;
 
-    while ((opt = def_getopt(argc, argv, "FC:s:ed", NULL, "migrate", 2)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'C':
-            config_filename = optarg;
-            break;
-        case 's':
-            ssh_command = optarg;
-            break;
-        case 'F':
-            daemonize = 0;
-            break;
-        case 'e':
-            daemonize = 0;
-            monitor = 0;
-            break;
-        case 'd':
-            debug = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "FC:s:ed", NULL, "migrate", 2) {
+    case 0: case 2:
+        return opt;
+    case 'C':
+        config_filename = optarg;
+        break;
+    case 's':
+        ssh_command = optarg;
+        break;
+    case 'F':
+        daemonize = 0;
+        break;
+    case 'e':
+        daemonize = 0;
+        monitor = 0;
+        break;
+    case 'd':
+        debug = 1;
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -3675,8 +3675,10 @@ int main_dump_core(int argc, char **argv
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "dump-core", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "dump-core", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     core_dump_domain(find_domain(argv[optind]), argv[optind + 1]);
     return 0;
@@ -3686,8 +3688,10 @@ int main_pause(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "pause", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "pause", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     pause_domain(find_domain(argv[optind]));
 
@@ -3698,8 +3702,10 @@ int main_unpause(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "unpause", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "unpause", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     unpause_domain(find_domain(argv[optind]));
 
@@ -3710,8 +3716,10 @@ int main_destroy(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "destroy", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "destroy", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     destroy_domain(find_domain(argv[optind]));
     return 0;
@@ -3815,23 +3823,18 @@ int main_list(int argc, char **argv)
     libxl_dominfo *info, *info_free=0;
     int nb_domain, rc;
 
-    while ((opt = def_getopt(argc, argv, "lvhZ", opts, "list", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'l':
-            details = 1;
-            break;
-        case 'h':
-            help("list");
-            return 0;
-        case 'v':
-            verbose = 1;
-            break;
-        case 'Z':
-            context = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "lvhZ", opts, "list", 0) {
+    case 0: case 2:
+        return opt;
+    case 'l':
+        details = 1;
+        break;
+    case 'v':
+        verbose = 1;
+        break;
+    case 'Z':
+        context = 1;
+        break;
     }
 
     if (optind >= argc) {
@@ -3877,8 +3880,10 @@ int main_vm_list(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "vm-list", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "vm-list", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     list_vm();
     return 0;
@@ -3908,45 +3913,40 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    while ((opt = def_getopt(argc, argv, "Fhnqf:pcdeVA", opts, "create", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'f':
-            filename = optarg;
-            break;
-        case 'p':
-            paused = 1;
-            break;
-        case 'c':
-            console_autoconnect = 1;
-            break;
-        case 'd':
-            debug = 1;
-            break;
-        case 'F':
-            daemonize = 0;
-            break;
-        case 'e':
-            daemonize = 0;
-            monitor = 0;
-            break;
-        case 'h':
-            help("create");
-            return 0;
-        case 'n':
-            dryrun_only = 1;
-            break;
-        case 'q':
-            quiet = 1;
-            break;
-        case 'V':
-            vnc = 1;
-            break;
-        case 'A':
-            vnc = vncautopass = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "Fhnqf:pcdeVA", opts, "create", 0) {
+    case 0: case 2:
+        return opt;
+    case 'f':
+        filename = optarg;
+        break;
+    case 'p':
+        paused = 1;
+        break;
+    case 'c':
+        console_autoconnect = 1;
+        break;
+    case 'd':
+        debug = 1;
+        break;
+    case 'F':
+        daemonize = 0;
+        break;
+    case 'e':
+        daemonize = 0;
+        monitor = 0;
+        break;
+    case 'n':
+        dryrun_only = 1;
+        break;
+    case 'q':
+        quiet = 1;
+        break;
+    case 'V':
+        vnc = 1;
+        break;
+    case 'A':
+        vnc = vncautopass = 1;
+        break;
     }
 
     extra_config[0] = '\0';
@@ -4014,17 +4014,15 @@ int main_config_update(int argc, char **
         argc--; argv++;
     }
 
-    while ((opt = def_getopt(argc, argv, "dhqf:", opts, "config_update", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'd':
-            debug = 1;
-            break;
-        case 'f':
-            filename = optarg;
-            break;
-        }
+    FOREACH_OPT(opt, "dhqf:", opts, "config_update", 0) {
+    case 0: case 2:
+        return opt;
+    case 'd':
+        debug = 1;
+        break;
+    case 'f':
+        filename = optarg;
+        break;
     }
 
     extra_config[0] = '\0';
@@ -4112,8 +4110,11 @@ int main_button_press(int argc, char **a
     fprintf(stderr, "WARNING: \"button-press\" is deprecated. "
             "Please use \"trigger\"\n");
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "button-press", 2)) != -1)
+
+    FOREACH_OPT(opt, "", NULL, "button-press", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     button_press(find_domain(argv[optind]), argv[optind + 1]);
 
@@ -4253,8 +4254,10 @@ int main_vcpulist(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpu-list", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpu-list", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     vcpulist(argc - optind, argv + optind);
     return 0;
@@ -4314,8 +4317,10 @@ int main_vcpupin(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "vcpu-pin", 3)) != -1)
+    FOREACH_OPT(opt, "", NULL, "vcpu-pin", 3) {
+    case 0: case 2:
         return opt;
+    }
 
     vcpupin(find_domain(argv[optind]), argv[optind+1] , argv[optind+2]);
     return 0;
@@ -4350,8 +4355,10 @@ int main_vcpuset(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "vcpu-set", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "vcpu-set", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     vcpuset(find_domain(argv[optind]), argv[optind+1]);
     return 0;
@@ -4533,14 +4540,12 @@ int main_info(int argc, char **argv)
     };
     int numa = 0;
 
-    while ((opt = def_getopt(argc, argv, "hn", opts, "info", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'n':
-            numa = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "hn", opts, "info", 0) {
+    case 0: case 2:
+        return opt;
+    case 'n':
+        numa = 1;
+        break;
     }
 
     print_info(numa);
@@ -4574,8 +4579,10 @@ int main_sharing(int argc, char **argv)
     libxl_dominfo *info, *info_free = NULL;
     int nb_domain, rc;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "sharing", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "sharing", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     if (optind >= argc) {
         info = libxl_list_domain(ctx, &nb_domain);
@@ -4855,36 +4862,34 @@ int main_sched_credit(int argc, char **a
         {0, 0, 0, 0}
     };
 
-    while ((opt = def_getopt(argc, argv, "d:w:c:p:t:r:hs", opts, "sched-credit", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'd':
-            dom = optarg;
-            break;
-        case 'w':
-            weight = strtol(optarg, NULL, 10);
-            opt_w = 1;
-            break;
-        case 'c':
-            cap = strtol(optarg, NULL, 10);
-            opt_c = 1;
-            break;
-        case 't':
-            tslice = strtol(optarg, NULL, 10);
-            opt_t = 1;
-            break;
-        case 'r':
-            ratelimit = strtol(optarg, NULL, 10);
-            opt_r = 1;
-            break;
-        case 's':
-            opt_s = 1;
-            break;
-        case 'p':
-            cpupool = optarg;
-            break;
-        }
+    FOREACH_OPT(opt, "d:w:c:p:t:r:hs", opts, "sched-credit", 0) {
+    case 0: case 2:
+        return opt;
+    case 'd':
+        dom = optarg;
+        break;
+    case 'w':
+        weight = strtol(optarg, NULL, 10);
+        opt_w = 1;
+        break;
+    case 'c':
+        cap = strtol(optarg, NULL, 10);
+        opt_c = 1;
+        break;
+    case 't':
+        tslice = strtol(optarg, NULL, 10);
+        opt_t = 1;
+        break;
+    case 'r':
+        ratelimit = strtol(optarg, NULL, 10);
+        opt_r = 1;
+        break;
+    case 's':
+        opt_s = 1;
+        break;
+    case 'p':
+        cpupool = optarg;
+        break;
     }
 
     if ((cpupool || opt_s) && (dom || opt_w || opt_c)) {
@@ -4974,21 +4979,19 @@ int main_sched_credit2(int argc, char **
         {0, 0, 0, 0}
     };
 
-    while ((opt = def_getopt(argc, argv, "d:w:p:h", opts, "sched-credit2", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'd':
-            dom = optarg;
-            break;
-        case 'w':
-            weight = strtol(optarg, NULL, 10);
-            opt_w = 1;
-            break;
-        case 'p':
-            cpupool = optarg;
-            break;
-        }
+    FOREACH_OPT(opt, "d:w:p:h", opts, "sched-credit2", 0) {
+    case 0: case 2:
+        return opt;
+    case 'd':
+        dom = optarg;
+        break;
+    case 'w':
+        weight = strtol(optarg, NULL, 10);
+        opt_w = 1;
+        break;
+    case 'p':
+        cpupool = optarg;
+        break;
     }
 
     if (cpupool && (dom || opt_w)) {
@@ -5049,37 +5052,35 @@ int main_sched_sedf(int argc, char **arg
         {0, 0, 0, 0}
     };
 
-    while ((opt = def_getopt(argc, argv, "d:p:s:l:e:w:c:h", opts, "sched-sedf", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'd':
-            dom = optarg;
-            break;
-        case 'p':
-            period = strtol(optarg, NULL, 10);
-            opt_p = 1;
-            break;
-        case 's':
-            slice = strtol(optarg, NULL, 10);
-            opt_s = 1;
-            break;
-        case 'l':
-            latency = strtol(optarg, NULL, 10);
-            opt_l = 1;
-            break;
-        case 'e':
-            extra = strtol(optarg, NULL, 10);
-            opt_e = 1;
-            break;
-        case 'w':
-            weight = strtol(optarg, NULL, 10);
-            opt_w = 1;
-            break;
-        case 'c':
-            cpupool = optarg;
-            break;
-        }
+    FOREACH_OPT(opt, "d:p:s:l:e:w:c:h", opts, "sched-sedf", 0) {
+    case 0: case 2:
+        return opt;
+    case 'd':
+        dom = optarg;
+        break;
+    case 'p':
+        period = strtol(optarg, NULL, 10);
+        opt_p = 1;
+        break;
+    case 's':
+        slice = strtol(optarg, NULL, 10);
+        opt_s = 1;
+        break;
+    case 'l':
+        latency = strtol(optarg, NULL, 10);
+        opt_l = 1;
+        break;
+    case 'e':
+        extra = strtol(optarg, NULL, 10);
+        opt_e = 1;
+        break;
+    case 'w':
+        weight = strtol(optarg, NULL, 10);
+        opt_w = 1;
+        break;
+    case 'c':
+        cpupool = optarg;
+        break;
     }
 
     if (cpupool && (dom || opt_p || opt_s || opt_l || opt_e || opt_w)) {
@@ -5146,8 +5147,10 @@ int main_domid(int argc, char **argv)
     int opt;
     const char *domname = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "domid", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "domid", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     domname = argv[optind];
 
@@ -5168,8 +5171,10 @@ int main_domname(int argc, char **argv)
     char *domname = NULL;
     char *endptr = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "domname", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "domname", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = strtol(argv[optind], &endptr, 10);
     if (domid == 0 && !strcmp(endptr, argv[optind])) {
@@ -5196,8 +5201,10 @@ int main_rename(int argc, char **argv)
     int opt;
     const char *dom, *new_name;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "rename", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "rename", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     dom = argv[optind++];
     new_name = argv[optind];
@@ -5220,8 +5227,10 @@ int main_trigger(int argc, char **argv)
     const char *trigger_name = NULL;
     libxl_trigger trigger;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "trigger", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "trigger", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind++]);
 
@@ -5250,8 +5259,10 @@ int main_sysrq(int argc, char **argv)
     int opt;
     const char *sysrq = NULL;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "sysrq", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "sysrq", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind++]);
 
@@ -5273,8 +5284,10 @@ int main_debug_keys(int argc, char **arg
     int opt;
     char *keys;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "debug-keys", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "debug-keys", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     keys = argv[optind];
 
@@ -5293,14 +5306,12 @@ int main_dmesg(int argc, char **argv)
     char *line;
     int opt, ret = 1;
 
-    while ((opt = def_getopt(argc, argv, "c", NULL, "dmesg", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'c':
-            clear = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "c", NULL, "dmesg", 0) {
+    case 0: case 2:
+        return opt;
+    case 'c':
+        clear = 1;
+        break;
     }
 
     cr = libxl_xen_console_read_start(ctx, clear);
@@ -5319,8 +5330,10 @@ int main_top(int argc, char **argv)
 {
     int opt;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "top", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "top", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     return system("xentop");
 }
@@ -5336,8 +5349,10 @@ int main_networkattach(int argc, char **
     int i;
     unsigned int val;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "network-attach", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "network-attach", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     if (argc-optind > 11) {
         help("network-attach");
@@ -5423,8 +5438,10 @@ int main_networklist(int argc, char **ar
     libxl_nicinfo nicinfo;
     int nb, i;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "network-list", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "network-list", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
     printf("%-3s %-2s %-17s %-6s %-5s %-6s %5s/%-5s %-30s\n",
@@ -5460,8 +5477,10 @@ int main_networkdetach(int argc, char **
     int opt;
     libxl_device_nic nic;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "network-detach", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "network-detach", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
 
@@ -5491,8 +5510,10 @@ int main_blockattach(int argc, char **ar
     libxl_device_disk disk = { 0 };
     XLU_Config *config = 0;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "block-attach", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "block-attach", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     if (domain_qualifier_to_domid(argv[optind], &fe_domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
@@ -5526,8 +5547,10 @@ int main_blocklist(int argc, char **argv
     libxl_device_disk *disks;
     libxl_diskinfo diskinfo;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "block-list", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "block-list", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     printf("%-5s %-3s %-6s %-5s %-6s %-8s %-30s\n",
            "Vdev", "BE", "handle", "state", "evt-ch", "ring-ref", "BE-path");
@@ -5562,8 +5585,10 @@ int main_blockdetach(int argc, char **ar
     int opt, rc = 0;
     libxl_device_disk disk;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "block-detach", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "block-detach", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     domid = find_domain(argv[optind]);
 
@@ -5746,14 +5771,12 @@ int main_uptime(int argc, char **argv)
     int nb_doms = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "s", NULL, "uptime", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 's':
-            short_mode = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "s", NULL, "uptime", 1) {
+    case 0: case 2:
+        return opt;
+    case 's':
+        short_mode = 1;
+        break;
     }
 
     for (;(dom = argv[optind]) != NULL; nb_doms++,optind++)
@@ -5773,17 +5796,15 @@ int main_tmem_list(int argc, char **argv
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "al", NULL, "tmem-list", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'l':
-            use_long = 1;
-            break;
-        case 'a':
-            all = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "al", NULL, "tmem-list", 0) {
+    case 0: case 2:
+        return opt;
+    case 'l':
+        use_long = 1;
+        break;
+    case 'a':
+        all = 1;
+        break;
     }
 
     dom = argv[optind];
@@ -5814,14 +5835,12 @@ int main_tmem_freeze(int argc, char **ar
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "a", NULL, "tmem-freeze", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'a':
-            all = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "a", NULL, "tmem-freeze", 0) {
+    case 0: case 2:
+        return opt;
+    case 'a':
+        all = 1;
+        break;
     }
 
     dom = argv[optind];
@@ -5847,14 +5866,12 @@ int main_tmem_thaw(int argc, char **argv
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "a", NULL, "tmem-thaw", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'a':
-            all = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "a", NULL, "tmem-thaw", 0) {
+    case 0: case 2:
+        return opt;
+    case 'a':
+        all = 1;
+        break;
     }
 
     dom = argv[optind];
@@ -5882,26 +5899,24 @@ int main_tmem_set(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "aw:c:p:", NULL, "tmem-set", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'a':
-            all = 1;
-            break;
-        case 'w':
-            weight = strtol(optarg, NULL, 10);
-            opt_w = 1;
-            break;
-        case 'c':
-            cap = strtol(optarg, NULL, 10);
-            opt_c = 1;
-            break;
-        case 'p':
-            compress = strtol(optarg, NULL, 10);
-            opt_p = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "aw:c:p:", NULL, "tmem-set", 0) {
+    case 0: case 2:
+        return opt;
+    case 'a':
+        all = 1;
+        break;
+    case 'w':
+        weight = strtol(optarg, NULL, 10);
+        opt_w = 1;
+        break;
+    case 'c':
+        cap = strtol(optarg, NULL, 10);
+        opt_c = 1;
+        break;
+    case 'p':
+        compress = strtol(optarg, NULL, 10);
+        opt_p = 1;
+        break;
     }
 
     dom = argv[optind];
@@ -5943,20 +5958,18 @@ int main_tmem_shared_auth(int argc, char
     int all = 0;
     int opt;
 
-    while ((opt = def_getopt(argc, argv, "au:A:", NULL, "tmem-shared-auth", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'a':
-            all = 1;
-            break;
-        case 'u':
-            uuid = optarg;
-            break;
-        case 'A':
-            autharg = optarg;
-            break;
-        }
+    FOREACH_OPT(opt, "au:A:", NULL, "tmem-shared-auth", 0) {
+    case 0: case 2:
+        return opt;
+    case 'a':
+        all = 1;
+        break;
+    case 'u':
+        uuid = optarg;
+        break;
+    case 'A':
+        autharg = optarg;
+        break;
     }
 
     dom = argv[optind];
@@ -5993,8 +6006,10 @@ int main_tmem_freeable(int argc, char **
     int opt;
     int mb;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "tmem-freeable", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "tmem-freeale", 0) {
+    case 0: case 2:
         return opt;
+    }
 
     mb = libxl_tmem_freeable(ctx);
     if (mb == -1)
@@ -6033,17 +6048,15 @@ int main_cpupoolcreate(int argc, char **
     libxl_cputopology *topology;
     int rc = -ERROR_FAIL;
 
-    while ((opt = def_getopt(argc, argv, "hnf:", opts, "cpupool-create", 0)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-        case 'f':
-            filename = optarg;
-            break;
-        case 'n':
-            dryrun_only = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "hnf:", opts, "cpupool-create", 0) {
+    case 0: case 2:
+        return opt;
+    case 'f':
+        filename = optarg;
+        break;
+    case 'n':
+        dryrun_only = 1;
+        break;
     }
 
     memset(extra_config, 0, sizeof(extra_config));
@@ -6218,14 +6231,12 @@ int main_cpupoollist(int argc, char **ar
     char *name;
     int ret = 0;
 
-    while ((opt = def_getopt(argc, argv, "hc", opts, "cpupool-list", 1)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            break;
-        case 'c':
-            opt_cpus = 1;
-            break;
-        }
+    FOREACH_OPT(opt, "hc", opts, "cpupool-list", 1) {
+    case 0: case 2:
+        break;
+    case 'c':
+        opt_cpus = 1;
+        break;
     }
 
     if (optind < argc) {
@@ -6285,8 +6296,10 @@ int main_cpupooldestroy(int argc, char *
     const char *pool;
     uint32_t poolid;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-destroy", 1)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-destroy", 1) {
+    case 0: case 2:
         return opt;
+    }
 
     pool = argv[optind];
 
@@ -6306,8 +6319,10 @@ int main_cpupoolrename(int argc, char **
     const char *new_name;
     uint32_t poolid;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-rename", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-rename", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     pool = argv[optind++];
 
@@ -6336,8 +6351,10 @@ int main_cpupoolcpuadd(int argc, char **
     int node;
     int n;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-cpu-add", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-cpu-add", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     pool = argv[optind++];
     node = -1;
@@ -6380,8 +6397,10 @@ int main_cpupoolcpuremove(int argc, char
     int node;
     int n;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-cpu-remove", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-cpu-remove", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     pool = argv[optind++];
     node = -1;
@@ -6423,8 +6442,10 @@ int main_cpupoolmigrate(int argc, char *
     const char *dom;
     uint32_t domid;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-migrate", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-migrate", 2) {
+    case 0: case 2:
         return opt;
+    }
 
     dom = argv[optind++];
     pool = argv[optind];
@@ -6463,8 +6484,11 @@ int main_cpupoolnumasplit(int argc, char
     libxl_cputopology *topology;
     libxl_dominfo info;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "cpupool-numa-split", 0)) != -1)
+    FOREACH_OPT(opt, "", NULL, "cpupool-numa-split", 0) {
+    case 0: case 2:
         return opt;
+    }
+
     ret = 0;
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);
@@ -6716,27 +6740,24 @@ int main_remus(int argc, char **argv)
     r_info.blackhole = 0;
     r_info.compression = 1;
 
-    while ((opt = def_getopt(argc, argv, "bui:s:e", NULL, "remus", 2)) != -1) {
-        switch (opt) {
-        case 0: case 2:
-            return opt;
-
-        case 'i':
-	    r_info.interval = atoi(optarg);
-            break;
-        case 'b':
-            r_info.blackhole = 1;
-            break;
-        case 'u':
-	    r_info.compression = 0;
-            break;
-        case 's':
-            ssh_command = optarg;
-            break;
-        case 'e':
-            daemonize = 0;
-            break;
-        }
+    FOREACH_OPT(opt, "bui:s:e", NULL, "remus", 2) {
+    case 0: case 2:
+        return opt;
+    case 'i':
+        r_info.interval = atoi(optarg);
+        break;
+    case 'b':
+        r_info.blackhole = 1;
+        break;
+    case 'u':
+        r_info.compression = 0;
+        break;
+    case 's':
+        ssh_command = optarg;
+        break;
+    case 'e':
+        daemonize = 0;
+        break;
     }
 
     domid = find_domain(argv[optind]);
#11 lore raw

Re: [PATCH 5 of 5] xl: Introduce helper macro for option parsing

Ian Campbell writes ("[PATCH 5 of 5] xl: Introduce helper macro for option parsing"):

Quoted 1 line

xl: Introduce helper macro for option parsing.

The idea is a reasonable one, but I have some quibbles.

tools/libxl/xl_cmdimpl.c static int def_getopt(int argc, char * c · line 2287

Quoted 4 lines
tools/libxl/xl_cmdimpl.c +4 -0
+#define FOREACH_OPT(_opt, _opts, _lopts, _help, _req)           \
+    while (((_opt) = def_getopt(argc, argv, (_opts), (_lopts),  \
+                                (_help), (_req))) != -1)        \
+        switch (opt)

This macro name should have the word SWITCH in it too so you know it has to take cases and that "break" doesn't do what you think. FOREACH_SWITCH_OPT maybe.

tools/libxl/xl_cmdimpl.c int main_memmax(int argc, char **argv) · line 2315

Quoted 9 lines
tools/libxl/xl_cmdimpl.c +3 -1
@@ -2304,8 +2309,10 @@ int main_memmax(int argc, char **argv)
     char *mem;
     int rc;
 
-    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
+    case 0: case 2:
         return opt;
+    }

This error handling behaviour (?) is rather opaque. I RTFM getopt_long and AFAICT it returns 0 only for certain long options and it doesn't seem to mention returning 2.

Perhaps this would all be clearer if FOREACH_OPT had a big comment explaining how to use it.

Thanks,
Ian.

#12 lore raw

Re: [PATCH 5 of 5] xl: Introduce helper macro for option parsing

Quoted 5 lines · Ian Jackson

Ian Campbell writes ("[PATCH 5 of 5] xl: Introduce helper macro for option parsing"):

Quoted 1 line

xl: Introduce helper macro for option parsing.

The idea is a reasonable one, but I have some quibbles.

Quoted 8 lines

tools/libxl/xl_cmdimpl.c static int def_getopt(int argc, char * c · line 2287

Quoted 4 lines
tools/libxl/xl_cmdimpl.c +4 -0
+#define FOREACH_OPT(_opt, _opts, _lopts, _help, _req)           \
+    while (((_opt) = def_getopt(argc, argv, (_opts), (_lopts),  \
+                                (_help), (_req))) != -1)        \
+        switch (opt)

This macro name should have the word SWITCH in it too so you know it has to take cases and that "break" doesn't do what you think. FOREACH_SWITCH_OPT maybe.

Good idea.

Quoted 5 lines
Quoted 5 lines
tools/libxl/xl_cmdimpl.c +0 -0
@@ -2304,8 +2309,10 @@ int main_memmax(int argc, char **argv)
     char *mem;
     int rc;
 
Quoted 9 lines

tools/libxl/xl_cmdimpl.c int main_memmax(int argc, char **argv) · line 2315

Quoted 5 lines
tools/libxl/xl_cmdimpl.c +3 -1
-    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
+    FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
+    case 0: case 2:
         return opt;
+    }

This error handling behaviour (?) is rather opaque. I RTFM getopt_long and AFAICT it returns 0 only for certain long options and it doesn't seem to mention returning 2.

2 comes from our existing implementation of def_getopt, which returns it in the "not enough args" case. I don't think any callers differentiate between that and 0 which is the other error indication though.

Quoted 2 lines

Perhaps this would all be clearer if FOREACH_OPT had a big comment explaining how to use it.

Yes, will do.

#13 lore raw
In reply to Ian Campbell

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

--

Sander

Quoted 10 lines · you

The following implements xm compatibility for the xl shutdown command and a prerequisite bug fix.

In particular add --all and --wait which are used by the xendomains initscript. It also adds the same options to xl reboot.

Lastly it cleans up and simplifies option parsing.

The xl shutdown/reboot patches should go into 4.2.1, the option parsing cleanups should not.

#14 lore raw
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Therefore the user needs to be aware of when they can safely use it.

Ian.

#15 lore raw
Quoted 13 lines · you
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

Quoted 6 lines

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Therefore the user needs to be aware of when they can safely use it.

Well yes and no:
- can't remember having (to make) that choice with xm ?
- On shutdown with xl as toolstack and when the guest doesn't support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Quoted 1 line

Ian.

#16 lore raw
Quoted 20 lines · Sander Eikelenboom
Quoted 17 lines · you
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Quoted 4 lines
Quoted 1 line

Therefore the user needs to be aware of when they can safely use it.

Well yes and no:
- can't remember having (to make) that choice with xm ?

Looks like xend uses SCHEDOP_remote_shutdown. It's unclear to me why this is better than just shooting the domain with destroy...

Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

Ian.

#17 lore raw
Quoted 37 lines · you
Quoted 20 lines · Sander Eikelenboom
Quoted 17 lines · you
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Quoted 4 lines
Quoted 1 line

Therefore the user needs to be aware of when they can safely use it.

Well yes and no:
- can't remember having (to make) that choice with xm ?

Looks like xend uses SCHEDOP_remote_shutdown. It's unclear to me why this is better than just shooting the domain with destroy...

Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Quoted 2 lines

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

I'm not totally convinced (yet):
- In case of a system shutdown, it seems better to at least *try* instead of just halting the system without shutting such a guest down. For the shutdown scripts the problems is that there is no way to give it the "full knowledge" of how to shutdown a particular guest. It would be possible to use the -F flag in the sysconfig xendomains file, since the pv shutdown is always tried first.

- Not everyone seems to be aware, say an IanJ ;-)
Under the assumption that the guests in the xen-test-harness do react in the right way to a acpi powerbutton event,
it seems the "never passes" in "Guest-stop" row of http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/
Will probably pass when the -F option will be used, see http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/test-amd64-i386-xl-win7-amd64/13.ts-guest-stop.log

--
Sander

Quoted 1 line

Ian.

#18 lore raw
Quoted 39 lines · Sander Eikelenboom
Quoted 36 lines · you
Quoted 20 lines · Sander Eikelenboom
Quoted 17 lines · you
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Quoted 4 lines
Quoted 1 line

Therefore the user needs to be aware of when they can safely use it.

Well yes and no:
- can't remember having (to make) that choice with xm ?

Looks like xend uses SCHEDOP_remote_shutdown. It's unclear to me why this is better than just shooting the domain with destroy...

Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Quoted 5 lines
Quoted 2 lines

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

I'm not totally convinced (yet):
- In case of a system shutdown, it seems better to at least *try* instead of just halting the system without shutting such a guest down.

You might as well do xl shutdown, wait a bit, xl destroy in the initscript. "xm shutdown" is effectively "xm destroy" for guests without PV drivers.

Quoted 2 lines

For the shutdown scripts the problems is that there is no way to give it the "full knowledge" of how to shutdown a particular guest. It would be possible to use the -F flag in the sysconfig xendomains file, since the pv shutdown is always tried first.

It would be possible for an individual administrator to add -F if that is compatible with their configuration. It would be inappropriate for upstream to do this by default because we *don't know* what -F does to an arbitrary guest.

Quoted 4 lines

- Not everyone seems to be aware, say an IanJ ;-)
Under the assumption that the guests in the xen-test-harness do react in the right way to a acpi powerbutton event,
it seems the "never passes" in "Guest-stop" row of http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/
Will probably pass when the -F option will be used, see http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/test-amd64-i386-xl-win7-amd64/13.ts-guest-stop.log

Ian J is well aware that -F could help here (this is the main reason I implemented this option). It would be appropriate here because we can control the guest OS configuration in the harness (and if we can't we shouldn't use -F).

He just hasn't implemented it in the harness yet (TBH I suspect he has forgotten ;-)).

Ian.

#19 lore raw
Quoted 50 lines · you
Quoted 39 lines · Sander Eikelenboom
Quoted 36 lines · you
Quoted 20 lines · Sander Eikelenboom
Quoted 17 lines · you
Quoted 11 lines · Sander Eikelenboom

Hi Ian,

Great thanks !
Only thing i was wondering about:

Shouldn't the "-F" option be dropped in favour of always trying the "acpi fallback" when the pv shutdown fails.

This because the shutdown scripts still don't work for domains without pv shutdown and i don't see a down side to just trying that as fallback.

It is guess OS dependent what the ACPI button press event does, it can reboot, shutdown or hibernate etc depending on the OS type and its configuration. (in theory I suppose it is completely arbitrary e.g. it could be configured to eject the CD-ROM or something equally random).

Quoted 4 lines
Quoted 1 line

Therefore the user needs to be aware of when they can safely use it.

Well yes and no:
- can't remember having (to make) that choice with xm ?

Looks like xend uses SCHEDOP_remote_shutdown. It's unclear to me why this is better than just shooting the domain with destroy...

Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Quoted 5 lines
Quoted 2 lines

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

I'm not totally convinced (yet):
- In case of a system shutdown, it seems better to at least *try* instead of just halting the system without shutting such a guest down.

You might as well do xl shutdown, wait a bit, xl destroy in the initscript. "xm shutdown" is effectively "xm destroy" for guests without PV drivers.

Quoted 7 lines
Quoted 2 lines

For the shutdown scripts the problems is that there is no way to give it the "full knowledge" of how to shutdown a particular guest. It would be possible to use the -F flag in the sysconfig xendomains file, since the pv shutdown is always tried first.

It would be possible for an individual administrator to add -F if that is compatible with their configuration. It would be inappropriate for upstream to do this by default because we *don't know* what -F does to an arbitrary guest.

It's just that i think upstream should use sensible defaults that work in the majority of cases (unless it will have a devastating effect in de minority of cases).
In this case both hvm guests with fairly old windows and linux install seem to respond correctly to the acpi powerbutton, so perhaps the majority of cases.
In the minority of cases it will perhaps do something else, but could/will that really be more devastating than the pulling the rug from under a guests feet when the machine powers down without the guest being shutdown ?

But any how, since it's not acceptable to upstream i will keep it in my private scripts and shut up :-)

Quoted 14 lines
Quoted 4 lines

- Not everyone seems to be aware, say an IanJ ;-)
Under the assumption that the guests in the xen-test-harness do react in the right way to a acpi powerbutton event,
it seems the "never passes" in "Guest-stop" row of http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/
Will probably pass when the -F option will be used, see http://www.chiark.greenend.org.uk/~xensrcts/logs/13967/test-amd64-i386-xl-win7-amd64/13.ts-guest-stop.log

Ian J is well aware that -F could help here (this is the main reason I implemented this option). It would be appropriate here because we can control the guest OS configuration in the harness (and if we can't we shouldn't use -F).

He just hasn't implemented it in the harness yet (TBH I suspect he has forgotten ;-)).

Ian.

#20 lore raw
In reply to Ian Campbell

Ian Campbell writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 11 lines · Sander Eikelenboom
Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

I'm still with Sander on this, I'm afraid.

Certainly in the init scripts the default should be to try
xl shutdown -F
and only do destroy if that didn't seem to work.

Ian.

#21 lore raw
Quoted 10 lines · you

Ian Campbell writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 9 lines · Sander Eikelenboom
Quoted 7 lines

- On shutdown with xl as toolstack and when the guest doesn't
support pv shutdown, the init.d/xendomains script doesn't even attempt to shutdown this guest by acpi fallback.
- As a result when using xl as toolstack, the guest is terminated
non gracefully when the whole machine finally shutsdown, which seems less desirable then at least *trying* to shut it down gracefully by using the acpi button.

Quoted 8 lines
Quoted 2 lines

Using the ACPI fallback is a decision which can only be made locally with full knowledge of the configuration of the guests.

I'm still with Sander on this, I'm afraid.

Certainly in the init scripts the default should be to try
xl shutdown -F
and only do destroy if that didn't seem to work.

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

I think option B is acceptable and preferable:
- It reduces config options
- Will work out of the box in most cases (for domains without PV drivers), since most of the time the acpi power button event is wired correctly.
- In most cases nothing devastating will happen when it doesn't work.
- Only in very special cases, probably setup explicitly, a admin should shutdown the domain by other means, but he has that knowledge in those cases and shouldn't use xl shutdown on that domain, and shut the domain down before shutting down the machine (and running the xendomains init script)

--
Sander

Quoted 1 line

Ian.

#22 lore raw

Sander Eikelenboom writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 10 lines

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

I think option B is acceptable and preferable:

I agree with you. But I think we need to convince Ian C.

Ian.

#23 lore raw
Quoted 10 lines · you

Sander Eikelenboom writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 9 lines

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

Quoted 3 lines
Quoted 1 line

I think option B is acceptable and preferable:

I agree with you. But I think we need to convince Ian C.

Ok I tried with the reasoning above.
I agree with his argumentation that for domains that do not properly shutdown with pv and acpi fallback intervention by a admin was and is required.
But i try to explain that for this special case it doesn't matter if xl shutdown tries to do the acpi fallback automatically, since this admin shouldn't use xl shutdown on this domain anyway.

Quoted 1 line

Ian.

#24 lore raw
Quoted 14 lines · Sander Eikelenboom
Quoted 11 lines · you

Sander Eikelenboom writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 10 lines

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

I think option B is acceptable and preferable:

Quoted 7 lines
Quoted 1 line

I agree with you. But I think we need to convince Ian C.

Ok I tried with the reasoning above.

I agree with his argumentation that for domains that do not properly shutdown with pv and acpi fallback intervention by a admin was and is required.

ACPI fallback != shutdown. It might just as likely be a hibernate or (more likely) a reboot.

Are we going to add code to xl which forces on_reboot = destroy when xl shutdown -F is invoked on a domain?

Quoted 3 lines

But i try to explain that for this special case it doesn't matter if xl shutdown tries to do the acpi fallback automatically, since this admin shouldn't use xl shutdown on this domain anyway.

Whether they should or not the xendomains script is going to magically do it for them.

Ian.

#25 lore raw
Quoted 23 lines · you
Quoted 14 lines · Sander Eikelenboom
Quoted 11 lines · you

Sander Eikelenboom writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 10 lines

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

I think option B is acceptable and preferable:

Quoted 7 lines
Quoted 1 line

I agree with you. But I think we need to convince Ian C.

Ok I tried with the reasoning above.

I agree with his argumentation that for domains that do not properly shutdown with pv and acpi fallback intervention by a admin was and is required.

Quoted 5 lines

ACPI fallback != shutdown. It might just as likely be a hibernate or (more likely) a reboot.

Are we going to add code to xl which forces on_reboot = destroy when xl shutdown -F is invoked on a domain?

If you don't want your guests to be shutdown, you would presumably use save and restore instead of shutdown for the xendomains init scripts ?

Quoted 6 lines
Quoted 3 lines

But i try to explain that for this special case it doesn't matter if xl shutdown tries to do the acpi fallback automatically, since this admin shouldn't use xl shutdown on this domain anyway.

Whether they should or not the xendomains script is going to magically do it for them.

Well at the moment the xl shutdown fails, the script continues and the hardware shutdown. So the guest is also destroyed in the end anyhow.

So if a admin has a special guest that doesn't properly shutdown on either pv or acpi he should shut it down manually before invoking the xendomains init script. That's the case at present (without the change) and would be the future case (when changing to try the fallback).

Quoted 1 line

Ian.

#26 lore raw
In reply to Ian Campbell

Ian Campbell writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 5 lines

ACPI fallback != shutdown. It might just as likely be a hibernate or (more likely) a reboot.

Are we going to add code to xl which forces on_reboot = destroy when xl shutdown -F is invoked on a domain?

We might need to do something along those lines.

Quoted 6 lines
Quoted 3 lines

But i try to explain that for this special case it doesn't matter if xl shutdown tries to do the acpi fallback automatically, since this admin shouldn't use xl shutdown on this domain anyway.

Whether they should or not the xendomains script is going to magically do it for them.

Well, at the moment the xendomains script will try "xl shutdown" and if that fails the guest will be shot in the head. Trying the acpi fallback is surely better?

Ian.

#27 lore raw
Quoted 13 lines · you

Ian Campbell writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 5 lines

ACPI fallback != shutdown. It might just as likely be a hibernate or (more likely) a reboot.

Are we going to add code to xl which forces on_reboot = destroy when xl shutdown -F is invoked on a domain?

We might need to do something along those lines.

Quoted 4 lines
Quoted 3 lines

But i try to explain that for this special case it doesn't matter if xl shutdown tries to do the acpi fallback automatically, since this admin shouldn't use xl shutdown on this domain anyway.

Quoted 6 lines
Quoted 2 lines

Whether they should or not the xendomains script is going to magically do it for them.

Well, at the moment the xendomains script will try "xl shutdown" and if that fails the guest will be shot in the head. Trying the acpi fallback is surely better?

Perhaps it helps to keep the 2 "use cases", 2 possible changes and their arguments apart:
1) xl shutdown invoked by the xendomains init script
2) xl shutdown invoked manually by a admin or admin-scripts

Possible variants:
Change A) Only change the xendomains init/sysconfig script, and add the -F option to the command.
Or
Change B) Change the xl shutdown command and remove the -F option, always try acpi fallback when pv fails.

1) xl shutdown invoked by the xendomains init script (also used in a non machine shutdown scenario) At present:

               - a guest which can handle the pv shutdown:                                     Guest will shutdown properly.
               - a guest which can't handle pv but would do *shutdown* on the acpi fallback:   Xl shutdown will fail, guest will be shot through the head on hardware shutdown.
               - a guest which can't handle pv but would do *something* on the acpi fallback:  Xl shutdown will fail, guest will be shot through the head on hardware shutdown.
               - a guest which can't handle pv and does nothing on acpi fallback:              Xl shutdown will fail, guest will be shot through the head on hardware shutdown.

So at present if the domain doesn't react to pv it will be shot through the head anyway, so it could eat your data at present as well, (but in my case i seem to be testing and relying on journalling filesystems for some time) :-)

After change (both A or B):
- a guest which can handle the pv shutdown: Guest will shutdown properly.
- a guest which can't handle pv but would do *shutdown* on the acpi fallback: Guest will shutdown properly.
- a guest which can't handle pv but would do *something* on the acpi fallback: Xl shutdown will fail, guest will do *somehting* AND be shot through the head on hardware shutdown.
- a guest which can't handle pv and does nothing on acpi fallback: Xl shutdown will fail, guest will be shot through the head on hardware shutdown.

So both change A or B seem to have no serious negative side effects unless the acpi fallback triggers something like a "rm -rf /" in the guest. Such disastrous behavior seems unlikely, but if present, it would probably be known to the admin. In that case he should take special care of this guest anyway.

It does make a guest that supports properly shutting down on acpi power button to do just that, so in my opinion both change A or B would be a win in this use case.

2) xl shutdown invoked manually by a admin or admin-scripts (also used in a non machine shutdown scenario)
At present:
- a guest which can handle the pv shutdown: Guest will shutdown properly.
- a guest which can't handle pv but would do *shutdown* on the acpi fallback: Xl shutdown will fail, admin can supply -F after which the guest will shutdown properly.
- a guest which can't handle pv but would do *something* on the acpi fallback: Xl shutdown will fail, admin can supply -F after which the guest will do *something* but doesn't shutdown.
- a guest which can't handle pv and does nothing on acpi fallback: Xl shutdown will fail, guest will do nothing and doesn't shutdown.

After change (A)):
- Same as present

After change (B)):
- a guest which can handle the pv shutdown: Guest will shutdown properly.
- a guest which can't handle pv but would do *shutdown* on the acpi fallback: Guest will shutdown properly.
- a guest which can't handle pv but would do *something* on the acpi fallback: Guest will do *something* but doesn't shutdown.
- a guest which can't handle pv and does nothing on acpi fallback: Xl shutdown will fail, guest will do nothing and doesn't shutdown.

In this case the win is for the guests that support shutdown on the acpi fallback, it removes the need for a user intervention and supplying the -F option. The downside would be a admin using xl shutdown on a domain that does *something* on acpi fallback, where *something* is some devastating action.
This seems unlikely, but if present, it would probably be known to the admin. In that case he shouldn't use xl shutdown, and probably won't use it anyway since it won't help him anyway.

Summary:

- change (A) looks quite uncontroversial to me
- change (B) has as additional benefit that it's one option and manual intervention less to care about (especially for scripts, since it's quite possible to forget the -F option there (see IanJ with the test harness scripts)

--
Sander

Quoted 1 line

Ian.

#28 lore raw
In reply to Ian Jackson
Quoted 10 lines · Ian Jackson

Sander Eikelenboom writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 9 lines

So Ian, what would your prefer ?

A) only fix the xendomains init script, since it's automated and a administator can not intervene, for manual usage of xl shutdown keep the current behaviour
B) Drop the -F option and let xl shutdown always try the acpi fallback. In this case you can very well turn around IanC's argumentation:
An administrator who knows that a domain can't be shutdown with either the pv or acpi fallback just shouldn't try to use xl shutdown manually. (and if he does probably nothing devastating will happen)
C) Invert the -F option, to NOT try to use the apci fallback

Quoted 3 lines
Quoted 1 line

I think option B is acceptable and preferable:

I agree with you. But I think we need to convince Ian C.

My main concern is that triggering a hibernation or suspend is, AUIU, a reasonably common out of the box configuration for some versions of Windows, which combined with hibernation being (historically at least) notoriously flakey on real hardware (and I would expect more so on a virtualised platform) worries me. Also note that we don't actually implement suspend to RAM -- the domain will quiesce itself and come to a stop but AFAIK it won't actually die and the domain's RAM won't be saved anywhere. Presumably this is roughly equivalent to your battery running out while suspended and therefore not all that dangerous.

But anyway I seem to be in the minority. We can always revert it if it starts eating peoples data.

Ian.

#29 lore raw

Ian Campbell writes ("Re: [PATCH 0 of 5] xl shutdown compatibility with xm"):

Quoted 9 lines

My main concern is that triggering a hibernation or suspend is, AUIU, a reasonably common out of the box configuration for some versions of Windows, which combined with hibernation being (historically at least) notoriously flakey on real hardware (and I would expect more so on a virtualised platform) worries me. Also note that we don't actually implement suspend to RAM -- the domain will quiesce itself and come to a stop but AFAIK it won't actually die and the domain's RAM won't be saved anywhere. Presumably this is roughly equivalent to your battery running out while suspended and therefore not all that dangerous.

If it triggers a suspend, the domain won't end up shutting down so if we wait for it there will be no data loss. If we time out and do a destroy then doing the suspend first was probably a big improvement.

If it triggers a hibernation, then if the hibernation is broken the effect will be just as if the user pressed the power button and the hibernation is broken: the machine will fail to come back and effectively will have been crashed. This is probably worse than doing nothing (in the case where the caller/user isn't going to time out and do a destroy instead) but it is definitely better than a destroy.

Quoted 2 lines

But anyway I seem to be in the minority. We can always revert it if it starts eating peoples data.

OK, I think that's a go-ahead. Thanks.

Ian.

#30 lore raw
In reply to Ian Campbell
Quoted 5 lines · Ian Campbell

The following implements xm compatibility for the xl shutdown command and a prerequisite bug fix.

In particular add --all and --wait which are used by the xendomains initscript. It also adds the same options to xl reboot.

Applied these bits (first three patches) with Ian's ack.

Quoted 1 line

Lastly it cleans up and simplifies option parsing.

Ian had comments on #5 and it logically belongs with #4 so I have skipped both for now.

Thanks.

Quoted 8 lines

The xl shutdown/reboot patches should go into 4.2.1, the option parsing cleanups should not.

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel