[Qemu-devel] [RFC PATCH v2 0/4] sdbus: testing sdcards

[Qemu-devel] [RFC PATCH v2 0/4] sdbus: testing sdcards

[Philippe Mathieu-Daudé]

Hi,

This RFC series intends to add a sdcard test in C using the libqos.

As I intended to explain previous series threads, I want a way to
access slave devices via their bus rather than the bus master (SDHCI
in this case).
This way I can be sure the slave behaves correctly, then I can test
the host master device.

After spending too long different devices, I came back to the original
idea than using QMP is the simplest way to do it.

- patch 1 adds a QMP command to run commands on the bus,
- patch 2 exposes the libqos sdbus API,
- patch 3 intends to implement the previous API for the QMP command
(the API is here since I tried few other devices before, as you can see in [1])
  it is way broken and leaks lot of qobjects, but worked enough to verify the
  previous series (SDCard: improve SPI, introduce new Specs)
- patch 4 is the WIP qtest I started to backport from my previous python qtests

Since v1:
- dropped Python
- prefix QMP command with 'x-debug-' (Kevin Wolf)

Regards,

Phil.

some pain shared here:
[1]: http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg00080.html

Based-on: 20180103212436.15762-26-f4bug@amsat.org

Philippe Mathieu-Daudé (4):
  sdbus: add a QMP command to access a SDBus
  libqos: add a sdbus API
  libqos: implement sdbus QMP driver
  tests: add some sdcard qtest

 qapi-schema.json         |  41 +++++++++++++
 tests/libqos/sdbus.h     |  45 ++++++++++++++
 hw/sd/sdbus-qmp.c        |  65 ++++++++++++++++++++
 stubs/qmp_sdbus.c        |  12 ++++
 tests/libqos/sdbus-qmp.c | 130 ++++++++++++++++++++++++++++++++++++++++
 tests/libqos/sdbus.c     |  74 +++++++++++++++++++++++
 tests/sdbus-test.c       | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/sd/Makefile.objs      |   2 +-
 stubs/Makefile.objs      |   1 +
 tests/Makefile.include   |   3 +
 10 files changed, 523 insertions(+), 1 deletion(-)
 create mode 100644 tests/libqos/sdbus.h
 create mode 100644 hw/sd/sdbus-qmp.c
 create mode 100644 stubs/qmp_sdbus.c
 create mode 100644 tests/libqos/sdbus-qmp.c
 create mode 100644 tests/libqos/sdbus.c
 create mode 100644 tests/sdbus-test.c

-- 
2.15.1

[Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Philippe Mathieu-Daudé]

Use Base64 to serialize the binary blobs in JSON.
So far at most 512 bytes will be transfered, which result
in a 684 bytes payload.
Since this command is intented for qtesting, it is acceptable.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 qapi-schema.json    | 41 +++++++++++++++++++++++++++++++++
 hw/sd/sdbus-qmp.c   | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 stubs/qmp_sdbus.c   | 12 ++++++++++
 hw/sd/Makefile.objs |  2 +-
 stubs/Makefile.objs |  1 +
 5 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 hw/sd/sdbus-qmp.c
 create mode 100644 stubs/qmp_sdbus.c

