Patchwork [V12,1/4] add def_value_str and use it in qemu_opts_print

login
register
mail settings
Submitter Robert Wang
Date Feb. 28, 2013, 9:20 a.m.
Message ID <1362043236-4635-2-git-send-email-wdongxu@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/223840/
State New
Headers show

Comments

Robert Wang - Feb. 28, 2013, 9:20 a.m.
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(-)
Markus Armbruster - March 21, 2013, 3:59 p.m.
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 :)

Patch

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;
 }