Re: [Qemu-devel] [v19 03/25] improve some functions in qemu-option.c

[Kevin Wolf <kwolf@redhat.com>]

Am 20.01.2014 um 15:19 hat Chunyan Liu geschrieben:
> Improve opt_get and opt_set group of functions. For opt_get, check and handle
> NUlL input; for opt_set, when set to an existing option, rewrite the option
> with new value.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

Why do we want to allow NULL opts? Silently ignoring NULL instead of
crashing leads to more subtle failure. Is there a legitimate user
passing NULL?

>  util/qemu-option.c |   80 ++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 66 insertions(+), 14 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index fd84f95..8944b62 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt;
>  
> +    if (!opts)
> +        return NULL;

The qemu coding style requires braces here (and in the following
instances).

> +
>      QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
>          if (strcmp(opt->name, name) != 0)
>              continue;
> @@ -509,9 +512,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
>      const QemuOptDesc *desc;
>  
> +    if (!opts)
> +        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) {
> @@ -535,10 +542,15 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>  
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
>      const QemuOptDesc *desc;
>      Error *local_err = NULL;
>  
> +    if (!opts)
> +        return defval;
> +
> +    opt = qemu_opt_find(opts, name);
> +
>      if (opt == NULL) {
>          desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> @@ -553,10 +565,15 @@ 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)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
>      const QemuOptDesc *desc;
>      Error *local_err = NULL;
>  
> +    if (!opts)
> +        return defval;
> +
> +    opt = qemu_opt_find(opts, name);
> +
>      if (opt == NULL) {
>          desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> @@ -571,10 +588,14 @@ 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)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
>      const QemuOptDesc *desc;
>      Error *local_err = NULL;
>  
> +    if (!opts)
> +        return defval;
> +
> +    opt = qemu_opt_find(opts, name);
>      if (opt == NULL) {
>          desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> @@ -612,6 +633,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>  
>  static void qemu_opt_del(QemuOpt *opt)
>  {
> +    if (!opt) {
> +        return;
> +    }
> +
>      QTAILQ_REMOVE(&opt->opts->head, opt, next);
>      g_free((/* !const */ char*)opt->name);
>      g_free((/* !const */ char*)opt->str);
> @@ -664,6 +689,13 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>          return;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char*)opt->str);
> +        opt->str = g_strdup(value);

Why is qemu_opt_parse() not needed here?

> +        return;
> +    }
> +
>      opt = g_malloc0(sizeof(*opt));
>      opt->name = g_strdup(name);
>      opt->opts = opts;
> @@ -704,16 +736,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>  {
>      QemuOpt *opt;
> -    const QemuOptDesc *desc = opts->list->desc;
> +    const QemuOptDesc *desc;
>  
> -    opt = g_malloc0(sizeof(*opt));
> -    opt->desc = find_desc_by_name(desc, name);
> -    if (!opt->desc && !opts_accepts_any(opts)) {
> +    desc = find_desc_by_name(opts->list->desc, name);
> +    if (!desc && !opts_accepts_any(opts)) {
>          qerror_report(QERR_INVALID_PARAMETER, name);
> -        g_free(opt);
>          return -1;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char*)opt->str);
> +        opt->value.boolean =val;

Missing space after =

> +        opt->str = g_strdup(val ? "on" : "off");
> +        return 0;
> +    }
> +
> +    opt = g_malloc0(sizeof(*opt));
> +    opt->desc = desc;
>      opt->name = g_strdup(name);
>      opt->opts = opts;
>      opt->value.boolean = !!val;

Kevin