diff --git a/qapi-schema.json b/qapi-schema.json
index 18457954a8..be26e8cd34 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3200,3 +3200,44 @@
 # Since: 2.11
 ##
 { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
+
+##
+# @SDBusCommandResponse:
+#
+# SD Bus command response.
+#
+# @base64: the command response encoded as a Base64 string, if any (optional)
+#
+# Since: 2.11
+##
+{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} }
+
+##
+# @x-debug-sdbus-command:
+#
+# Execute a command on a SD Bus return the response (if any).
+#
+# @qom-path: the SD Bus path
+# @command: the SD protocol command to execute in the bus
+# @arg: a 64-bit command argument (optional)
+# @crc: the command/argument CRC (optional)
+#
+# Returns: the response of the command encoded as a Base64 string
+#
+# Since: 2.11
+#
+# -> { "execute": "x-debug-sdbus-command",
+#      "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0",
+#                     "command": 0x01
+#      }
+#    }
+# <- { "return": {'base64': 'A='} }
+#
+##
+{ 'command': 'x-debug-sdbus-command',
+  'data': { 'qom-path': 'str',
+            'command': 'uint8',
+            '*arg': 'uint64',
+            '*crc': 'uint16' },
+  'returns': 'SDBusCommandResponse'
+}
diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
new file mode 100644
index 0000000000..8c4b6f2aee
--- /dev/null
+++ b/hw/sd/sdbus-qmp.c
@@ -0,0 +1,65 @@
+/*
+ * SD card bus QMP debugging interface (for QTesting).
+ *
+ * Copyright (c) 2017 ?
+ *
+ * Author:
+ *  Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu/osdep.h"
+#include "hw/sd/sd.h"
+#include "qmp-commands.h"
+
+SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
+                                                uint8_t command,
+                                                bool has_arg, uint64_t arg,
+                                                bool has_crc, uint16_t crc,
+                                                Error **errp)
+{
+    uint8_t response[16 + 1];
+    SDBusCommandResponse *res;
+    bool ambiguous = false;
+    Object *obj;
+    SDBus *sdbus;
+    int sz;
+
+    obj = object_resolve_path(qom_path, &ambiguous);
+    if (obj == NULL) {
+        if (ambiguous) {
+            error_setg(errp, "Path '%s' is ambiguous", qom_path);
+        } else {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", qom_path);
+        }
+        return NULL;
+    }
+    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);
+    if (sdbus == NULL) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Device '%s' not a sd-bus", qom_path);
+        return NULL;
+    }
+
+    res = g_new0(SDBusCommandResponse, 1);
+    sz = sdbus_do_command(sdbus,
+                          &(SDRequest){ command, arg, has_crc ? crc : -1 },
+                          response);
+    if (sz > 0) {
+        res->has_base64 = true;
+        res->base64 = g_base64_encode(response, sz);
+    }
+
+    return res;
+}
diff --git a/stubs/qmp_sdbus.c b/stubs/qmp_sdbus.c
new file mode 100644
index 0000000000..d9bd75ec71
--- /dev/null
+++ b/stubs/qmp_sdbus.c
@@ -0,0 +1,12 @@
+#include "qemu/osdep.h"
+#include "qmp-commands.h"
+#include "hw/sd/sd.h"
+
+SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
+                                                uint8_t command,
+                                                bool has_arg, uint64_t arg,
+                                                bool has_crc, uint16_t crc,
+                                                Error **errp)
+{
+    return NULL;
+}
diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index c2b7664264..3a70477bba 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -1,6 +1,6 @@
 common-obj-$(CONFIG_PL181) += pl181.o
 common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
-common-obj-$(CONFIG_SD) += sd.o core.o
+common-obj-$(CONFIG_SD) += sd.o core.o sdbus-qmp.o
 common-obj-$(CONFIG_SDHCI) += sdhci.o
 
 obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 8cfe34328a..a46cb3b992 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -35,6 +35,7 @@ stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += qmp_pc_dimm.o
+stub-obj-y += qmp_sdbus.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o
-- 
2.15.1

[Qemu-devel] [RFC PATCH v2 2/4] libqos: add a sdbus API

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/libqos/sdbus.h   | 45 ++++++++++++++++++++++++++++++
 tests/libqos/sdbus.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |  1 +
 3 files changed, 120 insertions(+)
 create mode 100644 tests/libqos/sdbus.h
 create mode 100644 tests/libqos/sdbus.c

diff --git a/tests/libqos/sdbus.h b/tests/libqos/sdbus.h
new file mode 100644
index 0000000000..2057faf176
--- /dev/null
+++ b/tests/libqos/sdbus.h
@@ -0,0 +1,45 @@
+/*
+ * SD/MMC Bus libqos
+ *
+ * Copyright (c) 2017 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef LIBQOS_SDBUS_H
+#define LIBQOS_SDBUS_H
+
+enum NCmd {
+    GO_IDLE_STATE       =  0,
+    ALL_SEND_CID        =  2,
+    SEND_RELATIVE_ADDR  =  3,
+    SELECT_CARD         =  7,
+    SEND_IF_COND        =  8,
+    SEND_CSD            =  9,
+};
+
+enum ACmd {
+    SEND_STATUS         = 13,
+    SEND_OP_COND        = 41,
+    SEND_SCR            = 51,
+};
+
+typedef struct SDBusAdapter SDBusAdapter;
+struct SDBusAdapter {
+
+    ssize_t (*do_command)(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
+                          uint8_t **response);
+    void (*write_byte)(SDBusAdapter *adapter, uint8_t value);
+    uint8_t (*read_byte)(SDBusAdapter *adapter);
+};
+
+ssize_t sdbus_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
+                     uint8_t **response);
+ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
+                      uint16_t address, uint8_t **response);
+void sdbus_write_byte(SDBusAdapter *adapter, uint8_t value);
+uint8_t sdbus_read_byte(SDBusAdapter *adapter);
+
+SDBusAdapter *qmp_sdbus_create(const char *bus_name);
+
+#endif
diff --git a/tests/libqos/sdbus.c b/tests/libqos/sdbus.c
new file mode 100644
index 0000000000..15f38c2bb8
--- /dev/null
+++ b/tests/libqos/sdbus.c
@@ -0,0 +1,74 @@
+/*
+ * QTest SD/MMC Bus driver
+ *
+ * Copyright (c) 2017 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "libqos/sdbus.h"
+#include "libqtest.h"
+#include "qemu-common.h"
+
+static bool verbose;
+#define DPRINTF(fmt, ...)                           \
+    do {                                            \
+        if (verbose) {                              \
+            fprintf(stderr, fmt, ## __VA_ARGS__);   \
+        }                                           \
+    } while (0)
+
+static ssize_t do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
+                      uint8_t **response, bool is_app_cmd)
+{
+    const char *s_cmd = is_app_cmd ? "ACMD" : "CMD";
+    ssize_t sz;
+
+    verbose = !!getenv("V");
+    if (verbose && !is_app_cmd && (cmd == 55)) {
+        verbose = false;
+    }
+
+    DPRINTF("-> %s%02u (0x%08x)\n", s_cmd, cmd, arg);
+    sz = adapter->do_command(adapter, cmd, arg, response);
+    if (response) {
+        if (sz < 0) {
+            DPRINTF("<- %s%02u (len: %ld)\n", s_cmd, cmd, sz);
+        } else if (verbose) {
+            char *pfx = g_strdup_printf("<- %s%02u (len: %ld)", s_cmd, cmd, sz);
+
+            qemu_hexdump((const char *)*response, stderr, pfx, sz);
+            g_free(pfx);
+        }
+    } else {
+        DPRINTF("<- %s%02u\n", s_cmd, cmd);
+    }
+
+    return sz;
+}
+
+ssize_t sdbus_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
+                     uint8_t **response)
+{
+    return do_cmd(adapter, cmd, arg, response, false);
+}
+
+ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
+                      uint16_t address, uint8_t **response)
+{
+    do_cmd(adapter, 55, address << 16, NULL, false);
+    // TODO check rv?
+
+    return do_cmd(adapter, acmd, arg, response, true);
+}
+
+void sdbus_write_byte(SDBusAdapter *adapter, uint8_t value)
+{
+    adapter->write_byte(adapter, value);
+}
+
+uint8_t sdbus_read_byte(SDBusAdapter *adapter)
+{
+    return adapter->read_byte(adapter);
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index cd18ab4519..c22925d4db 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -715,6 +715,7 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
 
 libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
 libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
+libqos-obj-y += tests/libqos/sdbus.o
 libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
 libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
 libqos-spapr-obj-y += tests/libqos/rtas.o
-- 
2.15.1

[Qemu-devel] [RFC PATCH v2 3/4] libqos: implement sdbus QMP driver

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/libqos/sdbus-qmp.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include   |   2 +-
 2 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 tests/libqos/sdbus-qmp.c

diff --git a/tests/libqos/sdbus-qmp.c b/tests/libqos/sdbus-qmp.c
new file mode 100644
index 0000000000..565e2481db
--- /dev/null
+++ b/tests/libqos/sdbus-qmp.c
@@ -0,0 +1,130 @@
+/*
+ * QTest SD/MMC Bus QMP driver
+ *
+ * Copyright (c) 2017 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
+
+#include "libqos/sdbus.h"
+#include "libqtest.h"
+
+typedef struct QMPSDBus {
+    SDBusAdapter parent;
+
+    const char *qom_path;
+} QMPSDBus;
+
+
+static const char *qmp_sdbus_getpath(const char *blkname)
+{
+    QDict *response, *minfo;
+    QList *list;
+    const QListEntry *le;
+    QString *qstr;
+    const char *mname;
+    QObject *qobj;
+
+    response = qmp("{ 'execute': 'query-block' }");
+    g_assert_nonnull(response);
+    list = qdict_get_qlist(response, "return");
+    g_assert_nonnull(list);
+
+    QLIST_FOREACH_ENTRY(list, le) {
+        QDict *response2;
+
+        minfo = qobject_to_qdict(qlist_entry_obj(le));
+        g_assert(minfo);
+        qobj = qdict_get(minfo, "qdev");
+        if (!qobj) {
+            continue;
+        }
+        qstr = qobject_to_qstring(qobj);
+        g_assert(qstr);
+        mname = qstring_get_str(qstr);
+
+        response2 = qmp("{ 'execute': 'qom-get',"
+                        "  'arguments': { 'path': %s,"
+                        "   'property': \"parent_bus\"}"
+                        "}", mname);
+        g_assert(response2);
+        g_assert(qdict_haskey(response2, "return"));
+        qobj = qdict_get(response2, "return");
+        qstr = qobject_to_qstring(qobj);
+        g_assert(qstr);
+        mname = qstring_get_str(qstr);
+
+        return mname;
+    }
+    return NULL;
+}
+
+static ssize_t qmp_mmc_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
+                              uint8_t **response)
+{
+    QMPSDBus *s = (QMPSDBus *)adapter;
+    QDict *response1;
+    QObject *qobj;
+
+    response1 = qmp("{ 'execute': 'x-debug-sdbus-command',"
+                    "  'arguments': { 'qom-path': %s,"
+                    "                 'command': %u, 'arg': %u}"
+                    "}",
+                    s->qom_path, cmd, arg);
+    g_assert(qdict_haskey(response1, "return"));
+    qobj = qdict_get(response1, "return");
+    //QDECREF(response);
+
+    if (!qobj) {
+        return -1;
+    }
+
+    {
+        QString *qstr;
+        const gchar *mname;
+        guchar *uc;
+        gsize out_len;
+        QDict *response2 = qobject_to_qdict(qobj);
+
+        if (!qdict_haskey(response2, "base64")) {
+            return 0;
+        }
+        qobj = qdict_get(response2, "base64");
+        qstr = qobject_to_qstring(qobj);
+        if (!qstr) {
+            puts("!qstr");
+            return 0;
+        }
+        mname = qstring_get_str(qstr);
+
+        uc = g_base64_decode(mname, &out_len);
+        if (response) {
+            *response = uc;
+        } else {
+            g_free(uc);
+        }
+        return out_len;
+
+    }
+
+    return 0;
+}
+
+SDBusAdapter *qmp_sdbus_create(const char *bus_name)
+{
+    QMPSDBus *s;
+    SDBusAdapter *mmc;
+
+    s = g_new(QMPSDBus, 1);
+    s->qom_path = qmp_sdbus_getpath(bus_name);
+    g_assert_nonnull(s->qom_path);
+
+    mmc = (SDBusAdapter *)s;
+    mmc->do_command = qmp_mmc_do_cmd;
+
+    return mmc;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index c22925d4db..409784a189 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -715,7 +715,7 @@ tests/test-crypto-block$(EXESUF): tests/test-crypto-block.o $(test-crypto-obj-y)
 
 libqos-obj-y = tests/libqos/pci.o tests/libqos/fw_cfg.o tests/libqos/malloc.o
 libqos-obj-y += tests/libqos/i2c.o tests/libqos/libqos.o
-libqos-obj-y += tests/libqos/sdbus.o
+libqos-obj-y += tests/libqos/sdbus.o tests/libqos/sdbus-qmp.o
 libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
 libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
 libqos-spapr-obj-y += tests/libqos/rtas.o
-- 
2.15.1

[Qemu-devel] [RFC PATCH v2 4/4] tests: add some sdcard qtest

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdbus-test.c     | 151 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |   2 +
 2 files changed, 153 insertions(+)
 create mode 100644 tests/sdbus-test.c

diff --git a/tests/sdbus-test.c b/tests/sdbus-test.c
new file mode 100644
index 0000000000..9c38be13cb
--- /dev/null
+++ b/tests/sdbus-test.c
@@ -0,0 +1,151 @@
+/*
+ * QTest testcase for the SD/MMC cards
+ *
+ * Copyright (c) 2017 Philippe Mathieu-Daudé
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "qemu/cutils.h"
+
+#include "libqtest.h"
+#include "libqos/sdbus.h"
+
+enum {
+    PROTO_SD,
+    PROTO_MMC,
+    PROTO_SPI,
+    PROTO_COUNT
+};
+
+static const char *proto_name[PROTO_COUNT] = {
+    [PROTO_SD]  = "sd",
+    [PROTO_MMC] = "mmc",
+    [PROTO_SPI] = "spi"
+};
+
+static const char *machines[PROTO_COUNT] = {
+    [PROTO_SD] = "nuri",
+    //[PROTO_MMC] = "vexpress-a9",
+    //[PROTO_SPI] = "lm3s6965evb"
+};
+
+static const uint64_t sizes[] = {
+    //512 * M_BYTE,
+    //1 * G_BYTE,
+    4 * G_BYTE,
+    //64 * G_BYTE,
+};
+
+typedef struct {
+    int protocol;
+    uint64_t size;
+} TestCase;
+
+static void test1(SDBusAdapter *mmc, uint64_t size)
+{
+    uint8_t *response;
+    uint16_t rca;
+    ssize_t sz;
+
+    sz = sdbus_do_cmd(mmc, GO_IDLE_STATE, 0, NULL);
+    g_assert_cmpuint(sz, ==, 0);
+
+    sz = sdbus_do_cmd(mmc, SEND_IF_COND, 0x1aa, NULL);
+    //g_assert_cmpuint(sz, ==, 0);
+
+    sz = sdbus_do_acmd(mmc, SEND_OP_COND, 0x40300000, 0, NULL);
+    g_assert_cmpuint(sz, ==, 4);
+
+    /* CID */
+    sz = sdbus_do_cmd(mmc, ALL_SEND_CID, 0, &response);
+    g_assert_cmpuint(sz, ==, 16);
+    g_assert_cmpmem (&response[3], 5, "QEMU!", 5);
+    g_assert_cmphex(be32_to_cpu(*(uint32_t *)&response[9]), ==, 0xdeadbeef);
+    g_free(response);
+
+    /* RCA */
+    sz = sdbus_do_cmd(mmc, SEND_RELATIVE_ADDR, 0, &response);
+    g_assert_cmpuint(sz, ==, 4);
+    rca = be16_to_cpu(*(uint16_t *)response);
+    g_assert_cmphex(rca, ==, 0x4567);
+    g_free(response);
+
+    /* CSD */
+    sz = sdbus_do_cmd(mmc, SEND_CSD, rca << 16, &response);
+    g_assert_cmpuint(sz, ==, 16);
+    g_assert_cmphex(response[3], ==, 0x32);
+    g_assert_cmphex(response[4], ==, 0x5b); /* class */
+    g_assert_cmphex(response[5], ==, 0x59);
+    /* (SDHC test) */
+    g_assert_cmphex(be32_to_cpu(*(uint32_t *)&response[6]),
+                    ==, (size >> 19) - 1);
+    g_assert_cmphex(response[10], ==, 0x7f);
+    g_assert_cmphex(response[11], ==, 0x80);
+    g_assert_cmphex(response[12], ==, 0x0a);
+    g_assert_cmphex(response[13], ==, 0x40);
+    g_assert_cmphex(response[14], ==, 0);
+    g_assert_cmphex(response[15], ==, 0);
+    g_free(response);
+
+    sz = sdbus_do_cmd(mmc, SELECT_CARD, rca << 16, NULL);
+
+    sz = sdbus_do_acmd(mmc, SEND_SCR, 0, rca, &response);
+    g_assert_cmpuint(sz, ==, 4);
+    g_free(response);
+
+    // TODO 8x: sdcard_read_data len 512
+
+    //sz = sdbus_do_acmd(mmc, SEND_STATUS, 0, rca, &response);
+    //g_free(response);
+}
+
+static void sdcard_tests(gconstpointer data)
+{
+    const TestCase *test = data;
+    SDBusAdapter *sdbus;
+
+    global_qtest = qtest_startf("-machine %s "
+                         "-drive if=sd,driver=null-co,size=%lu,id=mmc0",
+                         machines[test->protocol], test->size);
+    sdbus = qmp_sdbus_create("sd-bus");
+
+    test1(sdbus, test->size);
+    g_free(sdbus);
+
+    qtest_quit(global_qtest);
+}
+
+int main(int argc, char **argv)
+{
+    const char *arch = qtest_get_arch();
+    int iproto, isize;
+    gchar *path;
+    TestCase *test;
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
+        for (iproto = 0; iproto < PROTO_COUNT; iproto++) {
+            if (!machines[iproto]) {
+                continue;
+            }
+            for (isize = 0; isize < ARRAY_SIZE(sizes); isize++) {
+                test = g_new(TestCase, 1);
+
+                test->protocol = iproto;
+                test->size = sizes[isize];
+
+                path = g_strdup_printf("sdcard/%s/%lu", proto_name[iproto], sizes[isize]);
+                qtest_add_data_func(path, test, sdcard_tests);
+                g_free(path);
+                // g_free(test)?
+            }
+        }
+    }
+
+    return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 409784a189..e4434cdfff 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -348,6 +348,7 @@ check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
+check-qtest-arm-y = tests/sdbus-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
 check-qtest-arm-y += tests/m25p80-test$(EXESUF)
 gcov-files-arm-y += hw/misc/tmp105.c
