Ian Campbell
2010-Aug-23 14:50 UTC
[Xen-devel] [PATCH 0 of 2] xl: fixes and improvements to xl command line parsing
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Aug-23 14:50 UTC
[Xen-devel] [PATCH 1 of 2] xl: correct argument parsing for some sub-commands
# 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,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2010-Aug-23 14:50 UTC
[Xen-devel] [PATCH 2 of 2] xl: treat sub-command main function like a regular C main() function
# 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");
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel