[Qemu-devel] [PATCH 0/8] Basic device state visualization

[Qemu-devel] [PATCH 0/8] Basic device state visualization

[Jan Kiszka]

While recently fixing the SCSI reset issues, I once again had the need
for displaying the state of involved devices. So far the common approach
is to attach gdb to qemu (or even inject some printf). But that time I
hacked up a 30-minute patch to dump the vmstate of any (fully converted)
qdev device.

This series now lays the ground for more sophisticated visulization. It
adds the monitor command 'device_show <qdev-path>', freezes the vmstate
of the addressed device, sticks it into a QMP dict, and either transmit
this via QMP or pretty-prints it on a monitor console. Some example:

(qemu) device_show /i440FX-pcihost/pci.0/piix3-usb-uhci
dev: piix3-usb-uhci, id ""
  dev.
    version_id:         00000002
    config:             a0 7d d1 00 00 00 00 00 - b0 7e d1 00 00 00 00 00
                        ...
    irq_state:          00 00 00 00 00 00 00 00 - 00 00 00 00 00 00 00 00
  num_ports_vmstate:    02
  ports[00].
      ctrl:             0083
  ports[01].
      ctrl:             0080
  cmd:                  00c1
  status:               0000
  intr:                 0000
  frnum:                0077
  fl_base_addr:         0fffc000
  sof_timing:           40
  status2:              00
  frame_timer:          0000000000cb2bd0

Basically, this is the level of support I recently saw in a
demonstration of some commercial simulator as well. We are just lacking
support for the yet unconverted devices. And I think we can even do
better on the long term, e.g. by annotating state variables that contain
flags, or by pretty-printing buffers like the PCI config space, or...

Let's give this a start, I bet it will be helpful while adding complex
device models like AHCI or EHCI. Looking forward to feedback!

Jan Kiszka (8):
  qdev: Allow device addressing via 'driver.instance'
  Add base64 encoder/decoder
  Add QBuffer
  monitor: Add basic device state visualization
  qmp: Teach basic capability negotiation to python example
  qmp: Fix python helper /wrt long return strings
  Add QLIST_INSERT_TAIL
  qdev: Add new devices/buses at the tail

 Makefile        |    3 +-
 Makefile.objs   |    4 +-
 QMP/qmp-shell   |    1 +
 QMP/qmp.py      |    2 +-
 QMP/vm-info     |    1 +
 base64.c        |  202 +++++++++++++++++++++++++++++++++++
 base64.h        |   18 +++
 check-qbuffer.c |  172 ++++++++++++++++++++++++++++++
 configure       |    2 +-
 hw/hw.h         |    2 +
 hw/qdev.c       |  314 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/qdev.h       |    2 +
 qbuffer.c       |  116 ++++++++++++++++++++
 qbuffer.h       |   33 ++++++
 qemu-monitor.hx |   16 +++
 qemu-queue.h    |    9 ++
 qjson.c         |   16 +++
 qobject.h       |    1 +
 18 files changed, 902 insertions(+), 12 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 1/8] qdev: Allow device addressing via 'driver.instance'

[Jan Kiszka]

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

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

diff --git a/hw/qdev.c b/hw/qdev.c
index d3bf0fa..fe49e71 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -515,28 +515,41 @@ static BusState *qbus_find_bus(DeviceState *dev, char *elem)
     return NULL;
 }
 
-static DeviceState *qbus_find_dev(BusState *bus, char *elem)
+static DeviceState *qbus_find_dev(BusState *bus, const char *elem)
 {
     DeviceState *dev;
+    int instance, n;
+    char buf[128];
 
     /*
      * try to match in order:
      *   (1) instance id, if present
-     *   (2) driver name
-     *   (3) driver alias, if present
+     *   (2) driver name [.instance]
+     *   (3) driver alias [.instance], if present
      */
     QLIST_FOREACH(dev, &bus->children, sibling) {
         if (dev->id  &&  strcmp(dev->id, elem) == 0) {
             return dev;
         }
     }
+
+    if (sscanf(elem, "%127[^.].%u", buf, &instance) == 2) {
+        elem = buf;
+    } else {
+        instance = 0;
+    }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (strcmp(dev->info->name, elem) == 0) {
+        if (strcmp(dev->info->name, elem) == 0 && n++ == instance) {
             return dev;
         }
     }
+
+    n = 0;
     QLIST_FOREACH(dev, &bus->children, sibling) {
-        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0) {
+        if (dev->info->alias && strcmp(dev->info->alias, elem) == 0 &&
+            n++ == instance) {
             return dev;
         }
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/8] Add base64 encoder/decoder

[Jan Kiszka]

Will be used by QBuffer.

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

diff --git a/Makefile.objs b/Makefile.objs
index ecdd53e..3d2a27a 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..dd01787
--- /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 char *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 char *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 char *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 void *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[srclen] = 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, char *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, char *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, char *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, void *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..9a0e03a
--- /dev/null
+++ b/base64.h
@@ -0,0 +1,18 @@
+/*
+ * 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 <unistd.h>
+
+void base64_encode(const void *src, size_t srclen, char *dest);
+int base64_decode(const char *src, size_t srclen, void *dest);
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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. To decode
this kind of strings back to a QBuffer, the receiving side has to be
aware of their semantic, which is normally no problem within QMP.

The first use case of this type 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        |    3 +-
 Makefile.objs   |    2 +-
 check-qbuffer.c |  172 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure       |    2 +-
 qbuffer.c       |  116 +++++++++++++++++++++++++++++++++++++
 qbuffer.h       |   33 +++++++++++
 qjson.c         |   16 +++++
 qobject.h       |    1 +
 8 files changed, 342 insertions(+), 3 deletions(-)
 create mode 100644 check-qbuffer.c
 create mode 100644 qbuffer.c
 create mode 100644 qbuffer.h

diff --git a/Makefile b/Makefile
index eb9e02b..065c1a5 100644
--- a/Makefile
+++ b/Makefile
@@ -149,7 +149,8 @@ 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 3d2a27a..52c8ec7 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/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 36d028f..9ff9308 100755
--- a/configure
+++ b/configure
@@ -2280,7 +2280,7 @@ if test `expr "$target_list" : ".*softmmu.*"` != 0 ; then
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
       tools="qemu-nbd\$(EXESUF) $tools"
     if [ "$check_utests" = "yes" ]; then
-      tools="check-qint check-qstring check-qdict check-qlist $tools"
+      tools="check-qint check-qstring check-qdict check-qlist check-qbuffer $tools"
       tools="check-qfloat check-qjson $tools"
     fi
   fi
diff --git a/qbuffer.c b/qbuffer.c
new file mode 100644
index 0000000..704d170
--- /dev/null
+++ b/qbuffer.c
@@ -0,0 +1,116 @@
+/*
+ * QBuffer Module
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qbuffer.h"
+#include "qobject.h"
+#include "qemu-common.h"
+#include "base64.h"
+
+static void qbuffer_destroy_obj(QObject *obj);
+
+static const QType qbuffer_type = {
+    .code = QTYPE_QBUFFER,
+    .destroy = qbuffer_destroy_obj,
+};
+
+/**
+ * qbuffer_from_data(): Create a new QBuffer from an existing data blob
+ *
+ * Returns strong reference.
+ */
+QBuffer *qbuffer_from_data(const void *data, size_t size)
+{
+    QBuffer *qb;
+
+    qb = qemu_malloc(sizeof(*qb));
+    qb->data = qemu_malloc(size);
+    memcpy(qb->data, data, size);
+    qb->size = size;
+    QOBJECT_INIT(qb, &qbuffer_type);
+
+    return qb;
+}
+
+/**
+ * qbuffer_from_qstring(): Create a new QBuffer from a QString object that
+ * contains the data as a stream of hex-encoded bytes
+ *
+ * Returns strong reference.
+ */
+QBuffer *qbuffer_from_qstring(const QString *string)
+{
+    const char *str = qstring_get_str(string);
+    size_t str_len;
+    QBuffer *qb;
+
+    qb = qemu_malloc(sizeof(*qb));
+
+    str_len = strlen(str);
+    while (str_len > 0 && str[str_len - 1] == '=') {
+        str_len--;
+    }
+    qb->size = (str_len / 4) * 3 + ((str_len % 4) * 3) / 4;
+    qb->data = qemu_malloc(qb->size);
+
+    QOBJECT_INIT(qb, &qbuffer_type);
+
+    if (base64_decode(str, str_len, qb->data) < 0) {
+        qbuffer_destroy_obj(QOBJECT(qb));
+        return NULL;
+    }
+
+    return qb;
+}
+
+/**
+ * qbuffer_get_data(): Return pointer to stored data
+ *
+ * NOTE: Should be used with caution, if the object is deallocated
+ * this pointer becomes invalid.
+ */
+const void *qbuffer_get_data(const QBuffer *qb)
+{
+    return qb->data;
+}
+
+/**
+ * qbuffer_get_size(): Return size of stored data
+ */
+size_t qbuffer_get_size(const QBuffer *qb)
+{
+    return qb->size;
+}
+
+/**
+ * qobject_to_qbool(): Convert a QObject into a QBuffer
+ */
+QBuffer *qobject_to_qbuffer(const QObject *obj)
+{
+    if (qobject_type(obj) != QTYPE_QBUFFER)
+        return NULL;
+
+    return container_of(obj, QBuffer, base);
+}
+
+/**
+ * qbuffer_destroy_obj(): Free all memory allocated by a QBuffer object
+ */
+static void qbuffer_destroy_obj(QObject *obj)
+{
+    QBuffer *qb;
+
+    assert(obj != NULL);
+    qb = qobject_to_qbuffer(obj);
+    qemu_free(qb->data);
+    qemu_free(qb);
+}
diff --git a/qbuffer.h b/qbuffer.h
new file mode 100644
index 0000000..2e01078
--- /dev/null
+++ b/qbuffer.h
@@ -0,0 +1,33 @@
+/*
+ * QBuffer Module
+ *
+ * Copyright (C) 2010 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QBUFFER_H
+#define QBUFFER_H
+
+#include <stdint.h>
+#include "qobject.h"
+#include "qstring.h"
+
+typedef struct QBuffer {
+    QObject_HEAD;
+    void *data;
+    size_t size;
+} QBuffer;
+
+QBuffer *qbuffer_from_data(const void *data, size_t size);
+QBuffer *qbuffer_from_qstring(const QString *string);
+const void *qbuffer_get_data(const QBuffer *qb);
+size_t qbuffer_get_size(const QBuffer *qb);
+QBuffer *qobject_to_qbuffer(const QObject *obj);
+
+#endif /* QBUFFER_H */
diff --git a/qjson.c b/qjson.c
index 483c667..4d1c21a 100644
--- a/qjson.c
+++ b/qjson.c
@@ -19,7 +19,9 @@
 #include "qlist.h"
 #include "qbool.h"
 #include "qfloat.h"
+#include "qbuffer.h"
 #include "qdict.h"
+#include "base64.h"
 
 typedef struct JSONParsingState
 {
@@ -235,6 +237,20 @@ 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 + 3);
+
+        buffer[0] = '"';
+        base64_encode(qbuffer_get_data(val), data_size, buffer + 1);
+        buffer[str_len + 1] = '"';
+        buffer[str_len + 2] = 0;
+        qstring_append(str, buffer);
+        qemu_free(buffer);
+        break;
+    }
     case QTYPE_QERROR:
         /* XXX: should QError be emitted? */
     case QTYPE_NONE:
diff --git a/qobject.h b/qobject.h
index 07de211..45c4fa0 100644
--- a/qobject.h
+++ b/qobject.h
@@ -44,6 +44,7 @@ typedef enum {
     QTYPE_QFLOAT,
     QTYPE_QBOOL,
     QTYPE_QERROR,
+    QTYPE_QBUFFER,
 } qtype_code;
 
 struct QObject;
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/8] monitor: Add basic device state visualization

[Jan Kiszka]

This introduces device_show, a monitor command that saves the vmstate of
a qdev device and visualizes it. QMP is also supported. 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.

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

diff --git a/hw/hw.h b/hw/hw.h
index 328b704..1ff8e40 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -299,6 +299,7 @@ enum VMStateFlags {
 
 typedef struct {
     const char *name;
+    const char *start_index;
     size_t offset;
     size_t size;
     size_t start;
@@ -414,6 +415,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 fe49e71..c989010 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;
 
@@ -824,3 +827,287 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     return qdev_unplug(dev);
 }
+
+static int print_array_index(Monitor *mon, const char *start, int index)
+{
+    char index_str[32];
+    int len;
+
+    if (start) {
+        len = snprintf(index_str, sizeof(index_str), "[%s+%02x]", start,
+                       index);
+    } else {
+        len = snprintf(index_str, sizeof(index_str), "[%02x]", index);
+    }
+    monitor_printf(mon, index_str);
+    return len;
+}
+
+#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*x\n", (int)size * 2,
+                       (int)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);
+            pos += print_array_index(mon, start, 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\"\n",
+                   qdict_get_str(qdict, "device"),
+                   qdict_get_str(qdict, "id"));
+
+    QLIST_FOREACH_ENTRY(qlist, entry) {
+        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
+    }
+}
+
+static void parse_vmstate(const VMStateDescription *vmsd, void *opaque,
+                          QList *qlist, int full_buffers)
+{
+    VMStateField *field = vmsd->fields;
+
+    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;
+            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;
+                }
+            }
+            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(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) {
+                    parse_vmstate(field->vmsd, addr, sub_elems, full_buffers);
+                } else {
+                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
+                        if (!full_buffers && size > 16) {
+                            size = 16;
+                        }
+                        qlist_append_obj(sub_elems,
+                                         QOBJECT(qbuffer_from_data(addr,
+                                                                   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)));
+                    }
+                }
+            }
+        }
+        field++;
+    }
+}
+
+static DeviceState *qdev_find(const char *path)
+{
+    const char *dev_name;
+    DeviceState *dev;
+    char *bus_path;
+    BusState *bus;
+
+    dev_name = strrchr(path, '/');
+    if (!dev_name) {
+        bus = main_system_bus;
+        dev_name = path;
+    } else {
+        dev_name++;
+        bus_path = qemu_strdup(path);
+        bus_path[dev_name - path] = 0;
+
+        bus = qbus_find(bus_path);
+        qemu_free(bus_path);
+
+        if (!bus) {
+            /* qbus_find already reported the error */
+            return NULL;
+        }
+    }
+    dev = qbus_find_dev(bus, dev_name);
+    if (!dev) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
+        if (!monitor_cur_is_qmp()) {
+            qbus_list_dev(bus);
+        }
+    }
+    return dev;
+}
+
+int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const VMStateDescription *vmsd;
+    DeviceState *dev;
+    QList *qlist;
+
+    dev = qdev_find_recursive(main_system_bus, path);
+    if (!dev) {
+        dev = qdev_find(path);
+        if (!dev) {
+            return -1;
+        }
+    }
+
+    vmsd = dev->info->vmsd;
+    if (!vmsd) {
+        qerror_report(QERR_UNDEFINED_ERROR);
+        if (!monitor_cur_is_qmp()) {
+            error_printf_unless_qmp("Device '%s' does not support state "
+                                    "dumping\n", path);
+        }
+        return -1;
+    }
+
+    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s }",
+                                   dev->info->name, dev->id ? : "");
+    qlist = qlist_new();
+    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
+    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
+
+    return 0;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index d8fbc73..f8436ec 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -177,6 +177,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 a8f194c..449f012 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -599,6 +599,22 @@ Remove device @var{id}.
 ETEXI
 
     {
+        .name       = "device_show",
+        .args_type  = "path:s,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,
+    },
+
+STEXI
+@item device_show @var{id} [@code{-f}]
+
+Show state of device @var{id} in a human-readable form. Buffers are cut after
+16 bytes unless a full dump is requested via @code{-f} 
+ETEXI
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .params     = "index",
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/8] qmp: Teach basic capability negotiation to python example

[Jan Kiszka]

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

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

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

[Qemu-devel] [PATCH 6/8] qmp: Fix python helper /wrt long return strings

[Jan Kiszka]

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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/QMP/qmp.py b/QMP/qmp.py
index d9da603..b8f1741 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -63,7 +63,7 @@ class QEMUMonitorProtocol:
 
     def __json_read(self):
         try:
-            return json.loads(self.sock.recv(1024))
+            return json.loads(self.sock.makefile().readline())
         except ValueError:
             return
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 7/8] Add QLIST_INSERT_TAIL

[Jan Kiszka]

As the QLIST has not tail pointer, this requires list walking. Still
useful when lists are short or insertion time doesn't matter.

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

diff --git a/qemu-queue.h b/qemu-queue.h
index 1d07745..99ed1f6 100644
--- a/qemu-queue.h
+++ b/qemu-queue.h
@@ -122,6 +122,15 @@ struct {                                                                \
         (elm)->field.le_prev = &(head)->lh_first;                       \
 } while (/*CONSTCOND*/0)
 
+#define QLIST_INSERT_TAIL(head, elm, field) do {                        \
+        typeof((head)->lh_first) *qlist_lastptr = &(head)->lh_first;    \
+        while (*qlist_lastptr)                                          \
+            qlist_lastptr = &(*qlist_lastptr)->field.le_next;           \
+        (elm)->field.le_next = NULL;                                    \
+        *qlist_lastptr = (elm);                                         \
+        (elm)->field.le_prev = qlist_lastptr;                           \
+} while (/*CONSTCOND*/0)
+
 #define QLIST_REMOVE(elm, field) do {                                   \
         if ((elm)->field.le_next != NULL)                               \
                 (elm)->field.le_next->field.le_prev =                   \
-- 
1.6.0.2

[Qemu-devel] [PATCH 8/8] qdev: Add new devices/buses at the tail

[Jan Kiszka]

Cosmetic change to align the instance number assignment with bus
ordering. The current ordering 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/qdev.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index c989010..3864478 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -91,7 +91,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     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);
+    QLIST_INSERT_TAIL(&bus->children, dev, sibling);
     if (qdev_hotplug) {
         assert(bus->allow_hotplug);
         dev->hotplugged = 1;
@@ -677,7 +677,7 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
 
     QLIST_INIT(&bus->children);
     if (parent) {
-        QLIST_INSERT_HEAD(&parent->child_bus, bus, sibling);
+        QLIST_INSERT_TAIL(&parent->child_bus, bus, sibling);
         parent->num_child_bus++;
     }
 
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH 0/8] Basic device state visualization

[Avi Kivity]

On 05/14/2010 04:20 PM, Jan Kiszka wrote:
> While recently fixing the SCSI reset issues, I once again had the need
> for displaying the state of involved devices. So far the common approach
> is to attach gdb to qemu (or even inject some printf). But that time I
> hacked up a 30-minute patch to dump the vmstate of any (fully converted)
> qdev device.
>    

Wonderful! may even motivate some more qmp conversions.

> This series now lays the ground for more sophisticated visulization. It
> adds the monitor command 'device_show<qdev-path>', freezes the vmstate
> of the addressed device, sticks it into a QMP dict, and either transmit
> this via QMP or pretty-prints it on a monitor console. Some example:
>
> (qemu) device_show /i440FX-pcihost/pci.0/piix3-usb-uhci
> dev: piix3-usb-uhci, id ""
>    dev.
>      version_id:         00000002
>      config:             a0 7d d1 00 00 00 00 00 - b0 7e d1 00 00 00 00 00
>                          ...
>      irq_state:          00 00 00 00 00 00 00 00 - 00 00 00 00 00 00 00 00
>    num_ports_vmstate:    02
>    ports[00].
>        ctrl:             0083
>    ports[01].
>        ctrl:             0080
>    cmd:                  00c1
>    status:               0000
>    intr:                 0000
>    frnum:                0077
>    fl_base_addr:         0fffc000
>    sof_timing:           40
>    status2:              00
>    frame_timer:          0000000000cb2bd0
>
> Basically, this is the level of support I recently saw in a
> demonstration of some commercial simulator as well. We are just lacking
> support for the yet unconverted devices. And I think we can even do
> better on the long term, e.g. by annotating state variables that contain
> flags, or by pretty-printing buffers like the PCI config space, or...
>
>    

