[Qemu-devel] [PATCH 0/5] QemuOpts patches.

[Qemu-devel] [PATCH 0/5] QemuOpts patches.

[Gerd Hoffmann]

  Hi,

Here are a bunch of small QemuOpts patches.  Some bugfixes, some new
functions, -device is switched to use QemuOpts, a new -drive-set option
was added to handle the filename issue.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/5] QemuOpts: add some functions

[Gerd Hoffmann]

qemu_opt_foreach: loop over all QemuOpts entries.
qemu_opts_id: return QemuOpts id.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-option.c |   19 +++++++++++++++++++
 qemu-option.h |    4 ++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 73c2175..86c0b09 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -606,6 +606,20 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
     return 0;
 }
 
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
+                     int abort_on_failure)
+{
+    QemuOpt *opt;
+    int rc = 0;
+
+    TAILQ_FOREACH(opt, &opts->head, next) {
+        rc = func(opt->name, opt->str, opaque);
+        if (abort_on_failure  &&  rc != 0)
+            break;
+    }
+    return rc;
+}
+
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
 {
     QemuOpts *opts;
@@ -662,6 +676,11 @@ int qemu_opts_set(QemuOptsList *list, const char *id,
     return qemu_opt_set(opts, name, value);
 }
 
+const char *qemu_opts_id(QemuOpts *opts)
+{
+    return opts->id;
+}
+
 void qemu_opts_del(QemuOpts *opts)
 {
     QemuOpt *opt;
diff --git a/qemu-option.h b/qemu-option.h
index 428c947..56c7eac 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -104,11 +104,15 @@ int qemu_opt_get_bool(QemuOpts *opts, const char *name, int defval);
 uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
 uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
+typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
+int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
+                     int abort_on_failure);
 
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists);
 int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value);
+const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_del(QemuOpts *opts);
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, const char *firstname);
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/5] QemuOpts: qemu_opts_parse: fix id= parsing

[Gerd Hoffmann]

We can't use get_param_value(), it can't handle parameters without
'=' in there.  Examples not working because of that:

  -device foo,id=bar
  -device file=/path/image,format=qcow2,snapshot,id=disk0

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-option.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 86c0b09..cee7dff 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -714,8 +714,13 @@ QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, const char *fi
     QemuOpts *opts;
     const char *p,*pe,*pc;
 
-    if (get_param_value(value, sizeof(value), "id", params))
+    if (strncmp(params, "id=", 3) == 0) {
+        get_opt_value(value, sizeof(value), params+3);
         id = qemu_strdup(value);
+    } else if ((p = strstr(params, ",id=")) != NULL) {
+        get_opt_value(value, sizeof(value), p+4);
+        id = qemu_strdup(value);
+    }
     opts = qemu_opts_create(list, id, 1);
     if (opts == NULL)
         return NULL;
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/5] QemuOpts: make the drive id actually show up in "info block".

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index bb56644..4f322c1 100644
--- a/vl.c
+++ b/vl.c
@@ -2213,7 +2213,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     /* init */
 
     dinfo = qemu_mallocz(sizeof(*dinfo));
-    if ((buf = qemu_opt_get(opts, "id")) != NULL) {
+    if ((buf = qemu_opts_id(opts)) != NULL) {
         dinfo->id = qemu_strdup(buf);
     } else {
         /* no id supplied -> create one */
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/5] QemuOpts: add -drive-set option

[Gerd Hoffmann]

The typical use case will be file (no filename quoting issues), i.e.

	-drive id=test,if=virtio
	-drive-set test.file=/vmdisk/test-virtio.img

It will work for any drive argument though.  Except id= for obvious
reasons ;).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-options.hx |    5 ++++-
 vl.c            |   25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 1b420a3..1b26588 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -95,8 +95,11 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none][,format=f][,serial=s]\n"
-    "       [,addr=A]\n"
+    "       [,addr=A][,id=name]\n"
     "                use 'file' as a drive image\n")
+DEF("drive-set", HAS_ARG, QEMU_OPTION_drive_set,
+    "-drive-set id.arg=value\n"
+    "                set arg parameter for drive id\n")
 STEXI
 @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
 
diff --git a/vl.c b/vl.c
index 4f322c1..e602369 100644
--- a/vl.c
+++ b/vl.c
@@ -1887,6 +1887,27 @@ QemuOpts *drive_add(const char *file, const char *fmt, ...)
     return opts;
 }
 
