[Qemu-devel] [PATCH v3 00/17] Basic device state visualization

[Qemu-devel] [PATCH v3 00/17] Basic device state visualization

[Jan Kiszka]

And here is v3. Dependencies remained, the changes are:
 - disabled device_show for QMP use (due to protocol instability)
 - reordered device tree path search: user-assigned ID comes last now
 - added vmstate version-id to device_show output
 - base64 cleanups according to review comments
 - fixed an off-by-one in the qmp.py changes

Git url remained the same:

	git://git.kiszka.org/qemu.git queues/device-show

Thanks again for the comments.

Jan Kiszka (17):
  Add dependency of JSON unit tests on config-host.h
  qdev: Fix scanning across single-bus devices
  qdev: Allow device addressing via 'driver.instance'
  qdev: Give qtree names precedence over user-assigned IDs
  qdev: Convert device and bus lists to QTAILQ
  qdev: Allow device specification by qtree path for device_del
  qdev: Push QMP mode checks into qbus_list_bus/dev
  monitor: Add completion for qdev paths
  Add base64 encoder/decoder
  QMP: Reserve namespace for complex object classes
  Add QBuffer
  monitor: return length of printed string via monitor_[v]printf
  monitor: Allow to exclude commands from QMP
  monitor: Add basic device state visualization
  QMP: Teach basic capability negotiation to python example
  QMP: Fix python helper /wrt long return strings
  QMP: Add support for buffer class to qmp python helper

 Makefile                 |    5 +-
 Makefile.objs            |    4 +-
 QMP/qmp-shell            |    1 +
 QMP/qmp-spec.txt         |   24 +++-
 QMP/qmp.py               |   29 +++-
 QMP/vm-info              |    1 +
 base64.c                 |  202 ++++++++++++++++++++++
 base64.h                 |   19 ++
 check-qbuffer.c          |  172 +++++++++++++++++++
 configure                |    2 +-
 docs/qdev-device-use.txt |   20 ++-
 hw/acpi_piix4.c          |    2 +-
 hw/hw.h                  |    2 +
 hw/i2c.c                 |    2 +-
 hw/pci-hotplug.c         |    2 +-
 hw/qdev.c                |  414 ++++++++++++++++++++++++++++++++++++++++-----
 hw/qdev.h                |   12 +-
 hw/ssi.c                 |    6 +-
 monitor.c                |  121 ++++++++++++--
 monitor.h                |    4 +-
 qbuffer.c                |  116 +++++++++++++
 qbuffer.h                |   33 ++++
 qemu-monitor.hx          |   29 +++-
 qemu-tool.c              |    6 +-
 qerror.c                 |    4 +
 qerror.h                 |    3 +
 qjson.c                  |   15 ++
 qobject.h                |    1 +
 28 files changed, 1165 insertions(+), 86 deletions(-)
 create mode 100644 base64.c
 create mode 100644 base64.h
 create mode 100644 check-qbuffer.c
 create mode 100644 qbuffer.c
 create mode 100644 qbuffer.h

[Qemu-devel] [PATCH v3 01/17] Add dependency of JSON unit tests on config-host.h

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 3a8a311..1514433 100644
--- a/Makefile
+++ b/Makefile
@@ -144,6 +144,8 @@ qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobj
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
 
+check-qint.o check-qstring.o check-qdict.o check-qlist.o check-qfloat.o check-qjson.o: $(GENERATED_HEADERS)
+
 check-qint: check-qint.o qint.o qemu-malloc.o
 check-qstring: check-qstring.o qstring.o qemu-malloc.o
 check-qdict: check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qemu-malloc.o qlist.o
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 02/17] qdev: Fix scanning across single-bus devices

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
there is only one child bus per device.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 docs/qdev-device-use.txt |    4 ++++
 hw/qdev.c                |   12 +++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index f252c8e..9ac1fa1 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -20,6 +20,10 @@ bus named pci.0.  To put a FOO device into its slot 4, use -device
 FOO,bus=/i440FX-pcihost/pci.0,addr=4.  The abbreviated form bus=pci.0
 also works as long as the bus name is unique.
 
+Furthermore, if a device only hosts a single bus, the bus name can be
+omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
+/i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
+
 Note: the USB device address can't be controlled at this time.
 
 === Block Devices ===
diff --git a/hw/qdev.c b/hw/qdev.c
index aa2ce01..2e50531 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -557,7 +557,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 
 static BusState *qbus_find(const char *path)
 {
-    DeviceState *dev;
+    DeviceState *dev, *next_dev;
     BusState *bus;
     char elem[128];
     int pos, len;
@@ -603,6 +603,7 @@ static BusState *qbus_find(const char *path)
             return NULL;
         }
 
+search_dev_bus:
         assert(path[pos] == '/' || !path[pos]);
         while (path[pos] == '/') {
             pos++;
@@ -633,6 +634,15 @@ static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
+            if (dev->num_child_bus == 1) {
+                /* Last element might have been a short-cut to a device on
+                 * the single child bus of the parent device. */
+                next_dev = qbus_find_dev(QTAILQ_FIRST(&dev->child_bus), elem);
+                if (next_dev) {
+                    dev = next_dev;
+                    goto search_dev_bus;
+                }
+            }
             qerror_report(QERR_BUS_NOT_FOUND, elem);
             if (!monitor_cur_is_qmp()) {
                 qbus_list_bus(dev);
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH v3 02/17] qdev: Fix scanning across single-bus devices

[Paul Brook]

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
> make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
> there is only one child bus per device.

How about we make this explicit in the syntax by having a different separator 
for bus names. e.g. /dev1:bus1/dev2:bus1. Omitting the :bus is then 
unambiguous.

Paul

Re: [Qemu-devel] [PATCH v3 02/17] qdev: Fix scanning across single-bus devices

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

Paul Brook wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> As long as we allow /dev.1 as shortcut for /dev1/bus1, we also have to
>> make sure that /dev1/dev2 works for /dev1/bus1/dev2/bus2 - as long as
>> there is only one child bus per device.
> 
> How about we make this explicit in the syntax by having a different separator 
> for bus names. e.g. /dev1:bus1/dev2:bus1. Omitting the :bus is then 
> unambiguous.

...as long as there will always be only one bus at dev1 and/or only one
dev2 on dev1's buses.

I think we should stick with the consensus of dropping these abbreviations.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v3 03/17] qdev: Allow device addressing via 'driver.instance'

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Extend qbus_find_dev to allow addressing of devices without an unique id
via an optional per-bus instance number. The new formats are
'driver.instance' and 'alias.instance'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 docs/qdev-device-use.txt |   14 +++++++++++++-
 hw/qdev.c                |   23 ++++++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index 9ac1fa1..74d4960 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -1,6 +1,6 @@
 = How to convert to -device & friends =
 
-=== Specifying Bus and Address on Bus ===
+=== Specifying Bus, Address on Bus, and Devices ===
 
 In qdev, each device has a parent bus.  Some devices provide one or
 more buses for children.  You can specify a device's parent bus with
@@ -24,6 +24,18 @@ Furthermore, if a device only hosts a single bus, the bus name can be
 omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
 /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
 
+Existing devices can be addressed either via a unique ID if it was
+assigned during creation or via the device tree path:
+
+/full_bus_address/driver_name[.instance_number]
+    or
+abbreviated_bus_address/driver_name[.instance_number]
+
+The instance number is zero-based.
+
+Example: /i440FX-pcihost/pci.0/e1000.1 addresses the second e1000
+adapter on the bus 'pci.0'.
+
 Note: the USB device address can't be controlled at this time.
 
 === Block Devices ===
diff --git a/hw/qdev.c b/hw/qdev.c
index 2e50531..6b4a629 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -527,28 +527,41 @@ static BusState *qbus_find_bus(DeviceState *dev, char *elem)
     return NULL;
 }
 
-static DeviceState *qbus_find_dev(BusState *bus, char *elem)
+static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
 {
     DeviceState *dev;
+    int instance, n;
+    char buf[128];
 
     /*
      * try to match in order:
      *   (1) instance id, if present
-     *   (2) driver name
-     *   (3) driver alias, if present
+     *   (2) driver name [.instance]
+     *   (3) driver alias [.instance], if present
      */
     QLIST_FOREACH(dev, &bus->children, sibling) {
         if (dev->id  &&  strcmp(dev->id, elem) == 0) {
             return dev;
         }
     }
+
+    if (sscanf(elem, "%127[^.].%u", buf, &instance) == 2) {
+        elem = buf;
+    } else {
+        instance = 0;
+    }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (strcmp(dev->info->name, elem) == 0) {
+        if (strcmp(dev->info->name, elem) == 0 && n++ == instance) {
             return dev;
         }
     }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0) {
+        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0 &&
+            n++ == instance) {
             return dev;
         }
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As the user may specify ambiguous device IDs, let's search for their
official names first before considering the user-supplied identifiers.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 docs/qdev-device-use.txt |    4 +++-
 hw/qdev.c                |   18 +++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index 74d4960..0160191 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -25,7 +25,9 @@ omitted in the path.  Example: /i440FX-pcihost/PIIX3 abbreviates
 /i440FX-pcihost/pci.0/PIIX3/isa.0 as none of the buses has siblings.
 
 Existing devices can be addressed either via a unique ID if it was
-assigned during creation or via the device tree path:
+assigned during creation or via the device tree path. In conflicts,
+the latter has precedence. A device tree path has the following
+structure:
 
 /full_bus_address/driver_name[.instance_number]
     or
diff --git a/hw/qdev.c b/hw/qdev.c
index 6b4a629..eeadf4a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -535,16 +535,10 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
 
     /*
      * try to match in order:
-     *   (1) instance id, if present
-     *   (2) driver name [.instance]
-     *   (3) driver alias [.instance], if present
+     *   (1) driver name [.instance]
+     *   (2) driver alias [.instance], if present
+     *   (3) instance id, if present
      */
-    QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->id  &&  strcmp(dev->id, elem) == 0) {
-            return dev;
-        }
-    }
-
     if (sscanf(elem, "%127[^.].%u", buf, &instance) == 2) {
         elem = buf;
     } else {
@@ -565,6 +559,12 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
             return dev;
         }
     }
+
+    QLIST_FOREACH(dev, &bus->children, sibling) {
+        if (dev->id && strcmp(dev->id, elem) == 0) {
+            return dev;
+        }
+    }
     return NULL;
 }
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> As the user may specify ambiguous device IDs, let's search for their
> official names first before considering the user-supplied identifiers.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

The problem is letting the user specify ambiguous device IDs in the
first place!  That way is madness...

Re: [Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Avi Kivity]

On 05/29/2010 11:01 AM, Markus Armbruster wrote:
> Jan Kiszka<jan.kiszka@web.de>  writes:
>
>    
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> As the user may specify ambiguous device IDs, let's search for their
>> official names first before considering the user-supplied identifiers.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>      
> The problem is letting the user specify ambiguous device IDs in the
> first place!  That way is madness...
>    

Agreed, we're sowing the seeds for future problems.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Markus Armbruster]

[cc: kraxel]

Avi Kivity <avi@redhat.com> writes:

> On 05/29/2010 11:01 AM, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@web.de>  writes:
>>
>>    
>>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>>
>>> As the user may specify ambiguous device IDs, let's search for their
>>> official names first before considering the user-supplied identifiers.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>      
>> The problem is letting the user specify ambiguous device IDs in the
>> first place!  That way is madness...
>>    
>
> Agreed, we're sowing the seeds for future problems.

On closer look, we have some protection against duplicate IDs, but it
got holes.

We don't assign IDs to default devices.

-device and device_add use the ID of a qemu_device_opts.  Which rejects
duplicate IDs.

pci_add nic -net use either the ID or option "name" of qemu_net_opts.
And there's our hole.  Reproducible with "-net user -net nic,id=foo
-device lsi,id=foo".

We better check for duplicates right where we create qdevs.  Gerd, what
do you think about the appended patch?


diff --git a/hw/qdev.c b/hw/qdev.c
index b91bed1..beb4235 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -40,6 +40,7 @@ DeviceInfo *device_info_list;
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
 static BusState *qbus_find(const char *path);
+static DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
@@ -242,6 +243,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     qdev = qdev_create_from_info(bus, info);
     id = qemu_opts_id(opts);
     if (id) {
+        if (qdev_find_recursive(main_system_bus, id)) {
+            qerror_report(QERR_DUPLICATE_ID, id, "device");
+            return NULL;
+        }
         qdev->id = id;
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {

Re: [Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Gerd Hoffmann]

> pci_add nic -net use either the ID or option "name" of qemu_net_opts.
> And there's our hole.  Reproducible with "-net user -net nic,id=foo
> -device lsi,id=foo".

Oh.  Well.  Yes, better plug that.

> @@ -242,6 +243,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>       qdev = qdev_create_from_info(bus, info);
>       id = qemu_opts_id(opts);
>       if (id) {
> +        if (qdev_find_recursive(main_system_bus, id)) {
> +            qerror_report(QERR_DUPLICATE_ID, id, "device");
> +            return NULL;
> +        }
>           qdev->id = id;
>       }
>       if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {

Looks good..

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

Re: [Qemu-devel] [PATCH v3 04/17] qdev: Give qtree names precedence over user-assigned IDs

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>> pci_add nic -net use either the ID or option "name" of qemu_net_opts.
>> And there's our hole.  Reproducible with "-net user -net nic,id=foo
>> -device lsi,id=foo".
>
> Oh.  Well.  Yes, better plug that.
>
>> @@ -242,6 +243,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>       qdev = qdev_create_from_info(bus, info);
>>       id = qemu_opts_id(opts);
>>       if (id) {
>> +        if (qdev_find_recursive(main_system_bus, id)) {
>> +            qerror_report(QERR_DUPLICATE_ID, id, "device");
>> +            return NULL;
>> +        }
>>           qdev->id = id;
>>       }
>>       if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>
> Looks good..
>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

I'll repost it as a well-formed patch.  While there, I'll outlaw "/".

What about requiring IDs to start with a letter?  Just in case we ever
want to add alias names that must not clash with user-specified IDs.

[Qemu-devel] [PATCH] qdev: Reject duplicate and anti-social device IDs

[Markus Armbruster]

We need Device IDs to be unique and not contain '/' so device tree
nodes can always be unambigously referenced by tree path.

We already have some protection against duplicate IDs, but it got
holes:

* We don't assign IDs to default devices.

* -device and device_add use the ID of a qemu_device_opts.  Which
  rejects duplicate IDs.

* pci_add nic -net use either the ID or option "name" of
  qemu_net_opts.  And there's our hole.  Reproducible with "-net user
  -net nic,id=foo -device lsi,id=foo".

Also require IDs to start with a letter to provide for possible future
extensions.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
Note: If requiring IDs to start with a letter should turn out to be
controversial, I'm happy to respin without that part.

 hw/qdev.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index af17486..877cdf4 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -40,6 +40,7 @@ DeviceInfo *device_info_list;
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
 static BusState *qbus_find(const char *path);
+static DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
@@ -242,6 +243,16 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     qdev = qdev_create_from_info(bus, info);
     id = qemu_opts_id(opts);
     if (id) {
+        if (!qemu_isalpha(id[0]) || strchr(id, '/')) {
+            qerror_report(QERR_INVALID_PARAMETER_VALUE, "id",
+                          "an identifier starting with a letter, "
+                          "and not containing '/'");
+            return NULL;
+        }
+        if (qdev_find_recursive(main_system_bus, id)) {
+            qerror_report(QERR_DUPLICATE_ID, id, "device");
+            return NULL;
+        }
         qdev->id = id;
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Gerd Hoffmann]

On 05/31/10 16:13, Markus Armbruster wrote:
> We need Device IDs to be unique and not contain '/' so device tree
> nodes can always be unambigously referenced by tree path.
>
> We already have some protection against duplicate IDs, but it got
> holes:
>
> * We don't assign IDs to default devices.
>
> * -device and device_add use the ID of a qemu_device_opts.  Which
>    rejects duplicate IDs.
>
> * pci_add nic -net use either the ID or option "name" of
>    qemu_net_opts.  And there's our hole.  Reproducible with "-net user
>    -net nic,id=foo -device lsi,id=foo".
>
> Also require IDs to start with a letter to provide for possible future
> extensions.
>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Luiz Capitulino]

On Mon, 31 May 2010 16:13:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> We need Device IDs to be unique and not contain '/' so device tree
> nodes can always be unambigously referenced by tree path.
> 
> We already have some protection against duplicate IDs, but it got
> holes:
> 
> * We don't assign IDs to default devices.
> 
> * -device and device_add use the ID of a qemu_device_opts.  Which
>   rejects duplicate IDs.
> 
> * pci_add nic -net use either the ID or option "name" of
>   qemu_net_opts.  And there's our hole.  Reproducible with "-net user
>   -net nic,id=foo -device lsi,id=foo".

 Two bugs that might not be related to this thread:

  * "id" member is not mandatory for the device_add command:

    { "execute": "device_add", "arguments": { "driver": "e1000" } }
    {"return": {}}

  * "id" member remains in use when the netdev_add command fails:

    { "execute": "netdev_add", "arguments": { "id": "foobar" } }
    {"error": {"class": "MissingParameter", "desc": "Parameter 'type' is missing", "data": {"name": "type"}}}

    { "execute": "netdev_add", "arguments": { "type": "user", "id": "foobar" } }
    {"error": {"class": "DuplicateId", "desc": "Duplicate ID 'foobar' for netdev", "data": {"object": "netdev", "id": "foobar"}}}

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Jan Kiszka]

Luiz Capitulino wrote:
>  Two bugs that might not be related to this thread:
> 
>   * "id" member is not mandatory for the device_add command:
> 
>     { "execute": "device_add", "arguments": { "driver": "e1000" } }
>     {"return": {}}

Once we enable qtree paths for device_del, this is no longer an issue,
devices will remain addressable.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Luiz Capitulino]

