Skip to contentSkip to conversation

xl: refactor common parts of command line parsing

PATCH In review Andre Przywara 3 participants 4 messages Apr 15, 2011 – May 4, 2011

lore
Share & tools
lore

Andre Przywara

author
#0 lore raw

PATCH 1/1 xl: refactor common parts of command line parsing

Hi,

xl command options are currently handled in each command's sub function, leading to a lot of duplicate code. This patch moves the common part of it into a separate function,
which handles the help switch, unknown options and an insufficient
number of parameters. This removes a lot of redundant code.

Due to the high number of commands this patch is rather large. If that would help reviewers, I could split it up, though this would be rather artificial. Just tell me.

Signed-off-by Andre Przywara <[email protected]>

Regards,
Andre.

Signature · 2 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany

commit 0a3c93adcc25763e177c3cb37806ad0cba0ab702

Author: Andre Przywara <[email protected]>
Date:   Thu Apr 14 11:11:37 2011 +0200

refactor xl command line parsing into a separate function

xl command options are currently handled in each command's sub function, leading to a lot of duplicate code.
This patch moves the common part of it into a separate function, which handles the help switch, unknown options and an insufficient number of parameters. This removes a lot of redundant code.

Signed-off-by Andre Przywara <[email protected]>
tools/libxl/xl_cmdimpl.c +162 -675
index b64fcc8..946d988 100644
@@ -1735,6 +1735,31 @@ static int64_t parse_mem_size_kb(const char *mem)
     return kbytes;
 }
 
+static int def_getopt(int argc, char * const argv[], const char *optstring,
+                      const char* helpstr, int reqargs)
+{
+    int opt;
+
+    opterr = 0;
+    while ((opt = getopt(argc, argv, optstring)) == '?') {
+        if (optopt == 'h') {
+            help(helpstr);
+            return 0;
+        }
+        fprintf(stderr, "option `%c' not supported.\n", optopt);
+    }
+    if (opt != -1)
+        return opt;
+
+    if (argc - optind <= reqargs - 1) {
+        fprintf(stderr, "'xl %s' requires at least %d argument%s.\n\n",
+                helpstr, reqargs, reqargs > 1 ? "s" : "");
+        help(helpstr);
+        return 2;
+    }
+    return -1;
+}
+
 static int set_memory_max(const char *p, const char *mem)
 {
     int64_t memorykb;

@@ -1759,20 +1784,8 @@ int main_memmax(int argc, char **argv)
     char *p = NULL, *mem;
     int rc;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("mem-max");
-            exit(0);
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 1) {
-        help("mem-max");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "mem-max", 2)) != -1)
+        return opt;
 
     p = argv[optind];
     mem = argv[optind + 1];

@@ -1806,20 +1819,8 @@ int main_memset(int argc, char **argv)
     int opt = 0;
     const char *p = NULL, *mem;
 
-    while ((opt = getopt(argc, argv, "h:")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("mem-set");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 1) {
-        help("mem-set");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "mem-set", 2)) != -1)
+        return opt;
 
     p = argv[optind];
     mem = argv[optind + 1];

@@ -1854,20 +1855,8 @@ int main_cd_eject(int argc, char **argv)
     int opt = 0;
     const char *p = NULL, *virtdev;
 
-    while ((opt = getopt(argc, argv, "hn:")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cd-eject");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 1) {
-        help("cd-eject");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "cd-eject", 2)) != -1)
+        return opt;
 
     p = argv[optind];
     virtdev = argv[optind + 1];

@@ -1882,20 +1871,8 @@ int main_cd_insert(int argc, char **argv)
     const char *p = NULL, *virtdev;
     char *file = NULL; /* modified by cd_insert tokenising it */
 
-    while ((opt = getopt(argc, argv, "hn:")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cd-insert");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 2) {
-        help("cd-insert");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "cd-insert", 3)) != -1)
+        return opt;
 
     p = argv[optind];
     virtdev = argv[optind + 1];

@@ -1910,11 +1887,10 @@ int main_console(int argc, char **argv)
     int opt = 0, num = 0;
     libxl_console_constype type = 0;
 
