Re: [Qemu-devel] [PATCH 7/8] util: add qemu_opt_get_all() to get repeated opts

[Eric Blake]

On 01/05/2017 10:07 AM, Daniel P. Berrange wrote:
> The QemuOpts parser accepts repeated option names, storing
> all the provided values. qemu_opt_get() then just returns
> the last value. There is no way to get the other values
> without using a callback function to iterate over all
> option names. Add a qemu_opt_get_all() method to return
> a string list of all values.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 1f9e3f9..689e0a8 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -65,6 +65,7 @@ struct QemuOptsList {
>  };
>  
>  const char *qemu_opt_get(QemuOpts *opts, const char *name);
> +size_t qemu_opt_get_all(QemuOpts *opts, const char *name, char ***vals);
>  char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>  /**
>   * qemu_opt_has_help_opt:
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 3467dc2..0418d71 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -332,6 +332,28 @@ const char *qemu_opt_get(QemuOpts *opts, const char 
> *name)
>      return opt ? opt->str : NULL;
>  }
>  
> +size_t qemu_opt_get_all(QemuOpts *opts, const char *name, char ***vals)

Documentation would be nice, including that the caller must free two
levels of results.

> +{
> +    QemuOpt *opt;
> +    size_t nvals = 0;
> +
> +    *vals = NULL;
> +
> +    QTAILQ_FOREACH(opt, &opts->head, next) {
> +        if (!g_str_equal(opt->name, name)) {
> +            continue;
> +        }
> +
> +        *vals = g_renew(char *, *vals, nvals + 1);
> +        (*vals)[nvals++] = g_strdup(opt->str);

This one is noticeably O(n^2) if the user supplies LOTS of repeated
options on the command line.  Should we be smarter about the allocation,
since unlike DNS resolution, we don't have quite as much assurance that
the user won't be doing that?  You do have an upper bound that this
won't call g_renew() any more than the number of opts present in the
command-line argument that you could use for O(1) instead of O(n) list
allocations (the O(n^2) is caused by O(n) allocations coupled with O(n)
copying between allocations), but I don't know if we have easy access to
that value, nor if it is wasteful to over-allocate when the more common
case is that most of the opts are not equal to name.

> +    }
> +    if (nvals) {
> +        *vals = g_renew(char *, *vals, nvals + 1);
> +        (*vals)[nvals] = NULL;
> +    }
> +    return nvals;
> +}
> +
>  /* Get a known option (or its default) and remove it from the list
>   * all in one action. Return a malloced string of the option value.
>   * Result must be freed by caller with g_free().
> 

The idea makes sense, though.

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

signature.asc
Description: OpenPGP digital signature