On Tue, 01 Jun 2010 15:09:34 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Luiz Capitulino wrote:
> >  Two bugs that might not be related to this thread:
> > 
> >   * "id" member is not mandatory for the device_add command:
> > 
> >     { "execute": "device_add", "arguments": { "driver": "e1000" } }
> >     {"return": {}}
> 
> Once we enable qtree paths for device_del, this is no longer an issue,
> devices will remain addressable.

 Main point is whether "id" is required or not, I think it should be.

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Jan Kiszka]

Luiz Capitulino wrote:
> On Tue, 01 Jun 2010 15:09:34 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Luiz Capitulino wrote:
>>>  Two bugs that might not be related to this thread:
>>>
>>>   * "id" member is not mandatory for the device_add command:
>>>
>>>     { "execute": "device_add", "arguments": { "driver": "e1000" } }
>>>     {"return": {}}
>> Once we enable qtree paths for device_del, this is no longer an issue,
>> devices will remain addressable.
> 
>  Main point is whether "id" is required or not, I think it should be.

And I think it might be recommended but should become mandatory
(specifically not for HMP).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Avi Kivity]

On 06/01/2010 04:19 PM, Jan Kiszka wrote:
>
>>   Main point is whether "id" is required or not, I think it should be.
>>      
> And I think it might be recommended but should become mandatory
> (specifically not for HMP).
>    

I agree.  Making id a mandatory argument will just result in clients 
being forced to manufacture unnecessary IDs, which is annoying.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Luiz Capitulino]

On Tue, 01 Jun 2010 15:19:59 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Luiz Capitulino wrote:
> > On Tue, 01 Jun 2010 15:09:34 +0200
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> Luiz Capitulino wrote:
> >>>  Two bugs that might not be related to this thread:
> >>>
> >>>   * "id" member is not mandatory for the device_add command:
> >>>
> >>>     { "execute": "device_add", "arguments": { "driver": "e1000" } }
> >>>     {"return": {}}
> >> Once we enable qtree paths for device_del, this is no longer an issue,
> >> devices will remain addressable.
> > 
> >  Main point is whether "id" is required or not, I think it should be.
> 
> And I think it might be recommended but should become mandatory
> (specifically not for HMP).

 Maybe you meant 'should not'?

 Anyway, if we keep it optional for device_add we should also change
the others _add commands to have the same behavior.

Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 31 May 2010 16:13:12 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> We need Device IDs to be unique and not contain '/' so device tree
>> nodes can always be unambigously referenced by tree path.
>> 
>> We already have some protection against duplicate IDs, but it got
>> holes:
>> 
>> * We don't assign IDs to default devices.
>> 
>> * -device and device_add use the ID of a qemu_device_opts.  Which
>>   rejects duplicate IDs.
>> 
>> * pci_add nic -net use either the ID or option "name" of
>>   qemu_net_opts.  And there's our hole.  Reproducible with "-net user
>>   -net nic,id=foo -device lsi,id=foo".
>
>  Two bugs that might not be related to this thread:
>
>   * "id" member is not mandatory for the device_add command:
>
>     { "execute": "device_add", "arguments": { "driver": "e1000" } }
>     {"return": {}}

Works as designed.

>   * "id" member remains in use when the netdev_add command fails:
>
>     { "execute": "netdev_add", "arguments": { "id": "foobar" } }
>     {"error": {"class": "MissingParameter", "desc": "Parameter 'type' is missing", "data": {"name": "type"}}}
>
>     { "execute": "netdev_add", "arguments": { "type": "user", "id": "foobar" } }
>     {"error": {"class": "DuplicateId", "desc": "Duplicate ID 'foobar' for netdev", "data": {"object": "netdev", "id": "foobar"}}}

Sounds like a bug.

Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Luiz Capitulino]

On Tue, 01 Jun 2010 16:44:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Mon, 31 May 2010 16:13:12 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> We need Device IDs to be unique and not contain '/' so device tree
> >> nodes can always be unambigously referenced by tree path.
> >> 
> >> We already have some protection against duplicate IDs, but it got
> >> holes:
> >> 
> >> * We don't assign IDs to default devices.
> >> 
> >> * -device and device_add use the ID of a qemu_device_opts.  Which
> >>   rejects duplicate IDs.
> >> 
> >> * pci_add nic -net use either the ID or option "name" of
> >>   qemu_net_opts.  And there's our hole.  Reproducible with "-net user
> >>   -net nic,id=foo -device lsi,id=foo".
> >
> >  Two bugs that might not be related to this thread:
> >
> >   * "id" member is not mandatory for the device_add command:
> >
> >     { "execute": "device_add", "arguments": { "driver": "e1000" } }
> >     {"return": {}}
> 
> Works as designed.

 What about netdev_add?

 { "execute": "netdev_add", "arguments": { "type": "user" } }
 {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}}

Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 01 Jun 2010 16:44:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Mon, 31 May 2010 16:13:12 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> We need Device IDs to be unique and not contain '/' so device tree
>> >> nodes can always be unambigously referenced by tree path.
>> >> 
>> >> We already have some protection against duplicate IDs, but it got
>> >> holes:
>> >> 
>> >> * We don't assign IDs to default devices.
>> >> 
>> >> * -device and device_add use the ID of a qemu_device_opts.  Which
>> >>   rejects duplicate IDs.
>> >> 
>> >> * pci_add nic -net use either the ID or option "name" of
>> >>   qemu_net_opts.  And there's our hole.  Reproducible with "-net user
>> >>   -net nic,id=foo -device lsi,id=foo".
>> >
>> >  Two bugs that might not be related to this thread:
>> >
>> >   * "id" member is not mandatory for the device_add command:
>> >
>> >     { "execute": "device_add", "arguments": { "driver": "e1000" } }
>> >     {"return": {}}
>> 
>> Works as designed.
>
>  What about netdev_add?
>
>  { "execute": "netdev_add", "arguments": { "type": "user" } }
>  {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}}

The only way to put a netdev to use is connecting it to a NIC with
-device DRIVER,netdev=ID,...  And that requires an ID.

Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Anthony Liguori]

On 06/01/2010 01:35 PM, Markus Armbruster wrote:
> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>
>    
>> On Tue, 01 Jun 2010 16:44:24 +0200
>> Markus Armbruster<armbru@redhat.com>  wrote:
>>
>>      
>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>
>>>        
>>>> On Mon, 31 May 2010 16:13:12 +0200
>>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>>
>>>>          
>>>>> We need Device IDs to be unique and not contain '/' so device tree
>>>>> nodes can always be unambigously referenced by tree path.
>>>>>
>>>>> We already have some protection against duplicate IDs, but it got
>>>>> holes:
>>>>>
>>>>> * We don't assign IDs to default devices.
>>>>>
>>>>> * -device and device_add use the ID of a qemu_device_opts.  Which
>>>>>    rejects duplicate IDs.
>>>>>
>>>>> * pci_add nic -net use either the ID or option "name" of
>>>>>    qemu_net_opts.  And there's our hole.  Reproducible with "-net user
>>>>>    -net nic,id=foo -device lsi,id=foo".
>>>>>            
>>>>   Two bugs that might not be related to this thread:
>>>>
>>>>    * "id" member is not mandatory for the device_add command:
>>>>
>>>>      { "execute": "device_add", "arguments": { "driver": "e1000" } }
>>>>      {"return": {}}
>>>>          
>>> Works as designed.
>>>        
>>   What about netdev_add?
>>
>>   { "execute": "netdev_add", "arguments": { "type": "user" } }
>>   {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is missing", "data": {"name": "id"}}}
>>      
> The only way to put a netdev to use is connecting it to a NIC with
> -device DRIVER,netdev=ID,...  And that requires an ID.
>    

To be honest, I think we should universally require an id parameter or 
we should auto-assign one and return it during creation.

Implicit/Null ids with QemuOpts are really difficult to work with and it 
certainly makes device addressing a lot easier.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] qdev: Reject duplicate and anti-social device IDs

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