@@ -748,6 +749,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
 	tests/boot-sector.o tests/acpi-utils.o $(libqos-obj-y)
 tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
+tests/sdbus-test$(EXESUF): tests/sdbus-test.o $(libqos-obj-y)
 tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
 tests/m25p80-test$(EXESUF): tests/m25p80-test.o
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
-- 
2.15.1

Re: [Qemu-devel] [RFC PATCH v2 0/4] sdbus: testing sdcards

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180103214925.16677-1-f4bug@amsat.org
Subject: [Qemu-devel] [RFC PATCH v2 0/4] sdbus: testing sdcards

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180102234108.32713-1-laurent@vivier.eu -> patchew/20180102234108.32713-1-laurent@vivier.eu
 t [tag update]            patchew/20180103162400.10396-1-f4bug@amsat.org -> patchew/20180103162400.10396-1-f4bug@amsat.org
 t [tag update]            patchew/20180103164117.11850-1-f4bug@amsat.org -> patchew/20180103164117.11850-1-f4bug@amsat.org
Switched to a new branch 'test'
2fea93749f tests: add some sdcard qtest
0b1bb02ca7 libqos: implement sdbus QMP driver
fab57dc594 libqos: add a sdbus API
5059e2a76e sdbus: add a QMP command to access a SDBus

