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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[PATCH 5 of 5] xl: Introduce helper macro for option 
parsing"):
> xl: Introduce helper macro for option parsing.

The idea is a reasonable one, but I have some quibbles.

> +#define FOREACH_OPT(_opt, _opts, _lopts, _help, _req)           \
> +    while (((_opt) = def_getopt(argc, argv, (_opts), (_lopts),  \
> +                                (_help), (_req))) != -1)        \
> +        switch (opt)

This macro name should have the word SWITCH in it too so you know it
has to take cases and that "break" doesn't do what you think.
FOREACH_SWITCH_OPT maybe.

> @@ -2304,8 +2309,10 @@ int main_memmax(int argc, char **argv)
>      char *mem;
>      int rc;
> 
> -    if ((opt = def_getopt(argc, argv, "", NULL, "mem-max", 2)) != -1)
> +    FOREACH_OPT(opt, "", NULL, "mem-max", 2) {
> +    case 0: case 2:
>          return opt;
> +    }

This error handling behaviour (?) is rather opaque.  I RTFM
getopt_long and AFAICT it returns 0 only for certain long options and
it doesn't seem to mention returning 2.

Perhaps this would all be clearer if FOREACH_OPT had a big comment
explaining how to use it.

Thanks,
Ian.

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