Anthony Liguori wrote:
> On 06/01/2010 01:35 PM, Markus Armbruster wrote:
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>   
>>> On Tue, 01 Jun 2010 16:44:24 +0200
>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>
>>>     
>>>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>>>
>>>>       
>>>>> On Mon, 31 May 2010 16:13:12 +0200
>>>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>>>
>>>>>         
>>>>>> We need Device IDs to be unique and not contain '/' so device tree
>>>>>> nodes can always be unambigously referenced by tree path.
>>>>>>
>>>>>> We already have some protection against duplicate IDs, but it got
>>>>>> holes:
>>>>>>
>>>>>> * We don't assign IDs to default devices.
>>>>>>
>>>>>> * -device and device_add use the ID of a qemu_device_opts.  Which
>>>>>>    rejects duplicate IDs.
>>>>>>
>>>>>> * pci_add nic -net use either the ID or option "name" of
>>>>>>    qemu_net_opts.  And there's our hole.  Reproducible with "-net
>>>>>> user
>>>>>>    -net nic,id=foo -device lsi,id=foo".
>>>>>>            
>>>>>   Two bugs that might not be related to this thread:
>>>>>
>>>>>    * "id" member is not mandatory for the device_add command:
>>>>>
>>>>>      { "execute": "device_add", "arguments": { "driver": "e1000" } }
>>>>>      {"return": {}}
>>>>>          
>>>> Works as designed.
>>>>        
>>>   What about netdev_add?
>>>
>>>   { "execute": "netdev_add", "arguments": { "type": "user" } }
>>>   {"error": {"class": "MissingParameter", "desc": "Parameter 'id' is
>>> missing", "data": {"name": "id"}}}
>>>      
>> The only way to put a netdev to use is connecting it to a NIC with
>> -device DRIVER,netdev=ID,...  And that requires an ID.
>>    
> 
> To be honest, I think we should universally require an id parameter or
> we should auto-assign one and return it during creation.
> 
> Implicit/Null ids with QemuOpts are really difficult to work with and it
> certainly makes device addressing a lot easier.

I really see no need for this additional burden. We always have unique
qtree paths for devices.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] qdev: Reject duplicate and anti-social device IDs

[Paul Brook]

> Also require IDs to start with a letter to provide for possible future
> extensions.

I'd go further than that, and require that user specified IDs match
[A-Za-z][A-Za-z0-9_-]*

Paul

Re: [Qemu-devel] [PATCH] qdev: Reject duplicate and anti-social device IDs

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

>> Also require IDs to start with a letter to provide for possible future
>> extensions.
>
> I'd go further than that, and require that user specified IDs match
> [A-Za-z][A-Za-z0-9_-]*

I talked with Dan (cc'ed) to make sure we don't trample on existing
libvirt usage without need.  What about

[A-Za-z][A-Za-z0-9_-:.]*

i.e. your regexp plus ':' and '.' in the second bracket?

Re: [Qemu-devel] [PATCH] qdev: Reject duplicate and anti-social device IDs

[Paul Brook]

> Paul Brook <paul@codesourcery.com> writes:
> >> Also require IDs to start with a letter to provide for possible future
> >> extensions.
> > 
> > I'd go further than that, and require that user specified IDs match
> > [A-Za-z][A-Za-z0-9_-]*
> 
> I talked with Dan (cc'ed) to make sure we don't trample on existing
> libvirt usage without need.  What about
> 
> [A-Za-z][A-Za-z0-9_-:.]*
> 
> i.e. your regexp plus ':' and '.' in the second bracket?

I was deliberately avoiding those as they're often used as separators - we 
already use ':' in other contexts so there's potential ambiguity and 
parsing/quoting issues. I'm not aware of any current issues with '.'.

Paul

P.S. Your regexp doesn't do what you think it does, but I know what you mean.

Re: [Qemu-devel] [PATCH] qdev: Reject duplicate and anti-social device IDs

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

>> Paul Brook <paul@codesourcery.com> writes:
>> >> Also require IDs to start with a letter to provide for possible future
>> >> extensions.
>> > 
>> > I'd go further than that, and require that user specified IDs match
>> > [A-Za-z][A-Za-z0-9_-]*
>> 
>> I talked with Dan (cc'ed) to make sure we don't trample on existing
>> libvirt usage without need.  What about
>> 
>> [A-Za-z][A-Za-z0-9_-:.]*
>> 
>> i.e. your regexp plus ':' and '.' in the second bracket?
>
> I was deliberately avoiding those as they're often used as separators - we 
> already use ':' in other contexts so there's potential ambiguity and 
> parsing/quoting issues. I'm not aware of any current issues with '.'.

I checked with Dan, and we can outlaw ':'.  I just posted a new patch
that covers all qemu-option IDs, not just qdev.

[...]

[Qemu-devel] [PATCH v3 05/17] qdev: Convert device and bus lists to QTAILQ

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Cosmetic change to align the instance number assignment with bus
ordering. The current ordering due to QLIST_INSERT_HEAD is a bit
annoying when you dump the qtree or address devices via
'driver.instance'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/acpi_piix4.c  |    2 +-
 hw/i2c.c         |    2 +-
 hw/pci-hotplug.c |    2 +-
 hw/qdev.c        |   43 ++++++++++++++++++++++---------------------
 hw/qdev.h        |    8 ++++----
 hw/ssi.c         |    6 +++---
 6 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0fce958..3cb3d11 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -536,7 +536,7 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
     PCIDevice *dev;
     int slot = ffs(val) - 1;
 
-    QLIST_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
+    QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
         dev = DO_UPCAST(PCIDevice, qdev, qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
             qdev_free(qdev);
diff --git a/hw/i2c.c b/hw/i2c.c
index bee8e88..61ab6fa 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -84,7 +84,7 @@ int i2c_start_transfer(i2c_bus *bus, uint8_t address, int recv)
     DeviceState *qdev;
     i2c_slave *slave = NULL;
 
-    QLIST_FOREACH(qdev, &bus->qbus.children, sibling) {
+    QTAILQ_FOREACH(qdev, &bus->qbus.children, sibling) {
         i2c_slave *candidate = I2C_SLAVE_FROM_QDEV(qdev);
         if (candidate->address == address) {
             slave = candidate;
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index cc45c50..a226d3c 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -77,7 +77,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
     SCSIBus *scsibus;
     SCSIDevice *scsidev;
 
-    scsibus = DO_UPCAST(SCSIBus, qbus, QLIST_FIRST(&adapter->child_bus));
+    scsibus = DO_UPCAST(SCSIBus, qbus, QTAILQ_FIRST(&adapter->child_bus));
     if (!scsibus || strcmp(scsibus->qbus.info->name, "SCSI") != 0) {
         error_report("Device is not a SCSI adapter");
         return -1;
diff --git a/hw/qdev.c b/hw/qdev.c
index eeadf4a..b3d375a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -85,10 +85,11 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     dev = qemu_mallocz(info->size);
     dev->info = info;
     dev->parent_bus = bus;
+    QTAILQ_INIT(&dev->child_bus);
     qdev_prop_set_defaults(dev, dev->info->props);
     qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
     qdev_prop_set_globals(dev);
-    QLIST_INSERT_HEAD(&bus->children, dev, sibling);
+    QTAILQ_INSERT_TAIL(&bus->children, dev, sibling);
     if (qdev_hotplug) {
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
@@ -337,7 +338,7 @@ void qdev_free(DeviceState *dev)
 
     if (dev->state == DEV_STATE_INITIALIZED) {
         while (dev->num_child_bus) {
-            bus = QLIST_FIRST(&dev->child_bus);
+            bus = QTAILQ_FIRST(&dev->child_bus);
             qbus_free(bus);
         }
         if (dev->info->vmsd)
@@ -348,7 +349,7 @@ void qdev_free(DeviceState *dev)
             qemu_opts_del(dev->opts);
     }
     qemu_unregister_reset(qdev_reset, dev);
-    QLIST_REMOVE(dev, sibling);
+    QTAILQ_REMOVE(&dev->parent_bus->children, dev, sibling);
     qemu_free(dev);
 }
 
@@ -432,7 +433,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
 {
     BusState *bus;
 
-    QLIST_FOREACH(bus, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(bus, &dev->child_bus, sibling) {
         if (strcmp(name, bus->name) == 0) {
             return bus;
         }
@@ -457,8 +458,8 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
         return bus;
     }
 
-    QLIST_FOREACH(dev, &bus->children, sibling) {
-        QLIST_FOREACH(child, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
+        QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
             ret = qbus_find_recursive(child, name, info);
             if (ret) {
                 return ret;
@@ -473,10 +474,10 @@ static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     DeviceState *dev, *ret;
     BusState *child;
 
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         if (dev->id && strcmp(dev->id, id) == 0)
             return dev;
-        QLIST_FOREACH(child, &dev->child_bus, sibling) {
+        QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
             ret = qdev_find_recursive(child, id);
             if (ret) {
                 return ret;
@@ -493,7 +494,7 @@ static void qbus_list_bus(DeviceState *dev)
 
     error_printf("child busses at \"%s\":",
                  dev->id ? dev->id : dev->info->name);
-    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
         sep = ", ";
     }
@@ -506,7 +507,7 @@ static void qbus_list_dev(BusState *bus)
     const char *sep = " ";
 
     error_printf("devices at \"%s\":", bus->name);
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         error_printf("%s\"%s\"", sep, dev->info->name);
         if (dev->id)
             error_printf("/\"%s\"", dev->id);
@@ -519,7 +520,7 @@ static BusState *qbus_find_bus(DeviceState *dev, char *elem)
 {
     BusState *child;
 
-    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
         if (strcmp(child->name, elem) == 0) {
             return child;
         }
@@ -546,21 +547,21 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
     }
 
     n = 0;
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         if (strcmp(dev->info->name, elem) == 0 && n++ == instance) {
             return dev;
         }
     }
 
     n = 0;
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         if (dev->info->alias && strcmp(dev->info->alias, elem) == 0 &&
             n++ == instance) {
             return dev;
         }
     }
 
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         if (dev->id && strcmp(dev->id, elem) == 0) {
             return dev;
         }
@@ -629,7 +630,7 @@ search_dev_bus:
                 qerror_report(QERR_DEVICE_NO_BUS, elem);
                 return NULL;
             case 1:
-                return QLIST_FIRST(&dev->child_bus);
+                return QTAILQ_FIRST(&dev->child_bus);
             default:
                 qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
                 if (!monitor_cur_is_qmp()) {
@@ -694,9 +695,9 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
         bus->name = buf;
     }
 
-    QLIST_INIT(&bus->children);
+    QTAILQ_INIT(&bus->children);
     if (parent) {
-        QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
+        QTAILQ_INSERT_TAIL(&parent->child_bus, bus, sibling);
         parent->num_child_bus++;
     }
 
@@ -716,11 +717,11 @@ void qbus_free(BusState *bus)
 {
     DeviceState *dev;
 
-    while ((dev = QLIST_FIRST(&bus->children)) != NULL) {
+    while ((dev = QTAILQ_FIRST(&bus->children)) != NULL) {
         qdev_free(dev);
     }
     if (bus->parent) {
-        QLIST_REMOVE(bus, sibling);
+        QTAILQ_REMOVE(&bus->parent->child_bus, bus, sibling);
         bus->parent->num_child_bus--;
     }
     if (bus->qdev_allocated) {
@@ -769,7 +770,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
     qdev_print_props(mon, dev, dev->parent_bus->info->props, "bus", indent);
     if (dev->parent_bus->info->print_dev)
         dev->parent_bus->info->print_dev(mon, dev, indent);
-    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
         qbus_print(mon, child, indent);
     }
 }
@@ -781,7 +782,7 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent)
     qdev_printf("bus: %s\n", bus->name);
     indent += 2;
     qdev_printf("type %s\n", bus->info->name);
-    QLIST_FOREACH(dev, &bus->children, sibling) {
+    QTAILQ_FOREACH(dev, &bus->children, sibling) {
         qdev_print(mon, dev, indent);
     }
 }
diff --git a/hw/qdev.h b/hw/qdev.h
index a44060e..53f5565 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -41,9 +41,9 @@ struct DeviceState {
     qemu_irq *gpio_out;
     int num_gpio_in;
     qemu_irq *gpio_in;
-    QLIST_HEAD(, BusState) child_bus;
+    QTAILQ_HEAD(, BusState) child_bus;
     int num_child_bus;
-    QLIST_ENTRY(DeviceState) sibling;
+    QTAILQ_ENTRY(DeviceState) sibling;
     int instance_id_alias;
     int alias_required_for_version;
 };
@@ -62,8 +62,8 @@ struct BusState {
     const char *name;
     int allow_hotplug;
     int qdev_allocated;
-    QLIST_HEAD(, DeviceState) children;
-    QLIST_ENTRY(BusState) sibling;
+    QTAILQ_HEAD(, DeviceState) children;
+    QTAILQ_ENTRY(BusState) sibling;
 };
 
 struct Property {
diff --git a/hw/ssi.c b/hw/ssi.c
index cfe7c07..2c18436 100644
--- a/hw/ssi.c
+++ b/hw/ssi.c
@@ -25,8 +25,8 @@ static int ssi_slave_init(DeviceState *dev, DeviceInfo *base_info)
     SSIBus *bus;
 
     bus = FROM_QBUS(SSIBus, qdev_get_parent_bus(dev));
-    if (QLIST_FIRST(&bus->qbus.children) != dev
-        || QLIST_NEXT(dev, sibling) != NULL) {
+    if (QTAILQ_FIRST(&bus->qbus.children) != dev
+        || QTAILQ_NEXT(dev, sibling) != NULL) {
         hw_error("Too many devices on SSI bus");
     }
 
@@ -61,7 +61,7 @@ uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
 {
     DeviceState *dev;
     SSISlave *slave;
-    dev = QLIST_FIRST(&bus->qbus.children);
+    dev = QTAILQ_FIRST(&bus->qbus.children);
     if (!dev) {
         return 0;
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Allow to specify the device to be removed via device_del not only by ID
but also by its full or abbreviated qtree path. For this purpose,
qdev_find is introduced which combines walking the qtree with searching
for device IDs if required.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c       |   49 +++++++++++++++++++++++++++++++++++++++++++------
 qemu-monitor.hx |   10 +++++-----
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index b3d375a..df945ed 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -469,7 +469,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
 {
     DeviceState *dev, *ret;
     BusState *child;
@@ -478,7 +478,7 @@ static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
         if (dev->id && strcmp(dev->id, id) == 0)
             return dev;
         QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_recursive(child, id);
+            ret = qdev_find_id_recursive(child, id);
             if (ret) {
                 return ret;
             }
@@ -666,6 +666,43 @@ search_dev_bus:
     }
 }
 
+static DeviceState *qdev_find(const char *path)
+{
+    const char *dev_name;
+    DeviceState *dev;
+    char *bus_path;
+    BusState *bus;
+
+    dev_name = strrchr(path, '/');
+    if (!dev_name) {
+        bus = main_system_bus;
+        dev_name = path;
+    } else {
+        dev_name++;
+        bus_path = qemu_strdup(path);
+        bus_path[dev_name - path] = 0;
+
+        bus = qbus_find(bus_path);
+        qemu_free(bus_path);
+
+        if (!bus) {
+            /* qbus_find already reported the error */
+            return NULL;
+        }
+    }
+    dev = qbus_find_dev(bus, dev_name);
+    if (!dev) {
+        dev = qdev_find_id_recursive(main_system_bus, path);
+        if (!dev) {
+            qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
+            if (!monitor_cur_is_qmp()) {
+                qbus_list_dev(bus);
+            }
+        }
+    }
+    return dev;
+}
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name)
 {
@@ -824,12 +861,12 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    const char *id = qdict_get_str(qdict, "id");
+    const char *path = qdict_get_str(qdict, "path");
     DeviceState *dev;
 
-    dev = qdev_find_recursive(main_system_bus, id);
-    if (NULL == dev) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+    dev = qdev_find(path);
+    if (!dev) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, path);
         return -1;
     }
     return qdev_unplug(dev);
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index c8f1789..754d71e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -703,7 +703,7 @@ EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "id:s",
+        .args_type  = "path:s",
         .params     = "device",
         .help       = "remove device",
         .user_print = monitor_user_noop,
@@ -711,10 +711,10 @@ EQMP
     },
 
 STEXI
-@item device_del @var{id}
+@item device_del @var{path}
 @findex device_del
 
-Remove device @var{id}.
+Remove device @var{path}.
 ETEXI
 SQMP
 device_del
@@ -724,11 +724,11 @@ Remove a device.
 
 Arguments:
 
-- "id": the device's ID (json-string)
+- "path": the device's qtree path or unique ID (json-string)
 
 Example:
 
--> { "execute": "device_del", "arguments": { "id": "net1" } }
+-> { "execute": "device_del", "arguments": { "path": "net1" } }
 <- { "return": {} }
 
 EQMP
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Luiz Capitulino]

On Sun, 23 May 2010 12:59:19 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Allow to specify the device to be removed via device_del not only by ID
> but also by its full or abbreviated qtree path. For this purpose,
> qdev_find is introduced which combines walking the qtree with searching
> for device IDs if required.

 [...]

>  Arguments:
>  
> -- "id": the device's ID (json-string)
> +- "path": the device's qtree path or unique ID (json-string)
>  
>  Example:
>  
> --> { "execute": "device_del", "arguments": { "id": "net1" } }
> +-> { "execute": "device_del", "arguments": { "path": "net1" } }

 Doesn't seem like a good change to me, besides being incompatible[1] we
shouldn't overload arguments this way in QMP as overloading leads to
interface degradation (harder to use, understand, maintain).

 Maybe we could have both arguments as optional, but one must be passed.

[1] It's 'legal' to break the protocol before 0.13, but this has to be
    coordinated with libvirt so we should have a good reason to do this

>  <- { "return": {} }
>  
>  EQMP

[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

Luiz Capitulino wrote:
> On Sun, 23 May 2010 12:59:19 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Allow to specify the device to be removed via device_del not only by ID
>> but also by its full or abbreviated qtree path. For this purpose,
>> qdev_find is introduced which combines walking the qtree with searching
>> for device IDs if required.
> 
>  [...]
> 
>>  Arguments:
>>  
>> -- "id": the device's ID (json-string)
>> +- "path": the device's qtree path or unique ID (json-string)
>>  
>>  Example:
>>  
>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
> 
>  Doesn't seem like a good change to me, besides being incompatible[1] we
> shouldn't overload arguments this way in QMP as overloading leads to
> interface degradation (harder to use, understand, maintain).

It's not overloaded, think of an ID as a (weak) symbolic link in the
qtree filesystem. The advantage of basing everything on top of full or
abbreviated qtree paths is that IDs are not always assigned, paths are.

> 
>  Maybe we could have both arguments as optional, but one must be passed.

This would at least require some way to keep the proposed unified path
specification for the human monitor (having separate arguments there is
really unhandy).

> 
> [1] It's 'legal' to break the protocol before 0.13, but this has to be
>     coordinated with libvirt so we should have a good reason to do this
> 
>>  <- { "return": {} }
>>  
>>  EQMP
> 

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Luiz Capitulino]

On Fri, 28 May 2010 00:19:48 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Sun, 23 May 2010 12:59:19 +0200
> > Jan Kiszka <jan.kiszka@web.de> wrote:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Allow to specify the device to be removed via device_del not only by ID
> >> but also by its full or abbreviated qtree path. For this purpose,
> >> qdev_find is introduced which combines walking the qtree with searching
> >> for device IDs if required.
> > 
> >  [...]
> > 
> >>  Arguments:
> >>  
> >> -- "id": the device's ID (json-string)
> >> +- "path": the device's qtree path or unique ID (json-string)
> >>  
> >>  Example:
> >>  
> >> --> { "execute": "device_del", "arguments": { "id": "net1" } }
> >> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
> > 
> >  Doesn't seem like a good change to me, besides being incompatible[1] we
> > shouldn't overload arguments this way in QMP as overloading leads to
> > interface degradation (harder to use, understand, maintain).
> 
> It's not overloaded, think of an ID as a (weak) symbolic link in the
> qtree filesystem. The advantage of basing everything on top of full or
> abbreviated qtree paths is that IDs are not always assigned, paths are.

 You mean there're cases where an ID is not assigned? I didn't know that,
I thought they were always assigned, at least via QMP.

 In any case, my main question here is that if this change really makes
sense for QMP or if it's something for HMP where we can have features
like tab completion.

> >  Maybe we could have both arguments as optional, but one must be passed.
> 
> This would at least require some way to keep the proposed unified path
> specification for the human monitor (having separate arguments there is
> really unhandy).

 Agreed, perhaps we have to decouple QMP and HMP in the way proposed by
Anthony: the HMP should work by making QMP calls (IIUC).

 This way HMP can accept whatever arguments make sense for it, but then
it should transform them in a QMP call.

 This is a long term project though.

[Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Jan Kiszka]

Luiz Capitulino wrote:
> On Fri, 28 May 2010 00:19:48 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> Luiz Capitulino wrote:
>>> On Sun, 23 May 2010 12:59:19 +0200
>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Allow to specify the device to be removed via device_del not only by ID
>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>> qdev_find is introduced which combines walking the qtree with searching
>>>> for device IDs if required.
>>>  [...]
>>>
>>>>  Arguments:
>>>>  
>>>> -- "id": the device's ID (json-string)
>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>  
>>>>  Example:
>>>>  
>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>> shouldn't overload arguments this way in QMP as overloading leads to
>>> interface degradation (harder to use, understand, maintain).
>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>> qtree filesystem. The advantage of basing everything on top of full or
>> abbreviated qtree paths is that IDs are not always assigned, paths are.
> 
>  You mean there're cases where an ID is not assigned? I didn't know that,
> I thought they were always assigned, at least via QMP.

Yes, platform devices or anything else that is auto-instantiated does
not have an ID. Granted, you normally don't want or even can remove such
devices, but you can perfectly display them. And someone asked for
unifying the paths accepted by device_show and device_del.

> 
>  In any case, my main question here is that if this change really makes
> sense for QMP or if it's something for HMP where we can have features
> like tab completion.

I don't think QMP would suffer from having separate arguments for ID and
qtree paths. We just need a way to map them on the same for HMP, ie. we
need argument types that only show up in one of both domains.

> 
>>>  Maybe we could have both arguments as optional, but one must be passed.
>> This would at least require some way to keep the proposed unified path
>> specification for the human monitor (having separate arguments there is
>> really unhandy).
> 
>  Agreed, perhaps we have to decouple QMP and HMP in the way proposed by
> Anthony: the HMP should work by making QMP calls (IIUC).
> 
>  This way HMP can accept whatever arguments make sense for it, but then
> it should transform them in a QMP call.
> 
>  This is a long term project though.

But the changes I propose already take effect today. We need at least an
interim solution.

We could postpone the device_del HMP format change e.g., but that would
take away some usability and leave criticized inconsistency behind.

Hmm, or we could simply introduce optional .hmp_args_type and
.hmp_params. HMP would favor them when provided by a command, otherwise
fall back to the standard params. Doesn't sound awfully complex and
invasive.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> Luiz Capitulino wrote:
>> On Sun, 23 May 2010 12:59:19 +0200
>> Jan Kiszka <jan.kiszka@web.de> wrote:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Allow to specify the device to be removed via device_del not only by ID
>>> but also by its full or abbreviated qtree path. For this purpose,
>>> qdev_find is introduced which combines walking the qtree with searching
>>> for device IDs if required.
>> 
>>  [...]
>> 
>>>  Arguments:
>>>  
>>> -- "id": the device's ID (json-string)
>>> +- "path": the device's qtree path or unique ID (json-string)
>>>  
>>>  Example:
>>>  
>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>> 
>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>> shouldn't overload arguments this way in QMP as overloading leads to
>> interface degradation (harder to use, understand, maintain).
>
> It's not overloaded, think of an ID as a (weak) symbolic link in the
> qtree filesystem. The advantage of basing everything on top of full or
> abbreviated qtree paths is that IDs are not always assigned, paths are.

As long as your patch doesn't change the interpretation of IDs, we can
keep the old name.

The recent review of QMP documentation may lead to a "clean up bad
names" flag day.  One more wouldn't make it worse, I guess.

>>  Maybe we could have both arguments as optional, but one must be passed.
>
> This would at least require some way to keep the proposed unified path
> specification for the human monitor (having separate arguments there is
> really unhandy).

Correct.

It would be nice to have device_del support paths in addition to IDs.
I'd expect management tools to slap IDs on everything, so they won't
care, but human users do.

As far as I know, we have two places where we let the user name a node
in the qtree: device_add bus=X and device_del X.  The former names a
bus, the latter a device.  But both are nodes in the same tree, so
consistency is in order.

Only devices have user-specified IDs.  Buses have names assigned by the
system.  Unique names, hopefully.

If the user doesn't specify a device ID, the driver name is used
instead.  If you put multiple instances of the same device on the same
bus, they have the *same* path.  For instance, here's a snippet of info
qtree after adding two usb-mouse:

      dev: piix3-usb-uhci, id ""
        bus-prop: addr = 01.2
        bus-prop: romfile = <null>
        bus-prop: rombar = 1
        class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
        bar 4: i/o at 0xffffffffffffffff [0x1e]
        bus: usb.0
          type USB
          dev: usb-hub, id ""
            addr 0.0, speed 12, name QEMU USB Hub, attached
          dev: usb-mouse, id "no2"
            addr 0.0, speed 12, name QEMU USB Mouse, attached
          dev: usb-mouse, id ""
            addr 0.0, speed 12, name QEMU USB Mouse, attached

Both devices have the same full path
/i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
Which one does your code pick?  Shouldn't it refuse to pick?

By the way, you *can* put '/' in IDs.  I call that a bug.

>> [1] It's 'legal' to break the protocol before 0.13, but this has to be
>>     coordinated with libvirt so we should have a good reason to do this
>> 
>>>  <- { "return": {} }
>>>  
>>>  EQMP
>> 
>
> Jan

Re: [Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Jan Kiszka]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> Luiz Capitulino wrote:
>>> On Sun, 23 May 2010 12:59:19 +0200
>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Allow to specify the device to be removed via device_del not only by ID
>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>> qdev_find is introduced which combines walking the qtree with searching
>>>> for device IDs if required.
>>>  [...]
>>>
>>>>  Arguments:
>>>>  
>>>> -- "id": the device's ID (json-string)
>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>  
>>>>  Example:
>>>>  
>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>> shouldn't overload arguments this way in QMP as overloading leads to
>>> interface degradation (harder to use, understand, maintain).
>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>> qtree filesystem. The advantage of basing everything on top of full or
>> abbreviated qtree paths is that IDs are not always assigned, paths are.
> 
> As long as your patch doesn't change the interpretation of IDs, we can
> keep the old name.
> 
> The recent review of QMP documentation may lead to a "clean up bad
> names" flag day.  One more wouldn't make it worse, I guess.
> 
>>>  Maybe we could have both arguments as optional, but one must be passed.
>> This would at least require some way to keep the proposed unified path
>> specification for the human monitor (having separate arguments there is
>> really unhandy).
> 
> Correct.
> 
> It would be nice to have device_del support paths in addition to IDs.
> I'd expect management tools to slap IDs on everything, so they won't
> care, but human users do.
> 
> As far as I know, we have two places where we let the user name a node
> in the qtree: device_add bus=X and device_del X.  The former names a
> bus, the latter a device.  But both are nodes in the same tree, so
> consistency is in order.
> 
> Only devices have user-specified IDs.  Buses have names assigned by the
> system.  Unique names, hopefully.

...but not necessarily. The bus name device_add accepts can also be a
full, thus unambiguous path.

> 
> If the user doesn't specify a device ID, the driver name is used
> instead.  If you put multiple instances of the same device on the same
> bus, they have the *same* path.  For instance, here's a snippet of info
> qtree after adding two usb-mouse:
> 
>       dev: piix3-usb-uhci, id ""
>         bus-prop: addr = 01.2
>         bus-prop: romfile = <null>
>         bus-prop: rombar = 1
>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>         bus: usb.0
>           type USB
>           dev: usb-hub, id ""
>             addr 0.0, speed 12, name QEMU USB Hub, attached
>           dev: usb-mouse, id "no2"
>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>           dev: usb-mouse, id ""
>             addr 0.0, speed 12, name QEMU USB Mouse, attached
> 
> Both devices have the same full path
> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
> Which one does your code pick?  Shouldn't it refuse to pick?

Patch 3 of this series resolves this as follows:

usb-mouse[.0] -> first listed instance
usb-mouse.1   -> second instance
...

We should probably include this numbering in the qtree dump, I guess.

> 
> By the way, you *can* put '/' in IDs.  I call that a bug.

Even if we prevent this, IDs can still collide with abbreviated device
or bus paths. Therefore I give paths precedence over IDs in patch 4.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Markus Armbruster]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> Luiz Capitulino wrote:
>>>> On Sun, 23 May 2010 12:59:19 +0200
>>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Allow to specify the device to be removed via device_del not only by ID
>>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>>> qdev_find is introduced which combines walking the qtree with searching
>>>>> for device IDs if required.
>>>>  [...]
>>>>
>>>>>  Arguments:
>>>>>  
>>>>> -- "id": the device's ID (json-string)
>>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>>  
>>>>>  Example:
>>>>>  
>>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>>> shouldn't overload arguments this way in QMP as overloading leads to
>>>> interface degradation (harder to use, understand, maintain).
>>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>>> qtree filesystem. The advantage of basing everything on top of full or
>>> abbreviated qtree paths is that IDs are not always assigned, paths are.
>> 
>> As long as your patch doesn't change the interpretation of IDs, we can
>> keep the old name.
>> 
>> The recent review of QMP documentation may lead to a "clean up bad
>> names" flag day.  One more wouldn't make it worse, I guess.
>> 
>>>>  Maybe we could have both arguments as optional, but one must be passed.
>>> This would at least require some way to keep the proposed unified path
>>> specification for the human monitor (having separate arguments there is
>>> really unhandy).
>> 
>> Correct.
>> 
>> It would be nice to have device_del support paths in addition to IDs.
>> I'd expect management tools to slap IDs on everything, so they won't
>> care, but human users do.
>> 
>> As far as I know, we have two places where we let the user name a node
>> in the qtree: device_add bus=X and device_del X.  The former names a
>> bus, the latter a device.  But both are nodes in the same tree, so
>> consistency is in order.
>> 
>> Only devices have user-specified IDs.  Buses have names assigned by the
>> system.  Unique names, hopefully.
>
> ...but not necessarily. The bus name device_add accepts can also be a
> full, thus unambiguous path.
>
>> 
>> If the user doesn't specify a device ID, the driver name is used
>> instead.  If you put multiple instances of the same device on the same
>> bus, they have the *same* path.  For instance, here's a snippet of info
>> qtree after adding two usb-mouse:
>> 
>>       dev: piix3-usb-uhci, id ""
>>         bus-prop: addr = 01.2
>>         bus-prop: romfile = <null>
>>         bus-prop: rombar = 1
>>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>>         bus: usb.0
>>           type USB
>>           dev: usb-hub, id ""
>>             addr 0.0, speed 12, name QEMU USB Hub, attached
>>           dev: usb-mouse, id "no2"
>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>           dev: usb-mouse, id ""
>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>> 
>> Both devices have the same full path
>> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
>> Which one does your code pick?  Shouldn't it refuse to pick?
>
> Patch 3 of this series resolves this as follows:
>
> usb-mouse[.0] -> first listed instance
> usb-mouse.1   -> second instance
> ...
>
> We should probably include this numbering in the qtree dump, I guess.
>
>> 
>> By the way, you *can* put '/' in IDs.  I call that a bug.
>
> Even if we prevent this, IDs can still collide with abbreviated device
> or bus paths. Therefore I give paths precedence over IDs in patch 4.

You're fixing problems in the overly complex and subtle path name lookup
by making it more complex and subtle.  Let's make it simple and
straightforward instead.

Re: [Qemu-devel] Re: [PATCH v3 06/17] qdev: Allow device specification by qtree path for device_del

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> Luiz Capitulino wrote:
>>>>> On Sun, 23 May 2010 12:59:19 +0200
>>>>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>>
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> Allow to specify the device to be removed via device_del not only by ID
>>>>>> but also by its full or abbreviated qtree path. For this purpose,
>>>>>> qdev_find is introduced which combines walking the qtree with searching
>>>>>> for device IDs if required.
>>>>>  [...]
>>>>>
>>>>>>  Arguments:
>>>>>>  
>>>>>> -- "id": the device's ID (json-string)
>>>>>> +- "path": the device's qtree path or unique ID (json-string)
>>>>>>  
>>>>>>  Example:
>>>>>>  
>>>>>> --> { "execute": "device_del", "arguments": { "id": "net1" } }
>>>>>> +-> { "execute": "device_del", "arguments": { "path": "net1" } }
>>>>>  Doesn't seem like a good change to me, besides being incompatible[1] we
>>>>> shouldn't overload arguments this way in QMP as overloading leads to
>>>>> interface degradation (harder to use, understand, maintain).
>>>> It's not overloaded, think of an ID as a (weak) symbolic link in the
>>>> qtree filesystem. The advantage of basing everything on top of full or
>>>> abbreviated qtree paths is that IDs are not always assigned, paths are.
>>> As long as your patch doesn't change the interpretation of IDs, we can
>>> keep the old name.
>>>
>>> The recent review of QMP documentation may lead to a "clean up bad
>>> names" flag day.  One more wouldn't make it worse, I guess.
>>>
>>>>>  Maybe we could have both arguments as optional, but one must be passed.
>>>> This would at least require some way to keep the proposed unified path
>>>> specification for the human monitor (having separate arguments there is
>>>> really unhandy).
>>> Correct.
>>>
>>> It would be nice to have device_del support paths in addition to IDs.
>>> I'd expect management tools to slap IDs on everything, so they won't
>>> care, but human users do.
>>>
>>> As far as I know, we have two places where we let the user name a node
>>> in the qtree: device_add bus=X and device_del X.  The former names a
>>> bus, the latter a device.  But both are nodes in the same tree, so
>>> consistency is in order.
>>>
>>> Only devices have user-specified IDs.  Buses have names assigned by the
>>> system.  Unique names, hopefully.
>> ...but not necessarily. The bus name device_add accepts can also be a
>> full, thus unambiguous path.
>>
>>> If the user doesn't specify a device ID, the driver name is used
>>> instead.  If you put multiple instances of the same device on the same
>>> bus, they have the *same* path.  For instance, here's a snippet of info
>>> qtree after adding two usb-mouse:
>>>
>>>       dev: piix3-usb-uhci, id ""
>>>         bus-prop: addr = 01.2
>>>         bus-prop: romfile = <null>
>>>         bus-prop: rombar = 1
>>>         class USB controller, addr 00:01.2, pci id 8086:7020 (sub 1af4:1100)
>>>         bar 4: i/o at 0xffffffffffffffff [0x1e]
>>>         bus: usb.0
>>>           type USB
>>>           dev: usb-hub, id ""
>>>             addr 0.0, speed 12, name QEMU USB Hub, attached
>>>           dev: usb-mouse, id "no2"
>>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>>           dev: usb-mouse, id ""
>>>             addr 0.0, speed 12, name QEMU USB Mouse, attached
>>>
>>> Both devices have the same full path
>>> /i440FX-pcihost/pci.0/piix3-usb-uhci/usb.0/usb-mouse
>>> Which one does your code pick?  Shouldn't it refuse to pick?
>> Patch 3 of this series resolves this as follows:
>>
>> usb-mouse[.0] -> first listed instance
>> usb-mouse.1   -> second instance
>> ...
>>
>> We should probably include this numbering in the qtree dump, I guess.
>>
>>> By the way, you *can* put '/' in IDs.  I call that a bug.
>> Even if we prevent this, IDs can still collide with abbreviated device
>> or bus paths. Therefore I give paths precedence over IDs in patch 4.
> 
> You're fixing problems in the overly complex and subtle path name lookup
> by making it more complex and subtle.  Let's make it simple and
> straightforward instead.

I have no problem with ripping out all of those abbreviations, requiring
the user to either specify a '/'-less unique ID or a full qtree path
that must start with a '/'. If paths get long, we now have interactive
completions.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v3 07/17] qdev: Push QMP mode checks into qbus_list_bus/dev

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Simplifies the usage.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index df945ed..e07ec98 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -492,6 +492,9 @@ static void qbus_list_bus(DeviceState *dev)
     BusState *child;
     const char *sep = " ";
 
+    if (monitor_cur_is_qmp()) {
+        return;
+    }
     error_printf("child busses at \"%s\":",
                  dev->id ? dev->id : dev->info->name);
     QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
@@ -506,6 +509,9 @@ static void qbus_list_dev(BusState *bus)
     DeviceState *dev;
     const char *sep = " ";
 
+    if (monitor_cur_is_qmp()) {
+        return;
+    }
     error_printf("devices at \"%s\":", bus->name);
     QTAILQ_FOREACH(dev, &bus->children, sibling) {
         error_printf("%s\"%s\"", sep, dev->info->name);
@@ -611,9 +617,7 @@ static BusState *qbus_find(const char *path)
         dev = qbus_find_dev(bus, elem);
         if (!dev) {
             qerror_report(QERR_DEVICE_NOT_FOUND, elem);
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_dev(bus);
-            }
+            qbus_list_dev(bus);
             return NULL;
         }
 
@@ -633,9 +637,7 @@ search_dev_bus:
                 return QTAILQ_FIRST(&dev->child_bus);
             default:
                 qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
-                if (!monitor_cur_is_qmp()) {
-                    qbus_list_bus(dev);
-                }
+                qbus_list_bus(dev);
                 return NULL;
             }
         }
@@ -658,9 +660,7 @@ search_dev_bus:
                 }
             }
             qerror_report(QERR_BUS_NOT_FOUND, elem);
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_bus(dev);
-            }
+            qbus_list_bus(dev);
             return NULL;
         }
     }
@@ -695,9 +695,7 @@ static DeviceState *qdev_find(const char *path)
         dev = qdev_find_id_recursive(main_system_bus, path);
         if (!dev) {
             qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_dev(bus);
-            }
+            qbus_list_dev(bus);
         }
     }
     return dev;
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 08/17] monitor: Add completion for qdev paths

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Implement monitor command line completion for device tree paths. The
first user is device_del.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c       |   50 ++++++++++++++++++++++----------
 hw/qdev.h       |    2 +
 monitor.c       |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-monitor.hx |    2 +-
 4 files changed, 123 insertions(+), 16 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index e07ec98..6f7d745 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -39,7 +39,7 @@ DeviceInfo *device_info_list;
 
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
-static BusState *qbus_find(const char *path);
+static BusState *qbus_find_internal(const char *path, bool report_errors);
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
@@ -217,7 +217,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     /* find bus */
     path = qemu_opt_get(opts, "bus");
     if (path != NULL) {
-        bus = qbus_find(path);
+        bus = qbus_find_internal(path, true);
         if (!bus) {
             return NULL;
         }
@@ -575,7 +575,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
     return NULL;
 }
 
