[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults
From: |
Leonid Bloch |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/6] qemu-option: Allow integer defaults |
Date: |
Fri, 11 Jan 2019 14:14:48 +0000 |
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.
- [Qemu-devel] [PATCH v3 0/6] include: Auto-generate the sizes lookup table, Eric Blake, 2019/01/10
- [Qemu-devel] [PATCH v3 3/6] Revert "vdi: Use a literal number of bytes for DEFAULT_CLUSTER_SIZE", Eric Blake, 2019/01/10
- [Qemu-devel] [PATCH v3 2/6] block: Take advantage of QemuOpt default integers, Eric Blake, 2019/01/10
- [Qemu-devel] [PATCH v3 4/6] qemu: Prefer '(x * MiB)' over 'S_xiB', Eric Blake, 2019/01/10
- [Qemu-devel] [PATCH v3 5/6] Revert "include: Add a comment to explain the origin of sizes' lookup table", Eric Blake, 2019/01/10
- [Qemu-devel] [PATCH v3 6/6] Revert "include: Add a lookup table of sizes", Eric Blake, 2019/01/10