Will be interesting to pass these annotations via qmp as well.

> Let's give this a start, I bet it will be helpful while adding complex
> device models like AHCI or EHCI. Looking forward to feedback!
>    

I'd like to see qmp command documentation for this.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 0/8] Basic device state visualization

[Jan Kiszka]

Avi Kivity wrote:
> On 05/14/2010 04:20 PM, Jan Kiszka wrote:
>> While recently fixing the SCSI reset issues, I once again had the need
>> for displaying the state of involved devices. So far the common approach
>> is to attach gdb to qemu (or even inject some printf). But that time I
>> hacked up a 30-minute patch to dump the vmstate of any (fully converted)
>> qdev device.
>>    
> 
> Wonderful! may even motivate some more qmp conversions.

Yeah, a few low handing fruits are already gone (the vmstate series I
sent out recently). I've some stuff for hpet here, but I did not tough
[IO]APIC yet as that should be based on top of Glauber's work for
in-kernel irqchip support.

> 
>> This series now lays the ground for more sophisticated visulization. It
>> adds the monitor command 'device_show<qdev-path>', freezes the vmstate
>> of the addressed device, sticks it into a QMP dict, and either transmit
>> this via QMP or pretty-prints it on a monitor console. Some example:
>>
>> (qemu) device_show /i440FX-pcihost/pci.0/piix3-usb-uhci
>> dev: piix3-usb-uhci, id ""
>>    dev.
>>      version_id:         00000002
>>      config:             a0 7d d1 00 00 00 00 00 - b0 7e d1 00 00 00 00 00
>>                          ...
>>      irq_state:          00 00 00 00 00 00 00 00 - 00 00 00 00 00 00 00 00
>>    num_ports_vmstate:    02
>>    ports[00].
>>        ctrl:             0083
>>    ports[01].
>>        ctrl:             0080
>>    cmd:                  00c1
>>    status:               0000
>>    intr:                 0000
>>    frnum:                0077
>>    fl_base_addr:         0fffc000
>>    sof_timing:           40
>>    status2:              00
>>    frame_timer:          0000000000cb2bd0
>>
>> Basically, this is the level of support I recently saw in a
>> demonstration of some commercial simulator as well. We are just lacking
>> support for the yet unconverted devices. And I think we can even do
>> better on the long term, e.g. by annotating state variables that contain
>> flags, or by pretty-printing buffers like the PCI config space, or...
>>
>>    
> 
> Will be interesting to pass these annotations via qmp as well.

For sure. We could e.g. convert QBuffers to QInts or QInts to QBools
once we know their names and positions. This is where the harder work
starts (that's why I proposed it for GSOC :) ).

> 
>> Let's give this a start, I bet it will be helpful while adding complex
>> device models like AHCI or EHCI. Looking forward to feedback!
>>    
> 
> I'd like to see qmp command documentation for this.
> 

Will add it once the doc baseline is merged.

Jan

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

[Qemu-devel] Re: [PATCH 0/8] Basic device state visualization

[Avi Kivity]

On 05/14/2010 07:24 PM, Jan Kiszka wrote:
>
>    
>> I'd like to see qmp command documentation for this.
>>
>>      
> Will add it once the doc baseline is merged.
>    

How about posting it as a patch on top of the documentation patch?  That 
allows us to review it (which IMO should be a condition for accepting 
any patch that adds or changes a monitor command) while not requiring 
merging to be delayed until after the doc patches makes it in.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCH 3/8] Add QBuffer

[Anthony Liguori]

On 05/14/2010 08:20 AM, Jan Kiszka wrote:
> diff --git a/qjson.c b/qjson.c
> index 483c667..4d1c21a 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -19,7 +19,9 @@
>   #include "qlist.h"
>   #include "qbool.h"
>   #include "qfloat.h"
> +#include "qbuffer.h"
>   #include "qdict.h"
> +#include "base64.h"
>
>   typedef struct JSONParsingState
>   {
> @@ -235,6 +237,20 @@ 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 + 3);
> +
> +        buffer[0] = '"';
> +        base64_encode(qbuffer_get_data(val), data_size, buffer + 1);
> +        buffer[str_len + 1] = '"';
> +        buffer[str_len + 2] = 0;
> +        qstring_append(str, buffer);
> +        qemu_free(buffer);
> +        break;
> +    }
>    

Instead of encoding just as a string, it would be a good idea to encode 
it as something like:

{'__class__': 'base64', 'data': ...}

We've discussed using hidden properties to describe special things like 
abstract classes and since we already have this namespace reserved, I 
think it's a good time to use it.

The advantage is that in a dynamic language like Python, the parser can 
convert base64 to binary strings automatically without having to 
understand the QMP protocol.

Regards,

Anthony Liguori

>       case QTYPE_QERROR:
>           /* XXX: should QError be emitted? */
>       case QTYPE_NONE:
> diff --git a/qobject.h b/qobject.h
> index 07de211..45c4fa0 100644
> --- a/qobject.h
> +++ b/qobject.h
> @@ -44,6 +44,7 @@ typedef enum {
>       QTYPE_QFLOAT,
>       QTYPE_QBOOL,
>       QTYPE_QERROR,
> +    QTYPE_QBUFFER,
>   } qtype_code;
>
>   struct QObject;
>    

Re: [Qemu-devel] [PATCH 0/8] Basic device state visualization

[Anthony Liguori]

On 05/14/2010 08:20 AM, Jan Kiszka wrote:
> While recently fixing the SCSI reset issues, I once again had the need
> for displaying the state of involved devices. So far the common approach
> is to attach gdb to qemu (or even inject some printf). But that time I
> hacked up a 30-minute patch to dump the vmstate of any (fully converted)
> qdev device.
>
> This series now lays the ground for more sophisticated visulization. It
> adds the monitor command 'device_show<qdev-path>', freezes the vmstate
> of the addressed device, sticks it into a QMP dict, and either transmit
> this via QMP or pretty-prints it on a monitor console. Some example:
>
> (qemu) device_show /i440FX-pcihost/pci.0/piix3-usb-uhci
> dev: piix3-usb-uhci, id ""
>    dev.
>      version_id:         00000002
>      config:             a0 7d d1 00 00 00 00 00 - b0 7e d1 00 00 00 00 00
>                          ...
>      irq_state:          00 00 00 00 00 00 00 00 - 00 00 00 00 00 00 00 00
>    num_ports_vmstate:    02
>    ports[00].
>        ctrl:             0083
>    ports[01].
>        ctrl:             0080
>    cmd:                  00c1
>    status:               0000
>    intr:                 0000
>    frnum:                0077
>    fl_base_addr:         0fffc000
>    sof_timing:           40
>    status2:              00
>    frame_timer:          0000000000cb2bd0
>
> Basically, this is the level of support I recently saw in a
> demonstration of some commercial simulator as well. We are just lacking
> support for the yet unconverted devices. And I think we can even do
> better on the long term, e.g. by annotating state variables that contain
> flags, or by pretty-printing buffers like the PCI config space, or...
>
> Let's give this a start, I bet it will be helpful while adding complex
> device models like AHCI or EHCI. Looking forward to feedback!
>    

Very neat!

Regards,

Anthony Liguori

> Jan Kiszka (8):
>    qdev: Allow device addressing via 'driver.instance'
>    Add base64 encoder/decoder
>    Add QBuffer
>    monitor: Add basic device state visualization
>    qmp: Teach basic capability negotiation to python example
>    qmp: Fix python helper /wrt long return strings
>    Add QLIST_INSERT_TAIL
>    qdev: Add new devices/buses at the tail
>
>   Makefile        |    3 +-
>   Makefile.objs   |    4 +-
>   QMP/qmp-shell   |    1 +
>   QMP/qmp.py      |    2 +-
>   QMP/vm-info     |    1 +
>   base64.c        |  202 +++++++++++++++++++++++++++++++++++
>   base64.h        |   18 +++
>   check-qbuffer.c |  172 ++++++++++++++++++++++++++++++
>   configure       |    2 +-
>   hw/hw.h         |    2 +
>   hw/qdev.c       |  314 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   hw/qdev.h       |    2 +
>   qbuffer.c       |  116 ++++++++++++++++++++
>   qbuffer.h       |   33 ++++++
>   qemu-monitor.hx |   16 +++
>   qemu-queue.h    |    9 ++
>   qjson.c         |   16 +++
>   qobject.h       |    1 +
>   18 files changed, 902 insertions(+), 12 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
>
>
>    

Re: [Qemu-devel] [PATCH 0/8] Basic device state visualization

[Blue Swirl]

On 5/14/10, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> While recently fixing the SCSI reset issues, I once again had the need
>  for displaying the state of involved devices. So far the common approach
>  is to attach gdb to qemu (or even inject some printf). But that time I
>  hacked up a 30-minute patch to dump the vmstate of any (fully converted)
>  qdev device.
>
>  This series now lays the ground for more sophisticated visulization. It
>  adds the monitor command 'device_show <qdev-path>', freezes the vmstate
>  of the addressed device, sticks it into a QMP dict, and either transmit
>  this via QMP or pretty-prints it on a monitor console. Some example:
>
>  (qemu) device_show /i440FX-pcihost/pci.0/piix3-usb-uhci
>  dev: piix3-usb-uhci, id ""
>   dev.
>     version_id:         00000002
>     config:             a0 7d d1 00 00 00 00 00 - b0 7e d1 00 00 00 00 00
>                         ...
>     irq_state:          00 00 00 00 00 00 00 00 - 00 00 00 00 00 00 00 00
>   num_ports_vmstate:    02
>   ports[00].
>       ctrl:             0083
>   ports[01].
>       ctrl:             0080
>   cmd:                  00c1
>   status:               0000
>   intr:                 0000
>   frnum:                0077
>   fl_base_addr:         0fffc000
>   sof_timing:           40
>   status2:              00
>   frame_timer:          0000000000cb2bd0
>
>  Basically, this is the level of support I recently saw in a
>  demonstration of some commercial simulator as well. We are just lacking
>  support for the yet unconverted devices. And I think we can even do
>  better on the long term, e.g. by annotating state variables that contain
>  flags, or by pretty-printing buffers like the PCI config space, or...
>
>  Let's give this a start, I bet it will be helpful while adding complex
>  device models like AHCI or EHCI. Looking forward to feedback!