-static BusState *qbus_find(const char *path)
+static BusState *qbus_find_internal(const char *path, bool report_errors)
 {
     DeviceState *dev, *next_dev;
     BusState *bus;
@@ -593,7 +593,9 @@ static BusState *qbus_find(const char *path)
         }
         bus = qbus_find_recursive(main_system_bus, elem, NULL);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
+            if (report_errors) {
+                qerror_report(QERR_BUS_NOT_FOUND, elem);
+            }
             return NULL;
         }
         pos = len;
@@ -616,8 +618,10 @@ static BusState *qbus_find(const char *path)
         pos += len;
         dev = qbus_find_dev(bus, elem);
         if (!dev) {
-            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
-            qbus_list_dev(bus);
+            if (report_errors) {
+                qerror_report(QERR_DEVICE_NOT_FOUND, elem);
+                qbus_list_dev(bus);
+            }
             return NULL;
         }
 
@@ -631,13 +635,17 @@ search_dev_bus:
              * one child bus accept it nevertheless */
             switch (dev->num_child_bus) {
             case 0:
-                qerror_report(QERR_DEVICE_NO_BUS, elem);
+                if (report_errors) {
+                    qerror_report(QERR_DEVICE_NO_BUS, elem);
+                }
                 return NULL;
             case 1:
                 return QTAILQ_FIRST(&dev->child_bus);
             default:
-                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
-                qbus_list_bus(dev);
+                if (report_errors) {
+                    qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
+                    qbus_list_bus(dev);
+                }
                 return NULL;
             }
         }