=== OUTPUT BEGIN ===
Checking PATCH 1/4: sdbus: add a QMP command to access a SDBus...
Checking PATCH 2/4: libqos: add a sdbus API...
ERROR: do not use C99 // comments
#91: FILE: tests/libqos/sdbus.c:61:
+    // TODO check rv?

total: 1 errors, 0 warnings, 126 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/4: libqos: implement sdbus QMP driver...
WARNING: line over 80 characters
#97: FILE: tests/libqos/sdbus-qmp.c:66:
+static ssize_t qmp_mmc_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,

ERROR: do not use C99 // comments
#111: FILE: tests/libqos/sdbus-qmp.c:80:
+    //QDECREF(response);

total: 1 errors, 1 warnings, 138 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: tests: add some sdcard qtest...
ERROR: do not use C99 // comments
#70: FILE: tests/sdbus-test.c:32:
+    //[PROTO_MMC] = "vexpress-a9",

ERROR: do not use C99 // comments
#71: FILE: tests/sdbus-test.c:33:
+    //[PROTO_SPI] = "lm3s6965evb"

ERROR: do not use C99 // comments
#75: FILE: tests/sdbus-test.c:37:
+    //512 * M_BYTE,

ERROR: do not use C99 // comments
#76: FILE: tests/sdbus-test.c:38:
+    //1 * G_BYTE,

ERROR: do not use C99 // comments
#78: FILE: tests/sdbus-test.c:40:
+    //64 * G_BYTE,

ERROR: do not use C99 // comments
#96: FILE: tests/sdbus-test.c:58:
+    //g_assert_cmpuint(sz, ==, 0);

ERROR: space prohibited between function name and open parenthesis '('
#104: FILE: tests/sdbus-test.c:66:
+    g_assert_cmpmem (&response[3], 5, "QEMU!", 5);

ERROR: do not use C99 // comments
#138: FILE: tests/sdbus-test.c:100:
+    // TODO 8x: sdcard_read_data len 512

ERROR: do not use C99 // comments
#140: FILE: tests/sdbus-test.c:102:
+    //sz = sdbus_do_acmd(mmc, SEND_STATUS, 0, rca, &response);

ERROR: do not use C99 // comments
#141: FILE: tests/sdbus-test.c:103:
+    //g_free(response);

WARNING: line over 80 characters
#180: FILE: tests/sdbus-test.c:142:
+                path = g_strdup_printf("sdcard/%s/%lu", proto_name[iproto], sizes[isize]);

ERROR: do not use C99 // comments
#183: FILE: tests/sdbus-test.c:145:
+                // g_free(test)?

total: 11 errors, 1 warnings, 165 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Stefan Hajnoczi]

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

On Wed, Jan 03, 2018 at 06:49:22PM -0300, Philippe Mathieu-Daudé wrote:
> Use Base64 to serialize the binary blobs in JSON.
> So far at most 512 bytes will be transfered, which result
> in a 684 bytes payload.
> Since this command is intented for qtesting, it is acceptable.

Not a requirement, but have you considered adding a qtest command
instead?

  > sdbus-command <qom-path> <args...>
  < OK <result...>

The (small) advantage is that it keeps test-only commands in the qtest
protocol and out of the QMP schema.

The downside of qtest is that qapi parsing is not available, so it
involves manual marshalling code.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 2/4] libqos: add a sdbus API

[Stefan Hajnoczi]

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

On Wed, Jan 03, 2018 at 06:49:23PM -0300, Philippe Mathieu-Daudé wrote:
> +typedef struct SDBusAdapter SDBusAdapter;
> +struct SDBusAdapter {
> +
> +    ssize_t (*do_command)(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
> +                          uint8_t **response);
> +    void (*write_byte)(SDBusAdapter *adapter, uint8_t value);
> +    uint8_t (*read_byte)(SDBusAdapter *adapter);
> +};

Is it necessary to expose the struct definition?  qmp_sdbus_create()
allocates this struct so the caller cannot embed it anyway and does not
need to know sizeof.

> +
> +ssize_t sdbus_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
> +                     uint8_t **response);
> +ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
> +                      uint16_t address, uint8_t **response);
> +void sdbus_write_byte(SDBusAdapter *adapter, uint8_t value);
> +uint8_t sdbus_read_byte(SDBusAdapter *adapter);
> +
> +SDBusAdapter *qmp_sdbus_create(const char *bus_name);

Does this function belong in another patch?