-    while ((opt = getopt(argc, argv, "hn:t:")) != -1) {
+    while ((opt = def_getopt(argc, argv, "n:t:", "console", 1)) != -1) {
         switch (opt) {
-        case 'h':
-            help("console");
-            return 0;
+        case 0: case 2:
+            return opt;
         case 't':
             if (!strcmp(optarg, "pv"))
                 type = LIBXL_CONSTYPE_PV;

@@ -1928,15 +1904,8 @@ int main_console(int argc, char **argv)
         case 'n':
             num = atoi(optarg);
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
-    if (optind >= argc) {
-        help("console");
-        return 2;
-    }
 
     find_domain(argv[optind]);
     if (!type)

@@ -2011,16 +1980,8 @@ int main_pcilist_assignable(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("pci-list-assignable-devices");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "pci-list-assignable-devices", 0)) != -1)
+        return opt;
 
     pcilist_assignable();
     return 0;

@@ -2050,20 +2011,8 @@ int main_pcilist(int argc, char **argv)
     int opt;
     const char *domname = NULL;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("pci-list");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("pci-list");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "pci-list", 1)) != -1)
+        return opt;
 
     domname = argv[optind];
 

@@ -2092,23 +2041,15 @@ int main_pcidetach(int argc, char **argv)
     int force = 0;
     const char *domname = NULL, *bdf = NULL;
 
-    while ((opt = getopt(argc, argv, "hf")) != -1) {
+    while ((opt = def_getopt(argc, argv, "f", "pci-detach", 2)) != -1) {
         switch (opt) {
-        case 'h':
-            help("pci-detach");
-            return 0;
+        case 0: case 2:
+            return opt;
         case 'f':
             force = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
-    if (optind >= argc - 1) {
-        help("pci-detach");
-        return 2;
-    }
 
     domname = argv[optind];
     bdf = argv[optind + 1];

@@ -2136,20 +2077,8 @@ int main_pciattach(int argc, char **argv)
     int opt;
     const char *domname = NULL, *bdf = NULL, *vs = NULL;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("pci-attach");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 1) {
-        help("pci-attach");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "pci-attach", 2)) != -1)
+        return opt;
 
     domname = argv[optind];
     bdf = argv[optind + 1];

@@ -2791,8 +2720,10 @@ int main_restore(int argc, char **argv)
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0;
     int opt, rc;
 