@@ -659,14 +667,21 @@ search_dev_bus:
                     goto search_dev_bus;
                 }
             }
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
-            qbus_list_bus(dev);
+            if (report_errors) {
+                qerror_report(QERR_BUS_NOT_FOUND, elem);
+                qbus_list_bus(dev);
+            }
             return NULL;
         }
     }
 }
 
-static DeviceState *qdev_find(const char *path)
+BusState *qbus_find(const char *path)
+{
+    return qbus_find_internal(path, false);
+}
+
+static DeviceState *qdev_find_internal(const char *path, bool report_errors)
 {
     const char *dev_name;
     DeviceState *dev;
@@ -682,7 +697,7 @@ static DeviceState *qdev_find(const char *path)
         bus_path = qemu_strdup(path);
         bus_path[dev_name - path] = 0;
 
-        bus = qbus_find(bus_path);
+        bus = qbus_find_internal(bus_path, report_errors);
         qemu_free(bus_path);
 
         if (!bus) {
@@ -693,7 +708,7 @@ static DeviceState *qdev_find(const char *path)
     dev = qbus_find_dev(bus, dev_name);
     if (!dev) {
         dev = qdev_find_id_recursive(main_system_bus, path);
-        if (!dev) {
+        if (!dev && report_errors) {
             qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
             qbus_list_dev(bus);
         }
@@ -701,6 +716,11 @@ static DeviceState *qdev_find(const char *path)
     return dev;
 }
 
+DeviceState *qdev_find(const char *path)
+{
+    return qdev_find_internal(path, false);
+}
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name)
 {
@@ -862,7 +882,7 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *path = qdict_get_str(qdict, "path");
     DeviceState *dev;
 
-    dev = qdev_find(path);
+    dev = qdev_find_internal(path, true);
     if (!dev) {
         qerror_report(QERR_DEVICE_NOT_FOUND, path);
         return -1;
diff --git a/hw/qdev.h b/hw/qdev.h
index 53f5565..b27d33b 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -165,6 +165,7 @@ void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
 CharDriverState *qdev_init_chardev(DeviceState *dev);
 
 BusState *qdev_get_parent_bus(DeviceState *dev);
+DeviceState *qdev_find(const char *path);
 
 /*** BUS API. ***/
 
@@ -172,6 +173,7 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
 void qbus_free(BusState *bus);
+BusState *qbus_find(const char *path);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
diff --git a/monitor.c b/monitor.c
index 4c95d7b..64de10a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -64,6 +64,7 @@
  *
  * 'F'          filename
  * 'B'          block device name
+ * 'Q'          qdev device path
  * 's'          string (accept optional quote)
  * 'O'          option string of the form NAME=VALUE,...
  *              parsed according to QemuOptsList given by its name
@@ -3437,6 +3438,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
         switch(c) {
         case 'F':
         case 'B':
+        case 'Q':
         case 's':
             {
                 int ret;
@@ -3461,6 +3463,10 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                         monitor_printf(mon, "%s: block device name expected\n",
                                        cmdname);
                         break;
+                    case 'Q':
+                        monitor_printf(mon, "%s: qdev device path expected\n",
+                                       cmdname);
+                        break;
                     default:
                         monitor_printf(mon, "%s: string expected\n", cmdname);
                         break;
@@ -3947,6 +3953,79 @@ static void block_completion_it(void *opaque, BlockDriverState *bs)
     }
 }
 
+static void add_qdev_completion(const char *parent_path, const char *name,
+                                bool trailing_slash)
+{
+    size_t parent_len = strlen(parent_path);
+    size_t name_len = strlen(name);
+    char *completion = qemu_malloc(parent_len + name_len + 2);
+
+    memcpy(completion, parent_path, parent_len);
+    memcpy(completion + parent_len, name, name_len);
+    if (trailing_slash) {
+        completion[parent_len + name_len] = '/';
+        completion[parent_len + name_len + 1] = 0;
+    } else {
+        completion[parent_len + name_len] = 0;
+    }
+    readline_add_completion(cur_mon->rs, completion);
+
+    qemu_free(completion);
+}
+
+static void qdev_completion(const char *input)
+{
+    size_t parent_len, name_len;
+    char *parent_path = NULL;
+    const char *p, *name;
+    DeviceState *dev;
+    BusState *bus;
+
+    p = strrchr(input, '/');
+    if (!p) {
+        if (*input == '\0') {
+            readline_add_completion(cur_mon->rs, "/");
+        }
+        return;
+    }
+
+    p++;
+    parent_path = qemu_strndup(input, p - input);
+    bus = qbus_find(parent_path);
+
+    if (bus) {
+        parent_len = strlen(parent_path);
+        name_len = strlen(bus->name);
+        if (bus->parent && strncmp(bus->name, p, strlen(p)) == 0 &&
+            (parent_len - 1 < name_len ||
+             strncmp(parent_path + parent_len - 1 - name_len, bus->name,
+                     name_len) != 0)) {
+            add_qdev_completion(parent_path, bus->name, true);
+        }
+        QTAILQ_FOREACH(dev, &bus->children, sibling) {
+            name = dev->id ? : dev->info->name;
+            if (strncmp(name, p, strlen(p)) == 0) {
+                add_qdev_completion(parent_path, name, false);
+                if (!QTAILQ_EMPTY(&dev->child_bus)) {
+                    add_qdev_completion(parent_path, name, true);
+                }
+            }
+        }
+    } else {
+        parent_path[strlen(parent_path) - 1] = 0;
+        dev = qdev_find(parent_path);
+        if (dev) {
+            parent_path[strlen(parent_path)] = '/';
+            QTAILQ_FOREACH(bus, &dev->child_bus, sibling) {
+                if (strncmp(bus->name, p, strlen(p)) == 0) {
+                    add_qdev_completion(parent_path, bus->name, true);
+                }
+            }
+        }
+    }
+    qemu_free(parent_path);
+}
+
 /* NOTE: this parser is an approximate form of the real command parser */
 static void parse_cmdline(const char *cmdline,
                          int *pnb_args, char **args)
@@ -4044,6 +4123,11 @@ static void monitor_find_completion(const char *cmdline)
             readline_set_completion_index(cur_mon->rs, strlen(str));
             bdrv_iterate(block_completion_it, (void *)str);
             break;
+        case 'Q':
+            /* qdev device path completion */
+            readline_set_completion_index(cur_mon->rs, strlen(str));
+            qdev_completion(str);
+            break;
         case 's':
             /* XXX: more generic ? */
             if (!strcmp(cmd->name, "info")) {
@@ -4122,6 +4206,7 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
     switch (cmd_args->type) {
         case 'F':
         case 'B':
+        case 'Q':
         case 's':
             if (qobject_type(value) != QTYPE_QSTRING) {
                 qerror_report(QERR_INVALID_PARAMETER_TYPE, name, "string");
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 754d71e..eb257a6 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -703,7 +703,7 @@ EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "path:s",
+        .args_type  = "path:Q",
         .params     = "device",
         .help       = "remove device",
         .user_print = monitor_user_noop,
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 09/17] Add base64 encoder/decoder

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Will be used by QBuffer.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Makefile.objs |    2 +-
 base64.c      |  202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 base64.h      |   19 ++++++
 3 files changed, 222 insertions(+), 1 deletions(-)
 create mode 100644 base64.c
 create mode 100644 base64.h

diff --git a/Makefile.objs b/Makefile.objs
index 1585101..81481c8 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -2,7 +2,7 @@
 # QObject
 qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
 qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
-qobject-obj-y += qerror.o
+qobject-obj-y += qerror.o base64.o
 
 #######################################################################
 # block-obj-y is code used by both qemu system emulation and qemu-img
diff --git a/base64.c b/base64.c
new file mode 100644
index 0000000..750d0fb
--- /dev/null
+++ b/base64.c
@@ -0,0 +1,202 @@
+/*
+ * Base64 encoder/decoder conforming to RFC 4648
+ * (based on Mozilla's nsprpub/lib/libc/src/base64.c)
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include <inttypes.h>
+#include "base64.h"
+
+static const char base[] =
+    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
+
+static void encode3to4(const uint8_t *src, char *dest)
+{
+    uint32_t b32 = 0;
+    int i, j = 18;
+
+    for (i = 0; i < 3; i++) {
+        b32 <<= 8;
+        b32 |= src[i];
+    }
+    for (i = 0; i < 4; i++) {
+        dest[i] = base[(b32 >> j) & 0x3F];
+        j -= 6;
+    }
+}
+
+static void encode2to4(const uint8_t *src, char *dest)
+{
+    dest[0] = base[(src[0] >> 2) & 0x3F];
+    dest[1] = base[((src[0] & 0x03) << 4) | ((src[1] >> 4) & 0x0F)];
+    dest[2] = base[(src[1] & 0x0F) << 2];
+    dest[3] = '=';
+}
+
+static void encode1to4(const uint8_t *src, char *dest)
+{
+    dest[0] = base[(src[0] >> 2) & 0x3F];
+    dest[1] = base[(src[0] & 0x03) << 4];
+    dest[2] = '=';
+    dest[3] = '=';
+}
+
+/*
+ * Encode data in 'src' of length 'srclen' to a base64 string, saving the
+ * null-terminated result in 'dest'. The size of the destition buffer must be
+ * at least ((srclen + 2) / 3) * 4 + 1.
+ */
+void base64_encode(const uint8_t *src, size_t srclen, char *dest)
+{
+    while (srclen >= 3) {
+        encode3to4(src, dest);
+        src += 3;
+        dest += 4;
+        srclen -= 3;
+    }
+    switch (srclen) {
+    case 2:
+        encode2to4(src, dest);
+        dest += 4;
+        break;
+    case 1:
+        encode1to4(src, dest);
+        dest += 4;
+        break;
+    case 0:
+        break;
+    }
+    dest[0] = 0;
+}
+
+static int32_t codetovalue(char c)
+{
+    if (c >= 'A' && c <= 'Z') {
+        return c - 'A';
+    } else if (c >= 'a' && c <= 'z') {
+        return c - 'a' + 26;
+    } else if (c >= '0' && c <= '9') {
+        return c - '0' + 52;
+    } else if (c == '+') {
+        return 62;
+    } else if ( c == '/') {
+        return 63;
+    } else {
+        return -1;
+    }
+}
+
+static int decode4to3 (const char *src, uint8_t *dest)
+{
+    uint32_t b32 = 0;
+    int32_t bits;
+    int i;
+
+    for (i = 0; i < 4; i++) {
+        bits = codetovalue(src[i]);
+        if (bits < 0) {
+            return bits;
+        }
+        b32 <<= 6;
+        b32 |= bits;
+    }
+    dest[0] = (b32 >> 16) & 0xFF;
+    dest[1] = (b32 >> 8) & 0xFF;
+    dest[2] = b32 & 0xFF;
+
+    return 0;
+}
+
+static int decode3to2(const char *src, uint8_t *dest)
+{
+    uint32_t b32 = 0;
+    int32_t bits;
+
+    bits = codetovalue(src[0]);
+    if (bits < 0) {
+        return bits;
+    }
+    b32 = (uint32_t)bits;
+    b32 <<= 6;
+
+    bits = codetovalue(src[1]);
+    if (bits < 0) {
+        return bits;
+    }
+    b32 |= (uint32_t)bits;
+    b32 <<= 4;
+
+    bits = codetovalue(src[2]);
+    if (bits < 0) {
+        return bits;
+    }
+    b32 |= ((uint32_t)bits) >> 2;
+
+    dest[0] = (b32 >> 8) & 0xFF;
+    dest[1] = b32 & 0xFF;
+
+    return 0;
+}
+
+static int decode2to1(const char *src, uint8_t *dest)
+{
+    uint32_t b32;
+    int32_t bits;
+
+    bits = codetovalue(src[0]);
+    if (bits < 0) {
+        return bits;
+    }
+    b32 = (uint32_t)bits << 2;
+
+    bits = codetovalue(src[1]);
+    if (bits < 0) {
+        return bits;
+    }
+    b32 |= ((uint32_t)bits) >> 4;
+
+    dest[0] = b32;
+
+    return 0;
+}
+
+/*
+ * Convert string 'src' of length 'srclen' from base64 to binary form,
+ * saving the result in 'dest'. The size of the destination buffer must be at
+ * least srclen * 3 / 4.
+ *
+ * Returns 0 on success, -1 on conversion error.
+ */
+int base64_decode(const char *src, size_t srclen, uint8_t *dest)
+{
+    int ret;
+
+    while (srclen >= 4) {
+        ret = decode4to3(src, dest);
+        if (ret < 0) {
+            return ret;
+        }
+        src += 4;
+        dest += 3;
+        srclen -= 4;
+    }
+
+    switch (srclen) {
+    case 3:
+        return decode3to2(src, dest);
+    case 2:
+        return decode2to1(src, dest);
+    case 1:
+        return -1;
+    default: /* 0 */
+        return 0;
+    }
+}
diff --git a/base64.h b/base64.h
new file mode 100644
index 0000000..07e72a8
--- /dev/null
+++ b/base64.h
@@ -0,0 +1,19 @@
+/*
+ * Base64 encoder/decoder conforming to RFC 4648
+ * (based on Mozilla's nsprpub/lib/libc/src/base64.c)
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include <inttypes.h>
+#include <stddef.h>
+
+void base64_encode(const uint8_t *src, size_t srclen, char *dest);
+int base64_decode(const char *src, size_t srclen, uint8_t *dest);
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 10/17] QMP: Reserve namespace for complex object classes

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This reserves JSON objects that contain the key '__class__' for QMP-specific
complex objects. First user will be the buffer class.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 QMP/qmp-spec.txt |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 9d30a8c..fa1dd62 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -146,6 +146,15 @@ The format is:
 For a listing of supported asynchronous events, please, refer to the
 qmp-events.txt file.
 
+2.6 Complex object classes
+--------------------------
+
+JSON objects that contain the key-value pair '"__class__": json-string' are
+reserved for QMP-specific complex object classes that. QMP specifies which
+further keys each of these objects include and how they are encoded.
+
+So far, no complex object class is specified.
+
 3. QMP Examples
 ===============
 
@@ -229,9 +238,10 @@ avoid modifying QMP.  Both upstream and downstream need to take care to
 preserve long-term compatibility and interoperability.
 
 To help with that, QMP reserves JSON object member names beginning with
-'__' (double underscore) for downstream use ("downstream names").  This
-means upstream will never use any downstream names for its commands,
-arguments, errors, asynchronous events, and so forth.
+'__' (double underscore) for downstream use ("downstream names").  Downstream
+names MUST NOT end with '__' as this pattern is reserved for QMP-defined JSON
+object classes.  Upstream will never use any downstream names for its
+commands, arguments, errors, asynchronous events, and so forth.
 
 Any new names downstream wishes to add must begin with '__'.  To
 ensure compatibility with other downstreams, it is strongly
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH v3 10/17] QMP: Reserve namespace for complex object classes

[Luiz Capitulino]

On Sun, 23 May 2010 12:59:23 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This reserves JSON objects that contain the key '__class__' for QMP-specific
> complex objects. First user will be the buffer class.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  QMP/qmp-spec.txt |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
> index 9d30a8c..fa1dd62 100644
> --- a/QMP/qmp-spec.txt
> +++ b/QMP/qmp-spec.txt
> @@ -146,6 +146,15 @@ The format is:
>  For a listing of supported asynchronous events, please, refer to the
>  qmp-events.txt file.
>  
> +2.6 Complex object classes
> +--------------------------
> +
> +JSON objects that contain the key-value pair '"__class__": json-string' are

 I'm not strong about this, but it's better to call it just a 'pair', as 'value'
is a bit problematic because of json-value.

> +reserved for QMP-specific complex object classes that. QMP specifies which

 Early full stop?

> +further keys each of these objects include and how they are encoded.
> +
> +So far, no complex object class is specified.
> +
>  3. QMP Examples
>  ===============
>  
> @@ -229,9 +238,10 @@ avoid modifying QMP.  Both upstream and downstream need to take care to
>  preserve long-term compatibility and interoperability.
>  
>  To help with that, QMP reserves JSON object member names beginning with
> -'__' (double underscore) for downstream use ("downstream names").  This
> -means upstream will never use any downstream names for its commands,
> -arguments, errors, asynchronous events, and so forth.
> +'__' (double underscore) for downstream use ("downstream names").  Downstream
> +names MUST NOT end with '__' as this pattern is reserved for QMP-defined JSON
> +object classes.  Upstream will never use any downstream names for its
> +commands, arguments, errors, asynchronous events, and so forth.

 Suggest mentioning subsection 2.6.

>  
>  Any new names downstream wishes to add must begin with '__'.  To
>  ensure compatibility with other downstreams, it is strongly

[Qemu-devel] Re: [PATCH v3 10/17] QMP: Reserve namespace for complex object classes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

Luiz Capitulino wrote:
> On Sun, 23 May 2010 12:59:23 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This reserves JSON objects that contain the key '__class__' for QMP-specific
>> complex objects. First user will be the buffer class.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  QMP/qmp-spec.txt |   16 +++++++++++++---
>>  1 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
>> index 9d30a8c..fa1dd62 100644
>> --- a/QMP/qmp-spec.txt
>> +++ b/QMP/qmp-spec.txt
>> @@ -146,6 +146,15 @@ The format is:
>>  For a listing of supported asynchronous events, please, refer to the
>>  qmp-events.txt file.
>>  
>> +2.6 Complex object classes
>> +--------------------------
>> +
>> +JSON objects that contain the key-value pair '"__class__": json-string' are
> 
>  I'm not strong about this, but it's better to call it just a 'pair', as 'value'
> is a bit problematic because of json-value.

Hmm, the official term is "name/value pairs". Will use that instead.

> 
>> +reserved for QMP-specific complex object classes that. QMP specifies which
> 
>  Early full stop?

Obviously. I just don't remember what I wanted to add.

> 
>> +further keys each of these objects include and how they are encoded.
>> +
>> +So far, no complex object class is specified.
>> +
>>  3. QMP Examples
>>  ===============
>>  
>> @@ -229,9 +238,10 @@ avoid modifying QMP.  Both upstream and downstream need to take care to
>>  preserve long-term compatibility and interoperability.
>>  
>>  To help with that, QMP reserves JSON object member names beginning with
>> -'__' (double underscore) for downstream use ("downstream names").  This
>> -means upstream will never use any downstream names for its commands,
>> -arguments, errors, asynchronous events, and so forth.
>> +'__' (double underscore) for downstream use ("downstream names").  Downstream
>> +names MUST NOT end with '__' as this pattern is reserved for QMP-defined JSON
>> +object classes.  Upstream will never use any downstream names for its
>> +commands, arguments, errors, asynchronous events, and so forth.
> 
>  Suggest mentioning subsection 2.6.

OK.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v3 11/17] Add QBuffer

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This introduces a buffer object for use with QMP. As a buffer is not
natively encodable in JSON, we encode it as a base64 string and
encapsulate the result in the new QMP object class "buffer".

The first use case for this is pushing the content of buffers that are
part of a device state into a qdict.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Makefile         |    5 +-
 Makefile.objs    |    2 +-
 QMP/qmp-spec.txt |   10 +++-
 check-qbuffer.c  |  172 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure        |    2 +-
 qbuffer.c        |  116 ++++++++++++++++++++++++++++++++++++
 qbuffer.h        |   33 ++++++++++
 qjson.c          |   15 +++++
 qobject.h        |    1 +
 9 files changed, 351 insertions(+), 5 deletions(-)
 create mode 100644 check-qbuffer.c
 create mode 100644 qbuffer.c
 create mode 100644 qbuffer.h

diff --git a/Makefile b/Makefile
index 1514433..89dda9e 100644
--- a/Makefile
+++ b/Makefile
@@ -144,14 +144,15 @@ qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(block-obj-y) $(qobj
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
 
-check-qint.o check-qstring.o check-qdict.o check-qlist.o check-qfloat.o check-qjson.o: $(GENERATED_HEADERS)
+check-qint.o check-qstring.o check-qdict.o check-qlist.o check-qfloat.o check-qjson.o check-qbuffer: $(GENERATED_HEADERS)
 
 check-qint: check-qint.o qint.o qemu-malloc.o
 check-qstring: check-qstring.o qstring.o qemu-malloc.o
 check-qdict: check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qemu-malloc.o qlist.o
 check-qlist: check-qlist.o qlist.o qint.o qemu-malloc.o
 check-qfloat: check-qfloat.o qfloat.o qemu-malloc.o
-check-qjson: check-qjson.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o qjson.o json-streamer.o json-lexer.o json-parser.o qemu-malloc.o
+check-qjson: check-qjson.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o qbuffer.o base64.o qjson.o json-streamer.o json-lexer.o json-parser.o qemu-malloc.o
+check-qbuffer: check-qbuffer.o qbuffer.o base64.o qstring.o qemu-malloc.o
 
 clean:
 # avoid old build problems by removing potentially incorrect old files
diff --git a/Makefile.objs b/Makefile.objs
index 81481c8..da55ec2 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -1,6 +1,6 @@
 #######################################################################
 # QObject
-qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
+qobject-obj-y = qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qbuffer.o
 qobject-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
 qobject-obj-y += qerror.o base64.o
 
diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index fa1dd62..820e39d 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -153,7 +153,15 @@ JSON objects that contain the key-value pair '"__class__": json-string' are
 reserved for QMP-specific complex object classes that. QMP specifies which
 further keys each of these objects include and how they are encoded.
 
-So far, no complex object class is specified.
+2.6.1 Buffer class
+------------------
+
+This QMP object class allows to transport binary data. A buffer object
+consists of the following keys:
+
+{ "__class__": "buffer", "data": json-string }
+
+The data string is base64 encoded according to RFC 4648.
 
 3. QMP Examples
 ===============
diff --git a/check-qbuffer.c b/check-qbuffer.c
new file mode 100644
index 0000000..b490230
--- /dev/null
+++ b/check-qbuffer.c
@@ -0,0 +1,172 @@
+/*
+ * QBuffer unit-tests.
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL version 2.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include <check.h>
+
+#include "qbuffer.h"
+#include "qemu-common.h"
+
+const char data[] = "some data";
+
+START_TEST(qbuffer_from_data_test)
+{
+    QBuffer *qbuffer;
+
+    qbuffer = qbuffer_from_data(data, sizeof(data));
+    fail_unless(qbuffer != NULL);
+    fail_unless(qbuffer->base.refcnt == 1);
+    fail_unless(memcmp(data, qbuffer->data, sizeof(data)) == 0);
+    fail_unless(qbuffer->size == sizeof(data));
+    fail_unless(qobject_type(QOBJECT(qbuffer)) == QTYPE_QBUFFER);
+
+    /* destroy doesn't exit yet */
+    qemu_free(qbuffer->data);
+    qemu_free(qbuffer);
+}
+END_TEST
+
+START_TEST(qbuffer_destroy_test)
+{
+    QBuffer *qbuffer = qbuffer_from_data(data, sizeof(data));
+
+    QDECREF(qbuffer);
+}
+END_TEST
+
+START_TEST(qbuffer_get_data_test)
+{
+    QBuffer *qbuffer;
+    const void *ret_data;
+
+    qbuffer = qbuffer_from_data(data, sizeof(data));
+    ret_data = qbuffer_get_data(qbuffer);
+    fail_unless(memcmp(ret_data, data, sizeof(data)) == 0);
+
+    QDECREF(qbuffer);
+}
+END_TEST
+
+START_TEST(qbuffer_get_size_test)
+{
+    QBuffer *qbuffer;
+
+    qbuffer = qbuffer_from_data(data, sizeof(data));
+    fail_unless(qbuffer_get_size(qbuffer) == sizeof(data));
+
+    QDECREF(qbuffer);
+}
+END_TEST
+
+START_TEST(qbuffer_from_qstring_test)
+{
+    const struct {
+        const char *encoded;
+        const char *decoded;
+    } pattern[3] = {
+        {
+            .encoded = "SGVsbG8sIFFCdWZmZXIhCg==",
+            .decoded = "Hello, QBuffer!",
+        },
+        {
+             .encoded = "SGVsbG8gUUJ1ZmZlcgo=",
+             .decoded = "Hello QBuffer",
+        },
+        {
+             .encoded = "SGVsbG8gUUJ1ZmZlciEK===",
+             .decoded = "Hello QBuffer!",
+        },
+    };
+    QBuffer *qbuffer;
+    QString *qstring;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pattern); i++) {
+        qstring = qstring_from_str(pattern[i].encoded);
+        qbuffer = qbuffer_from_qstring(qstring);
+        QDECREF(qstring);
+
+        fail_unless(qbuffer != NULL);
+        fail_unless(memcmp(qbuffer_get_data(qbuffer), pattern[i].decoded,
+                    sizeof(pattern[i].decoded)) == 0);
+
+        QDECREF(qbuffer);
+    }
+}
+END_TEST
+
+START_TEST(qbuffer_from_invalid_qstring_test)
+{
+    const char *pattern[] = {
+        "SGVsbG8sIFFCdWZmZXIhC",
+        "SGVsbG8gU=UJ1ZmZlcgo",
+        "SGVsbG8gUUJ1*ZmZlciEK",
+    };
+    QBuffer *qbuffer;
+    QString *qstring;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(pattern); i++) {
+        qstring = qstring_from_str(pattern[i]);
+        qbuffer = qbuffer_from_qstring(qstring);
+        QDECREF(qstring);
+
+        fail_unless(qbuffer == NULL);
+    }
+}
+END_TEST
+
+START_TEST(qobject_to_qbuffer_test)
+{
+    QBuffer *qbuffer;
+
+    qbuffer = qbuffer_from_data(data, sizeof(data));
+    fail_unless(qobject_to_qbuffer(QOBJECT(qbuffer)) == qbuffer);
+
+    QDECREF(qbuffer);
+}
+END_TEST
+
+static Suite *qbuffer_suite(void)
+{
+    Suite *s;
+    TCase *qbuffer_public_tcase;
+
+    s = suite_create("QBuffer test-suite");
+
+    qbuffer_public_tcase = tcase_create("Public Interface");
+    suite_add_tcase(s, qbuffer_public_tcase);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_from_data_test);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_destroy_test);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_get_data_test);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_get_size_test);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_from_qstring_test);
+    tcase_add_test(qbuffer_public_tcase, qbuffer_from_invalid_qstring_test);
+    tcase_add_test(qbuffer_public_tcase, qobject_to_qbuffer_test);
+
+    return s;
+}
+
+int main(void)
+{
+    int nf;
+    Suite *s;
+    SRunner *sr;
+
+    s = qbuffer_suite();
+    sr = srunner_create(s);
+
+    srunner_run_all(sr, CK_NORMAL);
+    nf = srunner_ntests_failed(sr);
+    srunner_free(sr);
+
+    return (nf == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}
diff --git a/configure b/configure
index 653c8d2..1c5c0f9 100755
--- a/configure
+++ b/configure
@@ -2290,7 +2290,7 @@ if test `expr "$target_list" : ".*softmmu.*"` != 0 ; then
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
       tools="qemu-nbd\$(EXESUF) $tools"
     if [ "$check_utests" = "yes" ]; then
-      tools="check-qint check-qstring check-qdict check-qlist $tools"
+      tools="check-qint check-qstring check-qdict check-qlist check-qbuffer $tools"
       tools="check-qfloat check-qjson $tools"
     fi
   fi
diff --git a/qbuffer.c b/qbuffer.c
new file mode 100644
index 0000000..704d170
--- /dev/null
+++ b/qbuffer.c
@@ -0,0 +1,116 @@
+/*
+ * QBuffer Module
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qbuffer.h"
+#include "qobject.h"
+#include "qemu-common.h"
+#include "base64.h"
+
+static void qbuffer_destroy_obj(QObject *obj);
+
+static const QType qbuffer_type = {
+    .code = QTYPE_QBUFFER,
+    .destroy = qbuffer_destroy_obj,
+};
+
+/**
+ * qbuffer_from_data(): Create a new QBuffer from an existing data blob
+ *
+ * Returns strong reference.
+ */
+QBuffer *qbuffer_from_data(const void *data, size_t size)
+{
+    QBuffer *qb;
+
+    qb = qemu_malloc(sizeof(*qb));
+    qb->data = qemu_malloc(size);
+    memcpy(qb->data, data, size);
+    qb->size = size;
+    QOBJECT_INIT(qb, &qbuffer_type);
+
+    return qb;
+}
+
+/**
+ * qbuffer_from_qstring(): Create a new QBuffer from a QString object that
+ * contains the data as a stream of hex-encoded bytes
+ *
+ * Returns strong reference.
+ */
+QBuffer *qbuffer_from_qstring(const QString *string)
+{
+    const char *str = qstring_get_str(string);
+    size_t str_len;
+    QBuffer *qb;
+
+    qb = qemu_malloc(sizeof(*qb));
+
+    str_len = strlen(str);
+    while (str_len > 0 && str[str_len - 1] == '=') {
+        str_len--;
+    }
+    qb->size = (str_len / 4) * 3 + ((str_len % 4) * 3) / 4;
+    qb->data = qemu_malloc(qb->size);
+
+    QOBJECT_INIT(qb, &qbuffer_type);
+
+    if (base64_decode(str, str_len, qb->data) < 0) {
+        qbuffer_destroy_obj(QOBJECT(qb));
+        return NULL;
+    }
+
+    return qb;
+}
+
+/**
+ * qbuffer_get_data(): Return pointer to stored data
+ *
+ * NOTE: Should be used with caution, if the object is deallocated
+ * this pointer becomes invalid.
+ */
+const void *qbuffer_get_data(const QBuffer *qb)
+{
+    return qb->data;
+}
+
+/**
+ * qbuffer_get_size(): Return size of stored data
+ */
+size_t qbuffer_get_size(const QBuffer *qb)
+{
+    return qb->size;
+}
+
+/**
+ * qobject_to_qbool(): Convert a QObject into a QBuffer
+ */
+QBuffer *qobject_to_qbuffer(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QBUFFER)
+        return NULL;
+
+    return container_of(obj, QBuffer, base);
+}
+
+/**
+ * qbuffer_destroy_obj(): Free all memory allocated by a QBuffer object
+ */
+static void qbuffer_destroy_obj(QObject *obj)
+{
+    QBuffer *qb;
+
+    assert(obj != NULL);
+    qb = qobject_to_qbuffer(obj);
+    qemu_free(qb->data);
+    qemu_free(qb);
+}
diff --git a/qbuffer.h b/qbuffer.h
new file mode 100644
index 0000000..2e01078
--- /dev/null
+++ b/qbuffer.h
@@ -0,0 +1,33 @@
+/*
+ * QBuffer Module
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QBUFFER_H
+#define QBUFFER_H
+
+#include <stdint.h>
+#include "qobject.h"
+#include "qstring.h"
+
+typedef struct QBuffer {
+    QObject_HEAD;
+    void *data;
+    size_t size;
+} QBuffer;
+
+QBuffer *qbuffer_from_data(const void *data, size_t size);
+QBuffer *qbuffer_from_qstring(const QString *string);
+const void *qbuffer_get_data(const QBuffer *qb);
+size_t qbuffer_get_size(const QBuffer *qb);
+QBuffer *qobject_to_qbuffer(const QObject *obj);
+
+#endif /* QBUFFER_H */
diff --git a/qjson.c b/qjson.c
index 483c667..419b4e7 100644
--- a/qjson.c
+++ b/qjson.c
@@ -19,7 +19,9 @@
 #include "qlist.h"
 #include "qbool.h"
 #include "qfloat.h"
