Re: [Qemu-block] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Leonid Bloch]

On 1/10/19 9:18 PM, Eric Blake wrote:
> Set the framework up for declaring integer options with an integer
> default, instead of our current insane approach of requiring the
> default value to be given as a string (which then has to be reparsed
> at every use that wants a number).  git grep '[^.]def_value_str' says
> that we have done a good job of NOT abusing the internal field
> outside of the implementation in qemu-option.c; therefore, it is not
> too hard to audit that all code can either handle the new integer
> defaults correctly or abort because a caller violated constraints.
> Sadly, we DO have a constraint that qemu_opt_get() should not be
> called on an option that has an integer type, because we have no
> where to stash a cached const char * result; but callers that want
> an integer should be using qemu_opt_get_number() and friends
> anyways.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>   include/qemu/option.h | 12 ++++++++
>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>   2 files changed, 72 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 844587cab39..46b80d5a6e1 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>       const char *name;
>       enum QemuOptType type;
>       const char *help;
> +    /*
> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
> +     * to a default value or leave NULL for no default.
> +     *
> +     * For other types: Initialize at most non-zero def_value_int or a
> +     * parseable def_value_str for a default (must use a string for an
> +     * explicit default of 0, although an implicit default generally
> +     * works).  If setting def_value_int, calling qemu_opt_get() on
> +     * that option will abort(); instead, call qemu_opt_get_del() or a
> +     * typed getter.
> +     */
> +    uint64_t def_value_int;
>       const char *def_value_str;
>   } QemuOptDesc;
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index de42e2a406a..06c4e8102a8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -321,7 +321,8 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>       opt = qemu_opt_find(opts, name);
>       if (!opt) {
>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> -        if (desc && desc->def_value_str) {
> +        if (desc) {
> +            assert(!desc->def_value_int);
>               return desc->def_value_str;
>           }
>       }
> @@ -364,8 +365,22 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
>       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);
> +        if (desc) {
> +            if (desc->def_value_str) {
> +                str = g_strdup(desc->def_value_str);
> +            } else if (desc->def_value_int) {
> +                switch (desc->type) {
> +                case QEMU_OPT_BOOL:
> +                    str = g_strdup("on");
> +                    break;
> +                case QEMU_OPT_NUMBER:
> +                case QEMU_OPT_SIZE:
> +                    str = g_strdup_printf("%" PRId64, desc->def_value_int);
> +                    break;
> +                default:
> +                    abort();
> +                }
> +            }
>           }
>           return str;
>       }
> @@ -400,8 +415,15 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, 
> const char *name,
>       opt = qemu_opt_find(opts, name);
>       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, &ret, &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return true;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_bool(name, desc->def_value_str, &ret,
> +                                  &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -436,8 +458,15 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts 
> *opts, const char *name,
>       opt = qemu_opt_find(opts, name);
>       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, &ret, 
> &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return desc->def_value_int;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_number(name, desc->def_value_str, &ret,
> +                                    &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -473,8 +502,15 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, 
> const char *name,
>       opt = qemu_opt_find(opts, name);
>       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, &ret, &error_abort);
> +        if (desc) {
> +            if (desc->def_value_int) {
> +                assert(desc->type != QEMU_OPT_STRING);
> +                return desc->def_value_int;
> +            }
> +            if (desc->def_value_str) {
> +                parse_option_size(name, desc->def_value_str, &ret,
> +                                  &error_abort);
> +            }
>           }
>           return ret;
>       }
> @@ -787,9 +823,23 @@ void qemu_opts_print(QemuOpts *opts, const char 
> *separator)
>       }
>       for (; desc && desc->name; desc++) {
>           const char *value;
> +        char *tmp = NULL;
>           opt = qemu_opt_find(opts, desc->name);
> 
>           value = opt ? opt->str : desc->def_value_str;
> +        if (!value && desc->def_value_int) {
> +            switch (desc->type) {
> +            case QEMU_OPT_BOOL:
> +                value = tmp = g_strdup("on");
> +                break;
> +            case QEMU_OPT_NUMBER:
> +            case QEMU_OPT_SIZE:
> +                value = tmp = g_strdup_printf("%" PRId64, 
> desc->def_value_int);
> +                break;
> +            default:
> +                abort();
> +            }
> +        }
>           if (!value) {
>               continue;
>           }
> @@ -803,6 +853,7 @@ void qemu_opts_print(QemuOpts *opts, const char 
> *separator)
>               printf("%s%s=%s", sep, desc->name, value);
>           }
>           sep = separator;
> +        g_free(tmp);
>       }
>   }
> 

If I understand correctly, the number will still be compiled into the 
binary as an expression string, but will be printed correctly during 
runtime?
If so, it still doesn't feel right to put expressions in binaries, but I 
agree that it's more elegant than my solution with the lookup table. I'd 
say the table would be more elegant if there would be more cases in 
which it's needed, but that's not the case.

The solution which Markus described as "attractively stupid" also could 
work, as there are exactly 2 places (which I know of) where it's really 
needed. Also, absolutely no changes is an option, as was mentioned before.

Leonid.