qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Luiz Capitulino
Subject: Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support
Date: Tue, 24 Jul 2012 12:15:25 -0300

On Tue, 24 Jul 2012 11:19:45 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Mon, 23 Jul 2012 20:33:52 +0200
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> writes:
> >> 
> >> > On Sat, 21 Jul 2012 10:11:39 +0200
> >> > Markus Armbruster <address@hidden> wrote:
> >> >
> >> >> Luiz Capitulino <address@hidden> writes:
> >> >> 
> >> >> > Allow for specifying an alias for each option name, see next commits
> >> >> > for examples.
> >> >> >
> >> >> > Signed-off-by: Luiz Capitulino <address@hidden>
> >> >> > ---
> >> >> >  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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]