+#include "qbuffer.h"
 #include "qdict.h"
+#include "base64.h"
 
 typedef struct JSONParsingState
 {
@@ -235,6 +237,19 @@ static void to_json(const QObject *obj, QString *str)
         }
         break;
     }
+    case QTYPE_QBUFFER: {
+        QBuffer *val = qobject_to_qbuffer(obj);
+        size_t data_size = qbuffer_get_size(val);
+        size_t str_len = ((data_size + 2) / 3) * 4;
+        char *buffer = qemu_malloc(str_len + 1);
+
+        base64_encode(qbuffer_get_data(val), data_size, buffer);
+        qstring_append(str, "{\"__class__\": \"buffer\", \"data\": \"");
+        qstring_append(str, buffer);
+        qstring_append(str, "\"}");
+        qemu_free(buffer);
+        break;
+    }
     case QTYPE_QERROR:
         /* XXX: should QError be emitted? */
     case QTYPE_NONE:
diff --git a/qobject.h b/qobject.h
index 07de211..45c4fa0 100644
--- a/qobject.h
+++ b/qobject.h
@@ -44,6 +44,7 @@ typedef enum {
     QTYPE_QFLOAT,
     QTYPE_QBOOL,
     QTYPE_QERROR,
+    QTYPE_QBUFFER,
 } qtype_code;
 
 struct QObject;
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 12/17] monitor: return length of printed string via monitor_[v]printf

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This simply forwards the result of the internal vsnprintf to the callers
of monitor_printf and monitor_vprintf. When invoked over a QMP session
or in absence of an active monitor, -1 is returned.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 monitor.c   |   23 +++++++++++++++--------
 monitor.h   |    4 ++--
 qemu-tool.c |    6 ++++--
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index 64de10a..6766e49 100644
--- a/monitor.c
+++ b/monitor.c
@@ -258,29 +258,36 @@ static void monitor_puts(Monitor *mon, const char *str)
     }
 }
 
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
     char buf[4096];
