* [PATCH] xl: refactor common parts of command line parsing
@ 2011-04-15 7:33 Andre Przywara
2011-04-15 9:36 ` Ian Campbell
2011-05-04 13:46 ` Ian Jackson
0 siblings, 2 replies; 4+ messages in thread
From: Andre Przywara @ 2011-04-15 7:33 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 634 bytes --]
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 <andre.przywara@amd.com>
Regards,
Andre.
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
[-- Attachment #2: xl_cmd_refactor.patch --]
[-- Type: text/x-patch, Size: 37686 bytes --]
commit 0a3c93adcc25763e177c3cb37806ad0cba0ab702
Author: Andre Przywara <andre.przywara@amd.com>
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 <andre.przywara@amd.com>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b64fcc8..946d988 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -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);
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xl: refactor common parts of command line parsing
2011-04-15 7:33 [PATCH] xl: refactor common parts of command line parsing Andre Przywara
@ 2011-04-15 9:36 ` Ian Campbell
2011-04-15 12:44 ` Andre Przywara
2011-05-04 13:46 ` Ian Jackson
1 sibling, 1 reply; 4+ messages in thread
From: Ian Campbell @ 2011-04-15 9:36 UTC (permalink / raw)
To: Andre Przywara; +Cc: xen-devel, Ian Jackson
On Fri, 2011-04-15 at 08:33 +0100, Andre Przywara wrote:
> 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 <andre.przywara@amd.com>
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xl: refactor common parts of command line parsing
2011-04-15 9:36 ` Ian Campbell
@ 2011-04-15 12:44 ` Andre Przywara
0 siblings, 0 replies; 4+ messages in thread
From: Andre Przywara @ 2011-04-15 12:44 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, Ian Jackson
On 04/15/2011 11:36 AM, Ian Campbell wrote:
> On Fri, 2011-04-15 at 08:33 +0100, Andre Przywara wrote:
>> 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<andre.przywara@amd.com>
Ian,
thanks for the review.
> Thanks, I bet the diffstat looks awesome!
tools/libxl/xl_cmdimpl.c | 837
+++++++++-------------------------------------
1 files changed, 162 insertions(+), 675 deletions(-)
> 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.
> 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.
> 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.
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xl: refactor common parts of command line parsing
2011-04-15 7:33 [PATCH] xl: refactor common parts of command line parsing Andre Przywara
2011-04-15 9:36 ` Ian Campbell
@ 2011-05-04 13:46 ` Ian Jackson
1 sibling, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2011-05-04 13:46 UTC (permalink / raw)
To: Andre Przywara; +Cc: Ian Campbell, xen-devel
Andre Przywara writes ("[Xen-devel] [PATCH] xl: refactor common parts of command line parsing"):
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-05-04 13:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-15 7:33 [PATCH] xl: refactor common parts of command line parsing Andre Przywara
2011-04-15 9:36 ` Ian Campbell
2011-04-15 12:44 ` Andre Przywara
2011-05-04 13:46 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.