Nice! I think this confirms that there is no future for the qdev side
of my yesterday's patch, this is much better.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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

Anthony Liguori wrote:
> On 05/14/2010 08:20 AM, Jan Kiszka wrote:
>> diff --git a/qjson.c b/qjson.c
>> index 483c667..4d1c21a 100644
>> --- a/qjson.c
>> +++ b/qjson.c
>> @@ -19,7 +19,9 @@
>>   #include "qlist.h"
>>   #include "qbool.h"
>>   #include "qfloat.h"
>> +#include "qbuffer.h"
>>   #include "qdict.h"
>> +#include "base64.h"
>>
>>   typedef struct JSONParsingState
>>   {
>> @@ -235,6 +237,20 @@ 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 + 3);
>> +
>> +        buffer[0] = '"';
>> +        base64_encode(qbuffer_get_data(val), data_size, buffer + 1);
>> +        buffer[str_len + 1] = '"';
>> +        buffer[str_len + 2] = 0;
>> +        qstring_append(str, buffer);
>> +        qemu_free(buffer);
>> +        break;
>> +    }
>>    
> 
> Instead of encoding just as a string, it would be a good idea to encode
> it as something like:
> 
> {'__class__': 'base64', 'data': ...}
> 
> We've discussed using hidden properties to describe special things like
> abstract classes and since we already have this namespace reserved, I
> think it's a good time to use it.
> 
> The advantage is that in a dynamic language like Python, the parser can
> convert base64 to binary strings automatically without having to
> understand the QMP protocol.

Indeed, was amazingly simple to add and works nicely with qmp-shell as
demonstrator. Will repost, also to fix a few remaining glitches I came
across in the meantime.

Jan


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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/15/2010 11:45 AM, Jan Kiszka wrote:
>
>> Instead of encoding just as a string, it would be a good idea to encode
>> it as something like:
>>
>> {'__class__': 'base64', 'data': ...}
>>
>> We've discussed using hidden properties to describe special things like
>> abstract classes and since we already have this namespace reserved, I
>> think it's a good time to use it.
>>
>> The advantage is that in a dynamic language like Python, the parser can
>> convert base64 to binary strings automatically without having to
>> understand the QMP protocol.
>>      
> Indeed, was amazingly simple to add and works nicely with qmp-shell as
> demonstrator. Will repost, also to fix a few remaining glitches I came
> across in the meantime.
>
>    

Is this __class__ stuff documented anywhere?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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

Avi Kivity wrote:
> On 05/15/2010 11:45 AM, Jan Kiszka wrote:
>>
>>> Instead of encoding just as a string, it would be a good idea to encode
>>> it as something like:
>>>
>>> {'__class__': 'base64', 'data': ...}
>>>
>>> We've discussed using hidden properties to describe special things like
>>> abstract classes and since we already have this namespace reserved, I
>>> think it's a good time to use it.
>>>
>>> The advantage is that in a dynamic language like Python, the parser can
>>> convert base64 to binary strings automatically without having to
>>> understand the QMP protocol.
>>>      
>> Indeed, was amazingly simple to add and works nicely with qmp-shell as
>> demonstrator. Will repost, also to fix a few remaining glitches I came
>> across in the meantime.
>>
>>    
> 
> Is this __class__ stuff documented anywhere?
> 

Not yet. Also, we should clarify the proposed private extension section
that only "__some_key" is reserved for downstream, not
'__some_other_key__' (i.e. downstream names must not end with '__').

Jan


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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>
>> Is this __class__ stuff documented anywhere?
>>
>>      
> Not yet. Also, we should clarify the proposed private extension section
> that only "__some_key" is reserved for downstream, not
> '__some_other_key__' (i.e. downstream names must not end with '__').
>
>    

Why use such weird names at all?  What's wrong with 'class'?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Paolo Bonzini]

On 05/15/2010 07:31 PM, Avi Kivity wrote:
> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>
>>> Is this __class__ stuff documented anywhere?
>>>
>> Not yet. Also, we should clarify the proposed private extension section
>> that only "__some_key" is reserved for downstream, not
>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>
>
> Why use such weird names at all? What's wrong with 'class'?

That it conflicts with e.g. PCI classes?

Paolo

[Qemu-devel] Re: [PATCH 7/8] Add QLIST_INSERT_TAIL

[Paolo Bonzini]

On 05/14/2010 03:20 PM, Jan Kiszka wrote:
> As the QLIST has not tail pointer, this requires list walking. Still
> useful when lists are short or insertion time doesn't matter.

True, but why not switch to QTAIL instead?

Paolo

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/16/2010 12:37 PM, Paolo Bonzini wrote:
> On 05/15/2010 07:31 PM, Avi Kivity wrote:
>> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>>
>>>> Is this __class__ stuff documented anywhere?
>>>>
>>> Not yet. Also, we should clarify the proposed private extension section
>>> that only "__some_key" is reserved for downstream, not
>>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>>
>>
>> Why use such weird names at all? What's wrong with 'class'?
>
> That it conflicts with e.g. PCI classes?

Won't the context tell it apart?  When you expect a pci function, 
'class': 'video' means one thing, when you read a buffer it means another.

The only reason to use a special name is if it's a protocol level 
feature that can happen in all contexts.  But in that case we're better 
off with a schema, we don't want to push class descriptors everywhere.

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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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

Avi Kivity wrote:
> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>
>>> Is this __class__ stuff documented anywhere?
>>>
>>>      
>> Not yet. Also, we should clarify the proposed private extension section
>> that only "__some_key" is reserved for downstream, not
>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>
>>    
> 
> Why use such weird names at all?  What's wrong with 'class'?

It's too generic and we may too easily create dict keys that will be
misinterpreted.

Jan


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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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

Avi Kivity wrote:
> On 05/16/2010 12:37 PM, Paolo Bonzini wrote:
>> On 05/15/2010 07:31 PM, Avi Kivity wrote:
>>> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>>>
>>>>> Is this __class__ stuff documented anywhere?
>>>>>
>>>> Not yet. Also, we should clarify the proposed private extension section
>>>> that only "__some_key" is reserved for downstream, not
>>>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>>>
>>>
>>> Why use such weird names at all? What's wrong with 'class'?
>>
>> That it conflicts with e.g. PCI classes?
> 
> Won't the context tell it apart?  When you expect a pci function,
> 'class': 'video' means one thing, when you read a buffer it means another.

The point is to make this notation context-independent so that you can do

    def __json_obj_hook(self, dct):
        if '__class__' in dct:
            if dct['__class__'] == 'buffer':
                return base64.b64decode(dct['data'])
            else:
                return
        return dct

and

        line = json.loads(self.sockfile.readline(),
                          object_hook=self.__json_obj_hook)

i.e. parse the QMP stream into a proper representation without known QMP
at all.

> 
> The only reason to use a special name is if it's a protocol level
> feature that can happen in all contexts.  But in that case we're better
> off with a schema, we don't want to push class descriptors everywhere.
> 

Not everywhere, just into those nodes that aren't expressible with
native JSON types.

Jan


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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Paolo Bonzini]

On 05/16/2010 11:50 AM, Avi Kivity wrote:
> On 05/16/2010 12:37 PM, Paolo Bonzini wrote:
>> On 05/15/2010 07:31 PM, Avi Kivity wrote:
>>> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>>>
>>>>> Is this __class__ stuff documented anywhere?
>>>>>
>>>> Not yet. Also, we should clarify the proposed private extension section
>>>> that only "__some_key" is reserved for downstream, not
>>>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>>>
>>>
>>> Why use such weird names at all? What's wrong with 'class'?
>>
>> That it conflicts with e.g. PCI classes?
>
> Won't the context tell it apart?

Yes, of course, it you need to know the schema.  If you don't know the 
schema you don't know the context.

This QBuffer thing is something that a client QMP library could create 
automatically.  Keys in a separate namespace (like '__class__') have the 
advantage of being easily picked up automatically by a wrapper of the 
JSON parser; if you used simply 'class' such as layer would need to know 
a schema, or it wouldn't know that "context".

(BTW I'd prefer something like '__encoding__'; the word "class" suggests 
much more than what it is in reality).

Paolo

[Qemu-devel] Re: [PATCH 7/8] Add QLIST_INSERT_TAIL

[Jan Kiszka]

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

Paolo Bonzini wrote:
> On 05/14/2010 03:20 PM, Jan Kiszka wrote:
>> As the QLIST has not tail pointer, this requires list walking. Still
>> useful when lists are short or insertion time doesn't matter.
> 
> True, but why not switch to QTAIL instead?

Would be more invasive, but I can do so if it's preferred.

Jan


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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/16/2010 01:16 PM, Paolo Bonzini wrote:
> On 05/16/2010 11:50 AM, Avi Kivity wrote:
>> On 05/16/2010 12:37 PM, Paolo Bonzini wrote:
>>> On 05/15/2010 07:31 PM, Avi Kivity wrote:
>>>> On 05/15/2010 11:59 AM, Jan Kiszka wrote:
>>>>>
>>>>>> Is this __class__ stuff documented anywhere?
>>>>>>
>>>>> Not yet. Also, we should clarify the proposed private extension 
>>>>> section
>>>>> that only "__some_key" is reserved for downstream, not
>>>>> '__some_other_key__' (i.e. downstream names must not end with '__').
>>>>>
>>>>
>>>> Why use such weird names at all? What's wrong with 'class'?
>>>
>>> That it conflicts with e.g. PCI classes?
>>
>> Won't the context tell it apart?
>
> Yes, of course, it you need to know the schema.  If you don't know the 
> schema you don't know the context.
>
> This QBuffer thing is something that a client QMP library could create 
> automatically.  Keys in a separate namespace (like '__class__') have 
> the advantage of being easily picked up automatically by a wrapper of 
> the JSON parser; if you used simply 'class' such as layer would need 
> to know a schema, or it wouldn't know that "context".

Makes sense.  So this is a protocol feature and needs to be documented 
as such.

> (BTW I'd prefer something like '__encoding__'; the word "class" 
> suggests much more than what it is in reality).

Agreed.

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

