[Qemu-devel] [PATCH v4 00/23] qdev path reworks & basic device state visualization

[Qemu-devel] [PATCH v4 00/23] qdev path reworks & basic device state visualization

[Jan Kiszka]

This is v4 of this series. Besides small fixes, it's main focus is on
the groundwork for the visualization command: qdev path usability.

The significant changes are:
 - drop many of the inconsistent or ambiguous qtree abbreviations
 - devices can now be address via unique ID (no '/' allowed here) or an
   absolute path (must start with '/')
 - buses remain addressable via their ambiguous local name (mostly to
   avoid libvirt breakages) or their absolute path
 - the per-bus instance numbers introduced in this series are now
   printed consistently
 - monitor command completion inside option lists is added
   (allows to expand "device_add ...,bus=" and "drive=")
 - HMP commands can now have their own argument set (not yet urgently
   needed for device_show but likely already useful for other commands)

Git url as usual:

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

Thanks once again for comments. Hope we can soon agree on a mergable
version for 0.13.

Jan Kiszka (23):
  qdev: Rework qtree path abbreviations
  qdev: Restrict direct bus addressing via its name
  qdev: Drop ID matching from qtree paths
  qdev: Allow device addressing via 'driver.instance'
  qdev: Convert device and bus lists to QTAILQ
  qdev: Push QMP mode checks into qbus_list_bus/dev
  qdev: Allow device specification by qtree path for device_del
  qdev: Introduce qdev_iterate_recursive
  monitor: Fix leakage during completion processing
  monitor: Fix command completion vs. boolean switches
  monitor: Add completion support for option lists
  monitor: Add completion for qdev paths
  monitor: Allow to specify HMP-specifc command arguments
  monitor: return length of printed string via monitor_[v]printf
  monitor: Establish cmd flags and convert the async tag
  monitor: Allow to exclude commands from QMP
  Add base64 encoder/decoder
  QMP: Reserve namespace for complex object classes
  QMP: Add QBuffer
  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 |   13 ++-
 hw/acpi_piix4.c          |    2 +-
 hw/hw.h                  |    2 +
 hw/i2c.c                 |    2 +-
 hw/pci-hotplug.c         |    2 +-
 hw/qdev.c                |  488 ++++++++++++++++++++++++++++++++++++++--------
 hw/qdev.h                |   16 ++-
 hw/ssi.c                 |    6 +-
 monitor.c                |  258 +++++++++++++++++++++----
 monitor.h                |    8 +-
 qbuffer.c                |  116 +++++++++++
 qbuffer.h                |   33 +++
 qemu-monitor.hx          |   34 +++-
 qemu-tool.c              |    6 +-
 qerror.c                 |    8 +-
 qerror.h                 |    6 +-
 qjson.c                  |   15 ++
 qobject.h                |    1 +
 28 files changed, 1315 insertions(+), 160 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 v4 01/23] qdev: Rework qtree path abbreviations

[Jan Kiszka]

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

Path abbreviations suffer from the inconsistency that bus names can only
be omitted at the end of a path. Drop this special rule, and also remove
the now obsolete QERR_DEVICE_MULTIPLE_BUSSES.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   22 ++++------------------
 qerror.c  |    4 ----
 qerror.h  |    3 ---
 3 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 61f999c..7c4f039 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -608,32 +608,18 @@ static BusState *qbus_find(const char *path)
             }
             return NULL;
         }
+        if (dev->num_child_bus == 0) {
+            qerror_report(QERR_DEVICE_NO_BUS, elem);
+            return NULL;
+        }
 
         assert(path[pos] == '/' || !path[pos]);
         while (path[pos] == '/') {
             pos++;
         }
-        if (path[pos] == '\0') {
-            /* last specified element is a device.  If it has exactly
-             * one child bus accept it nevertheless */
-            switch (dev->num_child_bus) {
-            case 0:
-                qerror_report(QERR_DEVICE_NO_BUS, elem);
-                return NULL;
-            case 1:
-                return QLIST_FIRST(&dev->child_bus);
-            default:
-                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
-                if (!monitor_cur_is_qmp()) {
-                    qbus_list_bus(dev);
-                }
-                return NULL;
-            }
-        }
 
         /* find bus */
         if (sscanf(path+pos, "%127[^/]%n", elem, &len) != 1) {
-            assert(0);
             elem[0] = len = 0;
         }
         pos += len;
diff --git a/qerror.c b/qerror.c
index 44d0bf8..786b5dc 100644
--- a/qerror.c
+++ b/qerror.c
@@ -77,10 +77,6 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is locked",
     },
     {
-        .error_fmt = QERR_DEVICE_MULTIPLE_BUSSES,
-        .desc      = "Device '%(device)' has multiple child busses",
-    },
-    {
         .error_fmt = QERR_DEVICE_NOT_ACTIVE,
         .desc      = "Device '%(device)' has not been activated by the guest",
     },
diff --git a/qerror.h b/qerror.h
index 77ae574..88474fb 100644
--- a/qerror.h
+++ b/qerror.h
@@ -73,9 +73,6 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
 
-#define QERR_DEVICE_MULTIPLE_BUSSES \
-    "{ 'class': 'DeviceMultipleBusses', 'data': { 'device': %s } }"
-
 #define QERR_DEVICE_NOT_ACTIVE \
     "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"
 
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 02/23] qdev: Restrict direct bus addressing via its name

[Jan Kiszka]

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

We allow to address a bus only using its local name. This is ambiguous
but unfortunately so handy that people (specifically libvirt) will
likely complain if bus=pci.0 needs to be replaced with
bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
drop at least support for starting a qtree walks with an abbreviated bus
name.

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

diff --git a/hw/qdev.c b/hw/qdev.c
index 7c4f039..c272c51 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -564,25 +564,17 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 static BusState *qbus_find(const char *path)
 {
     DeviceState *dev;
-    BusState *bus;
+    BusState *bus = main_system_bus;
     char elem[128];
-    int pos, len;
+    int len, pos = 0;
 
-    /* find start element */
-    if (path[0] == '/') {
-        bus = main_system_bus;
-        pos = 0;
-    } else {
-        if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
-            assert(!path[0]);
-            elem[0] = len = 0;
-        }
-        bus = qbus_find_recursive(main_system_bus, elem, NULL);
+    /* search for bus name recursively if path is not absolute */
+    if (path[0] != '/') {
+        bus = qbus_find_recursive(bus, path, NULL);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
-            return NULL;
+            qerror_report(QERR_BUS_NOT_FOUND, path);
         }
-        pos = len;
+        return bus;
     }
 
     for (;;) {
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 03/23] qdev: Drop ID matching from qtree paths

[Jan Kiszka]

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

Device IDs may conflict with device names or aliases. From now on we
only accept them outside qtree paths. This also makes dumping IDs in
qbus_list_dev/bus obsolete.

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

diff --git a/hw/qdev.c b/hw/qdev.c
index c272c51..aa25155 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -497,8 +497,7 @@ static void qbus_list_bus(DeviceState *dev)
     BusState *child;
     const char *sep = " ";
 
-    error_printf("child busses at \"%s\":",
-                 dev->id ? dev->id : dev->info->name);
+    error_printf("child busses at \"%s\":", dev->info->name);
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
         sep = ", ";
@@ -514,8 +513,6 @@ static void qbus_list_dev(BusState *bus)
     error_printf("devices at \"%s\":", bus->name);
     QLIST_FOREACH(dev, &bus->children, sibling) {
         error_printf("%s\"%s\"", sep, dev->info->name);
-        if (dev->id)
-            error_printf("/\"%s\"", dev->id);
         sep = ", ";
     }
     error_printf("\n");
@@ -539,15 +536,10 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
 
     /*
      * try to match in order:
-     *   (1) instance id, if present
-     *   (2) driver name
-     *   (3) driver alias, if present
+     *   (1) driver name
+     *   (2) driver alias, if present
      */
-    QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->id  &&  strcmp(dev->id, elem) == 0) {
-            return dev;
-        }
-    }
+
     QLIST_FOREACH(dev, &bus->children, sibling) {
         if (strcmp(dev->info->name, elem) == 0) {
             return dev;
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 04/23] 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'. Attach this name extension
whenever an instantiated device is printed.

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

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index f252c8e..58f2630 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
@@ -20,6 +20,17 @@ 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.
 
+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]
+
+The instance number counts devices managed by the same driver on a
+specifc bus. It 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 aa25155..f4ae4a6 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -492,12 +492,29 @@ static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     return NULL;
 }
 
+static int qdev_instance_no(DeviceState *dev)
+{
+    struct DeviceState *sibling;
+    int instance = 0;
+
+    QLIST_FOREACH(sibling, &dev->parent_bus->children, sibling) {
+        if (sibling->info == dev->info) {
+            if (sibling == dev) {
+                break;
+            }
+            instance++;
+        }
+    }
+    return instance;
+}
+
 static void qbus_list_bus(DeviceState *dev)
 {
     BusState *child;
     const char *sep = " ";
 
-    error_printf("child busses at \"%s\":", dev->info->name);
+    error_printf("child busses at \"%s.%d\":",
+                 dev->info->name, qdev_instance_no(dev));
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
         sep = ", ";
@@ -512,7 +529,8 @@ static void qbus_list_dev(BusState *bus)
 
     error_printf("devices at \"%s\":", bus->name);
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        error_printf("%s\"%s\"", sep, dev->info->name);
+        error_printf("%s\"%s.%d\"", sep, dev->info->name,
+                     qdev_instance_no(dev));
         sep = ", ";
     }
     error_printf("\n");
@@ -530,23 +548,35 @@ 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) driver name
-     *   (2) driver alias, if present
+     *   (1) driver name [.instance]
+     *   (2) driver alias [.instance], if present
      */
 
+    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;
         }
     }
@@ -710,8 +740,8 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
 static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     BusState *child;
