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

[Markus Armbruster <armbru@redhat.com>]

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?

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.

> Of course that it's possible to move this to a separate patch, but I don't
> think it's worth it, as name is always valid with the current code.
>
>> > diff --git a/qemu-option.h b/qemu-option.h
>> > index 951dec3..7106d2f 100644
>> > --- a/qemu-option.h
>> > +++ b/qemu-option.h
>> > @@ -94,6 +94,7 @@ enum QemuOptType {
>> >  
>> >  typedef struct QemuOptDesc {
>> >      const char *name;
>> > +    const char *alias;
>> >      enum QemuOptType type;
>> >      const char *help;
>> >  } QemuOptDesc;
>>