| Message ID | 1362043236-4635-2-git-send-email-wdongxu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes: > qemu_opts_print has no user now, so can re-write the function safely. > > qemu_opts_print will be used while using "qemu-img create", it will > produce the same output as previous code. > > The behavior of this function has changed: > > 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. The commit message needs updating for v12: def_value_str isn't just about qemu_opts_print() anymore. More serious, the default value job is only half done. My fault, because my description of what I want done in my review of v11 was confused and misleading. I apologize for that. Let me try again. You want qemu_opts_print() to print the effective value even for some options that aren't set, because you don't want to change output of qemu-img. Fair enough. Currently, what to do when an option isn't set is left to qemu_opt_get_FOO() callers: * qemu_opt_get() returns NULL when option isn't set. Caller can fall back to a default value, or do something else entirely. * qemu_opt_get_{bool,number,size}() let the caller supply a default value argument to be returned when the option isn't set. Trouble is qemu_opts_print() can't know what these callers do, so it can't show the effective value for options that aren't set. Your v11 solved this problem by putting the default value in QemuOptDesc, too, for use by qemu_opts_print(). Requires copying the caller's real default value there. You copied only the default values you actually need for your particular use of qemu_opts_print(). Keeping the copies consistent is manual. It's impossible when different callers use different default values, but that isn't the case for any of the ones you copied, as far as I can tell. I dislike having the default value in multiple places. Two ways to avoid the duplication: 1. Keep the default value entirely in the callers In cases where you want qemu_opts_print() to show the effective value, change the caller to also set the value in opts when it falls back to a default. Keep qemu_opts_print() showing only options that are actually set. This is fine, because the previous paragraph makes sure all the values you want to see are actually set. 2. Move the default value entirely to QemuOptDesc When getting the value of an option that hasn't been set, and QemuOptDesc has a default value, return that. Else, behave as before. Example: qemu_opt_get_number(opts, "foo", 42) If "foo" has been set in opts, return its value. Else, if opt's QemuOptDesc has a default value for "foo", return that. Else, return 42. Note that the last argument is useless when QemuOptDesc has a default value. Ugly. If it bothers us, assert that the argument equals the default from QemuOptDesc. Example: qemu_opt_get(opts, "bar") If "bar" has been set in opts, return its value. Else, if opt's QemuOptDesc has a default value for "bar", return that. Else, return NULL. Note that "no default in QemuOptDesc" does *not* mean "there is no default". There may or may not be a default, depending on how the option's users get the value. Hope I'm making sense this time :)
diff --git a/include/qemu/option.h b/include/qemu/option.h index ba197cd..394170a 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -96,6 +96,7 @@ typedef struct QemuOptDesc { const char *name; enum QemuOptType type; const char *help; + const char *def_value_str; } QemuOptDesc; struct QemuOptsList { diff --git a/util/qemu-option.c b/util/qemu-option.c index 5a1d03c..dbb77b9 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -646,6 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, } opt->desc = desc; opt->str = g_strdup(value); + opt->str = g_strdup(value ?: desc->def_value_str); qemu_opt_parse(opt, &local_err); if (error_is_set(&local_err)) { error_propagate(errp, local_err); @@ -863,13 +864,33 @@ void qemu_opts_del(QemuOpts *opts) int qemu_opts_print(QemuOpts *opts, void *dummy) { QemuOpt *opt; + QemuOptDesc *desc = opts->list->desc; - 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); + if (desc[0].name == NULL) { + QTAILQ_FOREACH(opt, &opts->head, next) { + printf("%s=\"%s\" ", opt->name, opt->str); + } + return 0; + } + for (; desc && desc->name; desc++) { + const char *value = desc->def_value_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); + } } - fprintf(stderr, "\n"); return 0; }
qemu_opts_print has no user now, so can re-write the function safely. qemu_opts_print will be used while using "qemu-img create", it will produce the same output as previous code. The behavior of this function has changed: 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. Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> --- v11->v12 1) make def_value_str become the real default value string in opt_set function. v10->v11: 1) print all values that have actually been assigned while accept-any cases. v7->v8: 1) print "elements => accept any params" while opts_accepts_any() == true. 2) since def_print_str is the default value if an option isn't set, so rename it to def_value_str. include/qemu/option.h | 1 + util/qemu-option.c | 31 ++++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-)