-    qdev_printf("dev: %s, id \"%s\"\n", dev->info->name,
-                dev->id ? dev->id : "");
+    qdev_printf("dev: %s.%d, id \"%s\"\n", dev->info->name,
+                qdev_instance_no(dev), dev->id ? dev->id : "");
     indent += 2;
     if (dev->num_gpio_in) {
         qdev_printf("gpio-in %d\n", dev->num_gpio_in);
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 05/23] 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        |   41 +++++++++++++++++++++--------------------
 hw/qdev.h        |    8 ++++----
 hw/ssi.c         |    6 +++---
 6 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 8d1a628..38e8289 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -564,7 +564,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 c39e640..2793269 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -74,7 +74,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 f4ae4a6..25f6e62 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;
@@ -338,7 +339,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)
@@ -349,7 +350,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);
     for (prop = dev->info->props; prop && prop->name; prop++) {
         if (prop->info->free) {
             prop->info->free(dev, prop);
@@ -438,7 +439,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;
         }
@@ -463,8 +464,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;
@@ -479,10 +480,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;
@@ -497,7 +498,7 @@ static int qdev_instance_no(DeviceState *dev)
     struct DeviceState *sibling;
     int instance = 0;
 
-    QLIST_FOREACH(sibling, &dev->parent_bus->children, sibling) {
+    QTAILQ_FOREACH(sibling, &dev->parent_bus->children, sibling) {
         if (sibling->info == dev->info) {
             if (sibling == dev) {
                 break;
@@ -515,7 +516,7 @@ static void qbus_list_bus(DeviceState *dev)
 
     error_printf("child busses at \"%s.%d\":",
                  dev->info->name, qdev_instance_no(dev));
-    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+    QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
         sep = ", ";
     }
@@ -528,7 +529,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.%d\"", sep, dev->info->name,
                      qdev_instance_no(dev));
         sep = ", ";
@@ -540,7 +541,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;
         }
@@ -567,14 +568,14 @@ 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;
@@ -677,9 +678,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++;
     }
 
@@ -699,11 +700,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--;
     }
     qemu_free((void*)bus->name);
@@ -753,7 +754,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);
     }
 }
@@ -765,7 +766,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 be5ad67..170a63a 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 v4 06/23] 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 |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 25f6e62..ac450cf 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -514,6 +514,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.%d\":",
                  dev->info->name, qdev_instance_no(dev));
     QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
@@ -528,6 +531,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.%d\"", sep, dev->info->name,
@@ -618,9 +624,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;
         }
         if (dev->num_child_bus == 0) {
@@ -641,9 +645,7 @@ static BusState *qbus_find(const char *path)
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
             qerror_report(QERR_BUS_NOT_FOUND, elem);
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_bus(dev);
-            }
+            qbus_list_bus(dev);
             return NULL;
         }
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 07/23] 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       |   75 ++++++++++++++++++++++++++++++++++++++++++++-----------
 qemu-monitor.hx |   10 +++---
 2 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index ac450cf..2d1d171 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(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(path, true);
         if (!bus) {
             return NULL;
         }
@@ -475,7 +475,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;
@@ -484,7 +484,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;
             }
@@ -590,7 +590,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
     return NULL;
 }
 
-static BusState *qbus_find(const char *path)
+static BusState *qbus_find(const char *path, bool report_errors)
 {
     DeviceState *dev;
     BusState *bus = main_system_bus;
@@ -600,7 +600,7 @@ static BusState *qbus_find(const char *path)
     /* search for bus name recursively if path is not absolute */
     if (path[0] != '/') {
         bus = qbus_find_recursive(bus, path, NULL);
-        if (!bus) {
+        if (!bus && report_errors) {
             qerror_report(QERR_BUS_NOT_FOUND, path);
         }
         return bus;
@@ -623,12 +623,16 @@ 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;
         }
         if (dev->num_child_bus == 0) {
-            qerror_report(QERR_DEVICE_NO_BUS, elem);
+            if (report_errors) {
+                qerror_report(QERR_DEVICE_NO_BUS, elem);
+            }
             return NULL;
         }
 
@@ -644,13 +648,55 @@ static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!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)
+{
+    const char *dev_name;
+    DeviceState *dev;
+    char *bus_path;
+    BusState *bus;
+
+    /* search for unique ID recursively if path is not absolute */
+    if (path[0] != '/') {
+        dev = qdev_find_id_recursive(main_system_bus, path);
+        if (!dev) {
+            qerror_report(QERR_DEVICE_NOT_FOUND, path);
+        }
+        return dev;
+    }
+
+    dev_name = strrchr(path, '/') + 1;
+
+    bus_path = qemu_strdup(path);
+    bus_path[dev_name - path] = 0;
+
+    bus = qbus_find(bus_path, false);
+    qemu_free(bus_path);
+    if (!bus) {
+        /* retry with full path to generate correct error message */
+        bus = qbus_find(path, true);
+        if (!bus) {
+            return NULL;
+        }
+        dev_name = "";
+    }
+
+    dev = qbus_find_dev(bus, dev_name);
+    if (!dev) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
+        qbus_list_dev(bus);
+    }
+    return dev;
+}
+
 void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name)
 {
@@ -810,12 +856,11 @@ 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, "device");
     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) {
         return -1;
     }
     return qdev_unplug(dev);
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f62b94..0ea0555 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -703,7 +703,7 @@ EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "id:s",
+        .args_type  = "device: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{device}
 @findex device_del
 
-Remove device @var{id}.
+Remove @var{device}, specified via its qtree path or unique ID.
 ETEXI
 SQMP
 device_del
@@ -724,11 +724,11 @@ Remove a device.
 
 Arguments:
 
-- "id": the device's ID (json-string)
+- "device": the device's qtree path or unique ID (json-string)
 
 Example:
 
--> { "execute": "device_del", "arguments": { "id": "net1" } }
+-> { "execute": "device_del", "arguments": { "device": "net1" } }
 <- { "return": {} }
 
 EQMP
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 08/23] qdev: Introduce qdev_iterate_recursive

[Jan Kiszka]

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

Add qdev_iterate_recursive to walk the complete qtree invoking a
callback for each device. Use this service to implement
qdev_find_id_recursive.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c |   29 +++++++++++++++++++++++++----
 hw/qdev.h |    3 +++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 2d1d171..466d8d5 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -475,16 +475,22 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
+void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
+                             void *opaque)
 {
     DeviceState *dev, *ret;
     BusState *child;
 
+    if (!bus) {
+        bus = main_system_bus;
+    }
     QTAILQ_FOREACH(dev, &bus->children, sibling) {
-        if (dev->id && strcmp(dev->id, id) == 0)
-            return dev;
+        ret = callback(dev, opaque);
+        if (ret) {
+            return ret;
+        }
         QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_id_recursive(child, id);
+            ret = qdev_iterate_recursive(child, callback, opaque);
             if (ret) {
                 return ret;
             }
@@ -493,6 +499,21 @@ static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
     return NULL;
 }
 
+static void *find_id_callback(DeviceState *dev, void *opaque)
+{
+    const char *id = opaque;
+
+    if (dev->id && strcmp(dev->id, id) == 0) {
+        return dev;
+    }
+    return NULL;
+}
+
+static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
+{
+    return qdev_iterate_recursive(bus, find_id_callback, (void *)id);
+}
+
 static int qdev_instance_no(DeviceState *dev)
 {
     struct DeviceState *sibling;
diff --git a/hw/qdev.h b/hw/qdev.h
index 170a63a..111c876 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -134,6 +134,7 @@ BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 typedef int (*qdev_initfn)(DeviceState *dev, DeviceInfo *info);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
+typedef void *(*qdev_iteratefn)(DeviceState *dev, void *opaque);
 
 struct DeviceInfo {
     const char *name;
@@ -168,6 +169,8 @@ 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);
+void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
+                             void *opaque);
 
 /*** BUS API. ***/
 
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 09/23] monitor: Fix leakage during completion processing

[Jan Kiszka]

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

Given too many arguments or an invalid command, we were leaking the
duplicated argument strings.

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

diff --git a/monitor.c b/monitor.c
index 05a7ed1..242aee6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3877,8 +3877,9 @@ static void monitor_find_completion(const char *cmdline)
        next arg */
     len = strlen(cmdline);
     if (len > 0 && qemu_isspace(cmdline[len - 1])) {
-        if (nb_args >= MAX_ARGS)
-            return;
+        if (nb_args >= MAX_ARGS) {
+            goto cleanup;
+        }
         args[nb_args++] = qemu_strdup("");
     }
     if (nb_args <= 1) {
@@ -3893,12 +3894,15 @@ static void monitor_find_completion(const char *cmdline)
         }
     } else {
         /* find the command */
-        for(cmd = mon_cmds; cmd->name != NULL; cmd++) {
-            if (compare_cmd(args[0], cmd->name))
-                goto found;
+        for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
+            if (compare_cmd(args[0], cmd->name)) {
+                break;
+            }
         }
-        return;
-    found:
+        if (!cmd->name) {
+            goto cleanup;
+        }
+
         ptype = next_arg_type(cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
@@ -3948,8 +3952,11 @@ static void monitor_find_completion(const char *cmdline)
             break;
         }
     }
-    for(i = 0; i < nb_args; i++)
+
+cleanup:
+    for (i = 0; i < nb_args; i++) {
         qemu_free(args[i]);
+    }
 }
 
 static int monitor_can_read(void *opaque)
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 10/23] monitor: Fix command completion vs. boolean switches

[Jan Kiszka]

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

We now have to move forward to the next argument type via next_arg_type.
This patch fixes completion for 'eject' and maybe also other commands.

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

diff --git a/monitor.c b/monitor.c
index 242aee6..c1006b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3913,7 +3913,7 @@ static void monitor_find_completion(const char *cmdline)
         }
         str = args[nb_args - 1];
         if (*ptype == '-' && ptype[1] != '\0') {
-            ptype += 2;
+            ptype = next_arg_type(ptype);
         }
         switch(*ptype) {
         case 'F':
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Jan Kiszka]

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