> +static bool verbose;
> +#define DPRINTF(fmt, ...)                           \
> +    do {                                            \
> +        if (verbose) {                              \

I suggest using if (getenv("V")) directly and removing the verbose
global.  The verbose global is error-prone because it requires that the
caller first initializes it.

> +ssize_t sdbus_do_acmd(SDBusAdapter *adapter, enum ACmd acmd, uint32_t arg,
> +                      uint16_t address, uint8_t **response)
> +{
> +    do_cmd(adapter, 55, address << 16, NULL, false);
> +    // TODO check rv?

Even if there is no actual error handling, g_assert_...() could be used
here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 3/4] libqos: implement sdbus QMP driver

[Stefan Hajnoczi]

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

On Wed, Jan 03, 2018 at 06:49:24PM -0300, Philippe Mathieu-Daudé wrote:
> +static const char *qmp_sdbus_getpath(const char *blkname)
> +{
> +    QDict *response, *minfo;
> +    QList *list;
> +    const QListEntry *le;
> +    QString *qstr;
> +    const char *mname;
> +    QObject *qobj;
> +
> +    response = qmp("{ 'execute': 'query-block' }");

This function implicitly uses global_qtest.  If you ever want to do
multi-VM (e.g. migration) tests then this may become a problem because
there's no way to say which QEMU process to talk to.

This can be addressed by adding a QTestState *s argument to these
functions so that global_qtest isn't hardcoded.  Use qtest_qmp()
instead.

> +    g_assert_nonnull(response);
> +    list = qdict_get_qlist(response, "return");
> +    g_assert_nonnull(list);
> +
> +    QLIST_FOREACH_ENTRY(list, le) {
> +        QDict *response2;
> +
> +        minfo = qobject_to_qdict(qlist_entry_obj(le));
> +        g_assert(minfo);
> +        qobj = qdict_get(minfo, "qdev");
> +        if (!qobj) {
> +            continue;
> +        }
> +        qstr = qobject_to_qstring(qobj);
> +        g_assert(qstr);
> +        mname = qstring_get_str(qstr);
> +
> +        response2 = qmp("{ 'execute': 'qom-get',"
> +                        "  'arguments': { 'path': %s,"
> +                        "   'property': \"parent_bus\"}"
> +                        "}", mname);
> +        g_assert(response2);
> +        g_assert(qdict_haskey(response2, "return"));
> +        qobj = qdict_get(response2, "return");
> +        qstr = qobject_to_qstring(qobj);
> +        g_assert(qstr);
> +        mname = qstring_get_str(qstr);
> +
> +        return mname;
> +    }
> +    return NULL;
> +}

response is leaked.  I think something like this is needed:

  char *mname = g_strdup(qstring_get_str(qstr));
  QDECREF(response);
  return mname;

The caller must g_free() the return value too and the type must be char
* instead of const char *.

> +
> +static ssize_t qmp_mmc_do_cmd(SDBusAdapter *adapter, enum NCmd cmd, uint32_t arg,
> +                              uint8_t **response)
> +{
> +    QMPSDBus *s = (QMPSDBus *)adapter;
> +    QDict *response1;
> +    QObject *qobj;
> +
> +    response1 = qmp("{ 'execute': 'x-debug-sdbus-command',"
> +                    "  'arguments': { 'qom-path': %s,"
> +                    "                 'command': %u, 'arg': %u}"
> +                    "}",
> +                    s->qom_path, cmd, arg);
> +    g_assert(qdict_haskey(response1, "return"));
> +    qobj = qdict_get(response1, "return");
> +    //QDECREF(response);
> +
> +    if (!qobj) {
> +        return -1;
> +    }
> +
> +    {
> +        QString *qstr;
> +        const gchar *mname;
> +        guchar *uc;
> +        gsize out_len;
> +        QDict *response2 = qobject_to_qdict(qobj);
> +
> +        if (!qdict_haskey(response2, "base64")) {
> +            return 0;
> +        }
> +        qobj = qdict_get(response2, "base64");
> +        qstr = qobject_to_qstring(qobj);
> +        if (!qstr) {
> +            puts("!qstr");
> +            return 0;
> +        }
> +        mname = qstring_get_str(qstr);
> +
> +        uc = g_base64_decode(mname, &out_len);
> +        if (response) {
> +            *response = uc;
> +        } else {
> +            g_free(uc);
> +        }
> +        return out_len;
> +
> +    }
> +
> +    return 0;
> +}

Please fix memory leaks here, too.  For example, response1 is leaked.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Eric Blake]

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

On 01/03/2018 03:49 PM, Philippe Mathieu-Daudé wrote:
> Use Base64 to serialize the binary blobs in JSON.
> So far at most 512 bytes will be transfered, which result

s/transfered/transferred/

> in a 684 bytes payload.
> Since this command is intented for qtesting, it is acceptable.

s/intented/intended/

Might be worth mentioning the actual command name,
x-debug-sdbus-command, in the commit message to make future git log
trawling easier.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> +##
> +# @SDBusCommandResponse:
> +#
> +# SD Bus command response.
> +#
> +# @base64: the command response encoded as a Base64 string, if any (optional)

s/ (optional)//, now that the documentation engine automatically takes
care of that.

Even if there is no response, isn't the empty string "" more accurate
than omitting the string altogether?  In other words, I'm not sure why
you made the 'base64' member optional.

> +#
> +# Since: 2.11

2.12

> +##
> +{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} }
> +
> +##
> +# @x-debug-sdbus-command:
> +#
> +# Execute a command on a SD Bus return the response (if any).
> +#

