Re: [Qemu-devel] [PATCH V4 07/10] add def_value and use it in qemu_opts_print.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Oct 25, 2012 at 08:57:25PM +0800, Dong Xu Wang wrote:
> qemu_opts_print has no user now, so I re-write it and use it in
> qemu-img.c.
> 
> qemu_opts_print will be used while using "qemu-img create", it will
> produce the same output as previous code.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  qemu-option.c |   41 ++++++++++++++++++++++++++++++++---------
>  qemu-option.h |    1 +
>  2 files changed, 33 insertions(+), 9 deletions(-)

The behavior of this function has changed.  I think you are trying to
achieve the following:

1. Print every possible option, whether a value has been set or not.
2. Option descriptors may provide a default value.
3. Print to stdout instead of stderr.

Previously the behavior was to print every option that has been set.
Options that have not been set would be skipped.

Please include the intention behind the change either in the commit
description or in a doc comment before the function.  It took me a while
to figure out "why" this change is useful.

> diff --git a/qemu-option.c b/qemu-option.c
> index eeb2c9c..54dbdd0 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -860,15 +860,38 @@ void qemu_opts_del(QemuOpts *opts)
>  
>  int qemu_opts_print(QemuOpts *opts, void *dummy)
>  {
> -    QemuOpt *opt;
> -
> -    fprintf(stderr, "%s: %s:", opts->list->name,
> -            opts->id ? opts->id : "<noid>");
> -    QTAILQ_FOREACH(opt, &opts->head, next) {
> -        fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
> -    }
> -    fprintf(stderr, "\n");
> -    return 0;
> +    QemuOpt *opt = NULL;
> +    QemuOptDesc *desc = opts->list->desc;
> +
> +    while (desc && desc->name) {
> +        opt = qemu_opt_find(opts, desc->name);
> +        switch (desc->type) {
> +        case QEMU_OPT_STRING:
> +            if (opt != NULL) {
> +                printf("%s='%s' ", opt->name, opt->str);
> +            }
> +            break;
> +        case QEMU_OPT_BOOL:
> +            printf("%s=%s ", desc->name, (opt && opt->str) ? "on" : "off");
> +            break;
> +        case QEMU_OPT_NUMBER:
> +        case QEMU_OPT_SIZE:
> +            if (strcmp(desc->name, "cluster_size")) {
> +                printf("%s=%" PRId64 " ", desc->name,
> +                    (opt && opt->value.uint) ? opt->value.uint : 0);
> +            } else {
> +                printf("%s=%" PRId64 " ", desc->name,
> +                    (opt && opt->value.uint) ?
> +                        opt->value.uint : desc->def_value);
> +            }
> +            break;
> +        default:
> +            printf("%s=(unknown type) ", desc->name);
> +            break;
> +        }

This is very specific to qemu-img output formatting.  An idea to make
the formatting rules more general:

1. Strings are printed with '%s', other values are printed unquoted %s.
2. If the option has not been set, then def_print_str is used.
3. If the option has not been set and def_print_str is NULL then the
   option is skipped.

The code becomes:

for (desc = opts->list->desc; desc && desc->name; desc++) {
    const char *value = desc->def_print_str;
    QemuOpt *opt;

    opt = qemu_opt_find(opts, desc->name);
    if (opt) {
        value = opt->str;
    }

    if (!value) {
        continue;
    }

    if (desc->type == QEMU_OPT_STRING) {
        printf("%s='%s' ", desc->name, value);
    } else {
        printf("%s=%s ", desc->name, value);
    }
}

If you want an option to print a default value, def_print_str must be
set.  Otherwise the option is skipped when no value has been set.

> +        desc++;
> +     }
> +     return 0;
>  }
>  
>  static int opts_do_parse(QemuOpts *opts, const char *params,
> diff --git a/qemu-option.h b/qemu-option.h
> index 002dd07..9ea59cf 100644
> --- a/qemu-option.h
> +++ b/qemu-option.h
> @@ -96,6 +96,7 @@ typedef struct QemuOptDesc {
>      const char *name;
>      enum QemuOptType type;
>      const char *help;
> +    uint64_t def_value;

The name "def_value" suggests that this is used as a default value for
the option.  In fact it's only used as the default value for printing.
I suggest calling it def_print_str.

Also, if you do the change I suggested above, then this becomes const
char * instead of being uint64_t - this way it can be used for any
option type (str, bool, number, size).

Stefan