This enables command line completion inside option strings. A list of
expected key names and their completion type can be appended to the 'O'
inside parentheses ('O(key:type,...)'). The first use case is block
device completion for the 'drive' option of 'device_add'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-monitor.hx |    2 +-
 2 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index c1006b4..3e0d862 100644
--- a/monitor.c
+++ b/monitor.c
@@ -68,6 +68,9 @@
  * 'O'          option string of the form NAME=VALUE,...
  *              parsed according to QemuOptsList given by its name
  *              Example: 'device:O' uses qemu_device_opts.
+ *              Command completion for specific keys can be requested via
+ *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
+ *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
  *              Restriction: only lists with empty desc are supported
  *              TODO lift the restriction
  * 'i'          32 bit integer
@@ -3353,6 +3356,11 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 QemuOptsList *opts_list;
                 QemuOpts *opts;
 
+                if (*typestr == '(') {
+                    while (*typestr++ != ')') {
+                        assert(*typestr != '\0');
+                    }
+                }
                 opts_list = qemu_find_opts(key);
                 if (!opts_list || opts_list->desc->name) {
                     goto bad_type;
@@ -3857,12 +3865,30 @@ static const char *next_arg_type(const char *typestr)
     return (p != NULL ? ++p : typestr);
 }
 
+static bool process_completion_type(char type, const char *str)
+{
+    switch(type) {
+    case 'F':
+        /* file completion */
+        readline_set_completion_index(cur_mon->rs, strlen(str));
+        file_completion(str);
+        return true;
+    case 'B':
+        /* block device name completion */
+        readline_set_completion_index(cur_mon->rs, strlen(str));
+        bdrv_iterate(block_completion_it, (void *)str);
+        return true;
+    default:
+        return false;
+    }
+}
+
 static void monitor_find_completion(const char *cmdline)
 {
     const char *cmdname;
     char *args[MAX_ARGS];
     int nb_args, i, len;
-    const char *ptype, *str;
+    const char *ptype, *str, *opt, *sep;
     const mon_cmd_t *cmd;
     const KeyDef *key;
 
@@ -3915,16 +3941,31 @@ static void monitor_find_completion(const char *cmdline)
         if (*ptype == '-' && ptype[1] != '\0') {
             ptype = next_arg_type(ptype);
         }
+        if (process_completion_type(*ptype, str)) {
+            goto cleanup;
+        }
         switch(*ptype) {
-        case 'F':
-            /* file completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            file_completion(str);
-            break;
-        case 'B':
-            /* block device name completion */
-            readline_set_completion_index(cur_mon->rs, strlen(str));
-            bdrv_iterate(block_completion_it, (void *)str);
+        case 'O':
+            sep = strrchr(str, ',');
+            opt = sep ? sep + 1 : str;
+            sep = strchr(opt, '=');
+            if (!sep) {
+                break;
+            }
+            len = sep - opt;
+            str = sep + 1;
+            ptype += 2;
+            while (*ptype != ')') {
+                if (strlen(ptype) > len+1 && ptype[len] == ':' &&
+                    strncmp(ptype, opt, len) == 0) {
+                    process_completion_type(ptype[len+1], str);
+                }
+                while (*ptype++ != ',') {
+                    if (*ptype == ')') {
+                        break;
+                    }
+                }
+            }
             break;
         case 's':
             /* XXX: more generic ? */
@@ -3934,7 +3975,7 @@ static void monitor_find_completion(const char *cmdline)
                     cmd_completion(str, cmd->name);
                 }
             } else if (!strcmp(cmd->name, "sendkey")) {
-                char *sep = strrchr(str, '-');
+                sep = strrchr(str, '-');
                 if (sep)
                     str = sep + 1;
                 readline_set_completion_index(cur_mon->rs, strlen(str));
@@ -4114,6 +4155,11 @@ static int monitor_check_qmp_args(const mon_cmd_t *cmd, QDict *args)
                 cmd_args.flag = *p++;
                 cmd_args.optional = 1;
             } else if (cmd_args.type == 'O') {
+                if (*p == '(') {
+                    while (*p++ != ')') {
+                        assert(*p != '\0');
+                    }
+                }
                 opts_list = qemu_find_opts(qstring_get_str(cmd_args.name));
                 assert(opts_list);
             } else if (*p == '?') {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 0ea0555..b5d0f6d 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -660,7 +660,7 @@ ETEXI
 
     {
         .name       = "device_add",
-        .args_type  = "device:O",
+        .args_type  = "device:O(drive:B)",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
         .user_print = monitor_user_noop,
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 12/23] 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 are device_add ('bus' option) and device_del.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/qdev.c       |   19 +++++----
 hw/qdev.h       |    3 +
 monitor.c       |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-monitor.hx |    4 +-
 4 files changed, 129 insertions(+), 12 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 466d8d5..dbc5b10 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -39,7 +39,6 @@ DeviceInfo *device_info_list;
 
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const BusInfo *info);
-static BusState *qbus_find(const char *path, bool report_errors);
 
 /* Register a new device type.  */
 void qdev_register(DeviceInfo *info)
@@ -514,7 +513,7 @@ static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
     return qdev_iterate_recursive(bus, find_id_callback, (void *)id);
 }
 
-static int qdev_instance_no(DeviceState *dev)
+int qdev_instance_no(DeviceState *dev)
 {
     struct DeviceState *sibling;
     int instance = 0;
@@ -611,7 +610,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
     return NULL;
 }
 
-static BusState *qbus_find(const char *path, bool report_errors)
+BusState *qbus_find(const char *path, bool report_errors)
 {
     DeviceState *dev;
     BusState *bus = main_system_bus;
@@ -678,7 +677,7 @@ static BusState *qbus_find(const char *path, bool report_errors)
     }
 }
 
