[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.