+static int drive_add_arg(const char *str)
+{
+    char id[32], arg[32], value[1024];
+    QemuOpts *opts;
+
+    if (sscanf(str, "%31[^.].%31[^=]=%s", id, arg, value) != 3) {
+        fprintf(stderr, "can't parse: \"%s\"\n", str);
+        return -1;
+    }
+    opts = qemu_opts_find(&drive_opt_list, id);
+    if (!opts) {
+        fprintf(stderr, "there is no drive \"%s\" defined\n", id);
+        return -1;
+    }
+    if (-1 == qemu_opt_set(opts, arg, value)) {
+        fprintf(stderr, "failed to set \"%s\" for drive \"%s\"\n", arg, id);
+        return -1;
+    }
+    return 0;
+}
+
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
 {
     DriveInfo *dinfo;
@@ -5031,6 +5052,10 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_drive:
                 drive_add(NULL, "%s", optarg);
 	        break;
+            case QEMU_OPTION_drive_set:
+                if (drive_add_arg(optarg) != 0)
+                    exit(1);
+	        break;
             case QEMU_OPTION_mtdblock:
                 drive_add(optarg, MTD_ALIAS);
                 break;
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/5] QemuOpts: switch over -device.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/qdev.c |   79 ++++++++++++++++++++++++++++++-------------------------------
 hw/qdev.h |    5 ++-
 vl.c      |   40 ++++++++++++++++++++-----------
 3 files changed, 68 insertions(+), 56 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 479eb72..1c63c8a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -105,18 +105,34 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     return dev;
 }
 