-static DeviceState *qdev_find(const char *path)
+DeviceState *qdev_find(const char *path, bool report_errors)
 {
     const char *dev_name;
     DeviceState *dev;
@@ -688,7 +687,7 @@ static DeviceState *qdev_find(const char *path)
     /* search for unique ID recursively if path is not absolute */
     if (path[0] != '/') {
         dev = qdev_find_id_recursive(main_system_bus, path);
-        if (!dev) {
+        if (!dev && report_errors) {
             qerror_report(QERR_DEVICE_NOT_FOUND, path);
         }
         return dev;
@@ -702,8 +701,10 @@ static DeviceState *qdev_find(const char *path)
     bus = qbus_find(bus_path, false);
     qemu_free(bus_path);
     if (!bus) {
-        /* retry with full path to generate correct error message */
-        bus = qbus_find(path, true);
+        if (report_errors) {
+            /* retry with full path to generate correct error message */
+            bus = qbus_find(path, report_errors);
+        }
         if (!bus) {
             return NULL;
         }
@@ -711,7 +712,7 @@ static DeviceState *qdev_find(const char *path)
     }
 
     dev = qbus_find_dev(bus, dev_name);
-    if (!dev) {
+    if (!dev && report_errors) {
         qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
         qbus_list_dev(bus);
     }
@@ -880,7 +881,7 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *path = qdict_get_str(qdict, "device");
     DeviceState *dev;
 
-    dev = qdev_find(path);
+    dev = qdev_find(path, true);
     if (!dev) {
         return -1;
     }
diff --git a/hw/qdev.h b/hw/qdev.h
index 111c876..59159a0 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -171,6 +171,8 @@ CharDriverState *qdev_init_chardev(DeviceState *dev);
 BusState *qdev_get_parent_bus(DeviceState *dev);
 void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
                              void *opaque);
+DeviceState *qdev_find(const char *path, bool report_errors);
+int qdev_instance_no(DeviceState *dev);
 
 /*** BUS API. ***/
 
@@ -178,6 +180,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, bool report_errors);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
diff --git a/monitor.c b/monitor.c
index 3e0d862..f535c56 100644
--- a/monitor.c
+++ b/monitor.c
@@ -64,12 +64,14 @@
  *
  * 'F'          filename
  * 'B'          block device name
+ * 'q'          qdev bus path
+ * '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
  *              Example: 'device:O' uses qemu_device_opts.
  *              Command completion for specific keys can be requested via
- *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
+ *              appending '(NAME:TYPE,...)' with 'F', 'B', 'q', 'Q' as type.
  *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
  *              Restriction: only lists with empty desc are supported
  *              TODO lift the restriction
@@ -3318,6 +3320,8 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
         switch(c) {
         case 'F':
         case 'B':
+        case 'q':
+        case 'Q':
         case 's':
             {
                 int ret;
@@ -3342,6 +3346,14 @@ 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 bus path 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;
@@ -3833,6 +3845,99 @@ 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 *inspect_qdev_id(DeviceState *dev, void *opaque)
+{
+    const char *input = opaque;
+
+    if (dev->id && strncmp(dev->id, input, strlen(input)) == 0) {
+        add_qdev_completion("", dev->id, false);
+    }
+    return NULL;
+}
+
+static void qdev_completion(const char *input, bool find_bus)
+{
+    size_t parent_len, name_len;
+    char *parent_path;
+    const char *p;
+    char *name;
+    DeviceState *dev;
+    BusState *bus;
+
+    p = strrchr(input, '/');
+    if (!p) {
+        if (*input == '\0') {
+            readline_add_completion(cur_mon->rs, "/");
+        }
+        if (!find_bus) {
+            qdev_iterate_recursive(NULL, inspect_qdev_id, (void *)input);
+        }
+        return;
+    }
+
+    p++;
+    parent_path = qemu_strndup(input, p - input);
+    bus = qbus_find(parent_path, false);
+
+    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_len = strlen(dev->info->name) + 16;
+            name = qemu_malloc(name_len);
+            snprintf(name, name_len, "%s.%d", dev->info->name,
+                     qdev_instance_no(dev));
+            if (strncmp(name, p, strlen(p)) == 0) {
+                if (!find_bus) {
+                    add_qdev_completion(parent_path, name, false);
+                }
+                if (!QTAILQ_EMPTY(&dev->child_bus)) {
+                    add_qdev_completion(parent_path, name, true);
+                }
+            }
+            qemu_free(name);
+        }
+    } else {
+        parent_path[strlen(parent_path) - 1] = 0;
+        dev = qdev_find(parent_path, false);
+        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)
@@ -3878,6 +3983,12 @@ static bool process_completion_type(char type, const char *str)
         readline_set_completion_index(cur_mon->rs, strlen(str));
         bdrv_iterate(block_completion_it, (void *)str);
         return true;
+    case 'q':
+    case 'Q':
+        /* qdev bus/device path completion */
+        readline_set_completion_index(cur_mon->rs, strlen(str));
+        qdev_completion(str, (type == 'q'));
+        return true;
     default:
         return false;
     }
@@ -4048,6 +4159,8 @@ static int check_arg(const CmdArgs *cmd_args, QDict *args)
     switch (cmd_args->type) {
         case 'F':
         case 'B':
+        case 'q':
+        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 b5d0f6d..0034fed 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -660,7 +660,7 @@ ETEXI
 
     {
         .name       = "device_add",
-        .args_type  = "device:O(drive:B)",
+        .args_type  = "device:O(bus:q,drive:B)",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
         .user_print = monitor_user_noop,
@@ -703,7 +703,7 @@ EQMP
 
     {
         .name       = "device_del",
-        .args_type  = "device:s",
+        .args_type  = "device:Q",
         .params     = "device",
         .help       = "remove device",
         .user_print = monitor_user_noop,
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 13/23] monitor: Allow to specify HMP-specifc command arguments

[Jan Kiszka]

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

As we may want to shrink or enhance the argument set used for monitor
command in HMP mode, add a separate, optional argument string for that
case. When an HMP request is parsed, this argument string, if available,
takes precedence over the standard string.

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

diff --git a/monitor.c b/monitor.c
index f535c56..7139c4e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -108,6 +108,7 @@ typedef struct mon_cmd_t {
     const char *params;
     const char *help;
     void (*user_print)(Monitor *mon, const QObject *data);
+    const char *user_args_type;
     union {
         void (*info)(Monitor *mon);
         void (*info_new)(Monitor *mon, QObject **ret_data);
@@ -3310,7 +3311,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
     }
 
     /* parse the parameters */
-    typestr = cmd->args_type;
+    typestr = cmd->user_args_type ? : cmd->args_type;
     for(;;) {
         typestr = key_get_info(typestr, &key);
         if (!typestr)
@@ -4040,7 +4041,7 @@ static void monitor_find_completion(const char *cmdline)
             goto cleanup;
         }
 
-        ptype = next_arg_type(cmd->args_type);
+        ptype = next_arg_type(cmd->user_args_type ? : cmd->args_type);
         for(i = 0; i < nb_args - 2; i++) {
             if (*ptype != '\0') {
                 ptype = next_arg_type(ptype);
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 14/23] 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 7139c4e..aa0bdd6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -263,29 +263,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

[Qemu-devel] [PATCH v4 15/23] monitor: Establish cmd flags and convert the async tag

[Jan Kiszka]

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

As we want to add more flags to monitor commands, convert the only so
far existing one accordingly.

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

diff --git a/monitor.c b/monitor.c
index aa0bdd6..2d3d70d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -118,7 +118,7 @@ typedef struct mon_cmd_t {
         int  (*cmd_async)(Monitor *mon, const QDict *params,
                           MonitorCompletion *cb, void *opaque);
     } mhandler;
-    int async;
+    int flags;
 } mon_cmd_t;
 
 /* file descriptors passed via SCM_RIGHTS */
@@ -340,7 +340,7 @@ static inline int monitor_handler_ported(const mon_cmd_t *cmd)
 
 static inline bool monitor_handler_is_async(const mon_cmd_t *cmd)
 {
-    return cmd->async != 0;
+    return cmd->flags & MONITOR_CMD_ASYNC;
 }
 
 static inline int monitor_has_error(const Monitor *mon)
@@ -2544,7 +2544,7 @@ static const mon_cmd_t info_cmds[] = {
         .help       = "show balloon information",
         .user_print = monitor_print_balloon,
         .mhandler.info_async = do_info_balloon,
-        .async      = 1,
+        .flags      = MONITOR_CMD_ASYNC,
     },
     {
         .name       = "qtree",
diff --git a/monitor.h b/monitor.h
index 32c0170..e3f0119 100644
--- a/monitor.h
+++ b/monitor.h
@@ -15,6 +15,9 @@ extern Monitor *default_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 
+/* flags for monitor commands */
+#define MONITOR_CMD_ASYNC       0x0001
+
 /* QMP events */
 typedef enum MonitorEvent {
     QEVENT_SHUTDOWN,
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 0034fed..2fe5ae8 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1287,7 +1287,7 @@ ETEXI
         .help       = "request VM to change its memory allocation (in MB)",
         .user_print = monitor_user_noop,
         .mhandler.cmd_async = do_balloon,
-        .async      = 1,
+        .flags      = MONITOR_CMD_ASYNC,
     },
 
 STEXI
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 16/23] 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 |   18 +++++++++++++++---
 monitor.h |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2d3d70d..9c0a91c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -343,6 +343,11 @@ static inline bool monitor_handler_is_async(const mon_cmd_t *cmd)
     return cmd->flags & MONITOR_CMD_ASYNC;
 }
 
+static inline bool monitor_cmd_user_only(const mon_cmd_t *cmd)
+{
+    return (cmd->flags & MONITOR_CMD_USER_ONLY);
+}
+
 static inline int monitor_has_error(const Monitor *mon)
 {
     return mon->error != NULL;
@@ -625,6 +630,11 @@ static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto help;
     }
 
+    if (monitor_ctrl_mode(mon) && monitor_cmd_user_only(cmd)) {
+        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);
@@ -722,13 +732,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) && !monitor_cmd_user_only(cmd) &&
+            !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) && !monitor_cmd_user_only(cmd)) {
             char buf[128];
             snprintf(buf, sizeof(buf), "query-%s", cmd->name);
             qlist_append_obj(cmd_list, get_cmd_dict(buf));
@@ -4376,7 +4387,8 @@ 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)
+            || monitor_cmd_user_only(cmd)) {
             qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
             goto err_input;
         }
diff --git a/monitor.h b/monitor.h
index e3f0119..ec1fe42 100644
--- a/monitor.h
+++ b/monitor.h
@@ -17,6 +17,7 @@ extern Monitor *default_mon;
 
 /* flags for monitor commands */
 #define MONITOR_CMD_ASYNC       0x0001
+#define MONITOR_CMD_USER_ONLY   0x0002
 
 /* QMP events */
 typedef enum MonitorEvent {
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 17/23] 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 2bfb6d1..670b8a8 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 v4 18/23] 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] [PATCH v4 19/23] QMP: 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 221fbd8..2267261 100644
--- a/Makefile
+++ b/Makefile
@@ -146,14 +146,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 670b8a8..82bc63f 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 c0d8aa5..6957593 100755
--- a/configure
+++ b/configure
@@ -2008,7 +2008,7 @@ if test "$softmmu" = yes ; 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 e4ee433..2996707 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
 {
@@ -238,6 +240,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 d42386d..4ec932b 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 v4 20/23] 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       |  248 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h       |    2 +
 qemu-monitor.hx |   20 +++++
 qerror.c        |    4 +
 qerror.h        |    3 +
 6 files changed, 279 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index a49d866..11b52e3 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -298,6 +298,7 @@ enum VMStateFlags {
 
 typedef struct {
     const char *name;
+    const char *start_index;
     size_t offset;
     size_t size;
     size_t start;
@@ -412,6 +413,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 dbc5b10..1a84ff5 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;
 
@@ -887,3 +890,248 @@ 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;
+    int name_len;
+    char *name;
+
+    dev = qdev_find(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;
+    }
+
+    name_len = strlen(dev->info->name) + 16;
+    name = qemu_malloc(name_len);
+    snprintf(name, name_len, "%s.%d", dev->info->name, qdev_instance_no(dev));
+    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s, 'version': %d }",
+                                   name, dev->id ? : "", vmsd->version_id);
+    qemu_free(name);
+    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 59159a0..e8235b6 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -190,6 +190,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 2fe5ae8..1d43a3e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -734,6 +734,26 @@ Example:
 EQMP
 
     {
+        .name       = "device_show",
+        .args_type  = "path:Q",
+        .user_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,
+        .flags      = MONITOR_CMD_USER_ONLY,
+    },
+
+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 786b5dc..51e0a30 100644
--- a/qerror.c
+++ b/qerror.c
@@ -97,6 +97,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 88474fb..4f57d57 100644
--- a/qerror.h
+++ b/qerror.h
@@ -88,6 +88,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 v4 21/23] 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 8ebaeb3..be5b038 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', 'kvm', 'status', 'uuid', 'balloon' ]:
         print cmd + ': ' + str(qemu.send('query-' + cmd))
-- 
1.6.0.2

[Qemu-devel] [PATCH v4 22/23] 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] [PATCH v4 23/23] 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

Re: [Qemu-devel] [PATCH v4 01/23] qdev: Rework qtree path abbreviations

[Markus Armbruster]

[cc: kraxel]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Path abbreviations suffer from the inconsistency that bus names can only
> be omitted at the end of a path. Drop this special rule, and also remove
> the now obsolete QERR_DEVICE_MULTIPLE_BUSSES.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Affects only -device and device_add option bus.  I support this.