Maybe mention that this command is only intended for use during unit
testing (that information is already implicit from the x-debug prefix,
but stating it explicitly doesn't hurt).

> +# @qom-path: the SD Bus path
> +# @command: the SD protocol command to execute in the bus
> +# @arg: a 64-bit command argument (optional)
> +# @crc: the command/argument CRC (optional)
> +#
> +# Returns: the response of the command encoded as a Base64 string
> +#
> +# Since: 2.11

2.12

> +#
> +# -> { "execute": "x-debug-sdbus-command",
> +#      "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0",
> +#                     "command": 0x01

That's invalid JSON (which does not understand hex numbers).  You need
"command": 1

> +#      }
> +#    }
> +# <- { "return": {'base64': 'A='} }
> +#
> +##
> +{ 'command': 'x-debug-sdbus-command',
> +  'data': { 'qom-path': 'str',
> +            'command': 'uint8',
> +            '*arg': 'uint64',
> +            '*crc': 'uint16' },
> +  'returns': 'SDBusCommandResponse'
> +}
> diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
> new file mode 100644
> index 0000000000..8c4b6f2aee
> --- /dev/null
> +++ b/hw/sd/sdbus-qmp.c
> @@ -0,0 +1,65 @@
> +/*
> + * SD card bus QMP debugging interface (for QTesting).
> + *
> + * Copyright (c) 2017 ?

The question mark in a copyright line is not right.  I don't know what
you meant to use, but unless you were doing the work on behalf of an
employer, your own name is probably correct.  You could include 2018 now
if you wanted.


> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    uint8_t response[16 + 1];
> +    SDBusCommandResponse *res;
> +    bool ambiguous = false;
> +    Object *obj;
> +    SDBus *sdbus;
> +    int sz;
> +
> +    obj = object_resolve_path(qom_path, &ambiguous);
> +    if (obj == NULL) {

I don't know if the style 'if (!obj) {' is any more prevalent; but it
doesn't really matter.

> +        if (ambiguous) {
> +            error_setg(errp, "Path '%s' is ambiguous", qom_path);
> +        } else {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", qom_path);
> +        }
> +        return NULL;
> +    }
> +    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);

Is the cast still necessary, or does object_dynamic_cast() return void*
so that you can omit the cast?

> +    if (sdbus == NULL) {
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "Device '%s' not a sd-bus", qom_path);
> +        return NULL;
> +    }
> +
> +    res = g_new0(SDBusCommandResponse, 1);
> +    sz = sdbus_do_command(sdbus,
> +                          &(SDRequest){ command, arg, has_crc ? crc : -1 },

It's probably safer to use specific initializer assignments, as in:

&(SDRequest){ .cmd = command, .arg = arg, ...


> +++ b/stubs/qmp_sdbus.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "qmp-commands.h"
> +#include "hw/sd/sd.h"
> +
> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    return NULL;

In addition to returning NULL, the stub should set errp.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [RFC PATCH v2 4/4] tests: add some sdcard qtest

[Stefan Hajnoczi]

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

On Wed, Jan 03, 2018 at 06:49:25PM -0300, Philippe Mathieu-Daudé wrote:
> +static const char *machines[PROTO_COUNT] = {
> +    [PROTO_SD] = "nuri",
> +    //[PROTO_MMC] = "vexpress-a9",
> +    //[PROTO_SPI] = "lm3s6965evb"
> +};
> +
> +static const uint64_t sizes[] = {
> +    //512 * M_BYTE,
> +    //1 * G_BYTE,
> +    4 * G_BYTE,
> +    //64 * G_BYTE,
> +};

Why are these commented out?

> +static void test1(SDBusAdapter *mmc, uint64_t size)
> +{
> +    uint8_t *response;
> +    uint16_t rca;
> +    ssize_t sz;
> +
> +    sz = sdbus_do_cmd(mmc, GO_IDLE_STATE, 0, NULL);
> +    g_assert_cmpuint(sz, ==, 0);
> +
> +    sz = sdbus_do_cmd(mmc, SEND_IF_COND, 0x1aa, NULL);
> +    //g_assert_cmpuint(sz, ==, 0);

Why is this commented out?

> +    // TODO 8x: sdcard_read_data len 512
> +
> +    //sz = sdbus_do_acmd(mmc, SEND_STATUS, 0, rca, &response);
> +    //g_free(response);

Why is there commenteded out code and TODO here?  Please either
implement this stuff or remove it from the patch.

> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +    int iproto, isize;
> +    gchar *path;
> +    TestCase *test;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
> +        for (iproto = 0; iproto < PROTO_COUNT; iproto++) {
> +            if (!machines[iproto]) {
> +                continue;
> +            }
> +            for (isize = 0; isize < ARRAY_SIZE(sizes); isize++) {
> +                test = g_new(TestCase, 1);
> +
> +                test->protocol = iproto;
> +                test->size = sizes[isize];
> +
> +                path = g_strdup_printf("sdcard/%s/%lu", proto_name[iproto], sizes[isize]);
> +                qtest_add_data_func(path, test, sdcard_tests);
> +                g_free(path);
> +                // g_free(test)?

Please remove this.  qtest_add_data_func() keeps the pointer to test so
it must not be freed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 0/4] sdbus: testing sdcards

[Stefan Hajnoczi]

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

On Wed, Jan 03, 2018 at 06:49:21PM -0300, Philippe Mathieu-Daudé wrote:
> This RFC series intends to add a sdcard test in C using the libqos.

Thanks for adding sdbus support to libqos!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [RFC PATCH v2 4/4] tests: add some sdcard qtest

[Philippe Mathieu-Daudé]

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

Hi Stefan,

On 01/05/2018 12:31 PM, Stefan Hajnoczi wrote:
> On Wed, Jan 03, 2018 at 06:49:25PM -0300, Philippe Mathieu-Daudé wrote:
>> +static const char *machines[PROTO_COUNT] = {
>> +    [PROTO_SD] = "nuri",
>> +    //[PROTO_MMC] = "vexpress-a9",
>> +    //[PROTO_SPI] = "lm3s6965evb"
>> +};
>> +
>> +static const uint64_t sizes[] = {
>> +    //512 * M_BYTE,
>> +    //1 * G_BYTE,
>> +    4 * G_BYTE,
>> +    //64 * G_BYTE,
>> +};
> 
> Why are these commented out?

As I didn't feel good feedback for the previous Python qtests, I
prefered to send this as RFC and to see if this was the good way to go
before spending more time learning QDECREF() and friends :)

However I again forgot to replace RFC -> NOTFORMERGE in the subject :(

These parameters are commented out because the current sd.c code doesn't
work with those cases :(
So to show the qtests is useful and pass Travis/Patchew build, they are
commented, and to show the current model is broken (until I succeed to
fix it), one can uncomment these and see failing tests :)

> 
>> +static void test1(SDBusAdapter *mmc, uint64_t size)
>> +{
>> +    uint8_t *response;
>> +    uint16_t rca;
>> +    ssize_t sz;
>> +
>> +    sz = sdbus_do_cmd(mmc, GO_IDLE_STATE, 0, NULL);
>> +    g_assert_cmpuint(sz, ==, 0);
>> +
>> +    sz = sdbus_do_cmd(mmc, SEND_IF_COND, 0x1aa, NULL);
>> +    //g_assert_cmpuint(sz, ==, 0);
> 
> Why is this commented out?

Actually this test is SD oriented, in MMC mode this command is incorrect.
I'll split this test in 3 (the SD/MMC/SPI protocols).

> 
>> +    // TODO 8x: sdcard_read_data len 512
>> +
>> +    //sz = sdbus_do_acmd(mmc, SEND_STATUS, 0, rca, &response);
>> +    //g_free(response);
> 
> Why is there commenteded out code and TODO here?  Please either
> implement this stuff or remove it from the patch.

Yes, I have to handle this differently for MMC vs SD/SPI.

> 
>> +int main(int argc, char **argv)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +    int iproto, isize;
>> +    gchar *path;
>> +    TestCase *test;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    if (strcmp(arch, "arm") == 0 || strcmp(arch, "aarch64") == 0) {
>> +        for (iproto = 0; iproto < PROTO_COUNT; iproto++) {
>> +            if (!machines[iproto]) {
>> +                continue;
>> +            }
>> +            for (isize = 0; isize < ARRAY_SIZE(sizes); isize++) {
>> +                test = g_new(TestCase, 1);
>> +
>> +                test->protocol = iproto;
>> +                test->size = sizes[isize];
>> +
>> +                path = g_strdup_printf("sdcard/%s/%lu", proto_name[iproto], sizes[isize]);
>> +                qtest_add_data_func(path, test, sdcard_tests);
>> +                g_free(path);
>> +                // g_free(test)?
> 
> Please remove this.  qtest_add_data_func() keeps the pointer to test so
> it must not be freed.

Ok!

Thanks for your review :)

Phil.


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

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Philippe Mathieu-Daudé]

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

Hi Eric,

On 01/05/2018 12:29 PM, Eric Blake wrote:
> On 01/03/2018 03:49 PM, Philippe Mathieu-Daudé wrote:
>> Use Base64 to serialize the binary blobs in JSON.
>> So far at most 512 bytes will be transfered, which result
> 
> s/transfered/transferred/
> 
>> in a 684 bytes payload.
>> Since this command is intented for qtesting, it is acceptable.
> 
> s/intented/intended/
> 
> Might be worth mentioning the actual command name,
> x-debug-sdbus-command, in the commit message to make future git log
> trawling easier.

Ok.

What about using 'x-qtest-sdbus-command'?

> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
>> +##
>> +# @SDBusCommandResponse:
>> +#
>> +# SD Bus command response.
>> +#
>> +# @base64: the command response encoded as a Base64 string, if any (optional)
> 
> s/ (optional)//, now that the documentation engine automatically takes
> care of that.
> 
> Even if there is no response, isn't the empty string "" more accurate
> than omitting the string altogether?  In other words, I'm not sure why
> you made the 'base64' member optional.

Indeed you right.

> 
>> +#
>> +# Since: 2.11
> 
> 2.12

Oops :)

> 
>> +##
>> +{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} }
>> +
>> +##
>> +# @x-debug-sdbus-command:
>> +#
>> +# Execute a command on a SD Bus return the response (if any).
>> +#
> 
> Maybe mention that this command is only intended for use during unit
> testing (that information is already implicit from the x-debug prefix,
> but stating it explicitly doesn't hurt).
> 
>> +# @qom-path: the SD Bus path
>> +# @command: the SD protocol command to execute in the bus
>> +# @arg: a 64-bit command argument (optional)
>> +# @crc: the command/argument CRC (optional)
>> +#
>> +# Returns: the response of the command encoded as a Base64 string
>> +#
>> +# Since: 2.11
> 
> 2.12
> 
>> +#
>> +# -> { "execute": "x-debug-sdbus-command",
>> +#      "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0",
>> +#                     "command": 0x01
> 
> That's invalid JSON (which does not understand hex numbers).  You need
> "command": 1

Yes, you right, I wonder how I ended using hex here (I don't have any in
my .qmp_history ...)

> 
>> +#      }
>> +#    }
>> +# <- { "return": {'base64': 'A='} }
>> +#
>> +##
>> +{ 'command': 'x-debug-sdbus-command',
>> +  'data': { 'qom-path': 'str',
>> +            'command': 'uint8',
>> +            '*arg': 'uint64',
>> +            '*crc': 'uint16' },
>> +  'returns': 'SDBusCommandResponse'
>> +}
>> diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
>> new file mode 100644
>> index 0000000000..8c4b6f2aee
>> --- /dev/null
>> +++ b/hw/sd/sdbus-qmp.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * SD card bus QMP debugging interface (for QTesting).
>> + *
>> + * Copyright (c) 2017 ?
> 
> The question mark in a copyright line is not right.  I don't know what
> you meant to use, but unless you were doing the work on behalf of an
> employer, your own name is probably correct.  You could include 2018 now
> if you wanted.

:)

> 
> 
>> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
>> +                                                uint8_t command,
>> +                                                bool has_arg, uint64_t arg,
>> +                                                bool has_crc, uint16_t crc,
>> +                                                Error **errp)
>> +{
>> +    uint8_t response[16 + 1];
>> +    SDBusCommandResponse *res;
>> +    bool ambiguous = false;
>> +    Object *obj;
>> +    SDBus *sdbus;
>> +    int sz;
>> +
>> +    obj = object_resolve_path(qom_path, &ambiguous);
>> +    if (obj == NULL) {
> 
> I don't know if the style 'if (!obj) {' is any more prevalent; but it
> doesn't really matter.

old habits are hard to change :)

> 
>> +        if (ambiguous) {
>> +            error_setg(errp, "Path '%s' is ambiguous", qom_path);
>> +        } else {
>> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                      "Device '%s' not found", qom_path);
>> +        }
>> +        return NULL;
>> +    }
>> +    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);
> 
> Is the cast still necessary, or does object_dynamic_cast() return void*
> so that you can omit the cast?

Good to know!

> 
>> +    if (sdbus == NULL) {
>> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> +                  "Device '%s' not a sd-bus", qom_path);
>> +        return NULL;
>> +    }
>> +
>> +    res = g_new0(SDBusCommandResponse, 1);
>> +    sz = sdbus_do_command(sdbus,
>> +                          &(SDRequest){ command, arg, has_crc ? crc : -1 },
> 
> It's probably safer to use specific initializer assignments, as in:
> 
> &(SDRequest){ .cmd = command, .arg = arg, ...

Ok.

> 
> 
>> +++ b/stubs/qmp_sdbus.c
>> @@ -0,0 +1,12 @@
>> +#include "qemu/osdep.h"
>> +#include "qmp-commands.h"
>> +#include "hw/sd/sd.h"
>> +
>> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
>> +                                                uint8_t command,
>> +                                                bool has_arg, uint64_t arg,
>> +                                                bool has_crc, uint16_t crc,
>> +                                                Error **errp)
>> +{
>> +    return NULL;
> 
> In addition to returning NULL, the stub should set errp.

Oh, I didn't know.

Thanks for your detailed review!

Phil.


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

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Eric Blake]

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

On 01/05/2018 10:06 AM, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 01/05/2018 12:29 PM, Eric Blake wrote:
>> On 01/03/2018 03:49 PM, Philippe Mathieu-Daudé wrote:
>>> Use Base64 to serialize the binary blobs in JSON.
>>> So far at most 512 bytes will be transfered, which result
>>
>> s/transfered/transferred/
>>
>>> in a 684 bytes payload.
>>> Since this command is intented for qtesting, it is acceptable.
>>
>> s/intented/intended/
>>
>> Might be worth mentioning the actual command name,
>> x-debug-sdbus-command, in the commit message to make future git log
>> trawling easier.
> 
> Ok.
> 
> What about using 'x-qtest-sdbus-command'?

x-debug matches existing practice, x-qtest does not.  I'm fine with the
name you had chosen, and was just asking that it be mentioned in the
commit log.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Philippe Mathieu-Daudé]

Hi Eric,

On Fri, Jan 5, 2018 at 12:29 PM, Eric Blake <eblake@redhat.com> wrote:
> On 01/03/2018 03:49 PM, Philippe Mathieu-Daudé wrote:
[...]
>> +        if (ambiguous) {
>> +            error_setg(errp, "Path '%s' is ambiguous", qom_path);
>> +        } else {
>> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                      "Device '%s' not found", qom_path);
>> +        }
>> +        return NULL;
>> +    }
>> +    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);
>
> Is the cast still necessary, or does object_dynamic_cast() return void*
> so that you can omit the cast?

Apparently the cast is necessary, since the object_dynamic_cast()
returns an Object pointer,
and the SD_BUS() macro uses object_dynamic_cast_assert() which
generates a runtime assert.

without casting:
hw/sd/sdbus-qmp.c:37:11: error: assignment from incompatible pointer
type [-Werror=incompatible-pointer-types]
     sdbus = object_dynamic_cast(obj, TYPE_SD_BUS);
           ^

>
>> +    if (sdbus == NULL) {
>> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> +                  "Device '%s' not a sd-bus", qom_path);
>> +        return NULL;
>> +    }

Re: [Qemu-devel] [RFC PATCH v2 4/4] tests: add some sdcard qtest

[Stefan Hajnoczi]

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

On Fri, Jan 05, 2018 at 12:55:41PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 01/05/2018 12:31 PM, Stefan Hajnoczi wrote:
> > On Wed, Jan 03, 2018 at 06:49:25PM -0300, Philippe Mathieu-Daudé wrote:
> >> +static const char *machines[PROTO_COUNT] = {
> >> +    [PROTO_SD] = "nuri",
> >> +    //[PROTO_MMC] = "vexpress-a9",
> >> +    //[PROTO_SPI] = "lm3s6965evb"
> >> +};
> >> +
> >> +static const uint64_t sizes[] = {
> >> +    //512 * M_BYTE,
> >> +    //1 * G_BYTE,
> >> +    4 * G_BYTE,
> >> +    //64 * G_BYTE,
> >> +};
> > 
> > Why are these commented out?
> 
> As I didn't feel good feedback for the previous Python qtests, I
> prefered to send this as RFC and to see if this was the good way to go
> before spending more time learning QDECREF() and friends :)
> 
> However I again forgot to replace RFC -> NOTFORMERGE in the subject :(
> 
> These parameters are commented out because the current sd.c code doesn't
> work with those cases :(
> So to show the qtests is useful and pass Travis/Patchew build, they are
> commented, and to show the current model is broken (until I succeed to
> fix it), one can uncomment these and see failing tests :)

If you want to commit commented out code then please include a comment
that explains why it's commented out.  That way readers understand why
it's there.

Also please run checkpatch.pl against all patches.  It can be run as a
commit hook so all your code is checked at commit time:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Philippe Mathieu-Daudé]

Hi Markus,

[Asking again from the correct series thread]

On 1/3/18 10:49 PM, Philippe Mathieu-Daudé wrote:
> Use Base64 to serialize the binary blobs in JSON.
> So far at most 512 bytes will be transfered, which result
> in a 684 bytes payload.
> Since this command is intented for qtesting, it is acceptable.

Any comment regarding QMP for this patch?

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  qapi-schema.json    | 41 +++++++++++++++++++++++++++++++++
>  hw/sd/sdbus-qmp.c   | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  stubs/qmp_sdbus.c   | 12 ++++++++++
>  hw/sd/Makefile.objs |  2 +-
>  stubs/Makefile.objs |  1 +
>  5 files changed, 120 insertions(+), 1 deletion(-)
>  create mode 100644 hw/sd/sdbus-qmp.c
>  create mode 100644 stubs/qmp_sdbus.c
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 18457954a8..be26e8cd34 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3200,3 +3200,44 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @SDBusCommandResponse:
> +#
> +# SD Bus command response.
> +#
> +# @base64: the command response encoded as a Base64 string, if any (optional)
> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'SDBusCommandResponse', 'data': {'*base64': 'str'} }
> +
> +##
> +# @x-debug-sdbus-command:
> +#
> +# Execute a command on a SD Bus return the response (if any).
> +#
> +# @qom-path: the SD Bus path
> +# @command: the SD protocol command to execute in the bus
> +# @arg: a 64-bit command argument (optional)
> +# @crc: the command/argument CRC (optional)
> +#
> +# Returns: the response of the command encoded as a Base64 string
> +#
> +# Since: 2.11
> +#
> +# -> { "execute": "x-debug-sdbus-command",
> +#      "arguments": { "qom-path": "/machine/unattached/device[32]/sd.0",
> +#                     "command": 0x01
> +#      }
> +#    }
> +# <- { "return": {'base64': 'A='} }
> +#
> +##
> +{ 'command': 'x-debug-sdbus-command',
> +  'data': { 'qom-path': 'str',
> +            'command': 'uint8',
> +            '*arg': 'uint64',
> +            '*crc': 'uint16' },
> +  'returns': 'SDBusCommandResponse'
> +}
> diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
> new file mode 100644
> index 0000000000..8c4b6f2aee
> --- /dev/null
> +++ b/hw/sd/sdbus-qmp.c
> @@ -0,0 +1,65 @@
> +/*
> + * SD card bus QMP debugging interface (for QTesting).
> + *
> + * Copyright (c) 2017 ?
> + *
> + * Author:
> + *  Philippe Mathieu-Daudé <f4bug@amsat.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "qemu/osdep.h"
> +#include "hw/sd/sd.h"
> +#include "qmp-commands.h"
> +
> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    uint8_t response[16 + 1];
> +    SDBusCommandResponse *res;
> +    bool ambiguous = false;
> +    Object *obj;
> +    SDBus *sdbus;
> +    int sz;
> +
> +    obj = object_resolve_path(qom_path, &ambiguous);
> +    if (obj == NULL) {
> +        if (ambiguous) {
> +            error_setg(errp, "Path '%s' is ambiguous", qom_path);
> +        } else {
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", qom_path);
> +        }
> +        return NULL;
> +    }
> +    sdbus = (SDBus *)object_dynamic_cast(obj, TYPE_SD_BUS);
> +    if (sdbus == NULL) {
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "Device '%s' not a sd-bus", qom_path);
> +        return NULL;
> +    }
> +
> +    res = g_new0(SDBusCommandResponse, 1);
> +    sz = sdbus_do_command(sdbus,
> +                          &(SDRequest){ command, arg, has_crc ? crc : -1 },
> +                          response);
> +    if (sz > 0) {
> +        res->has_base64 = true;
> +        res->base64 = g_base64_encode(response, sz);
> +    }
> +
> +    return res;
> +}
> diff --git a/stubs/qmp_sdbus.c b/stubs/qmp_sdbus.c
> new file mode 100644
> index 0000000000..d9bd75ec71
> --- /dev/null
> +++ b/stubs/qmp_sdbus.c
> @@ -0,0 +1,12 @@
> +#include "qemu/osdep.h"
> +#include "qmp-commands.h"
> +#include "hw/sd/sd.h"
> +
> +SDBusCommandResponse *qmp_x_debug_sdbus_command(const char *qom_path,
> +                                                uint8_t command,
> +                                                bool has_arg, uint64_t arg,
> +                                                bool has_crc, uint16_t crc,
> +                                                Error **errp)
> +{
> +    return NULL;
> +}
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index c2b7664264..3a70477bba 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -1,6 +1,6 @@
>  common-obj-$(CONFIG_PL181) += pl181.o
>  common-obj-$(CONFIG_SSI_SD) += ssi-sd.o
> -common-obj-$(CONFIG_SD) += sd.o core.o
> +common-obj-$(CONFIG_SD) += sd.o core.o sdbus-qmp.o
>  common-obj-$(CONFIG_SDHCI) += sdhci.o
>  
>  obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 8cfe34328a..a46cb3b992 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -35,6 +35,7 @@ stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += qmp_pc_dimm.o
> +stub-obj-y += qmp_sdbus.o
>  stub-obj-y += target-monitor-defs.o
>  stub-obj-y += target-get-monitor-def.o
>  stub-obj-y += pc_madt_cpu_entry.o
> 

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Thomas Huth]

On 08/03/2019 17.11, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> [Asking again from the correct series thread]
> 
> On 1/3/18 10:49 PM, Philippe Mathieu-Daudé wrote:
>> Use Base64 to serialize the binary blobs in JSON.
>> So far at most 512 bytes will be transfered, which result
>> in a 684 bytes payload.
>> Since this command is intented for qtesting, it is acceptable.
> 
> Any comment regarding QMP for this patch?

Is this useful for anybody else than qtest? If not, I think this should
rather go into the qtest protocol instead, since QMP is our "public"
protocol.

>> diff --git a/hw/sd/sdbus-qmp.c b/hw/sd/sdbus-qmp.c
>> new file mode 100644
>> index 0000000000..8c4b6f2aee
>> --- /dev/null
>> +++ b/hw/sd/sdbus-qmp.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * SD card bus QMP debugging interface (for QTesting).
>> + *
>> + * Copyright (c) 2017 ?

Weird line. Add you name here ?

 Thomas

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Eduardo Habkost]

On Mon, Mar 11, 2019 at 12:49:50PM +0100, Thomas Huth wrote:
> On 08/03/2019 17.11, Philippe Mathieu-Daudé wrote:
> > Hi Markus,
> > 
> > [Asking again from the correct series thread]
> > 
> > On 1/3/18 10:49 PM, Philippe Mathieu-Daudé wrote:
> >> Use Base64 to serialize the binary blobs in JSON.
> >> So far at most 512 bytes will be transfered, which result
> >> in a 684 bytes payload.
> >> Since this command is intented for qtesting, it is acceptable.
> > 
> > Any comment regarding QMP for this patch?
> 
> Is this useful for anybody else than qtest? If not, I think this should
> rather go into the qtest protocol instead, since QMP is our "public"
> protocol.

Extending qtest requires writing parsers by hand.  Do we really
want to go that route and start extending the qtest protocol more
often?

I also plan to add new debugging-only QMP commands for testing
CPU code, and I'm not looking forward to writing my own parser
inside qtest_process_command().

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Peter Maydell]

On Mon, 11 Mar 2019 at 13:43, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Mon, Mar 11, 2019 at 12:49:50PM +0100, Thomas Huth wrote:
> > On 08/03/2019 17.11, Philippe Mathieu-Daudé wrote:
> > > Hi Markus,
> > >
> > > [Asking again from the correct series thread]
> > >
> > > On 1/3/18 10:49 PM, Philippe Mathieu-Daudé wrote:
> > >> Use Base64 to serialize the binary blobs in JSON.
> > >> So far at most 512 bytes will be transfered, which result
> > >> in a 684 bytes payload.
> > >> Since this command is intented for qtesting, it is acceptable.
> > >
> > > Any comment regarding QMP for this patch?
> >
> > Is this useful for anybody else than qtest? If not, I think this should
> > rather go into the qtest protocol instead, since QMP is our "public"
> > protocol.
>
> Extending qtest requires writing parsers by hand.  Do we really
> want to go that route and start extending the qtest protocol more
> often?

Perhaps we could have qtest-only QMP commands that only get
recognized if qtest_enabled() ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] sdbus: add a QMP command to access a SDBus

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 11 Mar 2019 at 13:43, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>> On Mon, Mar 11, 2019 at 12:49:50PM +0100, Thomas Huth wrote:
>> > On 08/03/2019 17.11, Philippe Mathieu-Daudé wrote:
>> > > Hi Markus,
>> > >
>> > > [Asking again from the correct series thread]
>> > >
>> > > On 1/3/18 10:49 PM, Philippe Mathieu-Daudé wrote:
>> > >> Use Base64 to serialize the binary blobs in JSON.
>> > >> So far at most 512 bytes will be transfered, which result
>> > >> in a 684 bytes payload.
>> > >> Since this command is intented for qtesting, it is acceptable.
>> > >
>> > > Any comment regarding QMP for this patch?
>> >
>> > Is this useful for anybody else than qtest? If not, I think this should
>> > rather go into the qtest protocol instead, since QMP is our "public"
>> > protocol.
>>
>> Extending qtest requires writing parsers by hand.  Do we really
>> want to go that route and start extending the qtest protocol more
>> often?
>
> Perhaps we could have qtest-only QMP commands that only get
> recognized if qtest_enabled() ?

QMP commands are defined at compile time.  We just got rid of the hack
to "unrecognize" selected commands dynamically at run-time (commit
0b69f6f72ce), which we had because our compile-time facilities were
lacking.  I'd hate to bring this hack back.

What's easy is to have QMP commands that fail unless qtest_enabled() :)