+    int ret;
 
-    if (!mon)
-        return;
-
+    if (!mon) {
+        return -1;
+    }
     mon_print_count_inc(mon);
 
     if (monitor_ctrl_mode(mon)) {
-        return;
+        return -1;
     }
 
-    vsnprintf(buf, sizeof(buf), fmt, ap);
+    ret = vsnprintf(buf, sizeof(buf), fmt, ap);
     monitor_puts(mon, buf);
+
+    return ret;
 }
 
-void monitor_printf(Monitor *mon, const char *fmt, ...)
+int monitor_printf(Monitor *mon, const char *fmt, ...)
 {
     va_list ap;
+    int ret;
+
     va_start(ap, fmt);
-    monitor_vprintf(mon, fmt, ap);
+    ret = monitor_vprintf(mon, fmt, ap);
     va_end(ap);
+
+    return ret;
 }
 
 void monitor_print_filename(Monitor *mon, const char *filename)
diff --git a/monitor.h b/monitor.h
index ea15469..32c0170 100644
--- a/monitor.h
+++ b/monitor.h
@@ -45,8 +45,8 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
 
 int monitor_get_fd(Monitor *mon, const char *fdname);
 
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
-void monitor_printf(Monitor *mon, const char *fmt, ...)
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap);
+int monitor_printf(Monitor *mon, const char *fmt, ...)
     __attribute__ ((__format__ (__printf__, 2, 3)));
 void monitor_print_filename(Monitor *mon, const char *filename);
 void monitor_flush(Monitor *mon);
diff --git a/qemu-tool.c b/qemu-tool.c
index b39af86..f6ce6cd 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -43,12 +43,14 @@ void monitor_set_error(Monitor *mon, QError *qerror)
 {
 }
 
-void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
+int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
+    return -1;
 }
 
-void monitor_printf(Monitor *mon, const char *fmt, ...)
+int monitor_printf(Monitor *mon, const char *fmt, ...)
 {
+    return -1;
 }
 
 void monitor_print_filename(Monitor *mon, const char *filename)
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH v3 12/17] monitor: return length of printed string via monitor_[v]printf

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This simply forwards the result of the internal vsnprintf to the callers
> of monitor_printf and monitor_vprintf. When invoked over a QMP session
> or in absence of an active monitor, -1 is returned.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Appreciated!  Opens the door to addressing the TODOs in qemu-error.c.

[Qemu-devel] [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Ported commands that are marked 'user_only' will not be considered for
QMP monitor sessions. This allows to implement new commands that do not
(yet) provide a sufficiently stable interface for QMP use (e.g.
device_show).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 monitor.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6766e49..5768c6e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -114,6 +114,7 @@ typedef struct mon_cmd_t {
                           MonitorCompletion *cb, void *opaque);
     } mhandler;
     int async;
+    bool user_only;
 } mon_cmd_t;
 
 /* file descriptors passed via SCM_RIGHTS */
@@ -635,6 +636,11 @@ static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto help;
     }
 
+    if (monitor_ctrl_mode(mon) && cmd->user_only) {
+        qerror_report(QERR_COMMAND_NOT_FOUND, item);
+        return -1;
+    }
+
     if (monitor_handler_is_async(cmd)) {
         if (monitor_ctrl_mode(mon)) {
             qmp_async_info_handler(mon, cmd);
@@ -732,13 +738,14 @@ static void do_info_commands(Monitor *mon, QObject **ret_data)
     cmd_list = qlist_new();
 
     for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
-        if (monitor_handler_ported(cmd) && !compare_cmd(cmd->name, "info")) {
+        if (monitor_handler_ported(cmd) && !cmd->user_only &&
+            !compare_cmd(cmd->name, "info")) {
             qlist_append_obj(cmd_list, get_cmd_dict(cmd->name));
         }
     }
 
     for (cmd = info_cmds; cmd->name != NULL; cmd++) {
-        if (monitor_handler_ported(cmd)) {
+        if (monitor_handler_ported(cmd) && !cmd->user_only) {
             char buf[128];
             snprintf(buf, sizeof(buf), "query-%s", cmd->name);
             qlist_append_obj(cmd_list, get_cmd_dict(buf));
@@ -4416,7 +4423,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
                       qobject_from_jsonf("{ 'item': %s }", info_item));
     } else {
         cmd = monitor_find_command(cmd_name);
-        if (!cmd || !monitor_handler_ported(cmd)) {
+        if (!cmd || !monitor_handler_ported(cmd) || cmd->user_only) {
             qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
             goto err_input;
         }
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Luiz Capitulino]

On Sun, 23 May 2010 12:59:26 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Ported commands that are marked 'user_only' will not be considered for
> QMP monitor sessions. This allows to implement new commands that do not
> (yet) provide a sufficiently stable interface for QMP use (e.g.
> device_show).

 This is fine for me, but two things I've been wondering:

 1. Isn't a 'flags' struct member better? So that we can do (in the
    qemu-monitor.hx entry):

        .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,

    I'm not suggesting this is an async handler, just exemplifying multiple
    flags.

  2. Getting QMP handlers right in the first time might be difficult, so
     we could have a way to mark them unstable. Maybe a different namespace
     which is only enabled at configure time with:

         --enable-qmp-unstable-commands

     If this were possible, we could have device_show and any command we
     aren't sure is QMP-ready working in QMP this way.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  monitor.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 6766e49..5768c6e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -114,6 +114,7 @@ typedef struct mon_cmd_t {
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
>      int async;
> +    bool user_only;
>  } mon_cmd_t;
>  
>  /* file descriptors passed via SCM_RIGHTS */
> @@ -635,6 +636,11 @@ static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          goto help;
>      }
>  
> +    if (monitor_ctrl_mode(mon) && cmd->user_only) {
> +        qerror_report(QERR_COMMAND_NOT_FOUND, item);
> +        return -1;
> +    }
> +
>      if (monitor_handler_is_async(cmd)) {
>          if (monitor_ctrl_mode(mon)) {
>              qmp_async_info_handler(mon, cmd);
> @@ -732,13 +738,14 @@ static void do_info_commands(Monitor *mon, QObject **ret_data)
>      cmd_list = qlist_new();
>  
>      for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
> -        if (monitor_handler_ported(cmd) && !compare_cmd(cmd->name, "info")) {
> +        if (monitor_handler_ported(cmd) && !cmd->user_only &&
> +            !compare_cmd(cmd->name, "info")) {
>              qlist_append_obj(cmd_list, get_cmd_dict(cmd->name));
>          }
>      }
>  
>      for (cmd = info_cmds; cmd->name != NULL; cmd++) {
> -        if (monitor_handler_ported(cmd)) {
> +        if (monitor_handler_ported(cmd) && !cmd->user_only) {
>              char buf[128];
>              snprintf(buf, sizeof(buf), "query-%s", cmd->name);
>              qlist_append_obj(cmd_list, get_cmd_dict(buf));
> @@ -4416,7 +4423,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>                        qobject_from_jsonf("{ 'item': %s }", info_item));
>      } else {
>          cmd = monitor_find_command(cmd_name);
> -        if (!cmd || !monitor_handler_ported(cmd)) {
> +        if (!cmd || !monitor_handler_ported(cmd) || cmd->user_only) {
>              qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
>              goto err_input;
>          }

[Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

Luiz Capitulino wrote:
> On Sun, 23 May 2010 12:59:26 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Ported commands that are marked 'user_only' will not be considered for
>> QMP monitor sessions. This allows to implement new commands that do not
>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>> device_show).
> 
>  This is fine for me, but two things I've been wondering:
> 
>  1. Isn't a 'flags' struct member better? So that we can do (in the
>     qemu-monitor.hx entry):
> 
>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
> 
>     I'm not suggesting this is an async handler, just exemplifying multiple
>     flags.

Yes, can refactor this.

> 
>   2. Getting QMP handlers right in the first time might be difficult, so
>      we could have a way to mark them unstable. Maybe a different namespace
>      which is only enabled at configure time with:
> 
>          --enable-qmp-unstable-commands
> 
>      If this were possible, we could have device_show and any command we
>      aren't sure is QMP-ready working in QMP this way.

Do you suggest this as an alternative to this patch? Or an extension
later on? I have no opinion on this yet, I would just like to know how
to proceed for this series.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Luiz Capitulino]

On Fri, 28 May 2010 00:20:08 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Sun, 23 May 2010 12:59:26 +0200
> > Jan Kiszka <jan.kiszka@web.de> wrote:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Ported commands that are marked 'user_only' will not be considered for
> >> QMP monitor sessions. This allows to implement new commands that do not
> >> (yet) provide a sufficiently stable interface for QMP use (e.g.
> >> device_show).
> > 
> >  This is fine for me, but two things I've been wondering:
> > 
> >  1. Isn't a 'flags' struct member better? So that we can do (in the
> >     qemu-monitor.hx entry):
> > 
> >         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
> > 
> >     I'm not suggesting this is an async handler, just exemplifying multiple
> >     flags.
> 
> Yes, can refactor this.
> 
> > 
> >   2. Getting QMP handlers right in the first time might be difficult, so
> >      we could have a way to mark them unstable. Maybe a different namespace
> >      which is only enabled at configure time with:
> > 
> >          --enable-qmp-unstable-commands
> > 
> >      If this were possible, we could have device_show and any command we
> >      aren't sure is QMP-ready working in QMP this way.
> 
> Do you suggest this as an alternative to this patch? Or an extension
> later on? I have no opinion on this yet, I would just like to know how
> to proceed for this series.

 Both can be done worked later, as this is internal there's no problem in
living with a simpler solution for a while.

Re: [Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sun, 23 May 2010 12:59:26 +0200
> Jan Kiszka <jan.kiszka@web.de> wrote:
>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> Ported commands that are marked 'user_only' will not be considered for
>> QMP monitor sessions. This allows to implement new commands that do not
>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>> device_show).
>
>  This is fine for me, but two things I've been wondering:
>
>  1. Isn't a 'flags' struct member better? So that we can do (in the
>     qemu-monitor.hx entry):
>
>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
>
>     I'm not suggesting this is an async handler, just exemplifying multiple
>     flags.

We also have at least one command that makes only sense in QMP:
qmp_capabilities.  Maybe we could use separate flags controlling command
availability in human monitor and QMP.

[...]

Re: [Qemu-devel] Re: [PATCH v3 13/17] monitor: Allow to exclude commands from QMP

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Sun, 23 May 2010 12:59:26 +0200
>> Jan Kiszka <jan.kiszka@web.de> wrote:
>>
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Ported commands that are marked 'user_only' will not be considered for
>>> QMP monitor sessions. This allows to implement new commands that do not
>>> (yet) provide a sufficiently stable interface for QMP use (e.g.
>>> device_show).
>>  This is fine for me, but two things I've been wondering:
>>
>>  1. Isn't a 'flags' struct member better? So that we can do (in the
>>     qemu-monitor.hx entry):
>>
>>         .flags = MONITOR_USER_ONLY | MONITOR_HANDLER_ASYNC,
>>
>>     I'm not suggesting this is an async handler, just exemplifying multiple
>>     flags.
> 
> We also have at least one command that makes only sense in QMP:
> qmp_capabilities.  Maybe we could use separate flags controlling command
> availability in human monitor and QMP.

Sounds reasonable. Then I think I will lay the ground for this by
introducing flags already in this patch. A v4 run is required anyway.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v3 14/17] monitor: Add basic device state visualization

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This introduces device_show, a monitor command that saves the vmstate of
a qdev device and visualizes it. Buffers are cut after 16 byte by
default, but the full content can be requested via '-f'. To pretty-print
sub-arrays, vmstate is extended to store the start index name. A new
qerror is introduced to signal a missing vmstate. QMP is not supported
as we cannot provide a stable interface, at least at this point.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/hw.h         |    2 +
 hw/qdev.c       |  243 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h       |    2 +
 qemu-monitor.hx |   19 +++++
 qerror.c        |    4 +
 qerror.h        |    3 +
 6 files changed, 273 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index fc2d184..cc4bd5f 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -299,6 +299,7 @@ enum VMStateFlags {
 
 typedef struct {
     const char *name;
+    const char *start_index;
     size_t offset;
     size_t size;
     size_t start;
@@ -413,6 +414,7 @@ extern const VMStateInfo vmstate_info_unused_buffer;
     .size       = sizeof(_type),                                     \
     .flags      = VMS_ARRAY,                                         \
     .offset     = vmstate_offset_sub_array(_state, _field, _type, _start), \
+    .start_index = (stringify(_start)),                              \
 }
 
 #define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info, _type) {\
diff --git a/hw/qdev.c b/hw/qdev.c
index 6f7d745..b5bf72c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,6 +29,9 @@
 #include "qdev.h"
 #include "sysemu.h"
 #include "monitor.h"
+#include "qjson.h"
+#include "qint.h"
+#include "qbuffer.h"
 
 static int qdev_hotplug = 0;
 
@@ -889,3 +892,243 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     return qdev_unplug(dev);
 }