> ---
>  hw/qdev.c |   22 ++++------------------
>  qerror.c  |    4 ----
>  qerror.h  |    3 ---
>  3 files changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 61f999c..7c4f039 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -608,32 +608,18 @@ static BusState *qbus_find(const char *path)
>              }
>              return NULL;
>          }
> +        if (dev->num_child_bus == 0) {
> +            qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            return NULL;
> +        }

I'd kill this check as well.  The QERR_BUS_NOT_FOUND further down
suffices.

>  
>          assert(path[pos] == '/' || !path[pos]);
>          while (path[pos] == '/') {
>              pos++;
>          }
> -        if (path[pos] == '\0') {
> -            /* last specified element is a device.  If it has exactly
> -             * one child bus accept it nevertheless */
> -            switch (dev->num_child_bus) {
> -            case 0:
> -                qerror_report(QERR_DEVICE_NO_BUS, elem);
> -                return NULL;
> -            case 1:
> -                return QLIST_FIRST(&dev->child_bus);
> -            default:
> -                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
> -                if (!monitor_cur_is_qmp()) {
> -                    qbus_list_bus(dev);
> -                }
> -                return NULL;
> -            }
> -        }
>  
>          /* find bus */
>          if (sscanf(path+pos, "%127[^/]%n", elem, &len) != 1) {
> -            assert(0);
>              elem[0] = len = 0;
>          }
>          pos += len;
[...]

Re: [Qemu-devel] [PATCH v4 02/23] qdev: Restrict direct bus addressing via its name

[Markus Armbruster]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> We allow to address a bus only using its local name. This is ambiguous
> but unfortunately so handy that people (specifically libvirt) will
> likely complain if bus=pci.0 needs to be replaced with
> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
> drop at least support for starting a qtree walks with an abbreviated bus
> name.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Again, affects only -device and device_add option bus.

Before, an argument started either with '/' (path anchored at root) or a
bus name (path anchored at that bus).

Now, an argument started either with '/' (path anchored at root) or it
is a bus name.

If we decide to add sane relative paths to qdev, i.e. paths anchored at
unique ID, we get a slight anomaly here:

  bus=a        bus name, not a relative path
  bus=a/b      relative path anchored at node with unique ID a.

Works for me.

Re: [Qemu-devel] [PATCH v4 03/23] qdev: Drop ID matching from qtree paths

[Markus Armbruster]

[cc: kraxel]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Device IDs may conflict with device names or aliases. From now on we
> only accept them outside qtree paths. This also makes dumping IDs in
> qbus_list_dev/bus obsolete.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I don't like this at all.

1. Specific problem:

   With the current code, multiple devices with the same driver work
   only if I take care: addressing by driver name gets me only the
   first, so I better set suitable IDs.

   With your patch, multiple devices with the same driver don't work, no
   matter what I do.

2. General principle:

   When I set an ID, I want the system to accept that ID in all contexts
   where it makes sense.  Ambiguity created by badly chosen IDs is *my*
   problem.

Re: [Qemu-devel] [PATCH v4 04/23] qdev: Allow device addressing via 'driver.instance'

[Markus Armbruster]

[cc: kraxel]

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

> 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'. Attach this name extension
> whenever an instantiated device is printed.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

The instance number repairs the specific problem created by the previous
patch.  My objection to it on general principles stands.

That said, I'm neutral on the instance number feature.  Instance numbers
are not suitable where address stability is needed (see debate on
canonical addresses), but they might be convenient for interactive use.

There is some functional overlap to the [driver-name]@bus-address syntax
proposed elsewhere.  Especially visible for buses where the bus-address
is a small integer.  Could be confusing.

There is some conceptual overlap with the .NUMBER in default bus names
(see qbus_create_inplace()).  Instance numbers count for each name
independently.  Numbers in default bus names count for all names.  But I
think we agree the latter sucks.  Paul wants us to get rid of it.

> ---
>  docs/qdev-device-use.txt |   13 +++++++++++-
>  hw/qdev.c                |   48 +++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
> index f252c8e..58f2630 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
> @@ -20,6 +20,17 @@ 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.
>  
> +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]
> +
> +The instance number counts devices managed by the same driver on a
> +specifc bus. It is zero-based.
   ~~~~~~~ typo

> +
> +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 ===
[...]

Re: [Qemu-devel] [PATCH v4 01/23] qdev: Rework qtree path abbreviations

[Markus Armbruster]

Hmm, what happens if the path denotes a device instead of a bus?

qemu-system-x86_64: -device e1000,bus=/i440FX-pcihost: Bus '' not found

Not so nice.

Re: [Qemu-devel] [PATCH v4 07/23] qdev: Allow device specification by qtree path for device_del

[Markus Armbruster]

[cc: kraxel]
Jan Kiszka <jan.kiszka@web.de> writes:

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

I like accepting paths there.

Your previous decision to abolish ID in qdev paths makes plain ID a
special case here instead of a relative path that happens to be short.