Re: [Qemu-devel] [PATCH 3/8] Add QBuffer

[Jamie Lokier]

Anthony Liguori wrote:
> Instead of encoding just as a string, it would be a good idea to encode 
> it as something like:
> 
> {'__class__': 'base64', 'data': ...}

Is there a benefit to the class indirection, over simply a keyword?:

{'__base64__': ...}

__class__ seems to suggest much more than it's being used for here.

-- Jamie

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

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

Jamie Lokier wrote:
> Anthony Liguori wrote:
>> Instead of encoding just as a string, it would be a good idea to encode 
>> it as something like:
>>
>> {'__class__': 'base64', 'data': ...}
> 
> Is there a benefit to the class indirection, over simply a keyword?:
> 
> {'__base64__': ...}
> 
> __class__ seems to suggest much more than it's being used for here.
> 

Depending on how sophisticated your parser is, you could directly push
the result into an object of the proper type. And we can add more
complex objects in the future that do not only consists of a single data
key. Note that this extension is not just about encoding, it is about
typecasting (dict -> custom type).

Jan


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

Re: [Qemu-devel] [PATCH 3/8] Add QBuffer

[Anthony Liguori]

On Sun, May 16, 2010 at 12:38 PM, Jamie Lokier <jamie@shareable.org> wrote:
> Anthony Liguori wrote:
>> Instead of encoding just as a string, it would be a good idea to encode
>> it as something like:
>>
>> {'__class__': 'base64', 'data': ...}
>
> Is there a benefit to the class indirection, over simply a keyword?:
>
> {'__base64__': ...}
>
> __class__ seems to suggest much more than it's being used for here.

Yes.  The problem with JSON is that it's based on JavaScript and
JavaScript is goofy :-)

JavaScript's object mechanism doesn't map well to most other languages
since it's prototype based.  What we're calling QDict's are really
objects in JavaScript and they carry mostly no type information.  With
JS, it's very simple to treat a generic object as a specialized class
after instantiation which means objects don't need type information.

For non-prototype languages, which is the vast majority of clients,
it's necessary to have type information at instantiation time since
monkey patching is awkward at best.  That's why we need a special,
reserved, object member to carry type information.  The remainder of
the object members represent the serialized state of the object.

Another way to think of it, is that we're already transmitting objects
so we really just need a way to say, no, this isn't just a Dictionary,
it's really an instance of the following class.

Regards,

Anthony Liguori

> -- Jamie
>

Re: [Qemu-devel] [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 03:12 AM, Anthony Liguori wrote:
> On Sun, May 16, 2010 at 12:38 PM, Jamie Lokier<jamie@shareable.org>  wrote:
>    
>> Anthony Liguori wrote:
>>      
>>> Instead of encoding just as a string, it would be a good idea to encode
>>> it as something like:
>>>
>>> {'__class__': 'base64', 'data': ...}
>>>        
>> Is there a benefit to the class indirection, over simply a keyword?:
>>
>> {'__base64__': ...}
>>
>> __class__ seems to suggest much more than it's being used for here.
>>      
> Yes.  The problem with JSON is that it's based on JavaScript and
> JavaScript is goofy :-)
>
>    

I suggest completely ignoring JavaScript.  JSON is simply an encoding 
for numbers, strings, arrays, and key-value stores.  Where's the goofiness?

> JavaScript's object mechanism doesn't map well to most other languages
> since it's prototype based.  What we're calling QDict's are really
> objects in JavaScript and they carry mostly no type information.  With
> JS, it's very simple to treat a generic object as a specialized class
> after instantiation which means objects don't need type information.
>
> For non-prototype languages, which is the vast majority of clients,
> it's necessary to have type information at instantiation time since
> monkey patching is awkward at best.  That's why we need a special,
> reserved, object member to carry type information.  The remainder of
> the object members represent the serialized state of the object.
>    