-    while ((opt = getopt(argc, argv, "chpde")) != -1) {
+    while ((opt = def_getopt(argc, argv, "cpde", "restore", 1)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'c':
             console_autoconnect = 1;
             break;

@@ -2805,12 +2736,6 @@ int main_restore(int argc, char **argv)
         case 'e':
             daemonize = 0;
             break;
-        case 'h':
-            help("restore");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -2845,21 +2770,16 @@ int main_migrate_receive(int argc, char **argv)
     int debug = 0, daemonize = 1;
     int opt;
 
-    while ((opt = getopt(argc, argv, "hed")) != -1) {
+    while ((opt = def_getopt(argc, argv, "ed", "migrate-receive", 0)) != -1) {
         switch (opt) {
-        case 'h':
-            help("migrate-receive");
-            return 2;
-            break;
+        case 0: case 2:
+            return opt;
         case 'e':
             daemonize = 0;
             break;
         case 'd':
             debug = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -2878,21 +2798,17 @@ int main_save(int argc, char **argv)
     int checkpoint = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "hc")) != -1) {
+    while ((opt = def_getopt(argc, argv, "c", "save", 1)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'c':
             checkpoint = 1;
             break;
-        case 'h':
-            help("save");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
-    if (argc-optind < 1 || argc-optind > 3) {
+    if (argc-optind > 3) {
         help("save");
         return 2;
     }

@@ -2913,11 +2829,10 @@ int main_migrate(int argc, char **argv)
     char *host;
     int opt, daemonize = 1, debug = 0;
 
-    while ((opt = getopt(argc, argv, "hC:s:ed")) != -1) {
+    while ((opt = def_getopt(argc, argv, "C:s:ed", "migrate", 2)) != -1) {
         switch (opt) {
-        case 'h':
-            help("migrate");
-            return 0;
+        case 0: case 2:
+            return opt;
         case 'C':
             config_filename = optarg;
             break;

@@ -2930,17 +2845,9 @@ int main_migrate(int argc, char **argv)
         case 'd':
             debug = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 
-    if (argc-optind < 2 || argc-optind > 2) {
-        help("migrate");
-        return 2;
-    }
-
     p = argv[optind];
     host = argv[optind + 1];
 

@@ -2961,20 +2868,10 @@ int main_migrate(int argc, char **argv)
 int main_dump_core(int argc, char **argv)
 {
     int opt;
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("dump-core");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if ( argc-optind < 2 ) {
-        help("dump-core");
-        return 2;
-    }
+
+    if ((opt = def_getopt(argc, argv, "", "dump-core", 2)) != -1)
+        return opt;
+
     core_dump_domain(argv[optind], argv[optind + 1]);
     return 0;
 }

@@ -2982,80 +2879,35 @@ int main_dump_core(int argc, char **argv)
 int main_pause(int argc, char **argv)
 {
     int opt;
-    const char *p;
-    
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("pause");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("pause");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "pause", 1)) != -1)
+        return opt;
 
-    p = argv[optind];
+    pause_domain(argv[optind]);
 
-    pause_domain(p);
     return 0;
 }
 
 int main_unpause(int argc, char **argv)
 {
     int opt;
-    const char *p;
-    
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("unpause");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("unpause");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "unpause", 1)) != -1)
+        return opt;
 
-    p = argv[optind];
+    unpause_domain(argv[optind]);
 
-    unpause_domain(p);
     return 0;
 }
 
 int main_destroy(int argc, char **argv)
 {
     int opt;
-    const char *p;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("destroy");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("destroy");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "destroy", 1)) != -1)
+        return opt;
 
-    p = argv[optind];
-
-    destroy_domain(p);
+    destroy_domain(argv[optind]);
     return 0;
 }
 

@@ -3063,57 +2915,32 @@ int main_shutdown(int argc, char **argv)
 {
     int opt;
     int wait = 0;
-    const char *p;
 
-    while ((opt = getopt(argc, argv, "hw")) != -1) {
+    while ((opt = def_getopt(argc, argv, "w", "shutdown", 1)) != -1) {
         switch (opt) {
-        case 'h':
-            help("shutdown");
-            return 0;
+        case 0: case 2:
+            return opt;
         case 'w':
             wait = 1;
             break;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
-    if (optind >= argc) {
-        help("shutdown");
-        return 2;
-    }
-
-    p = argv[optind];
 
-    shutdown_domain(p, wait);
+    shutdown_domain(argv[optind], wait);
     return 0;
 }
 
 int main_reboot(int argc, char **argv)
 {
     int opt;
-    const char *p;
-
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("reboot");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("reboot");
-        return 2;
-    }
 
-    p = argv[optind];
+    if ((opt = def_getopt(argc, argv, "", "reboot", 1)) != -1)
+        return opt;
 
-    reboot_domain(p);
+    reboot_domain(argv[optind]);
     return 0;
 }
+
 int main_list(int argc, char **argv)
 {
     int opt, verbose = 0;

@@ -3191,16 +3018,8 @@ int main_list_vm(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("list-vm");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "list-vm", 0)) != -1)
+        return opt;
 
     list_vm();
     return 0;

@@ -3317,28 +3136,12 @@ static void button_press(const char *p, const char *b)
 int main_button_press(int argc, char **argv)
 {
     int opt;
-    const char *p;
-    const char *b;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("button-press");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc - 1) {
-        help("button-press");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "button-press", 2)) != -1)
+        return opt;
 
-    p = argv[optind];
-    b = argv[optind + 1];
+    button_press(argv[optind], argv[optind + 1]);
 
-    button_press(p, b);
     return 0;
 }
 

@@ -3481,16 +3284,8 @@ int main_vcpulist(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("vcpu-list");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpu-list", 0)) != -1)
+        return opt;
 
     vcpulist(argc - optind, argv + optind);
     return 0;

@@ -3572,21 +3367,8 @@ int main_vcpupin(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("vcpu-pin");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-
-    if (optind != argc - 3) {
-        help("vcpu-pin");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "vcpu-pin", 3)) != -1)
+        return opt;
 
     vcpupin(argv[optind], argv[optind+1] , argv[optind+2]);
     return 0;

@@ -3623,21 +3405,9 @@ int main_vcpuset(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-        help("vcpu-set");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "vcpu-set", 2)) != -1)
+        return opt;
 
-    if (optind >= argc - 1) {
-        help("vcpu-set");
-        return 2;
-    }
     vcpuset(argv[optind], argv[optind+1]);
     return 0;
 }

