[Qemu-devel] [PATCH 0/7] Clean up -device help

[Qemu-devel] [PATCH 0/7] Clean up -device help

[Markus Armbruster]

It has a number of issues:

* Some help is printed to stderr, which is wrong in the monitor.

* We terminate the program unsuccessfully after giving help.

* Help crept into -global, where it behaves funnily.

* Help on property values does not work for properties that accept the
  value "?".

Markus Armbruster (7):
  qemu-option: Make qemu_opts_foreach() accumulate return values
  qdev: Fix exit code for -device ?
  Revert "qdev: Add help for property value"
  Revert "qdev: Add help for device properties"
  qdev: Add help for device properties
  qdev: update help on -device
  qdev: Add rudimentary help for property value

 hw/qdev-properties.c |   24 ++++--------------------
 hw/qdev.c            |   41 ++++++++++++++++++++++++++++++++---------
 hw/qdev.h            |    1 +
 qemu-option.c        |    2 +-
 qemu-options.hx      |   21 +++++++++------------
 vl.c                 |    8 ++++++++
 6 files changed, 55 insertions(+), 42 deletions(-)

[Qemu-devel] [PATCH 1/7] qemu-option: Make qemu_opts_foreach() accumulate return values

[Markus Armbruster]

Return the bitwise inclusive or of all return values instead of the
last call's value.  This lets you find out whether any of the calls
returned a non-zero value.

No functional change, as existing users either don't care for the
value, or pass non-zero abort_on_failure, which breaks the loop on the
first non-zero return value.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-option.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 24392fc..a52a4c4 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -814,7 +814,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
     int rc = 0;
 
     QTAILQ_FOREACH(opts, &list->head, next) {
-        rc = func(opts, opaque);
+        rc |= func(opts, opaque);
         if (abort_on_failure  &&  rc != 0)
             break;
     }
-- 
1.6.6

Re: [Qemu-devel] [PATCH 1/7] qemu-option: Make qemu_opts_foreach() accumulate return values

[Anthony Liguori]

On 01/29/2010 12:48 PM, Markus Armbruster wrote:
> Return the bitwise inclusive or of all return values instead of the
> last call's value.  This lets you find out whether any of the calls
> returned a non-zero value.
>
> No functional change, as existing users either don't care for the
> value, or pass non-zero abort_on_failure, which breaks the loop on the
> first non-zero return value.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>    

Applied all.  Thanks.

It's great to see you working on this.  I'm really happy with how qdev 
has turned out but it desperately needs better online help.

Regards,

Anthony Liguori

> ---
>   qemu-option.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-option.c b/qemu-option.c
> index 24392fc..a52a4c4 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -814,7 +814,7 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>       int rc = 0;
>
>       QTAILQ_FOREACH(opts,&list->head, next) {
> -        rc = func(opts, opaque);
> +        rc |= func(opts, opaque);
>           if (abort_on_failure&&   rc != 0)
>               break;
>       }
>    

[Qemu-devel] [PATCH 2/7] qdev: Fix exit code for -device ?

[Markus Armbruster]

Help was shoehorned into device creation, qdev_device_add().  Since
help doesn't create a device, it returns NULL, which looks to callers
just like failed device creation.  Monitor handler do_device_add()
doesn't care, but main() exits unsuccessfully.

Move help out of device creation, into new qdev_device_help().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.c |   28 +++++++++++++++++++---------
 hw/qdev.h |    1 +
 vl.c      |    8 ++++++++
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index c643576..f47f0cb 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -153,6 +153,24 @@ static int set_property(const char *name, const char *value, void *opaque)
     return 0;
 }
 
+int qdev_device_help(QemuOpts *opts)
+{
+    const char *driver;
+    DeviceInfo *info;
+    char msg[256];
+
+    driver = qemu_opt_get(opts, "driver");
+    if (driver && !strcmp(driver, "?")) {
+        for (info = device_info_list; info != NULL; info = info->next) {
+            qdev_print_devinfo(info, msg, sizeof(msg));
+            qemu_error("%s\n", msg);
+        }
+        return 1;
+    }
+
+    return 0;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     const char *driver, *path, *id;
@@ -165,14 +183,6 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         qemu_error("-device: no driver specified\n");
         return NULL;
     }
