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

[Luiz Capitulino <lcapitulino@redhat.com>]

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.