@@ -3848,8 +3618,10 @@ int main_sched_credit(int argc, char **argv)
     int weight = 256, cap = 0, opt_w = 0, opt_c = 0;
     int opt, rc;
 
-    while ((opt = getopt(argc, argv, "hd:w:c:")) != -1) {
+    while ((opt = def_getopt(argc, argv, "d:w:c:", "sched-credit", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'd':
             dom = optarg;
             break;

@@ -3861,12 +3633,6 @@ int main_sched_credit(int argc, char **argv)
             cap = strtol(optarg, NULL, 10);
             opt_c = 1;
             break;
-        case 'h':
-            help("sched-credit");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -3918,23 +3684,10 @@ int main_domid(int argc, char **argv)
     int opt;
     const char *domname = NULL;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("domid");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "domid", 1)) != -1)
+        return opt;
 
     domname = argv[optind];
-    if (!domname) {
-        fprintf(stderr, "Must specify a domain name.\n\n");
-        help("domid");
-        return 1;
-    }
 
     if (libxl_name_to_domid(ctx, domname, &domid)) {
         fprintf(stderr, "Can't get domid of domain name '%s', maybe this domain does not exist.\n", domname);

@@ -3952,22 +3705,9 @@ int main_domname(int argc, char **argv)
     char *domname = NULL;
     char *endptr = NULL;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("domname");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "domname", 1)) != -1)
+        return opt;
 
-    if (!argv[optind]) {
-        fprintf(stderr, "Must specify a domain id.\n\n");
-        help("domname");
-        return 1;
-    }
     domid = strtol(argv[optind], &endptr, 10);
     if (domid == 0 && !strcmp(endptr, argv[optind])) {
         /*no digits at all*/

@@ -3993,23 +3733,10 @@ int main_rename(int argc, char **argv)
     const char *dom;
     const char *new_name;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("rename");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "rename", 2)) != -1)
+        return opt;
 
     dom = argv[optind++];
-    if (!dom || !argv[optind]) {
-        fprintf(stderr, "'xl rename' requires 2 arguments.\n\n");
-        help("rename");
-        return 1;
-    }
 
     find_domain(dom);
     new_name = argv[optind];

@@ -4030,23 +3757,10 @@ int main_trigger(int argc, char **argv)
     const char *dom = NULL;
     int vcpuid = 0;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("trigger");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "trigger", 2)) != -1)
+        return opt;
 
     dom = argv[optind++];
-    if (!dom || !argv[optind]) {
-        fprintf(stderr, "'xl trigger' requires between 2 and 3 arguments.\n\n");
-        help("trigger");
-        return 1;
-    }
 
     find_domain(dom);
 

@@ -4071,23 +3785,10 @@ int main_sysrq(int argc, char **argv)
     const char *sysrq = NULL;
     const char *dom = NULL;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("sysrq");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "sysrq", 2)) != -1)
+        return opt;
 
     dom = argv[optind++];
-    if (!dom || !argv[optind]) {
-        fprintf(stderr, "'xl sysrq' requires 2 arguments.\n\n");
-        help("sysrq");
-        return 1;
-    }
 
     find_domain(dom);
 

@@ -4109,20 +3810,8 @@ int main_debug_keys(int argc, char **argv)
     int opt;
     char *keys;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("debug-keys");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (optind >= argc) {
-        help("debug-keys");
-        return 2;
-    }
+    if ((opt = def_getopt(argc, argv, "", "debug-keys", 1)) != -1)
+        return opt;
 
     keys = argv[optind];
 

@@ -4141,17 +3830,13 @@ int main_dmesg(int argc, char **argv)
     char *line;
     int opt, ret = 1;
 
