Re: [Qemu-devel] [PATCH v22 05/25] add some QemuOpts functions for replace work

[Chunyan Liu]

2014-03-11 7:28 GMT+08:00 Eric Blake <address@hidden>:

On 03/10/2014 01:31 AM, Chunyan Liu wrote:
> Add some qemu_opt functions to replace the same functionality of
> QEMUOptionParameter handling.
>
> Signed-off-by: Dong Xu Wang <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  include/qemu/option.h |   9 +++
>  util/qemu-option.c    | 188 ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 184 insertions(+), 13 deletions(-)
>

> +++ b/util/qemu-option.c
> @@ -35,6 +35,7 @@
>
>  static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>                                              const char *name);
> +static void qemu_opt_del(QemuOpt *opt);

Again, any reason you can't hoist the function so that it occurs in
topological order, rather than needing to add a forward declaration?

>  }
>
> +static size_t count_opts_list(QemuOptsList *list)
> +{
> +    QemuOptDesc *desc = NULL;
> +    size_t num_opts = 0;
> +
> +    if (!list) {
> +        return 0;
> +    }
> +
> +    desc = list->desc;
> +    while (desc && desc->name) {
> +        num_opts++;
> +        desc++;
> +    }

This returns 0 for option lists where opts_accepts_any() is true.  Is
that going to bite us?  Let's see...

> +/* Create a new QemuOptsList with a desc of the merge of the first
> + * and second. It will allocate space for one new QemuOptsList plus
> + * enough space for QemuOptDesc in first and second QemuOptsList.
> + * First argument's QemuOptDesc members take precedence over second's.
> + * The result's name and implied_opt_name are not copied from them.
> + * Both merge_lists should not be set. Both lists can be NULL.
> + */
> +QemuOptsList *qemu_opts_append(QemuOptsList *dst,

Naming this 'dst' is confusing, as it is not the destination.  Rather,
you are creating a new list, and the new list is the destination of the
concatenation of first and second list arguments, where descriptions in
the first list have priority over any duplicates in the second list.

> +                               QemuOptsList *list)
> +{
> +    size_t num_opts, num_dst_opts;
> +    QemuOptsList *tmp;
> +    QemuOptDesc *desc;
> +
> +    if (!dst && !list) {
> +        return NULL;
> +    }
> +
> +    num_opts = count_opts_list(dst);
> +    num_opts += count_opts_list(list);
> +    tmp = g_malloc0(sizeof(QemuOptsList) +
> +                    (num_opts + 1) * sizeof(QemuOptDesc));

...Already I can see problems if either 'dst' or 'list' passes
opts_accepts_any().  If exactly one has a desc array with no name, then
that QemuOptsList accepts any option spelling, but the resulting list in
'tmp' will only accept the options in the other input.  Probably worth
asserting that neither input passes opts_accepts_any().

> +    QTAILQ_INIT(&tmp->head);
> +    num_dst_opts = 0;

This name is confusing - it is actually tracking how many descriptions
you have copied into 'tmp', and NOT how many options are in 'dst'.

> +
> +    /* copy dst->desc to new list */
> +    if (dst) {
> +        desc = dst->desc;
> +        while (desc && desc->name) {
> +            tmp->desc[num_dst_opts++] = *desc;
> +            tmp->desc[num_dst_opts].name = NULL;

Redundant assignment; your g_malloc0() above already guaranteed this.

> +            desc++;
> +        }
> +    }
> +
> +    /* add list->desc to new list */
> +    if (list) {
> +        desc = list->desc;
> +        while (desc && desc->name) {
> +            if (find_desc_by_name(tmp->desc, desc->name) == NULL) {

So every call to qemu_opts_append() is O(n^2) behavior - I guess it's
okay: as long as we are always passing short lists, we don't need it to
scale very well.

> +                tmp->desc[num_dst_opts++] = *desc;
> +                tmp->desc[num_dst_opts].name = NULL;

Again, redundant assignment to NULL.

> +            }
> +            desc++;
> +        }
> +    }
> +
> +    return tmp;

Okay, at the end of the day, 'tmp' is a single block of malloc'd
storage, where descriptions are shared with pre-existing opt lists
(probably worth documenting that the lifetime of the returned value of
this function is no longer than the lifetime of the two lists you
concatenated, and that a simple g_free() of the list will suffice - or
point to qemu_opts_free() as the recommended cleanup method).

> +}
> +
>  /*
>   * Parses a parameter string (param) into an option list (dest).
>   *
> @@ -574,6 +643,29 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>      return opt ? opt->str : NULL;
>  }
>
> +char *qemu_opt_get_del(QemuOpts *opts, const char *name)

Please, add a comment what this function does.  Something like:

If 'opts' already has a value for the key 'name', remove that key from
the options list and return the value.  Otherwise, if 'opts' has a
default value for the key, return that default.  Otherwise, return NULL.

Also, I think qemu_opt_get_del and qemu_opts_append may be better as two
separate commits, particularly if you are also enhancing the testsuite
with each commit to prove the functionality works.

Why are your later callers using a common helper function, but
qemu_opt_get_del() repeating a lot of the body of qemu_opt_get()?
Shouldn't qemu_opt_get() and qemu_opt_get_del() call into a common helper?

qemu_opt_get_del must return (char *), but the existing qemu_opt_get returns
(const char *). Could also change qemu_opt_get return value to (char *) type,
then these two functions could use a common helper function. But there are too
many places using qemu_opt_get already, if the return value type changes, it will
affect a lot.

 

>
> -bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> +static bool _qemu_opt_get_bool(QemuOpts *opts, const char *name,

Nothing else in the code base uses the '_qemu' namespace.  You need to
name this function something better. A comment for this method would be
helpful, something like:

Determine the boolean value for key 'name' from 'opts', defaulting to
'defval' if one has not already been added to the list and if the list
does not already provide a default.  Additionally, if 'del', remove the
value from the list.

> +                               bool defval, bool del)
>  {
>      QemuOpt *opt;
> +    bool ret = defval;
>
>      if (opts == NULL) {
>          return defval;
> @@ -599,19 +693,35 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_bool(name, desc->def_value_str, &defval, &error_abort);
> +            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;

This favors the QemuOptDesc default over the defval that the user passed
in.  I think that's an important detail to document that use of the
def_value_str means the defval argument is never used.

>      }
>      if (opt->desc) {
>          assert(opt->desc->type == QEMU_OPT_BOOL);
>      }
> -    return opt->value.boolean;
> +    ret = opt->value.boolean;

Same problem as in 4/25 - I don't think you can rely on
opt->value.boolean being valid if you don't track the union
discriminator, and right now, the union discriminator is tracked only
via opt->desc. 

Couldn't find a reliable way if the option is passed through opts_accept_any,
just no way to check the option type.
 

> +    if (del) {
> +        qemu_opt_del(opt);
> +    }
> +    return ret;

Couldn't this whole function be shortened to:

char *tmp;
if (del) {
    tmp = qemu_opt_get_del(opts, name);
} else {
    tmp = qemu_opt_get(opts, name);
}
if (!tmp) {
    return defval;
}
parse_option_bool(name, tmp, &defval, &error_abort);
g_free(tmp);
return defval;

or even shorter, if you combine qemu_opt_get{,_del} into calling a
common helper function?

Could be if changing qemu_opt_get return value type, but just as said before,

that will affect many codes.

 

>  }
>
> -uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> +bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> +{
> +    return _qemu_opt_get_bool(opts, name, defval, false);
> +}
> +
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
> +{
> +    return _qemu_opt_get_bool(opts, name, defval, true);
> +}

Other than the _qemu namespace, this is a good example of how a common
helper function makes it easier to implement two variants.

The _number and _size variants will need similar treatment as the _bool.

> @@ -1302,3 +1443,24 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>      loc_pop(&loc);
>      return rc;
>  }
> +
> +/* free a QemuOptsList, can accept NULL as arguments */
> +void qemu_opts_free(QemuOptsList *list)
> +{
> +    if (!list) {
> +        return;
> +    }
> +
> +    g_free(list);
> +}

g_free() is safe to call on NULL.  Which means qemu_opts_free(foo) is a
fancy way to spell g_free(foo).  Do we need this function?

> +
> +void qemu_opts_print_help(QemuOptsList *list)
> +{
> +    int i;
> +    printf("Supported options:\n");
> +    for (i = 0; list && list->desc[i].name; i++) {
> +        printf("%-16s %s\n", list->desc[i].name,
> +               list->desc[i].help ?
> +               list->desc[i].help : "");

This prints trailing spaces for any description that lacks help text.
Not the end of the world, but I try to be cleaner than that.

> +    }
> +}

Unrelated to either of the other two categories of functions - maybe
this patch should be split into three parts.

--
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org