On Tue, 24 Jul 2012 18:01:12 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Tue, 24 Jul 2012 11:19:45 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >>
> >> > On Mon, 23 Jul 2012 20:33:52 +0200
> >> > Markus Armbruster <arm...@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >> >>
> >> >> > On Sat, 21 Jul 2012 10:11:39 +0200
> >> >> > Markus Armbruster <arm...@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
> >> >> >>
> >> >> >> > Allow for specifying an alias for each option name, see next
> >> >> >> > commits
> >> >> >> > for examples.
> >> >> >> >
> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitul...@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.