-    while ((opt = getopt(argc, argv, "hc")) != -1) {
+    while ((opt = def_getopt(argc, argv, "c", "dmesg", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'c':
             clear = 1;
             break;
-        case 'h':
-            help("dmesg");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4171,16 +3856,8 @@ int main_top(int argc, char **argv)
 {
     int opt;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("top");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "top", 0)) != -1)
+        return opt;
 
     return system("xentop");
 }

@@ -4194,17 +3871,10 @@ int main_networkattach(int argc, char **argv)
     int i;
     unsigned int val;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("network-attach");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if ((argc-optind < 2) || (argc-optind > 11)) {
+    if ((opt = def_getopt(argc, argv, "", "network-attach", 1)) != -1)
+        return opt;
+
+    if (argc-optind > 11) {
         help("network-attach");
         return 0;
     }

@@ -4276,20 +3946,8 @@ int main_networklist(int argc, char **argv)
     libxl_nicinfo *nics;
     unsigned int nb, i;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-            case 'h':
-                help("network-list");
-                return 0;
-            default:
-                fprintf(stderr, "option `%c' not supported.\n", optopt);
-                break;
-        }
-    }
-    if (argc-optind < 1) {
-        help("network-list");
-        return 1;
-    }
+    if ((opt = def_getopt(argc, argv, "", "network-list", 0)) != -1)
+        return opt;
 
     /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
     printf("%-3s %-2s %-17s %-6s %-5s %-6s %5s/%-5s %-30s\n",

@@ -4325,20 +3983,8 @@ int main_networkdetach(int argc, char **argv)
     int opt;
     libxl_device_nic nic;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("network-detach");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (argc-optind != 2) {
-        help("network-detach");
-        return 0;
-    }
+    if ((opt = def_getopt(argc, argv, "", "network-detach", 2)) != -1)
+        return opt;
 
     if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);

@@ -4371,19 +4017,11 @@ int main_blockattach(int argc, char **argv)
     uint32_t fe_domid, be_domid = 0;
     libxl_device_disk disk = { 0 };
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("block-attach");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if ((argc-optind < 3) || (argc-optind > 5)) {
+    if ((opt = def_getopt(argc, argv, "", "block-attach", 2)) != -1)
+        return opt;
+    if (argc-optind > 5) {
         help("block-attach");
-        return 0;
+        return 2;
     }
 
     tok = strtok(argv[optind+1], ":");

@@ -4446,20 +4084,8 @@ int main_blocklist(int argc, char **argv)
     libxl_device_disk *disks;
     libxl_diskinfo diskinfo;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("block-list");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (argc-optind < 1) {
-        help("block-list");
-        return 0;
-    }
+    if ((opt = def_getopt(argc, argv, "", "block-list", 1)) != -1)
+        return opt;
 
     printf("%-5s %-3s %-6s %-5s %-6s %-8s %-30s\n",
            "Vdev", "BE", "handle", "state", "evt-ch", "ring-ref", "BE-path");

@@ -4492,20 +4118,8 @@ int main_blockdetach(int argc, char **argv)
     int opt;
     libxl_device_disk disk;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("block-detach");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
-    if (argc-optind != 2) {
-        help("block-detach");
-        return 0;
-    }
+    if ((opt = def_getopt(argc, argv, "", "block-detach", 2)) != -1)
+        return opt;
 
     if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
         fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);