-    if (strcmp(driver, "?") == 0) {
-        char msg[256];
-        for (info = device_info_list; info != NULL; info = info->next) {
-            qdev_print_devinfo(info, msg, sizeof(msg));
-            qemu_error("%s\n", msg);
-        }
-        return NULL;
-    }
 
     /* find driver */
     info = qdev_find_info(NULL, driver);
@@ -726,7 +736,7 @@ void do_device_add(Monitor *mon, const QDict *qdict)
 
     opts = qemu_opts_parse(&qemu_device_opts,
                            qdict_get_str(qdict, "config"), "driver");
-    if (opts)
+    if (opts && !qdev_device_help(opts))
         qdev_device_add(opts);
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 07b9603..0eb45b0 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -104,6 +104,7 @@ typedef struct GlobalProperty {
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 DeviceState *qdev_create(BusState *bus, const char *name);
+int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
diff --git a/vl.c b/vl.c
index 6f1e1ab..39833fc 100644
--- a/vl.c
+++ b/vl.c
@@ -4459,6 +4459,11 @@ char *qemu_find_file(int type, const char *name)
     return buf;
 }
 
+static int device_help_func(QemuOpts *opts, void *opaque)
+{
+    return qdev_device_help(opts);
+}
+
 static int device_init_func(QemuOpts *opts, void *opaque)
 {
     DeviceState *dev;
@@ -5828,6 +5833,9 @@ int main(int argc, char **argv, char **envp)
 
     module_call_init(MODULE_INIT_DEVICE);
 
+    if (qemu_opts_foreach(&qemu_device_opts, device_help_func, NULL, 0) != 0)
+        exit(0);
+
     if (watchdog) {
         i = select_watchdog(watchdog);
         if (i > 0)
-- 
1.6.6

[Qemu-devel] [PATCH 3/7] Revert "qdev: Add help for property value"

[Markus Armbruster]

This reverts commit 922910ce42d15bdb7c2347436b1b5798b5401de4.

The commit has four issues:

* When it runs from the monitor, e.g. "device_add e1000,mac=?", it
  prints to stderr instead of the monitor.

* Help looks to callers just like failed device creation.  This makes
  main() exit unsuccessfully on "-device e1000,mac=?".

* It has an undocumented side effect on -global: "-global e1000.mac=?"
  prints help, but only when we actually add an e1000 device.

* It does not work for properties that accept the value "?".

We need to do this differently.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f5ca05f..8547ad2 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -565,13 +565,8 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
         return -1;
     }
     if (prop->info->parse(dev, prop, value) != 0) {
-        if (strcmp(value, "?") != 0) {
-            fprintf(stderr, "property \"%s.%s\": failed to parse \"%s\"\n",
-                    dev->info->name, name, value);
-        } else {
-            fprintf(stderr, "%s.%s=%s\n",
-                    dev->info->name, name, prop->info->name);
-        }
+        fprintf(stderr, "property \"%s.%s\": failed to parse \"%s\"\n",
+                dev->info->name, name, value);
         return -1;
     }
     return 0;
-- 
1.6.6

[Qemu-devel] [PATCH 4/7] Revert "qdev: Add help for device properties"

[Markus Armbruster]

This reverts commit 2ba6edf0dd740166632df80caa85992b20791a68.

The commit has two issues:

* When it runs from the monitor, e.g. "device_add e1000,?", it prints
  to stderr instead of the monitor.

* Help looks to callers just like failed device creation.  This makes
  main() exit unsuccessfully on "-device e1000,?".

We need to do this differently.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev-properties.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 8547ad2..277ff9e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -544,19 +544,8 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 
     prop = qdev_prop_find(dev, name);
     if (!prop) {
-        if (strcmp(name, "?") != 0) {
-            fprintf(stderr, "property \"%s.%s\" not found\n",
-                    dev->info->name, name);
-        } else {
-            fprintf(stderr, "supported properties:\n");
-            if (dev->info->props != NULL) {
-                Property *props = dev->info->props;
-                while (props->name) {
-                    fprintf(stderr, "%s.%s\n", dev->info->name, props->name);
-                    props++;
-                }
-            }
-        }
+        fprintf(stderr, "property \"%s.%s\" not found\n",
+                dev->info->name, name);
         return -1;
     }
     if (!prop->info->parse) {
-- 
1.6.6

[Qemu-devel] [PATCH 5/7] qdev: Add help for device properties

[Markus Armbruster]

Option "-device DRIVER,?" and monitor command "device_add DRIVER,?"
print the supported properties instead of creating a device.  The
former also terminates the program.

This is commit 2ba6edf0 (just reverted) done right.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index f47f0cb..7c3701c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -158,6 +158,7 @@ int qdev_device_help(QemuOpts *opts)
     const char *driver;
     DeviceInfo *info;
     char msg[256];
+    Property *prop;
 
     driver = qemu_opt_get(opts, "driver");
     if (driver && !strcmp(driver, "?")) {
@@ -168,7 +169,19 @@ int qdev_device_help(QemuOpts *opts)
         return 1;
     }
 
-    return 0;
+    if (!qemu_opt_get(opts, "?")) {
+        return 0;
+    }
+
+    info = qdev_find_info(NULL, driver);
+    if (!info) {
+        return 0;
+    }
+
+    for (prop = info->props; prop && prop->name; prop++) {
+        qemu_error("%s.%s\n", info->name, prop->name);
+    }
+    return 1;
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts)
-- 
1.6.6

[Qemu-devel] [PATCH 6/7] qdev: update help on -device

[Markus Armbruster]

While there, use "property" rather than "option", for consistency with
-global.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-options.hx |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 5c9f482..4c1bcfb 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -403,20 +403,17 @@ Network adapter that supports CDC ethernet and RNDIS protocols.
 ETEXI
 
 DEF("device", HAS_ARG, QEMU_OPTION_device,
-    "-device driver[,option[=value][,...]]\n"
-    "                add device (based on driver) with default or\n"
-    "                user defined options\n"
+    "-device driver[,prop[=value][,...]]\n"
+    "                add device (based on driver)\n"
+    "                prop=value,... sets driver properties\n"
     "                use -device ? to print all possible drivers\n"
-    "                use -device driver,? to print all possible options\n"
-    "                use -device driver,option=? to print a help for value\n")
-STEXI
-@item -device @var{driver}[,@var{option}[=@var{value}][,...]]
-Add device @var{driver}. Depending on the device type,
-@var{option} (with default or given @var{value}) may be useful.
-To get a help on possible @var{driver}s, @var{option}s or @var{value}s, use
-@code{-device ?},
-@code{-device @var{driver},?} or
-@code{-device @var{driver},@var{option}=?}. 
+    "                use -device driver,? to print all possible properties\n")
+STEXI
+@item -device @var{driver}[,@var{prop}[=@var{value}][,...]]
+Add device @var{driver}.  @var{prop}=@var{value} sets driver
+properties.  Valid properties depend on the driver.  To get help on
+possible drivers and properties, use @code{-device ?} and
+@code{-device @var{driver},?}.
 ETEXI
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
1.6.6

[Qemu-devel] [PATCH 7/7] qdev: Add rudimentary help for property value

[Markus Armbruster]

This provides the same information as reverted commit 2ba6edf0.  Not
much, just better than nothing.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 7c3701c..539b5a2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -179,7 +179,7 @@ int qdev_device_help(QemuOpts *opts)
     }
 
     for (prop = info->props; prop && prop->name; prop++) {
-        qemu_error("%s.%s\n", info->name, prop->name);
+        qemu_error("%s.%s=%s\n", info->name, prop->name, prop->info->name);
     }
     return 1;
 }
-- 
1.6.6