The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type 
information (you can't even distinguish between a number and text) yet C 
clients have to problem extracting typed information from it.

Having __class__ everywhere means we're carrying the schema in every 
message instead of just once.

> Another way to think of it, is that we're already transmitting objects
> so we really just need a way to say, no, this isn't just a Dictionary,
> it's really an instance of the following class.
>    

Are there cases where the receiver cannot infer this from the context?

As I see it, dynamic type information is easiest for dynamicically typed 
languages.  You just have a dict of class names -> object constructor 
and call the constructors at runtime.  Statically typed languages will 
need a schema to use objects, since the field types have to be known at 
compile time, not just run time.

Another wart is arrays: statically typed languages usually contain only 
objects of the same type, but here this isn't known until we process the 
first member.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

Avi Kivity wrote:
> On 05/17/2010 03:12 AM, Anthony Liguori wrote:
>> On Sun, May 16, 2010 at 12:38 PM, Jamie Lokier<jamie@shareable.org> 
>> wrote:
>>   
>>> Anthony Liguori wrote:
>>>     
>>>> Instead of encoding just as a string, it would be a good idea to encode
>>>> it as something like:
>>>>
>>>> {'__class__': 'base64', 'data': ...}
>>>>        
>>> Is there a benefit to the class indirection, over simply a keyword?:
>>>
>>> {'__base64__': ...}
>>>
>>> __class__ seems to suggest much more than it's being used for here.
>>>      
>> Yes.  The problem with JSON is that it's based on JavaScript and
>> JavaScript is goofy :-)
>>
>>    
> 
> I suggest completely ignoring JavaScript.  JSON is simply an encoding
> for numbers, strings, arrays, and key-value stores.  Where's the goofiness?
> 
>> JavaScript's object mechanism doesn't map well to most other languages
>> since it's prototype based.  What we're calling QDict's are really
>> objects in JavaScript and they carry mostly no type information.  With
>> JS, it's very simple to treat a generic object as a specialized class
>> after instantiation which means objects don't need type information.
>>
>> For non-prototype languages, which is the vast majority of clients,
>> it's necessary to have type information at instantiation time since
>> monkey patching is awkward at best.  That's why we need a special,
>> reserved, object member to carry type information.  The remainder of
>> the object members represent the serialized state of the object.
>>    
> 
> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
> information (you can't even distinguish between a number and text) yet C
> clients have to problem extracting typed information from it.
> 
> Having __class__ everywhere means we're carrying the schema in every
> message instead of just once.

The device_show command is already an example where you don't have a
predefined schema. It is derived from the data stream the encodes the
vmstate fields. So far we have no collision between base64-encoded
buffers and real strings, but this may actually change when we start
annotating the fields with symbolic constants.

I really don't see the problem with __class__. Being a text protocol,
JSON is already fairly verbose.

Jan

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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>
>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
>> information (you can't even distinguish between a number and text) yet C
>> clients have to problem extracting typed information from it.
>>
>> Having __class__ everywhere means we're carrying the schema in every
>> message instead of just once.
>>      
> The device_show command is already an example where you don't have a
> predefined schema. It is derived from the data stream the encodes the
> vmstate fields. So far we have no collision between base64-encoded
> buffers and real strings, but this may actually change when we start
> annotating the fields with symbolic constants.
>    

What is the receiver to do with it?

If it doesn't know the schema (and there is no schema), then all it can 
do is display the key/values.  If it does know the schema, then 
__class__ is unnecessary.

My worry is that __class__ will make the protocol more ad-hoc.

> I really don't see the problem with __class__. Being a text protocol,
> JSON is already fairly verbose.
>    

The problem is not the verbosity, it's that information is carried too 
late.  Many clients want to know this information at compile time or 
initialization time, and we are providing it at object instantiating time.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

Avi Kivity wrote:
> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
>>> information (you can't even distinguish between a number and text) yet C
>>> clients have to problem extracting typed information from it.
>>>
>>> Having __class__ everywhere means we're carrying the schema in every
>>> message instead of just once.
>>>      
>> The device_show command is already an example where you don't have a
>> predefined schema. It is derived from the data stream the encodes the
>> vmstate fields. So far we have no collision between base64-encoded
>> buffers and real strings, but this may actually change when we start
>> annotating the fields with symbolic constants.
>>    
> 
> What is the receiver to do with it?
> 
> If it doesn't know the schema (and there is no schema), then all it can 
> do is display the key/values.  If it does know the schema, then 
> __class__ is unnecessary.

There is a schema describing the fields (name, size, number of
elements), but their types (int, buffer, sub-field, array of X) are
derived from the JSON objects (ie. the JSON parser does this job).

> 
> My worry is that __class__ will make the protocol more ad-hoc.
> 
>> I really don't see the problem with __class__. Being a text protocol,
>> JSON is already fairly verbose.
>>    
> 
> The problem is not the verbosity, it's that information is carried too 
> late.  Many clients want to know this information at compile time or 
> initialization time, and we are providing it at object instantiating time.

What clients do you have in mind?

Jan

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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 10:57 AM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>      
>>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
>>>> information (you can't even distinguish between a number and text) yet C
>>>> clients have to problem extracting typed information from it.
>>>>
>>>> Having __class__ everywhere means we're carrying the schema in every
>>>> message instead of just once.
>>>>
>>>>          
>>> The device_show command is already an example where you don't have a
>>> predefined schema. It is derived from the data stream the encodes the
>>> vmstate fields. So far we have no collision between base64-encoded
>>> buffers and real strings, but this may actually change when we start
>>> annotating the fields with symbolic constants.
>>>
>>>        
>> What is the receiver to do with it?
>>
>> If it doesn't know the schema (and there is no schema), then all it can
>> do is display the key/values.  If it does know the schema, then
>> __class__ is unnecessary.
>>      
> There is a schema describing the fields (name, size, number of
> elements),

Surely the schema has to describe the type as well?  If it does, you can 
use the schema to generate a classes at compile time.

>   but their types (int, buffer, sub-field, array of X) are
> derived from the JSON objects (ie. the JSON parser does this job).
>    

The names of fields are also type information.


>>> I really don't see the problem with __class__. Being a text protocol,
>>> JSON is already fairly verbose.
>>>
>>>        
>> The problem is not the verbosity, it's that information is carried too
>> late.  Many clients want to know this information at compile time or
>> initialization time, and we are providing it at object instantiating time.
>>      
> What clients do you have in mind?
>
>    

Any client that doesn't allow object types to be created dynamically; C, 
C++, Java, and the like could all benefit from a schema and wouldn't be 
able to do much with __class__ unless all classes were predefined.  
Python, JavaScript, and the like wouldn't care.

Another way of looking at it: if the client sees { __class__: foo, f1: 
10, f2: 9 }, it cannot derive any information from __class__ unless it 
was aware of foo beforehand.  If that's the case, let's make it part of 
the schema so it is available at compile time instead of runtime.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 11:10 AM, Avi Kivity wrote:
>
> Another way of looking at it: if the client sees { __class__: foo, f1: 
> 10, f2: 9 }, it cannot derive any information from __class__ unless it 
> was aware of foo beforehand.  If that's the case, let's make it part 
> of the schema so it is available at compile time instead of runtime.
>

Or both.  That's similar to a non-object field's type being described 
both in the schema at during runtime.  A static parser can verify 
__class__ matches the expectations, a dynamic parser can look up 
__class__ and call the appropriate constructor.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

Avi Kivity wrote:
> On 05/17/2010 10:57 AM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>>      
>>>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
>>>>> information (you can't even distinguish between a number and text) yet C
>>>>> clients have to problem extracting typed information from it.
>>>>>
>>>>> Having __class__ everywhere means we're carrying the schema in every
>>>>> message instead of just once.
>>>>>
>>>>>          
>>>> The device_show command is already an example where you don't have a
>>>> predefined schema. It is derived from the data stream the encodes the
>>>> vmstate fields. So far we have no collision between base64-encoded
>>>> buffers and real strings, but this may actually change when we start
>>>> annotating the fields with symbolic constants.
>>>>
>>>>        
>>> What is the receiver to do with it?
>>>
>>> If it doesn't know the schema (and there is no schema), then all it can
>>> do is display the key/values.  If it does know the schema, then
>>> __class__ is unnecessary.
>>>      
>> There is a schema describing the fields (name, size, number of
>> elements),
> 
> Surely the schema has to describe the type as well?  If it does, you can 
> use the schema to generate a classes at compile time.
> 
>>   but their types (int, buffer, sub-field, array of X) are
>> derived from the JSON objects (ie. the JSON parser does this job).
>>    
> 
> The names of fields are also type information.

Not in the case of device_show. The clients have no idea of the vmstate
structures before they were transfered. Granted, that will likely remain
a special case in the QMP command set.

> 
>>>> I really don't see the problem with __class__. Being a text protocol,
>>>> JSON is already fairly verbose.
>>>>
>>>>        
>>> The problem is not the verbosity, it's that information is carried too
>>> late.  Many clients want to know this information at compile time or
>>> initialization time, and we are providing it at object instantiating time.
>>>      
>> What clients do you have in mind?
>>
>>    
> 
> Any client that doesn't allow object types to be created dynamically; C, 
> C++, Java, and the like could all benefit from a schema and wouldn't be 
> able to do much with __class__ unless all classes were predefined.  
> Python, JavaScript, and the like wouldn't care.
> 
> Another way of looking at it: if the client sees { __class__: foo, f1: 
> 10, f2: 9 }, it cannot derive any information from __class__ unless it 
> was aware of foo beforehand.  If that's the case, let's make it part of 
> the schema so it is available at compile time instead of runtime.
> 

Maybe a misunderstanding on my side: I'm not arguing against predefining
what __class__ values exists and how dicts that carry such keys are encoded.

Jan

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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 11:55 AM, Jan Kiszka wrote:
>
>> The names of fields are also type information.
>>      
> Not in the case of device_show. The clients have no idea of the vmstate
> structures before they were transfered. Granted, that will likely remain
> a special case in the QMP command set.
>    

For that use case, I agree.  Maybe we should send both the parsed and 
unparsed information.

But if the client isn't going to interpret the object and only display 
it, then there is no need for __class__?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jan Kiszka]

Avi Kivity wrote:
> On 05/17/2010 11:55 AM, Jan Kiszka wrote:
>>> The names of fields are also type information.
>>>      
>> Not in the case of device_show. The clients have no idea of the vmstate
>> structures before they were transfered. Granted, that will likely remain
>> a special case in the QMP command set.
>>    
> 
> For that use case, I agree.  Maybe we should send both the parsed and 
> unparsed information.

Now I can't parse what you mean.

> 
> But if the client isn't going to interpret the object and only display 
> it, then there is no need for __class__?

For that uncommon case, yes. But the common one is to perform a bit more
than raw JSON dictionary printing.

Jan

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

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/17/2010 12:17 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 05/17/2010 11:55 AM, Jan Kiszka wrote:
>>      
>>>> The names of fields are also type information.
>>>>
>>>>          
>>> Not in the case of device_show. The clients have no idea of the vmstate
>>> structures before they were transfered. Granted, that will likely remain
>>> a special case in the QMP command set.
>>>
>>>        
>> For that use case, I agree.  Maybe we should send both the parsed and
>> unparsed information.
>>      
> Now I can't parse what you mean.
>    


Sorry.  I meant that if we have a raw buffer that we can decode (like 
pci config space and its fields) maybe it makes sense to send both 
formats.  The raw buffer is something likely to be stable, and the 
decoded fields are more readable.  But I realize now that doesn't make 
sense in the context of base64 encoding which started all this.


>> But if the client isn't going to interpret the object and only display
>> it, then there is no need for __class__?
>>      
> For that uncommon case, yes. But the common one is to perform a bit more
> than raw JSON dictionary printing.
>    

But the common one is likely to know the type of the object beforehand 
(or it can't do much beyond printing, since it has no idea what fields 
to expect).


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Anthony Liguori]

On 05/17/2010 02:45 AM, Avi Kivity wrote:
> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>
>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any 
>>> type
>>> information (you can't even distinguish between a number and text) 
>>> yet C
>>> clients have to problem extracting typed information from it.
>>>
>>> Having __class__ everywhere means we're carrying the schema in every
>>> message instead of just once.
>> The device_show command is already an example where you don't have a
>> predefined schema. It is derived from the data stream the encodes the
>> vmstate fields. So far we have no collision between base64-encoded
>> buffers and real strings, but this may actually change when we start
>> annotating the fields with symbolic constants.
>
> What is the receiver to do with it?
>
> If it doesn't know the schema (and there is no schema), then all it 
> can do is display the key/values.  If it does know the schema, then 
> __class__ is unnecessary.
>
> My worry is that __class__ will make the protocol more ad-hoc.
>
>> I really don't see the problem with __class__. Being a text protocol,
>> JSON is already fairly verbose.
>
> The problem is not the verbosity, it's that information is carried too 
> late.  Many clients want to know this information at compile time or 
> initialization time, and we are providing it at object instantiating 
> time.

Whether a protocol is self-describing is orthogonal to whether it's well 
defined (ala a schema).  A self-describing protocol is very convenient 
for dynamic languages like Python.  We should also provide a formal 
schema though for languages that require IDL to generate bindings (like C).

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Jamie Lokier]

Jan Kiszka wrote:
> Jamie Lokier wrote:
> > Anthony Liguori wrote:
> >> Instead of encoding just as a string, it would be a good idea to encode 
> >> it as something like:
> >>
> >> {'__class__': 'base64', 'data': ...}
> > 
> > Is there a benefit to the class indirection, over simply a keyword?:
> > 
> > {'__base64__': ...}
> > 
> > __class__ seems to suggest much more than it's being used for here.
> > 
> 
> Depending on how sophisticated your parser is, you could directly push
> the result into an object of the proper type. And we can add more
> complex objects in the future that do not only consists of a single data
> key. Note that this extension is not just about encoding, it is about
> typecasting (dict -> custom type).

Sure, if that's the plan.

Does it make sense to combine encoding and custom types in this way?
It looks like mixing syntax and semantics, which has consequences for
code using generic parsers with separate semantic layer, but I realise
there's no "correct" answer.

Back to the syntax: I'm under the impression from earlier discussion
that the '__*__' keyspace reserved, so even types could use the
compact syntax?

Or is there something Javascript-ish (and not merely JSON-ish) about
'__class__' in particular which makes it appropriate?

-- Jamie

Re: [Qemu-devel] [PATCH 4/8] monitor: Add basic device state visualization

[Markus Armbruster]

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

> This introduces device_show, a monitor command that saves the vmstate of
> a qdev device and visualizes it. QMP is also supported. 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.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Neato!  Comments inline.

> ---
>  hw/hw.h         |    2 +
>  hw/qdev.c       |  287 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h       |    2 +
>  qemu-monitor.hx |   16 +++
>  4 files changed, 307 insertions(+), 0 deletions(-)
>
> diff --git a/hw/hw.h b/hw/hw.h
> index 328b704..1ff8e40 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -299,6 +299,7 @@ enum VMStateFlags {
>  
>  typedef struct {
>      const char *name;
> +    const char *start_index;
>      size_t offset;
>      size_t size;
>      size_t start;

Why is start_index a string?

> @@ -414,6 +415,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 fe49e71..c989010 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;
>  
> @@ -824,3 +827,287 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      }
>      return qdev_unplug(dev);
>  }
> +
> +static int print_array_index(Monitor *mon, const char *start, int index)
> +{
> +    char index_str[32];
> +    int len;
> +
> +    if (start) {
> +        len = snprintf(index_str, sizeof(index_str), "[%s+%02x]", start,
> +                       index);
> +    } else {
> +        len = snprintf(index_str, sizeof(index_str), "[%02x]", index);
> +    }
> +    monitor_printf(mon, index_str);
> +    return len;

You go via the snprintf() detour just because monitor_printf() doesn't
return the number of characters printed.  Wouldn't it be better to fix
that?

> +}
> +
> +#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*x\n", (int)size * 2,
> +                       (int)qint_get_int(qobject_to_qint(qelem)));

I'm confused.  Doesn't casting qint_get_int() to int lose bits?

> +        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);
> +            pos += print_array_index(mon, start, 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\"\n",
> +                   qdict_get_str(qdict, "device"),
> +                   qdict_get_str(qdict, "id"));
> +
> +    QLIST_FOREACH_ENTRY(qlist, entry) {
> +        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
> +    }
> +}
> +
> +static void parse_vmstate(const VMStateDescription *vmsd, void *opaque,
> +                          QList *qlist, int full_buffers)
> +{
> +    VMStateField *field = vmsd->fields;
> +
> +    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;
> +            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;
> +                }
> +            }
> +            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(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) {
> +                    parse_vmstate(field->vmsd, addr, sub_elems, full_buffers);
> +                } else {
> +                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
> +                        if (!full_buffers && size > 16) {
> +                            size = 16;
> +                        }
> +                        qlist_append_obj(sub_elems,
> +                                         QOBJECT(qbuffer_from_data(addr,
> +                                                                   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)));
> +                    }
> +                }
> +            }
> +        }
> +        field++;
> +    }
> +}
> +
> +static DeviceState *qdev_find(const char *path)
> +{
> +    const char *dev_name;
> +    DeviceState *dev;
> +    char *bus_path;
> +    BusState *bus;
> +
> +    dev_name = strrchr(path, '/');
> +    if (!dev_name) {
> +        bus = main_system_bus;
> +        dev_name = path;
> +    } else {
> +        dev_name++;
> +        bus_path = qemu_strdup(path);
> +        bus_path[dev_name - path] = 0;
> +
> +        bus = qbus_find(bus_path);
> +        qemu_free(bus_path);
> +
> +        if (!bus) {
> +            /* qbus_find already reported the error */
> +            return NULL;
> +        }
> +    }
> +    dev = qbus_find_dev(bus, dev_name);
> +    if (!dev) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
> +        if (!monitor_cur_is_qmp()) {
> +            qbus_list_dev(bus);
> +        }
> +    }
> +    return dev;
> +}
> +
> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)