@@ -4687,17 +4301,13 @@ int main_uptime(int argc, char **argv)
     int nb_doms = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "hs")) != -1) {
+    while ((opt = def_getopt(argc, argv, "s", "uptime", 1)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 's':
             short_mode = 1;
             break;
-        case 'h':
-            help("uptime");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4719,20 +4329,16 @@ int main_tmem_list(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "alh")) != -1) {
+    while ((opt = def_getopt(argc, argv, "al", "tmem-list", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'l':
             use_long = 1;
             break;
         case 'a':
             all = 1;
             break;
-        case 'h':
-            help("tmem-list");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4763,17 +4369,13 @@ int main_tmem_freeze(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "ah")) != -1) {
+    while ((opt = def_getopt(argc, argv, "a", "tmem-freeze", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             all = 1;
             break;
-        case 'h':
-            help("tmem-freeze");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4799,17 +4401,13 @@ int main_tmem_destroy(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "ah")) != -1) {
+    while ((opt = def_getopt(argc, argv, "a", "tmem-destroy", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             all = 1;
             break;
-        case 'h':
-            help("tmem-destroy");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4835,17 +4433,13 @@ int main_tmem_thaw(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "ah")) != -1) {
+    while ((opt = def_getopt(argc, argv, "a", "tmem-thaw", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             all = 1;
             break;
-        case 'h':
-            help("tmem-thaw");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4873,8 +4467,10 @@ int main_tmem_set(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "aw:c:p:h")) != -1) {
+    while ((opt = def_getopt(argc, argv, "aw:c:p:", "tmem-set", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             all = 1;
             break;

@@ -4890,12 +4486,6 @@ int main_tmem_set(int argc, char **argv)
             compress = strtol(optarg, NULL, 10);
             opt_p = 1;
             break;
-        case 'h':
-            help("tmem-set");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4937,8 +4527,10 @@ int main_tmem_shared_auth(int argc, char **argv)
     int all = 0;
     int opt;
 
-    while ((opt = getopt(argc, argv, "au:A:h")) != -1) {
+    while ((opt = def_getopt(argc, argv, "au:A:", "tmem-shared-auth", 0)) != -1) {
         switch (opt) {
+        case 0: case 2:
+            return opt;
         case 'a':
             all = 1;
             break;

@@ -4948,12 +4540,6 @@ int main_tmem_shared_auth(int argc, char **argv)
         case 'A':
             autharg = optarg;
             break;
-        case 'h':
-            help("tmem-shared-auth");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
         }
     }
 

@@ -4991,16 +4577,8 @@ int main_tmem_freeable(int argc, char **argv)
     int opt;
     int mb;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("tmem-freeable");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "tmem-freeable", 0)) != -1)
+        return opt;
 
     mb = libxl_tmem_freeable(ctx);
     if (mb == -1)

@@ -5318,23 +4896,10 @@ int main_cpupooldestroy(int argc, char **argv)
     const char *pool;
     uint32_t poolid;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-destroy");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-destroy", 1)) != -1)
+        return opt;
 
     pool = argv[optind];
-    if (!pool) {
-        fprintf(stderr, "no cpupool specified\n");
-        help("cpupool-destroy");
-        return -ERROR_FAIL;
-    }
 
     if (cpupool_qualifier_to_cpupoolid(pool, &poolid, NULL) ||
         !libxl_cpupoolid_to_name(ctx, poolid)) {

@@ -5352,23 +4917,10 @@ int main_cpupoolrename(int argc, char **argv)
     const char *new_name;
     uint32_t poolid;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-rename");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-rename", 2)) != -1)
+        return opt;
 
     pool = argv[optind++];
-    if (!pool || !argv[optind]) {
-        fprintf(stderr, "'xl cpupool-rename' requires 2 arguments.\n\n");
-        help("cpupool-rename");
-        return 1;
-    }
 
     if (cpupool_qualifier_to_cpupoolid(pool, &poolid, NULL) ||
         !libxl_cpupoolid_to_name(ctx, poolid)) {

@@ -5395,29 +4947,10 @@ int main_cpupoolcpuadd(int argc, char **argv)
     int node;
     int n;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-cpu-add");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-cpu-add", 2)) != -1)
+        return opt;
 
     pool = argv[optind++];
-    if (!pool) {
-        fprintf(stderr, "no cpupool specified\n");
-        help("cpupool-cpu-add");
-        return -ERROR_FAIL;
-    }
-
-    if (!argv[optind]) {
-        fprintf(stderr, "no cpu specified\n");
-        help("cpupool-cpu-add");
-        return -ERROR_FAIL;
-    }
     node = -1;
     cpu = -1;
     if (strncmp(argv[optind], "node:", 5) == 0) {

@@ -5458,29 +4991,10 @@ int main_cpupoolcpuremove(int argc, char **argv)
     int node;
     int n;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-cpu-remove");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-cpu-remove", 2)) != -1)
+        return opt;
 
     pool = argv[optind++];
-    if (!pool) {
-        fprintf(stderr, "no cpupool specified\n");
-        help("cpupool-cpu-remove");
-        return -ERROR_FAIL;
-    }
-
-    if (!argv[optind]) {
-        fprintf(stderr, "no cpu specified\n");
-        help("cpupool-cpu-remove");
-        return -ERROR_FAIL;
-    }
     node = -1;
     cpu = -1;
     if (strncmp(argv[optind], "node:", 5) == 0) {

@@ -5520,30 +5034,11 @@ int main_cpupoolmigrate(int argc, char **argv)
     const char *dom;
     uint32_t domid;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-migrate");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-migrate", 2)) != -1)
+        return opt;
 
     dom = argv[optind++];