+
+#define NAME_COLUMN_WIDTH 23
+
+static void print_field(Monitor *mon, const QDict *qfield, int indent);
+
+static void print_elem(Monitor *mon, const QObject *qelem, size_t size,
+                       int column_pos, int indent)
+{
+    int64_t data_size;
+    const void *data;
+    int n;
+
+    if (qobject_type(qelem) == QTYPE_QDICT) {
+        if (column_pos >= 0) {
+            monitor_printf(mon, ".\n");
+        }
+    } else {
+        monitor_printf(mon, ":");
+        column_pos++;
+        if (column_pos < NAME_COLUMN_WIDTH) {
+            monitor_printf(mon, "%*c", NAME_COLUMN_WIDTH - column_pos, ' ');
+        }
+    }
+
+    switch (qobject_type(qelem)) {
+    case QTYPE_QDICT:
+        print_field(mon, qobject_to_qdict(qelem), indent + 2);
+        break;
+    case QTYPE_QBUFFER:
+        data = qbuffer_get_data(qobject_to_qbuffer(qelem));
+        data_size = qbuffer_get_size(qobject_to_qbuffer(qelem));
+        for (n = 0; n < data_size; ) {
+            monitor_printf(mon, " %02x", *((uint8_t *)data+n));
+            if (++n < size) {
+                if (n % 16 == 0) {
+                    monitor_printf(mon, "\n%*c", NAME_COLUMN_WIDTH, ' ');
+                } else if (n % 8 == 0) {
+                    monitor_printf(mon, " -");
+                }
+            }
+        }
+        if (data_size < size) {
+            monitor_printf(mon, " ...");
+        }
+        monitor_printf(mon, "\n");
+        break;
+    case QTYPE_QINT:
+        monitor_printf(mon, " %0*" PRIx64 "\n", (int)size * 2,
+                       qint_get_int(qobject_to_qint(qelem)));
+        break;
+    default:
+        assert(0);
+    }
+}
+
+static void print_field(Monitor *mon, const QDict *qfield, int indent)
+{
+    const char *name = qdict_get_str(qfield, "name");
+    const char *start = qdict_get_try_str(qfield, "start");
+    int64_t size = qdict_get_int(qfield, "size");
+    QList *qlist = qdict_get_qlist(qfield, "elems");
+    QListEntry *entry, *sub_entry;
+    QList *sub_list;
+    int elem_no = 0;
+
+    QLIST_FOREACH_ENTRY(qlist, entry) {
+        QObject *qelem = qlist_entry_obj(entry);
+        int pos = indent + strlen(name);
+
+        if (qobject_type(qelem) == QTYPE_QLIST) {
+            monitor_printf(mon, "%*c%s", indent, ' ', name);
+            if (start) {
+                pos += monitor_printf(mon, "[%s+%02x]", start, elem_no);
+            } else {
+                pos += monitor_printf(mon, "[%02x]", elem_no);
+            }
+            sub_list = qobject_to_qlist(qelem);
+            QLIST_FOREACH_ENTRY(sub_list, sub_entry) {
+                print_elem(mon, qlist_entry_obj(sub_entry), size, pos,
+                           indent + 2);
+                pos = -1;
+            }
+        } else {
+            if (elem_no == 0) {
+                monitor_printf(mon, "%*c%s", indent, ' ', name);
+            } else {
+                pos = -1;
+            }
+            print_elem(mon, qelem, size, pos, indent);
+        }
+        elem_no++;
+    }
+}
+
+void device_user_print(Monitor *mon, const QObject *data)
+{
+    QDict *qdict = qobject_to_qdict(data);
+    QList *qlist = qdict_get_qlist(qdict, "fields");
+    QListEntry *entry;
+
+    monitor_printf(mon, "dev: %s, id \"%s\", version %" PRId64 "\n",
+                   qdict_get_str(qdict, "device"),
+                   qdict_get_str(qdict, "id"),
+                   qdict_get_int(qdict, "version"));
+
+    QLIST_FOREACH_ENTRY(qlist, entry) {
+        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
+    }
+}
+
+static size_t parse_vmstate(const VMStateDescription *vmsd, void *opaque,
+                            QList *qlist, int full_buffers)
+{
+    VMStateField *field = vmsd->fields;
+    size_t overall_size = 0;
+
+    if (vmsd->pre_save) {
+        vmsd->pre_save(opaque);
+    }
+    while(field->name) {
+        if (!field->field_exists ||
+            field->field_exists(opaque, vmsd->version_id)) {
+            void *base_addr = opaque + field->offset;
+            int i, n_elems = 1;
+            int is_array = 1;
+            size_t size = field->size;
+            size_t real_size = 0;
+            size_t dump_size;
+            QDict *qfield = qdict_new();
+            QList *qelems = qlist_new();
+
+            qlist_append_obj(qlist, QOBJECT(qfield));
+
+            qdict_put_obj(qfield, "name",
+                          QOBJECT(qstring_from_str(field->name)));
+            qdict_put_obj(qfield, "elems", QOBJECT(qelems));
+
+            if (field->flags & VMS_VBUFFER) {
+                size = *(int32_t *)(opaque + field->size_offset);
+                if (field->flags & VMS_MULTIPLY) {
+                    size *= field->size;
+                }
+            }
+            if (field->start_index) {
+                qdict_put_obj(qfield, "start",
+                              QOBJECT(qstring_from_str(field->start_index)));
+            }
+
+            if (field->flags & VMS_ARRAY) {
+                n_elems = field->num;
+            } else if (field->flags & VMS_VARRAY_INT32) {
+                n_elems = *(int32_t *)(opaque + field->num_offset);
+            } else if (field->flags & VMS_VARRAY_UINT16) {
+                n_elems = *(uint16_t *)(opaque + field->num_offset);
+            } else {
+                is_array = 0;
+            }
+            if (field->flags & VMS_POINTER) {
+                base_addr = *(void **)base_addr + field->start;
+            }
+            for (i = 0; i < n_elems; i++) {
+                void *addr = base_addr + size * i;
+                QList *sub_elems = qelems;
+                int val;
+
+                if (is_array) {
+                    sub_elems = qlist_new();
+                    qlist_append_obj(qelems, QOBJECT(sub_elems));
+                }
+                if (field->flags & VMS_ARRAY_OF_POINTER) {
+                    addr = *(void **)addr;
+                }
+                if (field->flags & VMS_STRUCT) {
+                    real_size = parse_vmstate(field->vmsd, addr,
+                                              sub_elems, full_buffers);
+                } else {
+                    real_size = size;
+                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
+                        dump_size = (full_buffers || size <= 16) ? size : 16;
+                        qlist_append_obj(sub_elems,
+                                QOBJECT(qbuffer_from_data(addr, dump_size)));
+                    } else {
+                        switch (size) {
+                        case 1:
+                            val = *(uint8_t *)addr;
+                            break;
+                        case 2:
+                            val = *(uint16_t *)addr;
+                            break;
+                        case 4:
+                            val = *(uint32_t *)addr;
+                            break;
+                        case 8:
+                            val = *(uint64_t *)addr;
+                            break;
+                        default:
+                            assert(0);
+                        }
+                        qlist_append_obj(sub_elems,
+                                         QOBJECT(qint_from_int(val)));
+                    }
+                }
+                overall_size += real_size;
+            }
+            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(real_size)));
+        }
+        field++;
+    }
+    return overall_size;
+}
+
+int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const VMStateDescription *vmsd;
+    DeviceState *dev;
+    QList *qlist;
+
+    dev = qdev_find_internal(path, true);
+    if (!dev) {
+        return -1;
+    }
+
+    vmsd = dev->info->vmsd;
+    if (!vmsd) {
+        qerror_report(QERR_DEVICE_NO_STATE, dev->info->name);
+        error_printf_unless_qmp("Note: device may simply lack complete qdev "
+                                "conversion\n");
+        return -1;
+    }
+
+    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s, 'version': %d }",
+                                   dev->info->name, dev->id ? : "",
+                                   vmsd->version_id);
+    qlist = qlist_new();
+    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
+    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
+
+    return 0;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index b27d33b..447d13c 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -183,6 +183,8 @@ void do_info_qtree(Monitor *mon);
 void do_info_qdm(Monitor *mon);
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+void device_user_print(Monitor *mon, const QObject *data);
+int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 /*** qdev-properties.c ***/
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index eb257a6..f4c9ffd 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -734,6 +734,25 @@ Example:
 EQMP
 
     {
+        .name       = "device_show",
+        .args_type  = "path:Q,full:-f",
+        .params     = "device [-f]",
+        .help       = "show device state (specify -f for full buffer dumping)",
+        .user_print = device_user_print,
+        .mhandler.cmd_new = do_device_show,
+        .user_only  = true,
+    },
+
+STEXI
+@item device_show @var{path} [@code{-f}]
+
+Show state of device @var{path} in a human-readable form. @var{path} can be
+an unique id specified during device creation or a full path in the device
+tree (see @code{info qtree}). Buffers are cut after 16 bytes unless a full
+dump is requested via @code{-f}.
+ETEXI
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .params     = "index",
diff --git a/qerror.c b/qerror.c
index 034c7de..c50ff91 100644
--- a/qerror.c
+++ b/qerror.c
@@ -101,6 +101,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' has no child bus",
     },
     {
+        .error_fmt = QERR_DEVICE_NO_STATE,
+        .desc      = "No state available for device '%(device)'",
+    },
+    {
         .error_fmt = QERR_DUPLICATE_ID,
         .desc      = "Duplicate ID '%(id)' for %(object)",
     },
diff --git a/qerror.h b/qerror.h
index c98c61a..e5de6dd 100644
--- a/qerror.h
+++ b/qerror.h
@@ -91,6 +91,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NO_BUS \
     "{ 'class': 'DeviceNoBus', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NO_STATE \
+    "{ 'class': 'DeviceNoState', 'data': { 'device': %s } }"
+
 #define QERR_DUPLICATE_ID \
     "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
 
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 15/17] QMP: Teach basic capability negotiation to python example

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As sending "qmp_capabilities" on session start became mandatory, both
python examples were broken.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 QMP/qmp-shell |    1 +
 QMP/vm-info   |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-shell b/QMP/qmp-shell
index f89b9af..a5b72d1 100755
--- a/QMP/qmp-shell
+++ b/QMP/qmp-shell
@@ -42,6 +42,7 @@ def main():
 
     qemu = qmp.QEMUMonitorProtocol(argv[1])
     qemu.connect()
+    qemu.send("qmp_capabilities")
 
     print 'Connected!'
 
diff --git a/QMP/vm-info b/QMP/vm-info
index b150d82..d29e7f5 100755
--- a/QMP/vm-info
+++ b/QMP/vm-info
@@ -24,6 +24,7 @@ def main():
 
     qemu = qmp.QEMUMonitorProtocol(argv[1])
     qemu.connect()
+    qemu.send("qmp_capabilities")
 
     for cmd in [ 'version', 'hpet', 'kvm', 'status', 'uuid', 'balloon' ]:
         print cmd + ': ' + str(qemu.send('query-' + cmd))
-- 
1.6.0.2

[Qemu-devel] [PATCH v3 16/17] QMP: Fix python helper /wrt long return strings

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Remove the arbitrary limitation of 1024 characters per return string and
read complete lines instead. Required for device_show.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 QMP/qmp.py |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/QMP/qmp.py b/QMP/qmp.py
index d9da603..4062f84 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -63,10 +63,14 @@ class QEMUMonitorProtocol:
 
     def __json_read(self):
         try:
-            return json.loads(self.sock.recv(1024))
+            while True:
+                line = json.loads(self.sockfile.readline())
+                if not 'event' in line:
+                    return line
         except ValueError:
             return
 
     def __init__(self, filename):
         self.filename = filename
         self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        self.sockfile = self.sock.makefile()
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH v3 16/17] QMP: Fix python helper /wrt long return strings

[Luiz Capitulino]

On Sun, 23 May 2010 12:59:29 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Remove the arbitrary limitation of 1024 characters per return string and
> read complete lines instead. Required for device_show.

 Thanks for both fixes, I have started working on a better version of this
script that mimics better the user monitor but it's only half done.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  QMP/qmp.py |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/QMP/qmp.py b/QMP/qmp.py
> index d9da603..4062f84 100644
> --- a/QMP/qmp.py
> +++ b/QMP/qmp.py
> @@ -63,10 +63,14 @@ class QEMUMonitorProtocol:
>  
>      def __json_read(self):
>          try:
> -            return json.loads(self.sock.recv(1024))
> +            while True:
> +                line = json.loads(self.sockfile.readline())
> +                if not 'event' in line:
> +                    return line
>          except ValueError:
>              return
>  
>      def __init__(self, filename):
>          self.filename = filename
>          self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> +        self.sockfile = self.sock.makefile()

[Qemu-devel] [PATCH v3 17/17] QMP: Add support for buffer class to qmp python helper

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This demonstrates the conversion of QMP buffer objects and does some
minimalistic pretty-printing.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 QMP/qmp.py |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/QMP/qmp.py b/QMP/qmp.py
index 4062f84..4650918 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -8,7 +8,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 
-import socket, json
+import socket, json, binascii
 
 class QMPError(Exception):
     pass
@@ -16,6 +16,18 @@ class QMPError(Exception):
 class QMPConnectError(QMPError):
     pass
 
+class QMPBuffer:
+    def __init__(self, data):
+        self.data = binascii.a2b_base64(data)
+
+    def __repr__(self):
+        str = ''
+        for i in range(0, len(self.data)):
+            if i > 0:
+                str += ' '
+            str += binascii.b2a_hex(self.data[i])
+        return str
+
 class QEMUMonitorProtocol:
     def connect(self):
         self.sock.connect(self.filename)
@@ -61,10 +73,19 @@ class QEMUMonitorProtocol:
         # the Server won't read our input
         self.sock.send(json.dumps(cmd) + ' ')
 
+    def __json_obj_hook(self, dct):
+        if '__class__' in dct:
+            if dct['__class__'] == 'buffer':
+                return QMPBuffer(dct['data'])
+            else:
+                return
+        return dct
+
     def __json_read(self):
         try:
             while True:
-                line = json.loads(self.sockfile.readline())
+                line = json.loads(self.sockfile.readline(),
+                                  object_hook=self.__json_obj_hook)
                 if not 'event' in line:
                     return line
         except ValueError:
-- 
1.6.0.2