* [PATCH 0 of 2] xl: fixes and improvements to xl command line parsing
@ 2010-08-23 14:50 Ian Campbell
2010-08-23 14:50 ` [PATCH 1 of 2] xl: correct argument parsing for some sub-commands Ian Campbell
2010-08-23 14:50 ` [PATCH 2 of 2] xl: treat sub-command main function like a regular C main() function Ian Campbell
0 siblings, 2 replies; 3+ messages in thread
From: Ian Campbell @ 2010-08-23 14:50 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
First patch fixes a bunch of sub-commands which are broken due to not
correctly following the need to parse options wrt the global optind.
Second patch then does away with the confusing need to parse options
wrt the global optind.
(second patch isn't strictly necessary but I think it is a good idea)
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1 of 2] xl: correct argument parsing for some sub-commands
2010-08-23 14:50 [PATCH 0 of 2] xl: fixes and improvements to xl command line parsing Ian Campbell
@ 2010-08-23 14:50 ` Ian Campbell
2010-08-23 14:50 ` [PATCH 2 of 2] xl: treat sub-command main function like a regular C main() function Ian Campbell
1 sibling, 0 replies; 3+ messages in thread
From: Ian Campbell @ 2010-08-23 14:50 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1282574054 -3600
# Node ID 2210bb76868ff58c1c97738f43c52b3e893dd178
# Parent aa344916733f86fcde905953bad8b1cbc3020cd0
xl: correct argument parsing for some sub-commands.
XL sub-commands are expected to parse their arguments relative to the
global variable "optind" rather than treating argc+argv as zero
based. This is because the argc+argv passed to sub-commands include
the entire original command line, not just the sub command specific bits.
Not all commands do this and they are therefore broken if the user
uses "xl -v command", correct such problems
dump-core:
- did not handle "-h" option.
{network,network2,block}-{attach,list,detach} :
- handled arguments without reference to optind
- checked number of arguments before processing getopt loop,
breaking "-h" option handling
An example of the breakage:
# xl -v block-list d32-2
Vdev BE handle state evt-ch ring-ref BE-path
block-list is an invalid domain identifier
51712 0 1 4 13 8 /local/domain/0/backend/vbd/1/
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r aa344916733f -r 2210bb76868f tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Mon Aug 23 09:46:03 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c Mon Aug 23 15:34:14 2010 +0100
@@ -2870,6 +2870,17 @@ 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", opt);
+ break;
+ }
+ }
if ( argc-optind < 2 ) {
help("dump-core");
return 2;
@@ -4032,27 +4043,27 @@ int main_networkattach(int argc, char **
int i;
unsigned int val;
- if ((argc < 3) || (argc > 11)) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("network-attach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if ((argc-optind < 2) || (argc-optind > 11)) {
help("network-attach");
return 0;
}
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("network-attach");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
- }
-
- if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
+
+ if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
return 1;
}
init_nic_info(&nic, -1);
- for (argv += 3, argc -= 3; argc > 0; ++argv, --argc) {
+ for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
if (!strncmp("type=", *argv, 5)) {
if (!strncmp("vif", (*argv) + 5, 4)) {
nic.nictype = NICTYPE_VIF;
@@ -4113,10 +4124,6 @@ int main_networklist(int argc, char **ar
libxl_nicinfo *nics;
unsigned int nb, i;
- if (argc < 3) {
- help("network-list");
- return 1;
- }
while ((opt = getopt(argc, argv, "h")) != -1) {
switch (opt) {
case 'h':
@@ -4127,11 +4134,15 @@ int main_networklist(int argc, char **ar
break;
}
}
+ if (argc-optind < 1) {
+ help("network-list");
+ return 1;
+ }
/* Idx BE MAC Hdl Sta evch txr/rxr BE-path */
printf("%-3s %-2s %-17s %-6s %-5s %-6s %5s/%-5s %-30s\n",
"Idx", "BE", "Mac Addr.", "handle", "state", "evt-ch", "tx-", "rx-ring-ref", "BE-path");
- for (argv += 2, argc -= 2; argc > 0; --argc, ++argv) {
+ for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
continue;
@@ -4162,34 +4173,34 @@ int main_networkdetach(int argc, char **
int opt;
libxl_device_nic nic;
- if (argc != 4) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("network-detach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if (argc-optind != 2) {
help("network-detach");
return 0;
}
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("network-detach");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
- }
-
- if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
- return 1;
- }
-
- if (!strchr(argv[3], ':')) {
- if (libxl_devid_to_device_nic(&ctx, domid, argv[3], &nic)) {
- fprintf(stderr, "Unknown device %s.\n", argv[3]);
- return 1;
- }
- } else {
- if (libxl_mac_to_device_nic(&ctx, domid, argv[3], &nic)) {
- fprintf(stderr, "Unknown device %s.\n", argv[3]);
+
+ if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+ return 1;
+ }
+
+ if (!strchr(argv[optind+1], ':')) {
+ if (libxl_devid_to_device_nic(&ctx, domid, argv[optind+1], &nic)) {
+ fprintf(stderr, "Unknown device %s.\n", argv[optind+1]);
+ return 1;
+ }
+ } else {
+ if (libxl_mac_to_device_nic(&ctx, domid, argv[optind+1], &nic)) {
+ fprintf(stderr, "Unknown device %s.\n", argv[optind+1]);
return 1;
}
}
@@ -4208,22 +4219,22 @@ int main_blockattach(int argc, char **ar
uint32_t fe_domid, be_domid = 0;
libxl_device_disk disk = { 0 };
- if ((argc < 5) || (argc > 7)) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("block-attach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if ((argc-optind < 3) || (argc-optind > 5)) {
help("block-attach");
return 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", opt);
- break;
- }
- }
-
- tok = strtok(argv[3], ":");
+
+ tok = strtok(argv[optind+1], ":");
if (!strcmp(tok, "phy")) {
disk.phystype = PHYSTYPE_PHY;
} else if (!strcmp(tok, "file")) {
@@ -4251,22 +4262,23 @@ int main_blockattach(int argc, char **ar
fprintf(stderr, "Error: missing path to disk image.\n");
return 1;
}
- disk.virtpath = argv[4];
+ disk.virtpath = argv[optind+2];
disk.unpluggable = 1;
- disk.readwrite = ((argc <= 4) || (argv[5][0] == 'w'));
-
- if (domain_qualifier_to_domid(argv[2], &fe_domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
- return 1;
- }
- if (argc == 7) {
- if (domain_qualifier_to_domid(argv[6], &be_domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[6]);
+ disk.readwrite = ((argc-optind <= 2) || (argv[optind+3][0] == 'w'));
+
+ if (domain_qualifier_to_domid(argv[optind], &fe_domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+ return 1;
+ }
+ if (argc-optind == 5) {
+ if (domain_qualifier_to_domid(argv[optind+4], &be_domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind+4]);
return 1;
}
}
disk.domid = fe_domid;
disk.backend_domid = be_domid;
+
if (libxl_device_disk_add(&ctx, fe_domid, &disk)) {
fprintf(stderr, "libxl_device_disk_add failed.\n");
}
@@ -4280,24 +4292,24 @@ int main_blocklist(int argc, char **argv
libxl_device_disk *disks;
libxl_diskinfo diskinfo;
- if (argc < 3) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("block-list");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if (argc-optind < 1) {
help("block-list");
return 0;
- }
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("block-list");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
}
printf("%-5s %-3s %-6s %-5s %-6s %-8s %-30s\n",
"Vdev", "BE", "handle", "state", "evt-ch", "ring-ref", "BE-path");
- for (argv += 2, argc -= 2; argc > 0; --argc, ++argv) {
+ for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
continue;
@@ -4326,27 +4338,27 @@ int main_blockdetach(int argc, char **ar
int opt;
libxl_device_disk disk;
- if (argc != 4) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("block-detach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if (argc-optind != 2) {
help("block-detach");
return 0;
}
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("block-detach");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
- }
-
- if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
- return 1;
- }
- if (libxl_devid_to_device_disk(&ctx, domid, argv[3], &disk)) {
- fprintf(stderr, "Error: Device %s not connected.\n", argv[3]);
+
+ if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+ return 1;
+ }
+ if (libxl_devid_to_device_disk(&ctx, domid, argv[optind+1], &disk)) {
+ fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
return 1;
}
if (libxl_device_disk_del(&ctx, &disk, 1)) {
@@ -4364,27 +4376,27 @@ int main_network2attach(int argc, char *
unsigned int val, i;
libxl_device_net2 net2;
- if ((argc < 3) || (argc > 12)) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("network2-attach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if ((argc-optind < 1) || (argc-optind > 10)) {
help("network2-attach");
return 0;
}
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("network2-attach");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
- }
-
- if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[1]);
+
+ if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
return 1;
}
init_net2_info(&net2, -1);
- for (argv += 3, argc -= 3; argc > 0; --argc, ++argv) {
+ for (argv += optind+1, argc -= optind+1; argc > 0; --argc, ++argv) {
if (!strncmp("front_mac=", *argv, 10)) {
tok = strtok((*argv) + 10, ":");
for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
@@ -4457,25 +4469,25 @@ int main_network2list(int argc, char **a
unsigned int nb;
libxl_net2info *net2s;
- if (argc < 3) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("network2-list");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if (argc - optind < 1) {
help("network2-list");
return 0;
- }
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("network2-list");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
}
printf("%-3s %-2s %-5s %-17s %-17s %-7s %-6s %-30s\n",
"Idx", "BE", "state", "Mac Addr.", "Remote Mac Addr.",
"trusted", "filter", "backend");
- for (argv += 2, argc -=2; argc > 0; --argc, ++argv) {
+ for (argv += optind, argc -=optind; argc > 0; --argc, ++argv) {
if (domain_qualifier_to_domid(*argv, &domid, 0) < 0) {
fprintf(stderr, "%s is an invalid domain identifier\n", *argv);
continue;
@@ -4501,27 +4513,27 @@ int main_network2detach(int argc, char *
int opt;
libxl_device_net2 net2;
- if (argc != 4) {
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("network2-detach");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+ if (argc-optind != 2) {
help("network2-detach");
return 0;
}
- while ((opt = getopt(argc, argv, "h")) != -1) {
- switch (opt) {
- case 'h':
- help("network2-detach");
- return 0;
- default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
- break;
- }
- }
-
- if (domain_qualifier_to_domid(argv[2], &domid, 0) < 0) {
- fprintf(stderr, "%s is an invalid domain identifier\n", argv[2]);
- return 1;
- }
- if (libxl_devid_to_device_net2(&ctx, domid, argv[3], &net2)) {
- fprintf(stderr, "Error: Device %s not connected.\n", argv[3]);
+
+ if (domain_qualifier_to_domid(argv[optind], &domid, 0) < 0) {
+ fprintf(stderr, "%s is an invalid domain identifier\n", argv[optind]);
+ return 1;
+ }
+ if (libxl_devid_to_device_net2(&ctx, domid, argv[optind+1], &net2)) {
+ fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
return 1;
}
if (libxl_device_net2_del(&ctx, &net2, 1)) {
diff -r aa344916733f -r 2210bb76868f tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c Mon Aug 23 09:46:03 2010 +0100
+++ b/tools/libxl/xl_cmdtable.c Mon Aug 23 15:34:14 2010 +0100
@@ -241,7 +241,7 @@ struct cmd_spec cmd_table[] = {
"Create a new virtual network device",
"<Domain> [type=<type>] [mac=<mac>] [bridge=<bridge>] "
"[ip=<ip>] [script=<script>] [backend=<BackDomain>] [vifname=<name>] "
- "[rate=<rate>] [model=<model>][accel=<accel>]",
+ "[rate=<rate>] [model=<model>] [accel=<accel>]",
},
{ "network-list",
&main_networklist,
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 2 of 2] xl: treat sub-command main function like a regular C main() function
2010-08-23 14:50 [PATCH 0 of 2] xl: fixes and improvements to xl command line parsing Ian Campbell
2010-08-23 14:50 ` [PATCH 1 of 2] xl: correct argument parsing for some sub-commands Ian Campbell
@ 2010-08-23 14:50 ` Ian Campbell
1 sibling, 0 replies; 3+ messages in thread
From: Ian Campbell @ 2010-08-23 14:50 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1282574865 -3600
# Node ID c336fbc9175a6b5fec8a5cdf67bbe8354aacd5d0
# Parent 2210bb76868ff58c1c97738f43c52b3e893dd178
xl: treat sub-command main function like a regular C main() function
Currently xl passes the entire argc+argv to each subcommand and relies
on the preservation of the global optind variable to ensure that the
subcommand correctly handles argument parsing (e.g. accounting for "xl
[command]" vs "xl -v [command]").
This requirement for individual sub-commands to parse arguments
relative to optind is subtle and prone to being forgotten (or simply
not expected). Several sub-commands have recently been broken in this
way (now fixed).
Therefore arrange that the argv+argc passed to the sub-commands looks
like you would expect for a regular C main function and includes
argv[0] equal to the command name with command specific arguments in
argv[1] onwards.
Since all sub-commands (currently) correctly obey the optind it is
sufficient to reset it to 1 (as described in getopt(3)) in order to
not break the sub-commands own argument parsing.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r 2210bb76868f -r c336fbc9175a tools/libxl/xl.c
--- a/tools/libxl/xl.c Mon Aug 23 15:34:14 2010 +0100
+++ b/tools/libxl/xl.c Mon Aug 23 15:47:45 2010 +0100
@@ -53,7 +53,7 @@ int main(int argc, char **argv)
}
}
- cmd = argv[optind++];
+ cmd = argv[optind];
if (!cmd) {
help(NULL);
@@ -69,13 +69,18 @@ int main(int argc, char **argv)
exit(1);
}
+ /* Reset options for per-command use of getopt. */
+ argv += optind;
+ argc -= optind;
+ optind = 1;
+
srand(time(0));
cspec = cmdtable_lookup(cmd);
if (cspec)
ret = cspec->cmd_impl(argc, argv);
else if (!strcmp(cmd, "help")) {
- help(argv[optind]);
+ help(argv[1]);
ret = 0;
} else {
fprintf(stderr, "command not implemented\n");
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-08-23 14:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-23 14:50 [PATCH 0 of 2] xl: fixes and improvements to xl command line parsing Ian Campbell
2010-08-23 14:50 ` [PATCH 1 of 2] xl: correct argument parsing for some sub-commands Ian Campbell
2010-08-23 14:50 ` [PATCH 2 of 2] xl: treat sub-command main function like a regular C main() function Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).