Command documentation missing.

> +{
> +    const char *path = qdict_get_str(qdict, "path");
> +    const VMStateDescription *vmsd;
> +    DeviceState *dev;
> +    QList *qlist;
> +
> +    dev = qdev_find_recursive(main_system_bus, path);
> +    if (!dev) {
> +        dev = qdev_find(path);
> +        if (!dev) {
> +            return -1;
> +        }
> +    }

This is inconsistent with how do_device_del() looks up devices by ID.
Let's agree on one lookup, put it into a function, and use it
everywhere.

> +
> +    vmsd = dev->info->vmsd;
> +    if (!vmsd) {
> +        qerror_report(QERR_UNDEFINED_ERROR);
> +        if (!monitor_cur_is_qmp()) {
> +            error_printf_unless_qmp("Device '%s' does not support state "
> +                                    "dumping\n", path);
> +        }

Please use a suitable QERR_.  Define one if necessary.

> +        return -1;
> +    }
> +
> +    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s }",
> +                                   dev->info->name, dev->id ? : "");
> +    qlist = qlist_new();
> +    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
> +    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
> +
> +    return 0;
> +}
> diff --git a/hw/qdev.h b/hw/qdev.h
> index d8fbc73..f8436ec 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -177,6 +177,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 a8f194c..449f012 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -599,6 +599,22 @@ Remove device @var{id}.
>  ETEXI
>  
>      {
> +        .name       = "device_show",
> +        .args_type  = "path:s,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,
> +    },
> +
> +STEXI
> +@item device_show @var{id} [@code{-f}]
> +
> +Show state of device @var{id} in a human-readable form. Buffers are cut after
> +16 bytes unless a full dump is requested via @code{-f} 
> +ETEXI
> +
> +    {
>          .name       = "cpu",
>          .args_type  = "index:i",
>          .params     = "index",

Re: [Qemu-devel] [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Markus Armbruster]

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

> Extend qbus_find_dev to allow addressing of devices without an unique id
> via an optional instance number. The new formats are 'driver.instance'
> and 'alias.instance'.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

How's the instance number defined?  Should be documented.

Is an instance number a good user interface?  Gerd, what do you think?

[...]

Re: [Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Markus Armbruster]

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

> Avi Kivity wrote:
>> On 05/17/2010 10:57 AM, Jan Kiszka wrote:
>>> Avi Kivity wrote:
>>>    
>>>> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>>>      
>>>>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry any type
>>>>>> information (you can't even distinguish between a number and text) yet C
>>>>>> clients have to problem extracting typed information from it.
>>>>>>
>>>>>> Having __class__ everywhere means we're carrying the schema in every
>>>>>> message instead of just once.
>>>>>>
>>>>>>          
>>>>> The device_show command is already an example where you don't have a
>>>>> predefined schema. It is derived from the data stream the encodes the
>>>>> vmstate fields. So far we have no collision between base64-encoded
>>>>> buffers and real strings, but this may actually change when we start
>>>>> annotating the fields with symbolic constants.
>>>>>
>>>>>        
>>>> What is the receiver to do with it?
>>>>
>>>> If it doesn't know the schema (and there is no schema), then all it can
>>>> do is display the key/values.  If it does know the schema, then
>>>> __class__ is unnecessary.
>>>>      
>>> There is a schema describing the fields (name, size, number of
>>> elements),
>> 
>> Surely the schema has to describe the type as well?  If it does, you can 
>> use the schema to generate a classes at compile time.

Doesn't that tie you to a specific version of QMP at compile-time?

>>>   but their types (int, buffer, sub-field, array of X) are
>>> derived from the JSON objects (ie. the JSON parser does this job).
>>>    
>> 
>> The names of fields are also type information.
>
> Not in the case of device_show. The clients have no idea of the vmstate
> structures before they were transfered. Granted, that will likely remain
> a special case in the QMP command set.

qdev device properties are similar.  Right now, they occur only as
arguments of device_add.  When do_info_qtree() gets converted, they'll
appear in results.

[...]

Re: [Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/17/2010 02:45 AM, Avi Kivity wrote:
>> On 05/17/2010 10:40 AM, Jan Kiszka wrote:
>>>
>>>> The alternative is to have a schema.  Sun RPC/XDR doesn't carry
>>>> any type
>>>> information (you can't even distinguish between a number and text)
>>>> yet C
>>>> clients have to problem extracting typed information from it.
>>>>
>>>> Having __class__ everywhere means we're carrying the schema in every
>>>> message instead of just once.
>>> The device_show command is already an example where you don't have a
>>> predefined schema. It is derived from the data stream the encodes the
>>> vmstate fields. So far we have no collision between base64-encoded
>>> buffers and real strings, but this may actually change when we start
>>> annotating the fields with symbolic constants.
>>
>> What is the receiver to do with it?
>>
>> If it doesn't know the schema (and there is no schema), then all it
>> can do is display the key/values.  If it does know the schema, then
>> __class__ is unnecessary.
>>
>> My worry is that __class__ will make the protocol more ad-hoc.
>>
>>> I really don't see the problem with __class__. Being a text protocol,
>>> JSON is already fairly verbose.
>>
>> The problem is not the verbosity, it's that information is carried
>> too late.  Many clients want to know this information at compile
>> time or initialization time, and we are providing it at object
>> instantiating time.
>
> Whether a protocol is self-describing is orthogonal to whether it's
> well defined (ala a schema).  A self-describing protocol is very
> convenient for dynamic languages like Python.  We should also provide
> a formal schema though for languages that require IDL to generate
> bindings (like C).

And that schema should be available over QMP.

Re: [Qemu-devel] [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Gerd Hoffmann]

On 05/18/10 14:15, Markus Armbruster wrote:
> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>
>> Extend qbus_find_dev to allow addressing of devices without an unique id
>> via an optional instance number. The new formats are 'driver.instance'
>> and 'alias.instance'.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>
> How's the instance number defined?  Should be documented.

savevm instance id, used to identify multiple instances of some device 
on loadvm.  By default is just incrementing (0,1,2,...) for each new 
device instance I think.  Drivers can also specify one.  Most don't do 
that.  IIRC some ISA drivers use the base ioport as instance id, which 
sort-of makes sense as it makes sure the id identifies the correct 
device no matter what the initialization order is.

It probably makes sense to replace the instance id with the device path 
once all devices are converted over to qdev+vmstate, so we avoid the 
nasty ordering issues altogether.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.
>
> savevm instance id, used to identify multiple instances of some device
> on loadvm.  By default is just incrementing (0,1,2,...) for each new
> device instance I think.  Drivers can also specify one.  Most don't do
> that.  IIRC some ISA drivers use the base ioport as instance id, which
> sort-of makes sense as it makes sure the id identifies the correct
> device no matter what the initialization order is.
>
> It probably makes sense to replace the instance id with the device
> path once all devices are converted over to qdev+vmstate, so we avoid
> the nasty ordering issues altogether.

Agreed.  The problem here is that we sent the instance_id on the wire,
so for "legacy" devices that used an instance_id != -1, we are stuck
with it :(

Guess why a friend calls "backwards compatibility": A Curse of Bible
proportions :(

Later, Juan.

[Qemu-devel] Re: [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Gerd Hoffmann]

   Hi,

> Agreed.  The problem here is that we sent the instance_id on the wire,
> so for "legacy" devices that used an instance_id != -1, we are stuck
> with it :(

Another way would be to fill the instance id with something useful, say 
encode the pci address there for pci devices.  That should be good 
enough to reliably identify devices even after hot-plugging them in and out.

Of course this has transition isses too.  But maybe it is easier to 
handle than replacing instance_id with something entirely different.

And it also makes the instance id less useful to address devices in a 
human-friendly way.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Jan Kiszka]

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

Gerd Hoffmann wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.

OK, will do. For now it is the per-bus instance ID. I think that makes
most sense and is easily handleable. Still, we should probably print it
also via "info qtree" (and a future "query-qtree").

> 
> savevm instance id, used to identify multiple instances of some device
> on loadvm.  By default is just incrementing (0,1,2,...) for each new
> device instance I think.  Drivers can also specify one.  Most don't do
> that.  IIRC some ISA drivers use the base ioport as instance id, which
> sort-of makes sense as it makes sure the id identifies the correct
> device no matter what the initialization order is.

That io-address-based instance numbers have just been deprecated, see
4d2ffa08b601bdd40d9ccf225480c0a7e90ca078. ISA devices are already
converted, there are just a few non-PC devices remaining that don't use
the (auto-generated) vmstate instance number. But that is actually a
different, user-invisible numbering scheme.

> 
> It probably makes sense to replace the instance id with the device path
> once all devices are converted over to qdev+vmstate, so we avoid the
> nasty ordering issues altogether.

You are always free to address devices via a unique user-defined ID.

Jan


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

[Qemu-devel] Re: [PATCH 4/8] monitor: Add basic device state visualization

[Jan Kiszka]

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

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> This introduces device_show, a monitor command that saves the vmstate of
>> a qdev device and visualizes it. QMP is also supported. 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.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Neato!  Comments inline.
> 
>> ---
>>  hw/hw.h         |    2 +
>>  hw/qdev.c       |  287 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/qdev.h       |    2 +
>>  qemu-monitor.hx |   16 +++
>>  4 files changed, 307 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index 328b704..1ff8e40 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -299,6 +299,7 @@ enum VMStateFlags {
>>  
>>  typedef struct {
>>      const char *name;
>> +    const char *start_index;
>>      size_t offset;
>>      size_t size;
>>      size_t start;
> 
> Why is start_index a string?

It can, actually should be a symbolic constant (from e1000:
"mac_reg[RA+03]: ...", ie. "RA" is the start index here).

> 
>> @@ -414,6 +415,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 fe49e71..c989010 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;
>>  
>> @@ -824,3 +827,287 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>      }
>>      return qdev_unplug(dev);
>>  }
>> +
>> +static int print_array_index(Monitor *mon, const char *start, int index)
>> +{
>> +    char index_str[32];
>> +    int len;
>> +
>> +    if (start) {
>> +        len = snprintf(index_str, sizeof(index_str), "[%s+%02x]", start,
>> +                       index);
>> +    } else {
>> +        len = snprintf(index_str, sizeof(index_str), "[%02x]", index);
>> +    }
>> +    monitor_printf(mon, index_str);
>> +    return len;
> 
> You go via the snprintf() detour just because monitor_printf() doesn't
> return the number of characters printed.  Wouldn't it be better to fix
> that?

Yeah, good point. Will address this.

> 
>> +}
>> +
>> +#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*x\n", (int)size * 2,
>> +                       (int)qint_get_int(qobject_to_qint(qelem)));
> 
> I'm confused.  Doesn't casting qint_get_int() to int lose bits?

You are right, will fix!

> 
>> +        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);
>> +            pos += print_array_index(mon, start, 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\"\n",
>> +                   qdict_get_str(qdict, "device"),
>> +                   qdict_get_str(qdict, "id"));
>> +
>> +    QLIST_FOREACH_ENTRY(qlist, entry) {
>> +        print_field(mon, qobject_to_qdict(qlist_entry_obj(entry)), 2);
>> +    }
>> +}
>> +
>> +static void parse_vmstate(const VMStateDescription *vmsd, void *opaque,
>> +                          QList *qlist, int full_buffers)
>> +{
>> +    VMStateField *field = vmsd->fields;
>> +
>> +    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;
>> +            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;
>> +                }
>> +            }
>> +            qdict_put_obj(qfield, "size", QOBJECT(qint_from_int(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) {
>> +                    parse_vmstate(field->vmsd, addr, sub_elems, full_buffers);
>> +                } else {
>> +                    if (field->flags & (VMS_BUFFER | VMS_VBUFFER)) {
>> +                        if (!full_buffers && size > 16) {
>> +                            size = 16;
>> +                        }
>> +                        qlist_append_obj(sub_elems,
>> +                                         QOBJECT(qbuffer_from_data(addr,
>> +                                                                   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)));
>> +                    }
>> +                }
>> +            }
>> +        }
>> +        field++;
>> +    }
>> +}
>> +
>> +static DeviceState *qdev_find(const char *path)
>> +{
>> +    const char *dev_name;
>> +    DeviceState *dev;
>> +    char *bus_path;
>> +    BusState *bus;
>> +
>> +    dev_name = strrchr(path, '/');
>> +    if (!dev_name) {
>> +        bus = main_system_bus;
>> +        dev_name = path;
>> +    } else {
>> +        dev_name++;
>> +        bus_path = qemu_strdup(path);
>> +        bus_path[dev_name - path] = 0;
>> +
>> +        bus = qbus_find(bus_path);
>> +        qemu_free(bus_path);
>> +
>> +        if (!bus) {
>> +            /* qbus_find already reported the error */
>> +            return NULL;
>> +        }
>> +    }
>> +    dev = qbus_find_dev(bus, dev_name);
>> +    if (!dev) {
>> +        qerror_report(QERR_DEVICE_NOT_FOUND, dev_name);
>> +        if (!monitor_cur_is_qmp()) {
>> +            qbus_list_dev(bus);
>> +        }
>> +    }
>> +    return dev;
>> +}
>> +
>> +int do_device_show(Monitor *mon, const QDict *qdict, QObject **ret_data)
> 
> Command documentation missing.