-DeviceState *qdev_device_add(const char *cmdline)
+static int set_property(const char *name, const char *value, void *opaque)
 {
+    DeviceState *dev = opaque;
+
+    if (strcmp(name, "driver") == 0)
+        return 0;
+    if (strcmp(name, "bus") == 0)
+        return 0;
+
+    if (-1 == qdev_prop_parse(dev, name, value)) {
+        fprintf(stderr, "can't set property \"%s\" to \"%s\" for \"%s\"\n",
+                name, value, dev->info->name);
+        return -1;
+    }
+    return 0;
+}
+
+DeviceState *qdev_device_add(QemuOpts *opts)
+{
+    const char *driver, *path, *id;
     DeviceInfo *info;
     DeviceState *qdev;
     BusState *bus;
-    char driver[32], path[128] = "";
-    char tag[32], value[256];
-    const char *params = NULL;
-    int n = 0;
 
-    if (1 != sscanf(cmdline, "%32[^,],%n", driver, &n)) {
-        fprintf(stderr, "device parse error: \"%s\"\n", cmdline);
+    qemu_opts_print(opts, NULL);
+    driver = qemu_opt_get(opts, "driver");
+    if (!driver) {
+        fprintf(stderr, "-device: no driver specified\n");
         return NULL;
     }
     if (strcmp(driver, "?") == 0) {
@@ -125,10 +141,8 @@ DeviceState *qdev_device_add(const char *cmdline)
         }
         return NULL;
     }
-    if (n) {
-        params = cmdline + n;
-        get_param_value(path, sizeof(path), "bus",  params);
-    }
+
+    /* find driver */
     info = qdev_find_info(NULL, driver);
     if (!info) {
         fprintf(stderr, "Device \"%s\" not found.  Try -device '?' for a list.\n",
@@ -141,40 +155,26 @@ DeviceState *qdev_device_add(const char *cmdline)
         return NULL;
     }
 
-    if (strlen(path)) {
+    /* find bus */
+    path = qemu_opt_get(opts, "bus");
+    if (path != NULL) {
         bus = qbus_find(path);
-        if (!bus)
-            return NULL;
-        qdev = qdev_create(bus, driver);
     } else {
         bus = qbus_find_recursive(main_system_bus, NULL, info->bus_info);
-        if (!bus)
-            return NULL;
-        qdev = qdev_create(bus, driver);
     }
+    if (!bus)
+        return NULL;
 
-    if (params) {
-        while (params[0]) {
-            if (2 != sscanf(params, "%31[^=]=%255[^,]%n", tag, value, &n)) {
-                fprintf(stderr, "parse error at \"%s\"\n", params);
-                break;
-            }
-            params += n;
-            if (params[0] == ',')
-                params++;
-            if (strcmp(tag, "bus") == 0)
-                continue;
-            if (strcmp(tag, "id") == 0) {
-                qdev->id = qemu_strdup(value);
-                continue;
-            }
-            if (-1 == qdev_prop_parse(qdev, tag, value)) {
-                fprintf(stderr, "can't set property \"%s\" to \"%s\" for \"%s\"\n",
-                        tag, value, driver);
-            }
-        }
+    /* create device, set properties */
+    qdev = qdev_create(bus, driver);
+    id = qemu_opts_id(opts);
+    if (id) {
+        qdev->id = id;
+    }
+    if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
+        qdev_free(qdev);
+        return NULL;
     }
-
     qdev_init(qdev);
     return qdev;
 }
@@ -191,7 +191,6 @@ void qdev_init(DeviceState *dev)
 void qdev_free(DeviceState *dev)
 {
     LIST_REMOVE(dev, sibling);
-    qemu_free(dev->id);
     qemu_free(dev);
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index af2ee0f..9f9a940 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -3,6 +3,7 @@
 
 #include "hw.h"
 #include "sys-queue.h"
+#include "qemu-option.h"
 
 typedef struct Property Property;
 
@@ -19,7 +20,7 @@ typedef struct BusInfo BusInfo;
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
-    char *id;
+    const char *id;
     DeviceInfo *info;
     BusState *parent_bus;
     int num_gpio_out;
@@ -82,7 +83,7 @@ struct CompatProperty {
 /*** Board API.  This should go away once we have a machine config file.  ***/
 
 DeviceState *qdev_create(BusState *bus, const char *name);
-DeviceState *qdev_device_add(const char *cmdline);
+DeviceState *qdev_device_add(QemuOpts *opts);
 void qdev_init(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 
diff --git a/vl.c b/vl.c
index e602369..9400956 100644
--- a/vl.c
+++ b/vl.c
@@ -4802,9 +4802,27 @@ char *qemu_find_file(int type, const char *name)
     return buf;
 }
 
+static QemuOptsList device_opt_list = {
+    .name = "device",
+    .head = TAILQ_HEAD_INITIALIZER(device_opt_list.head),
+    .desc = {
+        /* no elements, accept any */
+        { /* end if list */ }
+    },
+};
+
+static int device_init_func(QemuOpts *opts, void *opaque)
+{
+    DeviceState *dev;
+
+    dev = qdev_device_add(opts);
+    if (!dev)
+        return -1;
+    return 0;
+}
+
 struct device_config {
     enum {
-        DEV_GENERIC,   /* -device      */
         DEV_USB,       /* -usbdevice   */
         DEV_BT,        /* -bt          */
     } type;
@@ -4838,16 +4856,6 @@ static int foreach_device_config(int type, int (*func)(const char *cmdline))
     return 0;
 }
 
-static int generic_parse(const char *cmdline)
-{
-    DeviceState *dev;
-
-    dev = qdev_device_add(cmdline);
-    if (!dev)
-        return -1;
-    return 0;
-}
-
 int main(int argc, char **argv, char **envp)
 {
     const char *gdbstub_dev = NULL;
@@ -4862,7 +4870,7 @@ int main(int argc, char **argv, char **envp)
     int cyls, heads, secs, translation;
     const char *net_clients[MAX_NET_CLIENTS];
     int nb_net_clients;
-    QemuOpts *hda_opts = NULL;
+    QemuOpts *hda_opts = NULL, *opts;
     int optind;
     const char *r, *optarg;
     CharDriverState *monitor_hd = NULL;
@@ -5472,7 +5480,11 @@ int main(int argc, char **argv, char **envp)
                 add_device_config(DEV_USB, optarg);
                 break;
             case QEMU_OPTION_device:
-                add_device_config(DEV_GENERIC, optarg);
+                opts = qemu_opts_parse(&device_opt_list, optarg, "driver");
+                if (!opts) {
+                    fprintf(stderr, "parse error: %s\n", optarg);
+                    exit(1);
+                }
                 break;
             case QEMU_OPTION_smp:
             {
@@ -6008,7 +6020,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     /* init generic devices */
-    if (foreach_device_config(DEV_GENERIC, generic_parse))
+    if (qemu_opts_foreach(&device_opt_list, device_init_func, NULL, 1) != 0)
         exit(1);
 
     if (!display_state)
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 4/5] QemuOpts: add -drive-set option

[Anthony Liguori]

Gerd Hoffmann wrote:
> The typical use case will be file (no filename quoting issues), i.e.
>
> 	-drive id=test,if=virtio
> 	-drive-set test.file=/vmdisk/test-virtio.img
>   

I'm not a big fan of this syntax because you still end up with not 
putting a filename as a single argument.  I don't think it extends very 
well to other option types either like -net.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/5] QemuOpts: add -drive-set option

[Gerd Hoffmann]

On 07/30/09 15:58, Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> The typical use case will be file (no filename quoting issues), i.e.
>>
>> -drive id=test,if=virtio
>> -drive-set test.file=/vmdisk/test-virtio.img
>
> I'm not a big fan of this syntax because you still end up with not
> putting a filename as a single argument.

You can take everything after '=' as-is though, so the parsing/quoting 
issues are gone nevertheless.

> I don't think it extends very
> well to other option types either like -net.

Problem with the "-drive.$id.$arg $value" syntax suggested by you is 
that the qemu option parser can handle fixed -option strings only.

Other ideas?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 4/5] QemuOpts: add -drive-set option

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 07/30/09 15:58, Anthony Liguori wrote:
>> Gerd Hoffmann wrote:
>>> The typical use case will be file (no filename quoting issues), i.e.
>>>
>>> -drive id=test,if=virtio
>>> -drive-set test.file=/vmdisk/test-virtio.img
>>
>> I'm not a big fan of this syntax because you still end up with not
>> putting a filename as a single argument.
>
> You can take everything after '=' as-is though, so the parsing/quoting 
> issues are gone nevertheless.
>
>> I don't think it extends very
>> well to other option types either like -net.
>
> Problem with the "-drive.$id.$arg $value" syntax suggested by you is 
> that the qemu option parser can handle fixed -option strings only.

Right, but that shouldn't stop us if we think it's the right syntax.

But in the very least, instead of a -drive-set, -net-set, etc., I would 
rather that we had a single -set command that was like:

-set drive.test.file=/vmdisk/test-virtio.img

Whereas this could map into a config file fragment.  Our host config 
file could then look like:

drive.test.file = "/vmdisk/test-virtio.img"

or:

[drive.test]
file = "/vmdisk/test-virtio.img"

I don't really know the right syntax, but that's where we should be heading.

Regards,

Anthony Liguori