[v22,05/25] add some QemuOpts functions for replace work

Add some qemu_opt functions to replace the same functionality of
QEMUOptionParameter handling.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 include/qemu/option.h |   9 +++
 util/qemu-option.c    | 188 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 184 insertions(+), 13 deletions(-)

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 <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@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?


>  
> -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.

> +    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?

>  }
>  
> -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.

2014-03-11 7:28 GMT+08:00 Eric Blake <eblake@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 <wdongxu@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cyliu@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
>
>

On 03/10/2014 11:29 PM, Chunyan Liu wrote:
>>
>> 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.

Going from 'char *' to 'const char *' is automatic.  Have your helper
return 'char *', and qemu_opt_get can call it just fine and give its
callers their desired const char *.

>>
>> 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.
> 

Like I said in 4/25, if the option is passed through opts_accept_any,
treat it as string only; and make the opt_get_bool function either
reject it outright, or to always do the string-to-bool conversion at the
time of the lookup:

if (opt->desc) {
    assert(opt->desc->type == QEMU_OPT_BOOL);
    return opt->value.boolean;
} else {
    code to parse opt->str
}


>>
> Could be if changing qemu_opt_get return value type, but just as said
> before,
> that will affect many codes.

Also, changing an existing function that returns 'const char *' into now
returning 'char *' will NOT break any callers if the semantics remain
unchanged (what WILL break is if the semantics change where the callers
must now free the result).  But in general, it should still be just fine
to have qemu_opt_get return 'const char *' even if the common helper
returns 'char *'.

2014-03-11 19:59 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 03/10/2014 11:29 PM, Chunyan Liu wrote:
> >>
> >> 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.
>
> Going from 'char *' to 'const char *' is automatic.  Have your helper
> return 'char *', and qemu_opt_get can call it just fine and give its
> callers their desired const char *.
>
> >>
> >> 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.
> >
>
> Like I said in 4/25, if the option is passed through opts_accept_any,
> treat it as string only; and make the opt_get_bool function either
> reject it outright, or to always do the string-to-bool conversion at the
> time of the lookup:
>
> if (opt->desc) {
>     assert(opt->desc->type == QEMU_OPT_BOOL);
>     return opt->value.boolean;
> } else {
>     code to parse opt->str
> }
>
>
> >>
> > Could be if changing qemu_opt_get return value type, but just as said
> > before,
> > that will affect many codes.
>
> Also, changing an existing function that returns 'const char *' into now
> returning 'char *' will NOT break any callers if the semantics remain
> unchanged (what WILL break is if the semantics change where the callers
> must now free the result)


Yes, that's the only change, we need to free the result. There are more
than 200
places using qemu_opt_get in existing code, we need to checkout the related
files
and free the result one by one.


>  But in general, it should still be just fine
> to have qemu_opt_get return 'const char *' even if the common helper
> returns 'char *'.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>

On 03/11/2014 09:10 PM, Chunyan Liu wrote:

>>>>
>>> Could be if changing qemu_opt_get return value type, but just as said
>>> before,
>>> that will affect many codes.
>>
>> Also, changing an existing function that returns 'const char *' into now
>> returning 'char *' will NOT break any callers if the semantics remain
>> unchanged (what WILL break is if the semantics change where the callers
>> must now free the result)
> 
> 
> Yes, that's the only change, we need to free the result. There are more
> than 200
> places using qemu_opt_get in existing code, we need to checkout the related
> files
> and free the result one by one.

You can still write your helper function so that if 'del' is true, you
strdup, if 'del' is false, you return the in-place memory.  You'll have
to cast away const in the helper, but qemu_opt_get can then restore the
const and you don't have to adjust any existing callers, while still
sharing the underlying implementation rather than duplicating code.  But
I guess I'll wait for v23 to see what you actually do in response to my
suggestions.  If nothing else, please document design decisions in your
commit message (for example, if you choose to duplicate code rather than
share code in a common helper, explain that the duplication is because
one function returns malloc'd memory while the other returns in-place
memory).

