Re: [PATCH 09/13] parse-options API: don't restrict OPT_SUBCOMMAND() to one *_fn type

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

 
On 2022-11-05 09:32:44+0100, René Scharfe <l.s.r@web.de> wrote:
> > diff --git a/parse-options.h b/parse-options.h
> > index b6ef86e0d15..61e3016c3fc 100644
> > --- a/parse-options.h
> > +++ b/parse-options.h
> > @@ -128,19 +128,24 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
> >   *			 the option takes optional argument.
> >   *
> >   * `callback`::
> > - *   pointer to the callback to use for OPTION_CALLBACK
> > + *   pointer to the callback to use for OPTION_CALLBACK and OPTION_SUBCOMMAND.
> >   *
> >   * `defval`::
> >   *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
> >   *   OPTION_{BIT,SET_INT} store the {mask,integer} to put in the value when met.
> > + *   OPTION_SUBCOMMAND stores the pointer the function selected for
> > + *   the subcommand.
> > + *
> >   *   CALLBACKS can use it like they want.
> >   *
> >   * `ll_callback`::
> >   *   pointer to the callback to use for OPTION_LOWLEVEL_CALLBACK
> >   *
> >   * `subcommand_fn`::
> > - *   pointer to a function to use for OPTION_SUBCOMMAND.
> > - *   It will be put in value when the subcommand is given on the command line.
> > + *   pointer to the callback used with OPT_SUBCOMMAND() and
> > + *   OPT_SUBCOMMAND_F(). Internally we store the same value in
> > + *   `defval`. This is only here to give the OPT_SUBCOMMAND{,_F}()
> > + *   common case type safety.
> >   */
> >  struct option {
> >  	enum parse_opt_type type;
> > @@ -217,12 +222,24 @@ struct option {
> >  #define OPT_ALIAS(s, l, source_long_name) \
> >  	{ OPTION_ALIAS, (s), (l), (source_long_name) }
> >
> > +static inline int parse_options_pick_subcommand_cb(const struct option *option,
> > +						   const char *arg UNUSED,
> > +						   int unset UNUSED)
> > +{
> > +	parse_opt_subcommand_fn *fn = (parse_opt_subcommand_fn *)option->defval;
> > +	*(parse_opt_subcommand_fn **)option->value = fn;
> 
> ->defval is of type intptr_t and ->value is a void pointer.  The result
> of converting a void pointer value to an intptr_t and back is a void
> pointer equal to the original pointer if I read 6.3.2.3 (Pointers,
> paragraphs 5 and 6) and 7.18.1.4 (Integer types capable of holding
> object pointers) in C99 correctly.
> 
> 6.3.2.3 paragraph 8 says that casting between function pointers of
> different type is OK and you can get your original function pointer
> back and use it in a call if you convert it back to the right type.
> 
> Casting between a function pointer and an object pointer is undefined,
> though.  They don't have to be of the same size, so a function pointer
> doesn't have to fit into an intptr_t.  I wouldn't be surprised if CHERI
> (https://www.cl.cam.ac.uk/research/security/ctsrd/cheri/) was an actual
> example of that.
> 
> Why is this trickery needed?  Above you write that callbacks in
> builtin/bisect--helper.c can't use subcommand_fn because they need
> their own argument.  Can we extend subcommand_fn or use a global
> variable to pass that extra thing instead?  The latter may be ugly, but
> at least it's valid C..

Not the author, but I fully agree with you, I think instead of adding new API
for some arbitrary subcommand_fn, I would change the subcommand_fn to
type:

	int (*)(int argc, const char **argv, const char *prefix, void *context)

The last argument would be an object pointer, which will be casted to
the correct type inside the callback.

Let me cherry-picking this series on top of mine to see how things
would progress.

> > +	return 0;
> > +}
> > +
> >  #define OPT_SUBCOMMAND_F(l, v, fn, f) { \
> >  	.type = OPTION_SUBCOMMAND, \
> >  	.long_name = (l), \
> >  	.value = (v), \
> >  	.flags = (f), \
> > -	.subcommand_fn = (fn) }
> > +	.defval = (intptr_t)(fn), \
> > +	.subcommand_fn = (fn), \
> > +	.callback = parse_options_pick_subcommand_cb, \
> 
> Getting the address of an inline function feels weird, but the compiler
> is free to emit to ignore that keyword and will provide an addressable
> function object here.
> 
> > +}
> >  #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)
> >
> >  /*
> 

-- 
Danh