Re: [Xen-devel] [PATCH 2 of 2] xl: Introduce helper macro for option parsing

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-12-13 at 15:05 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH 2 of 2] xl: Introduce helper macro for option 
> parsing"):
> > - s/FOREACH_OPT/SWITCH_FOREACH_OPT/
> > - Document the macro
> 
> Thanks.
> 
> > +/*
> > + * Wraps def_getopt into a convenient loop+switch to process all arguments.
> > + *
> > + * _opt:        an int variable, holds the current option during 
> > processing.
> > + * _opts:       short options, as per getopt_long(3)'s optstring argument.
> > + * _lopts:      long options, as per getopt_long(3)'s longopts argument. 
> > May
> > + *              be null.
> > + * _help:       name of this command, for usage string.
> > + * _req:        number of non-option command line parameters which are 
> > required.
> 
> Can we have a pseudo-prototype for this ?  Eg
> 
>     *   SWITCH_FOREACH_OPT(int &opt, const char *opts,
>     *                      const struct option *longopts,
>     *                      const char *commandname,
>     *                      int num_opts_req) { ...
>     *    case ...

Will do.

> Also, there is no need to prefix macro formal arguments with _.  Why
> do you do that ?  We don't do it elsewhere in libxl...

Not sure why I did that, will remove them.

> > + * Callers should treat SWITCH_FOREACH_OPT as they would a switch
> > + * statement over the value of _opt. Each option given in _opts (or
> > + * _lopts) should be handled by a case statement as if it were inside
> > + * a switch statement.
> > + *
> > + * In addition to the options provided in _opts callers must handle
> > + * two additional pseudo options:
> > + *  0 -- generated if the user passes a -h option. help will be printed,
> > + *       caller should return 0.
> > + *  2 -- generated if the user does not provided _req non-option arguments,
> > + *       caller should return 2.
> 
> I don't think you can mean "caller should return".  Your description
> of the macro doesn't specify anything in particular about the calling
> function so it can't possibly intend for you to return particular
> values from it.
> 
> Did you mean "cause the program to exit" ?

The existing (somewhat inferred, but it seems to be reasonably
consistent) semantics of xl's main_foo() dispatchers is to return 2,
causing the xl's main to exit under these circumstances. Why 2? I've no
idea.

Rather than trying to treat this as some sort of generic helper I'd be
more inclined to just document that it is to be called only by xl
main_foo() functions, since its main purpose is to make main_foo() more
consistent in their argument handling.

I originally preferred return from main_foo() over exit(2) so that we
could have a chance to free the xl context etc (mostly for the benefit
of people using valgrind). We have an atexit hook now so perhaps that
concern is no longer valid.

> And if so why not have the macro (or the function) do that ?

I don't like returns in macros.

> I haven't gone through your call sites to review them...
> 
> Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel