| Message ID | c145a7648025e9bbc2f47ab8bd5839c80c01933f.1650422518.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow use of JSON in domain configuration files | expand |
On Tue, Apr 19, 2022 at 06:56:03PM -0700, Elliott Mitchell wrote: > Hopefully simplify future changes by sorting options lists for > `xl create`. I'm not sure that sorting options make changes easier, as it would mean one has to make sure the new option is sorted as well ;-). But sorting the options in the help message is probably useful; I've looked at a few linux utilities and they tend to have them sorted so doing this for xl create sound fine. > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > --- > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c > index 435155a033..2ec4140258 100644 > --- a/tools/xl/xl_vmcontrol.c > +++ b/tools/xl/xl_vmcontrol.c > @@ -1169,13 +1169,13 @@ int main_create(int argc, char **argv) > int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0, > quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0; > int opt, rc; > - static struct option opts[] = { > + static const struct option opts[] = { Could you add a note in the commit message that "opts" is now const? > + {"defconfig", 1, 0, 'f'}, > {"dryrun", 0, 0, 'n'}, > + {"ignore-global-affinity-masks", 0, 0, 'i'}, > {"quiet", 0, 0, 'q'}, > - {"defconfig", 1, 0, 'f'}, > {"vncviewer", 0, 0, 'V'}, > {"vncviewer-autopass", 0, 0, 'A'}, > - {"ignore-global-affinity-masks", 0, 0, 'i'}, > COMMON_LONG_OPTS > }; > > @@ -1186,12 +1186,15 @@ int main_create(int argc, char **argv) > argc--; argv++; > } > > - SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) { > - case 'f': > - filename = optarg; > + SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) { The list of short options aren't really sorted here. Also -q doesn't take an argument, but -f should keep requiring one. Thanks,
On Fri, Apr 29, 2022 at 10:39:27AM +0100, Anthony PERARD wrote: > On Tue, Apr 19, 2022 at 06:56:03PM -0700, Elliott Mitchell wrote: > > Hopefully simplify future changes by sorting options lists for > > `xl create`. > > I'm not sure that sorting options make changes easier, as it would mean > one has to make sure the new option is sorted as well ;-). But sorting > the options in the help message is probably useful; I've looked at a few > linux utilities and they tend to have them sorted so doing this for xl > create sound fine. This ends up revolving around the question, is the work involved in keeping things sorted more or less than the annoyance caused by having them unsorted? I tend towards keep them sorted since I find trying to identify available option letters when they're in random order is rather troublesome. > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > > --- > > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c > > index 435155a033..2ec4140258 100644 > > --- a/tools/xl/xl_vmcontrol.c > > +++ b/tools/xl/xl_vmcontrol.c > > @@ -1169,13 +1169,13 @@ int main_create(int argc, char **argv) > > int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0, > > quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0; > > int opt, rc; > > - static struct option opts[] = { > > + static const struct option opts[] = { > > Could you add a note in the commit message that "opts" is now > const? Okay. > > + {"defconfig", 1, 0, 'f'}, > > {"dryrun", 0, 0, 'n'}, > > + {"ignore-global-affinity-masks", 0, 0, 'i'}, > > {"quiet", 0, 0, 'q'}, > > - {"defconfig", 1, 0, 'f'}, > > {"vncviewer", 0, 0, 'V'}, > > {"vncviewer-autopass", 0, 0, 'A'}, > > - {"ignore-global-affinity-masks", 0, 0, 'i'}, > > COMMON_LONG_OPTS > > }; > > > > @@ -1186,12 +1186,15 @@ int main_create(int argc, char **argv) > > argc--; argv++; > > } > > > > - SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) { > > - case 'f': > > - filename = optarg; > > + SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) { > > The list of short options aren't really sorted here. Also -q doesn't > take an argument, but -f should keep requiring one. Needed to reread the documentation on getopt() behavior. I remembered it being group before the colon didn't take options, ones after colon did take options. Instead it is colon for every option with argument. Other issue is dictionary sort versus ASCII sort. ie "AaBbCcDdEeFf..." versus "A-Za-z".
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 661323d488..f546beaceb 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -24,16 +24,16 @@ const struct cmd_spec cmd_table[] = { &main_create, 1, 1, "Create a domain from config file <filename>", "<ConfigFile> [options] [vars]", + "-c Connect to the console after the domain is created.\n" + "-d Enable debug messages.\n" + "-e Do not wait in the background for the death of the domain.\n" + "-F Run in foreground until death of the domain.\n" "-h Print this help.\n" "-p Leave the domain paused after it is created.\n" - "-c Connect to the console after the domain is created.\n" "-f FILE, --defconfig=FILE\n Use the given configuration file.\n" - "-q, --quiet Quiet.\n" "-n, --dryrun Dry run - prints the resulting configuration\n" " (deprecated in favour of global -N option).\n" - "-d Enable debug messages.\n" - "-F Run in foreground until death of the domain.\n" - "-e Do not wait in the background for the death of the domain.\n" + "-q, --quiet Quiet.\n" "-V, --vncviewer Connect to the VNC display after the domain is created.\n" "-A, --vncviewer-autopass\n" " Pass VNC password to viewer via stdin.\n" diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c index 435155a033..2ec4140258 100644 --- a/tools/xl/xl_vmcontrol.c +++ b/tools/xl/xl_vmcontrol.c @@ -1169,13 +1169,13 @@ int main_create(int argc, char **argv) int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0, quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0; int opt, rc; - static struct option opts[] = { + static const struct option opts[] = { + {"defconfig", 1, 0, 'f'}, {"dryrun", 0, 0, 'n'}, + {"ignore-global-affinity-masks", 0, 0, 'i'}, {"quiet", 0, 0, 'q'}, - {"defconfig", 1, 0, 'f'}, {"vncviewer", 0, 0, 'V'}, {"vncviewer-autopass", 0, 0, 'A'}, - {"ignore-global-affinity-masks", 0, 0, 'i'}, COMMON_LONG_OPTS }; @@ -1186,12 +1186,15 @@ int main_create(int argc, char **argv) argc--; argv++; } - SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) { - case 'f': - filename = optarg; + SWITCH_FOREACH_OPT(opt, "Ffnq:AVcdeip", opts, "create", 0) { + case 'A': + vnc = vncautopass = 1; break; - case 'p': - paused = 1; + case 'F': + daemonize = 0; + break; + case 'V': + vnc = 1; break; case 'c': console_autoconnect = 1; @@ -1199,28 +1202,25 @@ int main_create(int argc, char **argv) case 'd': debug = 1; break; - case 'F': - daemonize = 0; - break; case 'e': daemonize = 0; monitor = 0; break; + case 'f': + filename = optarg; + break; + case 'i': + ignore_masks = 1; + break; case 'n': dryrun_only = 1; break; + case 'p': + paused = 1; + break; case 'q': quiet = 1; break; - case 'V': - vnc = 1; - break; - case 'A': - vnc = vncautopass = 1; - break; - case 'i': - ignore_masks = 1; - break; } memset(&dom_info, 0, sizeof(dom_info));
Hopefully simplify future changes by sorting options lists for `xl create`. Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> --- tools/xl/xl_cmdtable.c | 10 +++++----- tools/xl/xl_vmcontrol.c | 40 ++++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 25 deletions(-)