2014-03-12 20:40 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 03/11/2014 09:10 PM, Chunyan Liu wrote:
>
> >>>>
> >>> Could be if changing qemu_opt_get return value type, but just as said
> >>> before,
> >>> that will affect many codes.
> >>
> >> Also, changing an existing function that returns 'const char *' into now
> >> returning 'char *' will NOT break any callers if the semantics remain
> >> unchanged (what WILL break is if the semantics change where the callers
> >> must now free the result)
> >
> >
> > Yes, that's the only change, we need to free the result. There are more
> > than 200
> > places using qemu_opt_get in existing code, we need to checkout the
> related
> > files
> > and free the result one by one.
>
> You can still write your helper function so that if 'del' is true, you
> strdup, if 'del' is false, you return the in-place memory.  You'll have
> to cast away const in the helper, but qemu_opt_get can then restore the
> const and you don't have to adjust any existing callers, while still
> sharing the underlying implementation rather than duplicating code.  But
> I guess I'll wait for v23 to see what you actually do in response to my
> suggestions.  If nothing else, please document design decisions in your
> commit message (for example, if you choose to duplicate code rather than
> share code in a common helper, explain that the duplication is because
> one function returns malloc'd memory while the other returns in-place
> memory).
>
> Got it. Thanks.


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

Hi,

