| Message ID | 1342212261-19903-5-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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;
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; >
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; >>
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.
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.
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.
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.
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.
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;
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(-)