Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 23 Jul 2012 20:33:52 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Sat, 21 Jul 2012 10:11:39 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > Allow for specifying an alias for each option name, see next commits
>> >> > for examples.
>> >> >
>> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> > ---
>> >> >  qemu-option.c | 5 +++--
>> >> >  qemu-option.h | 1 +
>> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/qemu-option.c b/qemu-option.c
>> >> > index 65ba1cf..b2f9e21 100644
>> >> > --- a/qemu-option.c
>> >> > +++ b/qemu-option.c
>> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>> >> >      int i;
>> >> >  
>> >> >      for (i = 0; desc[i].name != NULL; i++) {
>> >> > -        if (strcmp(desc[i].name, name) == 0) {
>> >> > +        if (strcmp(desc[i].name, name) == 0 ||
>> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>> >> >              return &desc[i];
>> >> >          }
>> >> >      }
>> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>> 
>>       static void opt_set(QemuOpts *opts, const char *name, const 
>>                           bool prepend, Error **errp)
>>       {
>>           QemuOpt *opt;
>>           const QemuOptDesc *desc;
>>           Error *local_err = NULL;
>> 
>>           desc = find_desc_by_name(opts->list->desc, name);
>>           if (!desc && !opts_accepts_any(opts)) {
>>               error_set(errp, QERR_INVALID_PARAMETER, name);
>>               return;
>> >> >      }
>> >> >  
>> >> >      opt = g_malloc0(sizeof(*opt));
>> >> > -    opt->name = g_strdup(name);
>> >> > +    opt->name = g_strdup(desc ? desc->name : name);
>> >> >      opt->opts = opts;
>> >> >      if (prepend) {
>> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
>> >> 
>> >> Are you sure this hunk belongs to this patch?  If yes, please explain
>> >> why :)
>> >
>> > Yes, I think it's fine because the change that makes it necessary to choose
>> > between desc->name and name is introduced by the hunk above.
>> >
>> 
>> I think I now get why you have this hunk:
>> 
>> We reach it only if the parameter with this name exists (desc non-null),
>> or opts accepts anthing (opts_accepts_any(opts).
>> 
>> If it exists, name equals desc->name before your patch.  But afterwards,
>> it could be either desc->name or desc->alias.  You normalize to
>> desc->name.
>> 
>> Else, all we can do is stick to name.
>> 
>> Correct?
>
> Yes.
>
>> If yes, then "normal" options and "accept any" options behave
>> differently: the former set opts->name to the canonical name, even when
>> the user uses an alias.  The latter set opts->name to whatever the user
>> uses.  I got a bad feeling about that.
>
> Why? Or, more importantly, how do you think we should do it?
>
> For 'normal' options, the alias is just a different name to refer to the
> option name. At least in theory, it shouldn't matter that the option was
> set through the alias.
>
> For 'accept any' options, it's up to the code handling the options know
> what to do with it. It can also support aliases if it wants to, or just
> refuse it.

Let's examine how opt->name is used.

* qemu_opt_find(): helper for the qemu_opt_get*() functions, compares
  opt->name to argument name.  opt->name must be a canonical name for
  that to work.

* qemu_opt_parse(): passes opt->name to parse_option_*() functions,
  which use it for error reporting.  opt->name should be whatever the
  user used, or else the error messages will confusing.

* qemu_opt_del(): passes it to g_free().

* qemu_opt_foreach(): passes it to the callback.  Whether the callback
  expects the canonical name or what the user used is unclear.  Current
  callbacks:

  - config_write_opt(): either works.  With the canonical name,
    -writeconfig canconicalizes option parameters.  With the user's
    name, it sticks to what the user used.

  - set_property(): compares it to qdev property names.  Needs canonical
    name.

  - net_init_slirp_configs(): compares it to string literals.  Needs
    canonical name.

  - cpudef_setfield(): compares it to string literals, and puts it into
    error messages.  The former needs the canonical name, the latter
    user's name.  Fun.

  - add_channel(): compares it to string literals.  Needs canonical
    name.

* qemu_opts_print(): unused.  Whether to print canonical name or user's
  name is unclear.

* qemu_opts_to_qdict(): only used for monitor argument type 'O'.  Needs
  canonical name.

* qemu_opts_validate(): expects user's names.

I think we need to define the meaning of opt->name.  We might have to
provide both names.

Your patch sets it to the canonical name unless desc is empty.  Aliases
break qemu_opt_parse() error reporting.  Looks fixable, e.g. set
opt->name to the user's name first, change it to the canonical name
after qemu_opt_parse().

Your patch sets it to the user's name if desc is empty, i.e. for
qemu_device_opts, qemu_netdev_opts, qemu_net_opts.

qemu_device_opts is handed off to set_property().  Bypasses you alias
code completely, thus no problem.

The other two get passed to qemu_opts_validate().  Since
qemu_opts_validate() doesn't change opts->name, the error messages from
qemu_opt_parse() are fine here.  But aliases break the later
qemu_opt_get() calls.  Fixable the same way: change opt->name to the
canonical name after qemu_opt_parse().

Instead of overloading opt->name to mean user's name before parse and
canonical name afterwards, you may want to provide separate QemuOpts
members.