2014-03-11 7:28 GMT+08:00 Eric Blake <ebl...@redhat.com>:
> 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 <wdon...@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cy...@suse.com>
> > ---
> > 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
>
>