> ---
>  hw/qdev.c       |   75 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  qemu-monitor.hx |   10 +++---
>  2 files changed, 65 insertions(+), 20 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index ac450cf..2d1d171 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(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(path, true);
>          if (!bus) {
>              return NULL;
>          }
> @@ -475,7 +475,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;
> @@ -484,7 +484,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;
>              }
> @@ -590,7 +590,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
>      return NULL;
>  }
>  
> -static BusState *qbus_find(const char *path)
> +static BusState *qbus_find(const char *path, bool report_errors)
>  {
>      DeviceState *dev;
>      BusState *bus = main_system_bus;
> @@ -600,7 +600,7 @@ static BusState *qbus_find(const char *path)
>      /* search for bus name recursively if path is not absolute */
>      if (path[0] != '/') {
>          bus = qbus_find_recursive(bus, path, NULL);
> -        if (!bus) {
> +        if (!bus && report_errors) {
>              qerror_report(QERR_BUS_NOT_FOUND, path);
>          }
>          return bus;
> @@ -623,12 +623,16 @@ 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;
>          }
>          if (dev->num_child_bus == 0) {
> -            qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            if (report_errors) {
> +                qerror_report(QERR_DEVICE_NO_BUS, elem);
> +            }
>              return NULL;
>          }
>  
> @@ -644,13 +648,55 @@ static BusState *qbus_find(const char *path)
>          pos += len;
>          bus = qbus_find_bus(dev, elem);
>          if (!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)
> +{
> +    const char *dev_name;
> +    DeviceState *dev;
> +    char *bus_path;
> +    BusState *bus;
> +
> +    /* search for unique ID recursively if path is not absolute */
> +    if (path[0] != '/') {
> +        dev = qdev_find_id_recursive(main_system_bus, path);
> +        if (!dev) {
> +            qerror_report(QERR_DEVICE_NOT_FOUND, path);
> +        }
> +        return dev;
> +    }
> +
> +    dev_name = strrchr(path, '/') + 1;
> +
> +    bus_path = qemu_strdup(path);
> +    bus_path[dev_name - path] = 0;
> +
> +    bus = qbus_find(bus_path, false);
> +    qemu_free(bus_path);
> +    if (!bus) {
> +        /* retry with full path to generate correct error message */
> +        bus = qbus_find(path, true);
> +        if (!bus) {
> +            return NULL;
> +        }
> +        dev_name = "";
> +    }

Awkward.

What happens here is you hack off the last path component (because it
must be a device), then resolve the rest as bus, suppressing errors.  If
it fails, you resolve the complete path as bus without error
suppression.  Since the first try failed, the second try must surely run
into the same failure before it reaches the last path component.  So it
doesn't matter that the whole path is supposed to be a device, not a
bus.

This leads to suboptimal error messages for arguments that are actually
buses, not devices.  For instance, /i440FX-pcihost/pci.0/piix3-ide/ide.0
will try to resolve /i440FX-pcihost/pci.0/piix3-ide as bus, fail, and
report it as "Bus '' not found." (I think).

I don't like separate qdev path resolution for buses and devices.  Just
resolve it, and if what you got is not of the kind you want, report
that.

> +
> +    dev = qbus_find_dev(bus, dev_name);
> +    if (!dev) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
> +        qbus_list_dev(bus);
> +    }
> +    return dev;
> +}
> +
>  void qbus_create_inplace(BusState *bus, BusInfo *info,
>                           DeviceState *parent, const char *name)
>  {
[...]

Re: [Qemu-devel] [PATCH v4 08/23] qdev: Introduce qdev_iterate_recursive

[Markus Armbruster]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Add qdev_iterate_recursive to walk the complete qtree invoking a
> callback for each device. Use this service to implement
> qdev_find_id_recursive.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/qdev.c |   29 +++++++++++++++++++++++++----
>  hw/qdev.h |    3 +++
>  2 files changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 2d1d171..466d8d5 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -475,16 +475,22 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>      return NULL;
>  }
>  
> -static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
> +void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
> +                             void *opaque)
>  {
>      DeviceState *dev, *ret;
>      BusState *child;
>  
> +    if (!bus) {
> +        bus = main_system_bus;
> +    }
>      QTAILQ_FOREACH(dev, &bus->children, sibling) {
> -        if (dev->id && strcmp(dev->id, id) == 0)
> -            return dev;
> +        ret = callback(dev, opaque);
> +        if (ret) {
> +            return ret;
> +        }
>          QTAILQ_FOREACH(child, &dev->child_bus, sibling) {
> -            ret = qdev_find_id_recursive(child, id);
> +            ret = qdev_iterate_recursive(child, callback, opaque);
>              if (ret) {
>                  return ret;
>              }

I'd call "iterate recursive" simply "walk".  It's common usage for
trees.

Except this isn't a walk or iteration, it's a search: the walk stops as
soon as the callback returns non-null.  Not obvious from the name,
therefore needs a comment.

[...]

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Markus Armbruster]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This enables command line completion inside option strings. A list of
> expected key names and their completion type can be appended to the 'O'
> inside parentheses ('O(key:type,...)'). The first use case is block
> device completion for the 'drive' option of 'device_add'.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  qemu-monitor.hx |    2 +-
>  2 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c1006b4..3e0d862 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -68,6 +68,9 @@
>   * 'O'          option string of the form NAME=VALUE,...
>   *              parsed according to QemuOptsList given by its name
>   *              Example: 'device:O' uses qemu_device_opts.
> + *              Command completion for specific keys can be requested via
> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>   *              Restriction: only lists with empty desc are supported
>   *              TODO lift the restriction
>   * 'i'          32 bit integer

Ugh.

Replacement of args_type by a proper data structure is long overdue.  We
keep piling features into that poor, hapless string.

Information on how to complete QemuOptsList options arguably belongs
into the option description, i.e. QemuOptDesc.

[...]

Re: [Qemu-devel] [PATCH v4 12/23] monitor: Add completion for qdev paths

[Markus Armbruster]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Implement monitor command line completion for device tree paths. The
> first user are device_add ('bus' option) and device_del.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/qdev.c       |   19 +++++----
>  hw/qdev.h       |    3 +
>  monitor.c       |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-monitor.hx |    4 +-
>  4 files changed, 129 insertions(+), 12 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 466d8d5..dbc5b10 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -39,7 +39,6 @@ DeviceInfo *device_info_list;
>  
>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
>                                       const BusInfo *info);
> -static BusState *qbus_find(const char *path, bool report_errors);
>  
>  /* Register a new device type.  */
>  void qdev_register(DeviceInfo *info)
> @@ -514,7 +513,7 @@ static DeviceState *qdev_find_id_recursive(BusState *bus, const char *id)
>      return qdev_iterate_recursive(bus, find_id_callback, (void *)id);
>  }
>  
> -static int qdev_instance_no(DeviceState *dev)
> +int qdev_instance_no(DeviceState *dev)
>  {
>      struct DeviceState *sibling;
>      int instance = 0;
> @@ -611,7 +610,7 @@ static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
>      return NULL;
>  }
>  
> -static BusState *qbus_find(const char *path, bool report_errors)
> +BusState *qbus_find(const char *path, bool report_errors)
>  {
>      DeviceState *dev;
>      BusState *bus = main_system_bus;
> @@ -678,7 +677,7 @@ static BusState *qbus_find(const char *path, bool report_errors)
>      }
>  }
>  
> -static DeviceState *qdev_find(const char *path)
> +DeviceState *qdev_find(const char *path, bool report_errors)
>  {
>      const char *dev_name;
>      DeviceState *dev;
> @@ -688,7 +687,7 @@ static DeviceState *qdev_find(const char *path)
>      /* search for unique ID recursively if path is not absolute */
>      if (path[0] != '/') {
>          dev = qdev_find_id_recursive(main_system_bus, path);
> -        if (!dev) {
> +        if (!dev && report_errors) {
>              qerror_report(QERR_DEVICE_NOT_FOUND, path);
>          }
>          return dev;
> @@ -702,8 +701,10 @@ static DeviceState *qdev_find(const char *path)
>      bus = qbus_find(bus_path, false);
>      qemu_free(bus_path);
>      if (!bus) {
> -        /* retry with full path to generate correct error message */
> -        bus = qbus_find(path, true);
> +        if (report_errors) {
> +            /* retry with full path to generate correct error message */
> +            bus = qbus_find(path, report_errors);
> +        }
>          if (!bus) {
>              return NULL;
>          }
> @@ -711,7 +712,7 @@ static DeviceState *qdev_find(const char *path)
>      }
>  
>      dev = qbus_find_dev(bus, dev_name);
> -    if (!dev) {
> +    if (!dev && report_errors) {
>          qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
>          qbus_list_dev(bus);
>      }
> @@ -880,7 +881,7 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      const char *path = qdict_get_str(qdict, "device");
>      DeviceState *dev;
>  
> -    dev = qdev_find(path);
> +    dev = qdev_find(path, true);
>      if (!dev) {
>          return -1;
>      }
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 111c876..59159a0 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -171,6 +171,8 @@ CharDriverState *qdev_init_chardev(DeviceState *dev);
>  BusState *qdev_get_parent_bus(DeviceState *dev);
>  void *qdev_iterate_recursive(BusState *bus, qdev_iteratefn callback,
>                               void *opaque);
> +DeviceState *qdev_find(const char *path, bool report_errors);
> +int qdev_instance_no(DeviceState *dev);
>  
>  /*** BUS API. ***/
>  
> @@ -178,6 +180,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, bool report_errors);
>  
>  #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
>  
> diff --git a/monitor.c b/monitor.c
> index 3e0d862..f535c56 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -64,12 +64,14 @@
>   *
>   * 'F'          filename
>   * 'B'          block device name
> + * 'q'          qdev bus path
> + * '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
>   *              Example: 'device:O' uses qemu_device_opts.
>   *              Command completion for specific keys can be requested via
> - *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> + *              appending '(NAME:TYPE,...)' with 'F', 'B', 'q', 'Q' as type.
>   *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>   *              Restriction: only lists with empty desc are supported
>   *              TODO lift the restriction

Nitpick: you add "Example: 'device:O(bus:Q)'" in patch 11, before 'Q'
gets defined in patch 12.

Re: [Qemu-devel] [PATCH v4 13/23] monitor: Allow to specify HMP-specifc command arguments

[Markus Armbruster]

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

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> As we may want to shrink or enhance the argument set used for monitor
> command in HMP mode, add a separate, optional argument string for that
> case. When an HMP request is parsed, this argument string, if available,
> takes precedence over the standard string.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

args_type strings are such a crude tool...  I fear having two of them
just breeds inconsistencies.

There's a big difference between QMP and HMP: QMP has only named
parameters, HMP only positional parameters (type 'O' provides named
options within a positional parameter).  I figure the proper way to deal
with this is to define QMP parameters in a data structure (we need that
for self-documentation anyway), then define a mapping from HMP
positional parameters to QMP named parameters.

Re: [Qemu-devel] [PATCH v4 14/23] 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>

Permits fixing the TODOs in qemu-error.c.  Feel free to leave that to
me.

Re: [Qemu-devel] [PATCH v4 00/23] qdev path reworks & basic device state visualization

[Markus Armbruster]

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

> This is v4 of this series. Besides small fixes, it's main focus is on
> the groundwork for the visualization command: qdev path usability.

Don't let my griping on individual patches mislead you: there's lots of
good stuff in here.

Re: [Qemu-devel] [PATCH v4 02/23] qdev: Restrict direct bus addressing via its name

[Jan Kiszka]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> We allow to address a bus only using its local name. This is ambiguous
>> but unfortunately so handy that people (specifically libvirt) will
>> likely complain if bus=pci.0 needs to be replaced with
>> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
>> drop at least support for starting a qtree walks with an abbreviated bus
>> name.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Again, affects only -device and device_add option bus.
> 
> Before, an argument started either with '/' (path anchored at root) or a
> bus name (path anchored at that bus).
> 
> Now, an argument started either with '/' (path anchored at root) or it
> is a bus name.
> 
> If we decide to add sane relative paths to qdev, i.e. paths anchored at
> unique ID, we get a slight anomaly here:
> 
>   bus=a        bus name, not a relative path
>   bus=a/b      relative path anchored at node with unique ID a.
> 
> Works for me.

If we allow ID-anchoring, we are in ambiguous land again unless we
reject IDs that happen to match any bus name in the system. Even then,
the problem will be that the aliases based on bus names need to be
auto-generated (for backward compatibility) while IDs are user-assigned.
If the user comes first, auto-generation will innocently produce an ugly
conflict and we will either have to reject bus instantiation or live
with the conflict.

Jan

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

Re: [Qemu-devel] [PATCH v4 03/23] qdev: Drop ID matching from qtree paths

[Jan Kiszka]

Markus Armbruster wrote:
> [cc: kraxel]
> 
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Device IDs may conflict with device names or aliases. From now on we
>> only accept them outside qtree paths. This also makes dumping IDs in
>> qbus_list_dev/bus obsolete.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I don't like this at all.
> 
> 1. Specific problem:
> 
>    With the current code, multiple devices with the same driver work
>    only if I take care: addressing by driver name gets me only the
>    first, so I better set suitable IDs.
> 
>    With your patch, multiple devices with the same driver don't work, no
>    matter what I do.

[ you already found how this is resolved ]

> 
> 2. General principle:
> 
>    When I set an ID, I want the system to accept that ID in all contexts
>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>    problem.

I disagree. IMO, QEMU should support the user to avoid such subtle
shadowing.

Jan

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

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Jan Kiszka]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> This enables command line completion inside option strings. A list of
>> expected key names and their completion type can be appended to the 'O'
>> inside parentheses ('O(key:type,...)'). The first use case is block
>> device completion for the 'drive' option of 'device_add'.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>  qemu-monitor.hx |    2 +-
>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c1006b4..3e0d862 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -68,6 +68,9 @@
>>   * 'O'          option string of the form NAME=VALUE,...
>>   *              parsed according to QemuOptsList given by its name
>>   *              Example: 'device:O' uses qemu_device_opts.
>> + *              Command completion for specific keys can be requested via
>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>   *              Restriction: only lists with empty desc are supported
>>   *              TODO lift the restriction
>>   * 'i'          32 bit integer
> 
> Ugh.
> 
> Replacement of args_type by a proper data structure is long overdue.  We
> keep piling features into that poor, hapless string.
> 
> Information on how to complete QemuOptsList options arguably belongs
> into the option description, i.e. QemuOptDesc.

For sure, that would be better. I just wonder how much of it should be
stuffed into this series. I guess I will drop this part for now, just
focusing on what device_show makes direct use of. Same for separate args
for HMP and QMP.

Jan

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

Re: [Qemu-devel] [PATCH v4 02/23] qdev: Restrict direct bus addressing via its name

[Markus Armbruster]

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

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> We allow to address a bus only using its local name. This is ambiguous
>>> but unfortunately so handy that people (specifically libvirt) will
>>> likely complain if bus=pci.0 needs to be replaced with
>>> bus=/i440FX-pcihost/pci.0 all over the place. So keep this for now but
>>> drop at least support for starting a qtree walks with an abbreviated bus
>>> name.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> Again, affects only -device and device_add option bus.
>> 
>> Before, an argument started either with '/' (path anchored at root) or a
>> bus name (path anchored at that bus).
>> 
>> Now, an argument started either with '/' (path anchored at root) or it
>> is a bus name.
>> 
>> If we decide to add sane relative paths to qdev, i.e. paths anchored at
>> unique ID, we get a slight anomaly here:
>> 
>>   bus=a        bus name, not a relative path
>>   bus=a/b      relative path anchored at node with unique ID a.
>> 
>> Works for me.
>
> If we allow ID-anchoring, we are in ambiguous land again unless we
> reject IDs that happen to match any bus name in the system. Even then,
> the problem will be that the aliases based on bus names need to be
> auto-generated (for backward compatibility) while IDs are user-assigned.
> If the user comes first, auto-generation will innocently produce an ugly
> conflict and we will either have to reject bus instantiation or live
> with the conflict.

No.

For backward compatibility, we need bus=a to be interpreted as bus name,
not as qdev path.  In other words, we overload bus to be either bus name
or qdev path.  It's a wart, but unavoidable unless we restrict bus to
bus names and add a separate option for paths.

As long as we require all qdev paths to be absolute, the wart is
relatively small.  The rule to resolve the overloading is simple: if the
value starts with '/', then it's a qdev path, else it's a bus name.

With relative qdev paths, the wart becomes more a bit more prominent.
The rule to resolve the overloading becomes: if the value contains '/',
it's a qdev path, else it's a bus name.

I can't see ambiguity here.

Re: [Qemu-devel] [PATCH v4 03/23] qdev: Drop ID matching from qtree paths

[Markus Armbruster]

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

> Markus Armbruster wrote:
>> [cc: kraxel]
>> 
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Device IDs may conflict with device names or aliases. From now on we
>>> only accept them outside qtree paths. This also makes dumping IDs in
>>> qbus_list_dev/bus obsolete.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> I don't like this at all.
>> 
>> 1. Specific problem:
>> 
>>    With the current code, multiple devices with the same driver work
>>    only if I take care: addressing by driver name gets me only the
>>    first, so I better set suitable IDs.
>> 
>>    With your patch, multiple devices with the same driver don't work, no
>>    matter what I do.
>
> [ you already found how this is resolved ]

Yes, but I don't like that the intermediate state is broken, and I don't
like the final state.

>> 2. General principle:
>> 
>>    When I set an ID, I want the system to accept that ID in all contexts
>>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>>    problem.
>
> I disagree. IMO, QEMU should support the user to avoid such subtle
> shadowing.

IDs let me refer to my devices without having to look up ever-changing
instance numbers.  This is especially relevant when I want to do refer
to buses without using (possibly ambigious) bus names.

I want to be able to say bus=scsi-ctrl.2/scsi.0 instead of
/i440FX-pcihost/pci.0/lsi53c895a.<ever-changing-number>/scsi.0

That's much more valuable to me than a well-meant attempt at protecting
me from choosing IDs badly.

Throw in multiple PCI buses, and a "no IDs in qdev paths" law becomes
even more annoying.

Re: [Qemu-devel] [PATCH v4 03/23] qdev: Drop ID matching from qtree paths

[Jan Kiszka]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> [cc: kraxel]
>>>
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Device IDs may conflict with device names or aliases. From now on we
>>>> only accept them outside qtree paths. This also makes dumping IDs in
>>>> qbus_list_dev/bus obsolete.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> I don't like this at all.
>>>
>>> 1. Specific problem:
>>>
>>>    With the current code, multiple devices with the same driver work
>>>    only if I take care: addressing by driver name gets me only the
>>>    first, so I better set suitable IDs.
>>>
>>>    With your patch, multiple devices with the same driver don't work, no
>>>    matter what I do.
>> [ you already found how this is resolved ]
> 
> Yes, but I don't like that the intermediate state is broken, and I don't
> like the final state.
> 
>>> 2. General principle:
>>>
>>>    When I set an ID, I want the system to accept that ID in all contexts
>>>    where it makes sense.  Ambiguity created by badly chosen IDs is *my*
>>>    problem.
>> I disagree. IMO, QEMU should support the user to avoid such subtle
>> shadowing.
> 
> IDs let me refer to my devices without having to look up ever-changing
> instance numbers.  This is especially relevant when I want to do refer
> to buses without using (possibly ambigious) bus names.
> 
> I want to be able to say bus=scsi-ctrl.2/scsi.0 instead of
> /i440FX-pcihost/pci.0/lsi53c895a.<ever-changing-number>/scsi.0

It would once be something like /i440FX-pcihost/pci.0/@04.0/scsi.0 (with
optional extension of '@04.0' to '@04.0:lsi53c895a.0' for more verbose
interactive use), so at least no volatile naming here. But letting a
qdev path start with an ID can of course shorten things, specifically on
the command line where we have no completion.

> 
> That's much more valuable to me than a well-meant attempt at protecting
> me from choosing IDs badly.
> 
> Throw in multiple PCI buses, and a "no IDs in qdev paths" law becomes
> even more annoying.

No IDs _inside_ qdev paths, please. Letting them start is imaginable if
we find proper rules how to resolve the possible conflicts with
auto-generated bus aliases ('pci.0' = '/i440FX-pcihost/pci.0').

Let's look at it as Paul suggested: relative qdev path beginnings are
aliases of the corresponding absolute bits. When registering a device
with an ID, we also register an alias 'ID' = '/absolute/path/to/device'.
Here we can (and already do) reject the registration if 'ID' is not
unique. When registering a bus, we would add an alias 'busname' =
'/absolute/path/to/bus'. This can already fail due to the fact that bus
names need not be unique. But it could also fail if some silly guy
registered a device with the same ID before.

Now what to do? Register the alias nevertheless and select whatever
comes first in the alias list, possibly creating instable paths again?
Or reject the bus registration (i.e. the registration of the parent
device? Or silently skip the failing alias registration (which would
mean having no such alias at all once the other device gets hot-removed)?

Jan

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

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Markus Armbruster]

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

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> This enables command line completion inside option strings. A list of
>>> expected key names and their completion type can be appended to the 'O'
>>> inside parentheses ('O(key:type,...)'). The first use case is block
>>> device completion for the 'drive' option of 'device_add'.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>  qemu-monitor.hx |    2 +-
>>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index c1006b4..3e0d862 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -68,6 +68,9 @@
>>>   * 'O'          option string of the form NAME=VALUE,...
>>>   *              parsed according to QemuOptsList given by its name
>>>   *              Example: 'device:O' uses qemu_device_opts.
>>> + *              Command completion for specific keys can be requested via
>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>>   *              Restriction: only lists with empty desc are supported
>>>   *              TODO lift the restriction
>>>   * 'i'          32 bit integer
>> 
>> Ugh.
>> 
>> Replacement of args_type by a proper data structure is long overdue.  We
>> keep piling features into that poor, hapless string.
>> 
>> Information on how to complete QemuOptsList options arguably belongs
>> into the option description, i.e. QemuOptDesc.
>
> For sure, that would be better. I just wonder how much of it should be
> stuffed into this series. I guess I will drop this part for now, just
> focusing on what device_show makes direct use of. Same for separate args
> for HMP and QMP.

Sensible.


Speaking of args_type.  We've accumulated too many ways to represent
dynamic data types, and too many ways to describe data types.

The most powerful dynamic data type is QObject.  It comes with a useful
plain-text representation (JSON).  We lack a data type for describing
QObjects.  We use args_type to describe special QObjects, namely monitor
command arguments.  We've clearly stretched that to the limit and then
some.  We could use a general solution.  JSON Schema as its plain-text
representation could be nice.

Then we have QemuOpts.  It's more limited (flat list instead of trees),
but it comes with a real data type to describe it (QemuOptsList).

There's also QEMUOptionParameter.  Serves a similar purpose.  I don't
know why we have both.

Over in qdev-land, we find Property, which can be viewed a data type to
describe (parts of) a struct.

Some unification would be nice.

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Luiz Capitulino]

On Wed, 23 Jun 2010 12:28:27 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Markus Armbruster wrote:
> > Jan Kiszka <jan.kiszka@web.de> writes:
> > 
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> This enables command line completion inside option strings. A list of
> >> expected key names and their completion type can be appended to the 'O'
> >> inside parentheses ('O(key:type,...)'). The first use case is block
> >> device completion for the 'drive' option of 'device_add'.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >>  qemu-monitor.hx |    2 +-
> >>  2 files changed, 58 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index c1006b4..3e0d862 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -68,6 +68,9 @@
> >>   * 'O'          option string of the form NAME=VALUE,...
> >>   *              parsed according to QemuOptsList given by its name
> >>   *              Example: 'device:O' uses qemu_device_opts.
> >> + *              Command completion for specific keys can be requested via
> >> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> >> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
> >>   *              Restriction: only lists with empty desc are supported
> >>   *              TODO lift the restriction
> >>   * 'i'          32 bit integer
> > 
> > Ugh.
> > 
> > Replacement of args_type by a proper data structure is long overdue.  We
> > keep piling features into that poor, hapless string.
> > 
> > Information on how to complete QemuOptsList options arguably belongs
> > into the option description, i.e. QemuOptDesc.
> 
> For sure, that would be better. I just wonder how much of it should be
> stuffed into this series. I guess I will drop this part for now, just
> focusing on what device_show makes direct use of. Same for separate args
> for HMP and QMP.

IIRC, the separate args idea use case was to allow commands like device_del
to have an ID argument and a path argument, right? If so, I think it doesn't
matter anymore as we have agreed on having a device argument which would
accept both, even under QMP, right Markus?

By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
and 22/23 in a different series, I could try pushing them in my next
pull request.

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Jan Kiszka]

