diff mbox series

[v2,2/6] qemu-option: pass QemuOptsList to opts_accepts_any

Message ID 20201109133931.979563-3-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show
Series Deprecate or forbid crazy QemuOpts cases | expand

Commit Message

Paolo Bonzini Nov. 9, 2020, 1:39 p.m. UTC
A QemuOptsList can be of one of two kinds: either it is pre-validated, or
it accepts any key and validation happens somewhere else (typically in
a Visitor or against a list of QOM properties).  opts_accepts_any
returns true if a QemuOpts instance was created from a QemuOptsList of
the latter kind, but there is no function to do the check on a QemuOptsList.

We will need it in the next patch; since almost all callers of
opts_accepts_any use opts->list anyway, simply repurpose it instead
of adding a new function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-option.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Markus Armbruster Nov. 9, 2020, 3:27 p.m. UTC | #1
Paolo Bonzini <pbonzini@redhat.com> writes:

> A QemuOptsList can be of one of two kinds: either it is pre-validated, or
> it accepts any key and validation happens somewhere else (typically in
> a Visitor or against a list of QOM properties).

For a value of "typically" :)

>                                                  opts_accepts_any
> returns true if a QemuOpts instance was created from a QemuOptsList of
> the latter kind, but there is no function to do the check on a QemuOptsList.
>
> We will need it in the next patch; since almost all callers of
> opts_accepts_any use opts->list anyway,

I'm not sure I get the "since" part.  Peeking ahead...  you're lifting
the first -> into the callers, which is obviously safe.  I *guess*
you're trying to say it's also not ugly: most callers already contain
the ->, which you reuse, so there's hardly any code duplication.

>                                         simply repurpose it instead
> of adding a new function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-option.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index ab3b58599e..59be4f9d21 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -460,16 +460,16 @@ static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
>      }
>  }
>  
> -static bool opts_accepts_any(const QemuOpts *opts)
> +static bool opts_accepts_any(const QemuOptsList *list)
>  {
> -    return opts->list->desc[0].name == NULL;
> +    return list->desc[0].name == NULL;
>  }
>  
>  int qemu_opt_unset(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>  
> -    assert(opts_accepts_any(opts));
> +    assert(opts_accepts_any(opts->list));
>  
>      if (opt == NULL) {
>          return -1;

Here, it's a straighforward lift of opts->.

> @@ -500,9 +500,10 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
>                           Error **errp)
>  {
>      const QemuOptDesc *desc;
> +    const QemuOptsList *list = opt->opts->list;
>  
> -    desc = find_desc_by_name(opt->opts->list->desc, opt->name);
> -    if (!desc && !opts_accepts_any(opt->opts)) {
> +    desc = find_desc_by_name(list->desc, opt->name);
> +    if (!desc && !opts_accepts_any(list)) {
>          error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
>          if (help_wanted && is_help_option(opt->name)) {
>              *help_wanted = true;

Here, you reuse the existing opts-> by splicing in a variable.

This isn't as obvious as the straighforward lift, and I guess this is
what made you mention "since" in the commit message.

> @@ -535,9 +536,10 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
>  {
>      QemuOpt *opt;
>      const QemuOptDesc *desc;
> +    const QemuOptsList *list = opts->list;
>  
> -    desc = find_desc_by_name(opts->list->desc, name);
> -    if (!desc && !opts_accepts_any(opts)) {
> +    desc = find_desc_by_name(list->desc, name);
> +    if (!desc && !opts_accepts_any(list)) {
>          error_setg(errp, QERR_INVALID_PARAMETER, name);
>          return false;
>      }
> @@ -557,9 +559,10 @@ bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
>  {
>      QemuOpt *opt;
>      const QemuOptDesc *desc;
> +    const QemuOptsList *list = opts->list;
>  
> -    desc = find_desc_by_name(opts->list->desc, name);
> -    if (!desc && !opts_accepts_any(opts)) {
> +    desc = find_desc_by_name(list->desc, name);
> +    if (!desc && !opts_accepts_any(list)) {
>          error_setg(errp, QERR_INVALID_PARAMETER, name);
>          return false;
>      }
> @@ -1110,7 +1113,7 @@ bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
>  {
>      QemuOpt *opt;
>  
> -    assert(opts_accepts_any(opts));
> +    assert(opts_accepts_any(opts->list));
>  
>      QTAILQ_FOREACH(opt, &opts->head, next) {
>          opt->desc = find_desc_by_name(desc, opt->name);

The commit message confused me a bit.  Regardless:

Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff mbox series

Patch

diff --git a/util/qemu-option.c b/util/qemu-option.c
index ab3b58599e..59be4f9d21 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -460,16 +460,16 @@  static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
     }
 }
 
-static bool opts_accepts_any(const QemuOpts *opts)
+static bool opts_accepts_any(const QemuOptsList *list)
 {
-    return opts->list->desc[0].name == NULL;
+    return list->desc[0].name == NULL;
 }
 
 int qemu_opt_unset(QemuOpts *opts, const char *name)
 {
     QemuOpt *opt = qemu_opt_find(opts, name);
 
-    assert(opts_accepts_any(opts));
+    assert(opts_accepts_any(opts->list));
 
     if (opt == NULL) {
         return -1;
@@ -500,9 +500,10 @@  static bool opt_validate(QemuOpt *opt, bool *help_wanted,
                          Error **errp)
 {
     const QemuOptDesc *desc;
+    const QemuOptsList *list = opt->opts->list;
 
-    desc = find_desc_by_name(opt->opts->list->desc, opt->name);
-    if (!desc && !opts_accepts_any(opt->opts)) {
+    desc = find_desc_by_name(list->desc, opt->name);
+    if (!desc && !opts_accepts_any(list)) {
         error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
         if (help_wanted && is_help_option(opt->name)) {
             *help_wanted = true;
@@ -535,9 +536,10 @@  bool qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
 {
     QemuOpt *opt;
     const QemuOptDesc *desc;
+    const QemuOptsList *list = opts->list;
 
-    desc = find_desc_by_name(opts->list->desc, name);
-    if (!desc && !opts_accepts_any(opts)) {
+    desc = find_desc_by_name(list->desc, name);
+    if (!desc && !opts_accepts_any(list)) {
         error_setg(errp, QERR_INVALID_PARAMETER, name);
         return false;
     }
@@ -557,9 +559,10 @@  bool qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
 {
     QemuOpt *opt;
     const QemuOptDesc *desc;
+    const QemuOptsList *list = opts->list;
 
-    desc = find_desc_by_name(opts->list->desc, name);
-    if (!desc && !opts_accepts_any(opts)) {
+    desc = find_desc_by_name(list->desc, name);
+    if (!desc && !opts_accepts_any(list)) {
         error_setg(errp, QERR_INVALID_PARAMETER, name);
         return false;
     }
@@ -1110,7 +1113,7 @@  bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
 {
     QemuOpt *opt;
 
-    assert(opts_accepts_any(opts));
+    assert(opts_accepts_any(opts->list));
 
     QTAILQ_FOREACH(opt, &opts->head, next) {
         opt->desc = find_desc_by_name(desc, opt->name);