On Mon, Mar 10, 2014 at 03:31:41PM +0800, Chunyan Liu wrote:
> Add some qemu_opt functions to replace the same functionality of
> QEMUOptionParameter handling.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  include/qemu/option.h |   9 +++
>  util/qemu-option.c    | 188 ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 184 insertions(+), 13 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index c3b0a91..de4912a 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -111,6 +111,7 @@ struct QemuOptsList {
>  };
>  
>  const char *qemu_opt_get(QemuOpts *opts, const char *name);
> +char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>  /**
>   * qemu_opt_has_help_opt:
>   * @opts: options to search for a help request
> @@ -126,6 +127,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts);
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
>  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
> +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> +                                 uint64_t defval);
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> +                               uint64_t defval);


I have initially understood you would remove these _del() suffixed functions in your
final cleanup, but it seems you haven't. Am I missing something?

Neither the functions nor the callers have been cleanup, why?

Regards..

--
Dorileo

>  int qemu_opt_unset(QemuOpts *opts, const char *name);
>  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
>  void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> @@ -161,4 +167,7 @@ void qemu_opts_print(QemuOpts *opts);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure);
>  
> +QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
> +void qemu_opts_free(QemuOptsList *list);
> +void qemu_opts_print_help(QemuOptsList *list);
>  #endif
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index df79235..2c450e0 100644
> --- a/util/qemu-option.c
> +++ 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);
>  
>  /*
>   * Extracts the name of an option from the parameter string (p points at the
> @@ -379,6 +380,74 @@ QEMUOptionParameter *append_option_parameters(QEMUOptionParameter *dest,
>      return dest;
>  }
>  
> +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++;
> +    }
> +
> +    return num_opts;
> +}
> +
> +/* 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,
> +                               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));
> +    QTAILQ_INIT(&tmp->head);
> +    num_dst_opts = 0;
> +
> +    /* 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;
> +            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) {
> +                tmp->desc[num_dst_opts++] = *desc;
> +                tmp->desc[num_dst_opts].name = NULL;
> +            }
> +            desc++;
> +        }
> +    }
> +
> +    return tmp;
> +}
> +
>  /*
>   * 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)
> +{
> +    QemuOpt *opt;
> +    const QemuOptDesc *desc;
> +    char *str = NULL;
> +
> +    if (opts == NULL) {
> +        return NULL;
> +    }
> +
> +    opt = qemu_opt_find(opts, name);
> +    if (!opt) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            str = g_strdup(desc->def_value_str);
> +        }
> +        return str;
> +    }
> +    str = g_strdup(opt->str);
> +    qemu_opt_del(opt);
> +    return str;
> +}
> +
>  bool qemu_opt_has_help_opt(QemuOpts *opts)
>  {
>      QemuOpt *opt;
> @@ -586,9 +678,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>      return false;
>  }
>  
> -bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> +static bool _qemu_opt_get_bool(QemuOpts *opts, const char *name,
> +                               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;
>      }
>      if (opt->desc) {
>          assert(opt->desc->type == QEMU_OPT_BOOL);
>      }
> -    return opt->value.boolean;
> +    ret = opt->value.boolean;
> +    if (del) {
> +        qemu_opt_del(opt);
> +    }
> +    return ret;
>  }
>  
> -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);
> +}
> +
> +static uint64_t _qemu_opt_get_number(QemuOpts *opts, const char *name,
> +                                     uint64_t defval, bool del)
>  {
>      QemuOpt *opt;
> +    uint64_t ret = defval;
>  
>      if (opts == NULL) {
>          return defval;
> @@ -622,20 +732,36 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_number(name, desc->def_value_str, &defval,
> -                                &error_abort);
> +            parse_option_number(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;
>      }
>      if (opt->desc) {
>          assert(opt->desc->type == QEMU_OPT_NUMBER);
>      }
> -    return opt->value.uint;
> +    ret = opt->value.uint;
> +    if (del) {
> +        qemu_opt_del(opt);
> +    }
> +    return ret;
>  }
>  
> -uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> +uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> +{
> +    return _qemu_opt_get_number(opts, name, defval, false);
> +}
> +
> +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> +                                 uint64_t defval)
> +{
> +    return _qemu_opt_get_number(opts, name, defval, true);
> +}
> +
> +static uint64_t _qemu_opt_get_size(QemuOpts *opts, const char *name,
> +                                   uint64_t defval, bool del)
>  {
>      QemuOpt *opt;
> +    uint64_t ret = defval;
>  
>      if (opts == NULL) {
>          return defval;
> @@ -645,14 +771,29 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_size(name, desc->def_value_str, &defval, &error_abort);
> +            parse_option_size(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;
>      }
>      if (opt->desc) {
>          assert(opt->desc->type == QEMU_OPT_SIZE);
>      }
> -    return opt->value.uint;
> +    ret = opt->value.uint;
> +    if (del) {
> +        qemu_opt_del(opt);
> +    }
> +    return ret;
> +}
> +
> +uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> +{
> +    return _qemu_opt_get_size(opts, name, defval, false);
> +}
> +
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> +                               uint64_t defval)
> +{
> +    return _qemu_opt_get_size(opts, name, defval, true);
>  }
>  
>  static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> @@ -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);
> +}
> +
> +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 : "");
> +    }
> +}
> -- 
> 1.7.12.4
> 
>

2014-03-18 3:35 GMT+08:00 Leandro Dorileo <l@dorileo.org>:

> Hi,
>
> On Mon, Mar 10, 2014 at 03:31:41PM +0800, Chunyan Liu wrote:
> > Add some qemu_opt functions to replace the same functionality of
> > QEMUOptionParameter handling.
> >
> > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cyliu@suse.com>
> > ---
> >  include/qemu/option.h |   9 +++
> >  util/qemu-option.c    | 188
> ++++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 184 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/qemu/option.h b/include/qemu/option.h
> > index c3b0a91..de4912a 100644
> > --- a/include/qemu/option.h
> > +++ b/include/qemu/option.h
> > @@ -111,6 +111,7 @@ struct QemuOptsList {*0*
> >  };
> >
> >  const char *qemu_opt_get(QemuOpts *opts, const char *name);
> > +char *qemu_opt_get_del(QemuOpts *opts, const char *name);
> >  /**
> >   * qemu_opt_has_help_opt:
> >   * @opts: options to search for a help request
> > @@ -126,6 +127,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts);
> >  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
> >  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t
> defval);
> >  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t
> defval);
> > +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool
> defval);
> > +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> > +                                 uint64_t defval);
> > +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> > +                               uint64_t defval);
>
>
> I have initially understood you would remove these _del() suffixed
> functions in your
> final cleanup, but it seems you haven't. Am I missing something?
>
> Neither the functions nor the callers have been cleanup, why?
>

No, it won't be removed. It replaces the existing handling of
QEMUOptionParameter.
For each driver, it handles the options they are expected to handle, then
remove those
options from the list; for the left options, they will be passed to
proto_drv for 2nd phase
handling.


>
> Regards..
>
> --
> Dorileo
>
> >  int qemu_opt_unset(QemuOpts *opts, const char *name);
> >  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
> >  void qemu_opt_set_err(QemuOpts *opts, const char *name, const char
> *value,
> > @@ -161,4 +167,7 @@ void qemu_opts_print(QemuOpts *opts);
> >  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void
> *opaque,
> >                        int abort_on_failure);
> >
> > +QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
> > +void qemu_opts_free(QemuOptsList *list);
> > +void qemu_opts_print_help(QemuOptsList *list);
> >  #endif
> > diff --git a/util/qemu-option.c b/util/qemu-option.c
> > index df79235..2c450e0 100644
> > --- a/util/qemu-option.c
> > +++ 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);
> >
> >  /*
> >   * Extracts the name of an option from the parameter string (p points
> at the
> > @@ -379,6 +380,74 @@ QEMUOptionParameter
> *append_option_parameters(QEMUOptionParameter *dest,
> >      return dest;
> >  }
> >
> > +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++;
> > +    }
> > +
> > +    return num_opts;
> > +}
> > +
> > +/* 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,
> > +                               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));
> > +    QTAILQ_INIT(&tmp->head);
> > +    num_dst_opts = 0;
> > +
> > +    /* 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;
> > +            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) {
> > +                tmp->desc[num_dst_opts++] = *desc;
> > +                tmp->desc[num_dst_opts].name = NULL;
> > +            }
> > +            desc++;
> > +        }
> > +    }
> > +
> > +    return tmp;
> > +}
> > +
> >  /*
> >   * 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)
> > +{
> > +    QemuOpt *opt;
> > +    const QemuOptDesc *desc;
> > +    char *str = NULL;
> > +
> > +    if (opts == NULL) {
> > +        return NULL;
> > +    }
> > +
> > +    opt = qemu_opt_find(opts, name);
> > +    if (!opt) {
> > +        desc = find_desc_by_name(opts->list->desc, name);
> > +        if (desc && desc->def_value_str) {
> > +            str = g_strdup(desc->def_value_str);
> > +        }
> > +        return str;
> > +    }
> > +    str = g_strdup(opt->str);
> > +    qemu_opt_del(opt);
> > +    return str;
> > +}
> > +
> >  bool qemu_opt_has_help_opt(QemuOpts *opts)
> >  {
> >      QemuOpt *opt;
> > @@ -586,9 +678,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
> >      return false;
> >  }
> >
> > -bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> > +static bool _qemu_opt_get_bool(QemuOpts *opts, const char *name,
> > +                               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;
> >      }
> >      if (opt->desc) {
> >          assert(opt->desc->type == QEMU_OPT_BOOL);
> >      }
> > -    return opt->value.boolean;
> > +    ret = opt->value.boolean;
> > +    if (del) {
> > +        qemu_opt_del(opt);
> > +    }
> > +    return ret;
> >  }
> >
> > -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);
> > +}
> > +
> > +static uint64_t _qemu_opt_get_number(QemuOpts *opts, const char *name,
> > +                                     uint64_t defval, bool del)
> >  {
> >      QemuOpt *opt;
> > +    uint64_t ret = defval;
> >
> >      if (opts == NULL) {
> >          return defval;
> > @@ -622,20 +732,36 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const
> char *name, uint64_t defval)
> >      if (opt == NULL) {
> >          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc,
> name);
> >          if (desc && desc->def_value_str) {
> > -            parse_option_number(name, desc->def_value_str, &defval,
> > -                                &error_abort);
> > +            parse_option_number(name, desc->def_value_str, &ret,
> &error_abort);
> >          }
> > -        return defval;
> > +        return ret;
> >      }
> >      if (opt->desc) {
> >          assert(opt->desc->type == QEMU_OPT_NUMBER);
> >      }
> > -    return opt->value.uint;
> > +    ret = opt->value.uint;
> > +    if (del) {
> > +        qemu_opt_del(opt);
> > +    }
> > +    return ret;
> >  }
> >
> > -uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t
> defval)
> > +uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t
> defval)
> > +{
> > +    return _qemu_opt_get_number(opts, name, defval, false);
> > +}
> > +
> > +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> > +                                 uint64_t defval)
> > +{
> > +    return _qemu_opt_get_number(opts, name, defval, true);
> > +}
> > +
> > +static uint64_t _qemu_opt_get_size(QemuOpts *opts, const char *name,
> > +                                   uint64_t defval, bool del)
> >  {
> >      QemuOpt *opt;
> > +    uint64_t ret = defval;
> >
> >      if (opts == NULL) {
> >          return defval;
> > @@ -645,14 +771,29 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const
> char *name, uint64_t defval)
> >      if (opt == NULL) {
> >          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc,
> name);
> >          if (desc && desc->def_value_str) {
> > -            parse_option_size(name, desc->def_value_str, &defval,
> &error_abort);
> > +            parse_option_size(name, desc->def_value_str, &ret,
> &error_abort);
> >          }
> > -        return defval;
> > +        return ret;
> >      }
> >      if (opt->desc) {
> >          assert(opt->desc->type == QEMU_OPT_SIZE);
> >      }
> > -    return opt->value.uint;
> > +    ret = opt->value.uint;
> > +    if (del) {
> > +        qemu_opt_del(opt);
> > +    }
> > +    return ret;
> > +}
> > +
> > +uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t
> defval)
> > +{
> > +    return _qemu_opt_get_size(opts, name, defval, false);
> > +}
> > +
> > +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> > +                               uint64_t defval)
> > +{
> > +    return _qemu_opt_get_size(opts, name, defval, true);
> >  }
> >
> >  static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> > @@ -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);
> > +}
> > +
> > +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 : "");
> > +    }
> > +}
> > --
> > 1.7.12.4
> >
> >
>
>

2014-03-12 20:40 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 03/11/2014 09:10 PM, Chunyan Liu wrote:
>
> >>>>
> >>> Could be if changing qemu_opt_get return value type, but just as said
> >>> before,
> >>> that will affect many codes.
> >>
> >> Also, changing an existing function that returns 'const char *' into now
> >> returning 'char *' will NOT break any callers if the semantics remain
> >> unchanged (what WILL break is if the semantics change where the callers
> >> must now free the result)
> >
> >
> > Yes, that's the only change, we need to free the result. There are more
> > than 200
> > places using qemu_opt_get in existing code, we need to checkout the
> related
> > files
> > and free the result one by one.
>
> You can still write your helper function so that if 'del' is true, you
> strdup, if 'del' is false, you return the in-place memory.  You'll have
> to cast away const in the helper, but qemu_opt_get can then restore the
> const and you don't have to adjust any existing callers, while still
> sharing the underlying implementation rather than duplicating code.


After trying, still has blocking here. qemu_opt_get returns either opt->str
(this
is char *) or desc->def_value_str (this is const char *). If not g_strdup
the result,
return (const char *) is OK, but return (char *) will has build error when
casting
(const char *) to (char *).

And since assert(opt->desc && opt->desc->type == xx) doesn't need change
for this patch series now, qemu_opt_get_bool could still return
opt->value.boolean, doesn't need to call qemu_opt_get to get opt->str and
parse that to bool. Same to qemu_opt_get_size/number. So, it's not so urgent
that qemu_opt_get/qemu_opt_get_del share common helper function. I'm
inclined
to keep duplicating code.


> But
> I guess I'll wait for v23 to see what you actually do in response to my
> suggestions.  If nothing else, please document design decisions in your
> commit message (for example, if you choose to duplicate code rather than
> share code in a common helper, explain that the duplication is because
> one function returns malloc'd memory while the other returns in-place
> memory).
>

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