Luiz Capitulino wrote:
> On Wed, 23 Jun 2010 12:28:27 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> This enables command line completion inside option strings. A list of
>>>> expected key names and their completion type can be appended to the 'O'
>>>> inside parentheses ('O(key:type,...)'). The first use case is block
>>>> device completion for the 'drive' option of 'device_add'.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>>  qemu-monitor.hx |    2 +-
>>>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index c1006b4..3e0d862 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -68,6 +68,9 @@
>>>>   * 'O'          option string of the form NAME=VALUE,...
>>>>   *              parsed according to QemuOptsList given by its name
>>>>   *              Example: 'device:O' uses qemu_device_opts.
>>>> + *              Command completion for specific keys can be requested via
>>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>>>   *              Restriction: only lists with empty desc are supported
>>>>   *              TODO lift the restriction
>>>>   * 'i'          32 bit integer
>>> Ugh.
>>>
>>> Replacement of args_type by a proper data structure is long overdue.  We
>>> keep piling features into that poor, hapless string.
>>>
>>> Information on how to complete QemuOptsList options arguably belongs
>>> into the option description, i.e. QemuOptDesc.
>> For sure, that would be better. I just wonder how much of it should be
>> stuffed into this series. I guess I will drop this part for now, just
>> focusing on what device_show makes direct use of. Same for separate args
>> for HMP and QMP.
> 
> IIRC, the separate args idea use case was to allow commands like device_del
> to have an ID argument and a path argument, right? If so, I think it doesn't
> matter anymore as we have agreed on having a device argument which would
> accept both, even under QMP, right Markus?

