Patchwork [4/8] qemu-option: add alias support

login
register
mail settings
Submitter Luiz Capitulino
Date July 13, 2012, 8:44 p.m.
Message ID <1342212261-19903-5-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/170962/
State New
Headers show

Comments

Luiz Capitulino - July 13, 2012, 8:44 p.m.
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(-)
Markus Armbruster - July 21, 2012, 8:11 a.m.
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,
>      }
>  
>      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 :)

> 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;
Luiz Capitulino - July 23, 2012, 4:17 p.m.
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,
> >      }
> >  
> >      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.

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;
>
Markus Armbruster - July 23, 2012, 6:33 p.m.
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;
>>
Luiz Capitulino - July 23, 2012, 8 p.m.
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.
Markus Armbruster - July 24, 2012, 9:19 a.m.
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.
Luiz Capitulino - July 24, 2012, 3:15 p.m.
On Tue, 24 Jul 2012 11:19:45 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> 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().

I'd prefer to store the alias and have a function like
qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
that either, returns the canonical name if the alias is not set, or the alias
if it's set.

Then any function that needs to know what the user passed can call
qemu_opt_get_passed_name().

What do you think?

> 
> 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().

Not sure I follow, how does this break qemu_opt_get() calls?

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

Yes, I can do that, but note that the hunk that generated this discussion
will stay the same in the meaning that opt->name will store the canonical
name (even if the alias was passed). If any option is to be accepted,
name will store the passed option.
Markus Armbruster - July 24, 2012, 4:01 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 24 Jul 2012 11:19:45 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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().
>
> I'd prefer to store the alias and have a function like
> qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
> that either, returns the canonical name if the alias is not set, or the alias
> if it's set.
>
> Then any function that needs to know what the user passed can call
> qemu_opt_get_passed_name().
>
> What do you think?

Parameters?  The QemuOpt, I guess.

Consider cpudef_setfield().  How do you plan to call
qemu_opt_get_passed_name() there?

>> 
>> 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().
>
> Not sure I follow, how does this break qemu_opt_get() calls?

Say you add alias "address" to parameter "addr" in
net_client_type[NET_CLIENT_TYPE_NIC].  Yes, that's pointless; it's just
an example.

If the user uses the alias, your opt_set() sets opt->name to "address".
net_client_init() passes opts to qemu_opts_validate(), which recognizes
"address".  It then passes opts to net_init_nic() (indirectly through
net_client_types[NET_CLIENT_TYPE_NIC].init()).  net_init_nic() calls
qemu_opt_get(opts, "addr"), which fails, because opt->str is still
"address".

>> Instead of overloading opt->name to mean user's name before parse and
>> canonical name afterwards, you may want to provide separate QemuOpts
>> members.
>
> Yes, I can do that, but note that the hunk that generated this discussion
> will stay the same in the meaning that opt->name will store the canonical
> name (even if the alias was passed). If any option is to be accepted,
> name will store the passed option.

My gut suggests one member for the canonical name (null until it becomes
known), and another member for the user's name.
Luiz Capitulino - July 25, 2012, 12:36 p.m.
On Tue, 24 Jul 2012 18:01:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Tue, 24 Jul 2012 11:19:45 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> 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().
> >
> > I'd prefer to store the alias and have a function like
> > qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
> > that either, returns the canonical name if the alias is not set, or the alias
> > if it's set.
> >
> > Then any function that needs to know what the user passed can call
> > qemu_opt_get_passed_name().
> >
> > What do you think?
> 
> Parameters?  The QemuOpt, I guess.

Yes.

> Consider cpudef_setfield().  How do you plan to call
> qemu_opt_get_passed_name() there?

Hmm, I expected qemu_opt_foreach() would pass opt to the callback function.
But that's not the case. It's doable to change all callbacks, but honestly,
the work in this series has gone beyond my original planning...

> 
> >> 
> >> 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().
> >
> > Not sure I follow, how does this break qemu_opt_get() calls?
> 
> Say you add alias "address" to parameter "addr" in
> net_client_type[NET_CLIENT_TYPE_NIC].  Yes, that's pointless; it's just
> an example.
> 
> If the user uses the alias, your opt_set() sets opt->name to "address".

opt->name is never set to the alias, it's always the canonical name.

> net_client_init() passes opts to qemu_opts_validate(), which recognizes
> "address".  It then passes opts to net_init_nic() (indirectly through
> net_client_types[NET_CLIENT_TYPE_NIC].init()).  net_init_nic() calls
> qemu_opt_get(opts, "addr"), which fails, because opt->str is still
> "address".
> 
> >> Instead of overloading opt->name to mean user's name before parse and
> >> canonical name afterwards, you may want to provide separate QemuOpts
> >> members.
> >
> > Yes, I can do that, but note that the hunk that generated this discussion
> > will stay the same in the meaning that opt->name will store the canonical
> > name (even if the alias was passed). If any option is to be accepted,
> > name will store the passed option.
> 
> My gut suggests one member for the canonical name (null until it becomes
> known), and another member for the user's name.

Yes, that's probably the right way to do this.

Patch

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,
     }
 
     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);
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;