Re: [PATCH 1/2] tools/xl: Sort create command options

[Elliott Mitchell <ehem+xen@xxxxxxx>]

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@xxxxxxx>
> > ---
> > 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".


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445