To my understanding: As a leading element in qdev path, at least to
address a device, maybe also to abbreviate only the beginning of a full
path (that's currently to major remaining open issue).

> 
> By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
> and 22/23 in a different series, I could try pushing them in my next
> pull request.

Do they need rebasing? If not, feel free to pick them up as you like. My
series requires a v5 round anyway once discussion on path construction
finally came to an end.

Jan

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

Re: [Qemu-devel] [PATCH v4 11/23] monitor: Add completion support for option lists

[Luiz Capitulino]

On Mon, 28 Jun 2010 16:40:58 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Luiz Capitulino wrote:
> > On Wed, 23 Jun 2010 12:28:27 +0200
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> Markus Armbruster wrote:
> >>> Jan Kiszka <jan.kiszka@web.de> writes:
> >>>
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> This enables command line completion inside option strings. A list of
> >>>> expected key names and their completion type can be appended to the 'O'
> >>>> inside parentheses ('O(key:type,...)'). The first use case is block
> >>>> device completion for the 'drive' option of 'device_add'.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> ---
> >>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
> >>>>  qemu-monitor.hx |    2 +-
> >>>>  2 files changed, 58 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/monitor.c b/monitor.c
> >>>> index c1006b4..3e0d862 100644
> >>>> --- a/monitor.c
> >>>> +++ b/monitor.c
> >>>> @@ -68,6 +68,9 @@
> >>>>   * 'O'          option string of the form NAME=VALUE,...
> >>>>   *              parsed according to QemuOptsList given by its name
> >>>>   *              Example: 'device:O' uses qemu_device_opts.
> >>>> + *              Command completion for specific keys can be requested via
> >>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
> >>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
> >>>>   *              Restriction: only lists with empty desc are supported
> >>>>   *              TODO lift the restriction
> >>>>   * 'i'          32 bit integer
> >>> Ugh.
> >>>
> >>> Replacement of args_type by a proper data structure is long overdue.  We
> >>> keep piling features into that poor, hapless string.
> >>>
> >>> Information on how to complete QemuOptsList options arguably belongs
> >>> into the option description, i.e. QemuOptDesc.
> >> For sure, that would be better. I just wonder how much of it should be
> >> stuffed into this series. I guess I will drop this part for now, just
> >> focusing on what device_show makes direct use of. Same for separate args
> >> for HMP and QMP.
> > 
> > IIRC, the separate args idea use case was to allow commands like device_del
> > to have an ID argument and a path argument, right? If so, I think it doesn't
> > matter anymore as we have agreed on having a device argument which would
> > accept both, even under QMP, right Markus?
> 
> To my understanding: As a leading element in qdev path, at least to
> address a device, maybe also to abbreviate only the beginning of a full
> path (that's currently to major remaining open issue).

I'm ok with it if it's unambiguous.

> > By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
> > and 22/23 in a different series, I could try pushing them in my next
> > pull request.
> 
> Do they need rebasing? If not, feel free to pick them up as you like. My
> series requires a v5 round anyway once discussion on path construction
> finally came to an end.

Done for them all, except 16/23 which mentions device_show in the changelog.

I should send a pull request until this Wednesday.

[Qemu-devel] [PATCH] monitor: Allow to exclude commands from QMP

[Jan Kiszka]

Luiz Capitulino wrote:
> On Mon, 28 Jun 2010 16:40:58 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Luiz Capitulino wrote:
>>> On Wed, 23 Jun 2010 12:28:27 +0200
>>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>
>>>> Markus Armbruster wrote:
>>>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>>>
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> This enables command line completion inside option strings. A list of
>>>>>> expected key names and their completion type can be appended to the 'O'
>>>>>> inside parentheses ('O(key:type,...)'). The first use case is block
>>>>>> device completion for the 'drive' option of 'device_add'.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>  monitor.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>>>>>  qemu-monitor.hx |    2 +-
>>>>>>  2 files changed, 58 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>> index c1006b4..3e0d862 100644
>>>>>> --- a/monitor.c
>>>>>> +++ b/monitor.c
>>>>>> @@ -68,6 +68,9 @@
>>>>>>   * 'O'          option string of the form NAME=VALUE,...
>>>>>>   *              parsed according to QemuOptsList given by its name
>>>>>>   *              Example: 'device:O' uses qemu_device_opts.
>>>>>> + *              Command completion for specific keys can be requested via
>>>>>> + *              appending '(NAME:TYPE,...)' with 'F', 'B' as type.
>>>>>> + *              Example: 'device:O(bus:Q)' to expand 'bus=...' as qtree path.
>>>>>>   *              Restriction: only lists with empty desc are supported
>>>>>>   *              TODO lift the restriction
>>>>>>   * 'i'          32 bit integer
>>>>> Ugh.
>>>>>
>>>>> Replacement of args_type by a proper data structure is long overdue.  We
>>>>> keep piling features into that poor, hapless string.
>>>>>
>>>>> Information on how to complete QemuOptsList options arguably belongs
>>>>> into the option description, i.e. QemuOptDesc.
>>>> For sure, that would be better. I just wonder how much of it should be
>>>> stuffed into this series. I guess I will drop this part for now, just
>>>> focusing on what device_show makes direct use of. Same for separate args
>>>> for HMP and QMP.
>>> IIRC, the separate args idea use case was to allow commands like device_del
>>> to have an ID argument and a path argument, right? If so, I think it doesn't
>>> matter anymore as we have agreed on having a device argument which would
>>> accept both, even under QMP, right Markus?
>> To my understanding: As a leading element in qdev path, at least to
>> address a device, maybe also to abbreviate only the beginning of a full
>> path (that's currently to major remaining open issue).
> 
> I'm ok with it if it's unambiguous.
> 
>>> By the way, if you send patches 09/23, 10/23, 15/23, (maybe) 16/23, 21/23
>>> and 22/23 in a different series, I could try pushing them in my next
>>> pull request.
>> Do they need rebasing? If not, feel free to pick them up as you like. My
>> series requires a v5 round anyway once discussion on path construction
>> finally came to an end.
> 
> Done for them all, except 16/23 which mentions device_show in the changelog.

If that's the only issue of 16/23, feel free to pick up the cleaned
version below.

> 
> I should send a pull request until this Wednesday.

Great, thanks.

Jan

-------------->

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

For Luiz' queue, depends on "monitor: Establish cmd flags and convert
the async tag" as posted earlier.

 monitor.c |   18 +++++++++++++++---
 monitor.h |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 281a6f2..99c07d2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -330,6 +330,11 @@ static inline bool monitor_handler_is_async(const mon_cmd_t *cmd)
     return cmd->flags & MONITOR_CMD_ASYNC;
 }
 
+static inline bool monitor_cmd_user_only(const mon_cmd_t *cmd)
+{
+    return (cmd->flags & MONITOR_CMD_USER_ONLY);
+}
+
 static inline int monitor_has_error(const Monitor *mon)
 {
     return mon->error != NULL;
@@ -612,6 +617,11 @@ static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto help;
     }
 
+    if (monitor_ctrl_mode(mon) && monitor_cmd_user_only(cmd)) {
+        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);
@@ -709,13 +719,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) && !monitor_cmd_user_only(cmd) &&
+            !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) && !monitor_cmd_user_only(cmd)) {
             char buf[128];
             snprintf(buf, sizeof(buf), "query-%s", cmd->name);
             qlist_append_obj(cmd_list, get_cmd_dict(buf));
@@ -4207,7 +4218,8 @@ 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)
+            || monitor_cmd_user_only(cmd)) {
             qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
             goto err_input;
         }
diff --git a/monitor.h b/monitor.h
index 9582b9c..38b22a4 100644
--- a/monitor.h
+++ b/monitor.h
@@ -17,6 +17,7 @@ extern Monitor *default_mon;
 
 /* flags for monitor commands */
 #define MONITOR_CMD_ASYNC       0x0001
+#define MONITOR_CMD_USER_ONLY   0x0002
 
 /* QMP events */
 typedef enum MonitorEvent {
-- 
1.7.1