In the make (depends on QMP doc infrastructure).

> 
>> +{
>> +    const char *path = qdict_get_str(qdict, "path");
>> +    const VMStateDescription *vmsd;
>> +    DeviceState *dev;
>> +    QList *qlist;
>> +
>> +    dev = qdev_find_recursive(main_system_bus, path);
>> +    if (!dev) {
>> +        dev = qdev_find(path);
>> +        if (!dev) {
>> +            return -1;
>> +        }
>> +    }
> 
> This is inconsistent with how do_device_del() looks up devices by ID.
> Let's agree on one lookup, put it into a function, and use it
> everywhere.

Will adjust device_del to the same scheme.

BTW, qdev path completion will also be available for both commands in my
next series.

> 
>> +
>> +    vmsd = dev->info->vmsd;
>> +    if (!vmsd) {
>> +        qerror_report(QERR_UNDEFINED_ERROR);
>> +        if (!monitor_cur_is_qmp()) {
>> +            error_printf_unless_qmp("Device '%s' does not support state "
>> +                                    "dumping\n", path);
>> +        }
> 
> Please use a suitable QERR_.  Define one if necessary.

Already fixed in my tree (forgot this bit for v1). Just this redundant
qmp check was still present.

> 
>> +        return -1;
>> +    }
>> +
>> +    *ret_data = qobject_from_jsonf("{ 'device': %s, 'id': %s }",
>> +                                   dev->info->name, dev->id ? : "");
>> +    qlist = qlist_new();
>> +    parse_vmstate(vmsd, dev, qlist, qdict_get_int(qdict, "full"));
>> +    qdict_put_obj(qobject_to_qdict(*ret_data), "fields", QOBJECT(qlist));
>> +
>> +    return 0;
>> +}
>> diff --git a/hw/qdev.h b/hw/qdev.h
>> index d8fbc73..f8436ec 100644
>> --- a/hw/qdev.h
>> +++ b/hw/qdev.h
>> @@ -177,6 +177,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 a8f194c..449f012 100644
>> --- a/qemu-monitor.hx
>> +++ b/qemu-monitor.hx
>> @@ -599,6 +599,22 @@ Remove device @var{id}.
>>  ETEXI
>>  
>>      {
>> +        .name       = "device_show",
>> +        .args_type  = "path:s,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,
>> +    },
>> +
>> +STEXI
>> +@item device_show @var{id} [@code{-f}]
>> +
>> +Show state of device @var{id} in a human-readable form. Buffers are cut after
>> +16 bytes unless a full dump is requested via @code{-f} 
>> +ETEXI
>> +
>> +    {
>>          .name       = "cpu",
>>          .args_type  = "index:i",
>>          .params     = "index",
> 
> 

Thanks for the comments!

Jan


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

Re: [Qemu-devel] Re: [PATCH 3/8] Add QBuffer

[Avi Kivity]

On 05/18/2010 03:27 PM, Markus Armbruster wrote:
>
>>> Surely the schema has to describe the type as well?  If it does, you can
>>> use the schema to generate a classes at compile time.
>>>        
> Doesn't that tie you to a specific version of QMP at compile-time?
>    

The client needs to ignore anything not provided by the schema to be 
forward compatible.

(alternatively we make sure anything new is explicitly enabled by the 
client, so it can parse strictly according to the schema).

>    
>>>>    but their types (int, buffer, sub-field, array of X) are
>>>> derived from the JSON objects (ie. the JSON parser does this job).
>>>>
>>>>          
>>> The names of fields are also type information.
>>>        
>> Not in the case of device_show. The clients have no idea of the vmstate
>> structures before they were transfered. Granted, that will likely remain
>> a special case in the QMP command set.
>>      
> qdev device properties are similar.  Right now, they occur only as
> arguments of device_add.  When do_info_qtree() gets converted, they'll
> appear in results.
>    

This sounds like a horror movie in the making, "sysfs: the return".  The 
qdev tree is completely undocumented, so once again the code dictates 
the protocol.  Any bug in the qdev hierarchy will be hardcoded forever 
and ever, or we have to add a new layer of indirection to have a 
separate internal qdev and an external qdev-for-qmp tree(s).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 1/8] qdev: Allow device addressing via 'driver.instance'

[Avi Kivity]

On 05/18/2010 03:31 PM, Gerd Hoffmann wrote:
> On 05/18/10 14:15, Markus Armbruster wrote:
>> Jan Kiszka<jan.kiszka@siemens.com>  writes:
>>
>>> Extend qbus_find_dev to allow addressing of devices without an 
>>> unique id
>>> via an optional instance number. The new formats are 'driver.instance'
>>> and 'alias.instance'.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> How's the instance number defined?  Should be documented.
>
> savevm instance id, used to identify multiple instances of some device 
> on loadvm.  By default is just incrementing (0,1,2,...) for each new 
> device instance I think.

That's an implementation detail that's being exposed in an external 
interface.  Granted, users shouldn't expect this to remain stable across 
invocations, yet it makes me uncomfortable.

Why not always use topology to locate devices?


>   Drivers can also specify one.  Most don't do that.  IIRC some ISA 
> drivers use the base ioport as instance id, which sort-of makes sense 
> as it makes sure the id identifies the correct device no matter what 
> the initialization order is.
>
> It probably makes sense to replace the instance id with the device 
> path once all devices are converted over to qdev+vmstate, so we avoid 
> the nasty ordering issues altogether.

Oh, that's what you're suggesting.  So we agree.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.