-    if (!dom) {
-       fprintf(stderr, "no domain specified\n");
-        help("cpupool-migrate");
-        return -ERROR_FAIL;
-    }
-
-    pool = argv[optind++];
-    if (!pool) {
-        fprintf(stderr, "no cpupool specified\n");
-        help("cpupool-migrate");
-        return -ERROR_FAIL;
-    }
+    pool = argv[optind];
 
     if (domain_qualifier_to_domid(dom, &domid, NULL) ||
         !libxl_domid_to_name(ctx, domid)) {

@@ -5578,16 +5073,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
     libxl_topologyinfo topology;
     libxl_dominfo info;
 
-    while ((opt = getopt(argc, argv, "h")) != -1) {
-        switch (opt) {
-        case 'h':
-            help("cpupool-numa-split");
-            return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", optopt);
-            break;
-        }
-    }
+    if ((opt = def_getopt(argc, argv, "", "cpupool-numa-split", 0)) != -1)
+        return opt;
     ret = 0;
 
     poolinfo = libxl_list_cpupool(ctx, &n_pools);

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

#1 lore raw
Quoted 8 lines · Andre Przywara

Hi,

xl command options are currently handled in each command's sub function, leading to a lot of duplicate code.
This patch moves the common part of it into a separate function, which handles the help switch, unknown options and an insufficient number of parameters. This removes a lot of redundant code.

Quoted 6 lines

Due to the high number of commands this patch is rather large. If that would help reviewers, I could split it up, though this would be rather artificial. Just tell me.

Signed-off-by Andre Przywara <[email protected]>

Thanks, I bet the diffstat looks awesome!

One thing I noticed is that the def_getopt function doesn't add 'h' to the optstring, which confused me at first but I see now that you handle that by handling it in the case where getopt returns '?', clever.

Looks like you also found a few unused options along they way ("n:" seems to have been cut-and-pasted into a bunch of incorrect places).

Due to the length I've not reviewed in great detail but I think we should just take this and fix up any fallout as it is discovered.

Ian.

author
#2 lore raw
Quoted 9 lines · Ian Campbell
Quoted 8 lines · Andre Przywara

Hi,

xl command options are currently handled in each command's sub function, leading to a lot of duplicate code.
This patch moves the common part of it into a separate function, which handles the help switch, unknown options and an insufficient number of parameters. This removes a lot of redundant code.

Quoted 6 lines
Quoted 6 lines

Due to the high number of commands this patch is rather large. If that would help reviewers, I could split it up, though this would be rather artificial. Just tell me.

Signed-off-by Andre Przywara<[email protected]>

Ian,

thanks for the review.

Quoted 1 line

Thanks, I bet the diffstat looks awesome!

 tools/libxl/xl_cmdimpl.c |  837 
+++++++++-------------------------------------
 1 files changed, 162 insertions(+), 675 deletions(-)
Quoted 3 lines

One thing I noticed is that the def_getopt function doesn't add 'h' to the optstring, which confused me at first but I see now that you handle that by handling it in the case where getopt returns '?', clever.

I tried at least three different approaches, most of them had serious drawbacks. But this one looks finally sane.

Quoted 2 lines

Looks like you also found a few unused options along they way ("n:" seems to have been cut-and-pasted into a bunch of incorrect places).

I guessed that these would be copy & paste errors. I am not sure if any of these were intentionally left in to maintain compatibility with xm, but since we only warn on extra options (and don't break) I decided to remove them. While testing I found some artifacts in the help messages, I will check if they are not yet implemented options or if they can go away, too.

One thing I was not sure about is whether we want to break on not recognized options. Currently we just warn, which could be easily overseen by users, especially with longish outputs. This is now a trivial one-liner, though.

Quoted 2 lines

Due to the length I've not reviewed in great detail but I think we should just take this and fix up any fallout as it is discovered.

Sounds fair. I also only tested some selected commands and checked the rest with some greps and diffs.

Regards,
Andre.

Signature · 2 lines
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
#3 lore raw
In reply to Andre Przywara

Andre Przywara writes ("[Xen-devel] [PATCH] xl: refactor common parts of command line parsing"):

Quoted 5 lines

xl command options are currently handled in each command's sub function, leading to a lot of duplicate code.
This patch moves the common part of it into a separate function, which handles the help switch, unknown options and an insufficient number of parameters. This removes a lot of redundant code.

Thanks, I have applied this.

Ian.