Leaving interpolation into JSON to qmp() is more robust than building
QMP input manually, as explained in the commit before previous.
The case in qpci_plug_device_test() is a bit complicated: it
interpolates several JSON object members, not just a value. Clean it
up by passing them in as QDict rather than string, so we can leave
interpolation to qmp() here and to qobject_from_jsonf() in callers.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/ivshmem-test.c | 10 +++++-----
tests/libqos/pci.c | 33 ++++++++++++++++++---------------
tests/libqos/pci.h | 2 +-
tests/virtio-blk-test.c | 5 ++++-
4 files changed, 28 insertions(+), 22 deletions(-)
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index 3776342..38044bb 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -14,6 +14,7 @@
#include "libqos/libqos-pc.h"
#include "libqos/libqos-spapr.h"
#include "libqtest.h"
+#include "qapi/qmp/qjson.h"
#include "qemu-common.h"
#define TMPSHMSIZE (1 << 20)
@@ -419,19 +420,18 @@ static void test_ivshmem_server_irq(void)
static void test_ivshmem_hotplug(void)
{
const char *arch = qtest_get_arch();
- gchar *opts;
+ QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
+ tmpshm);
qtest_start("");
- opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
-
- qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
+ qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP,
+ qobject_to_qdict(extra_args));
if (strcmp(arch, "ppc64") != 0) {
qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
}
qtest_end();
- g_free(opts);
}
static void test_ivshmem_memdev(void)
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index 2dcdead..4068305 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -14,6 +14,7 @@
#include "libqos/pci.h"
#include "hw/pci/pci_regs.h"
+#include "qapi/qmp/qjson.h"
#include "qemu/host-utils.h"
void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
@@ -392,23 +393,25 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
}
void qpci_plug_device_test(const char *driver, const char *id,
- uint8_t slot, const char *opts)
+ uint8_t slot, QDict *extra_args)
{
- QDict *response;
- char *cmd;
-
- cmd = g_strdup_printf("{'execute': 'device_add',"
- " 'arguments': {"
- " 'driver': '%s',"
- " 'addr': '%d',"
- " %s%s"
- " 'id': '%s'"
- "}}", driver, slot,
- opts ? opts : "", opts ? "," : "",
- id);
- response = qmp(cmd);
- g_free(cmd);
+ char addr[8];
+ QDict *args, *response;
+
+ sprintf(addr, "%d", slot);
+ args = qobject_to_qdict(
+ qobject_from_jsonf("{ 'driver': %s, 'addr': %s, 'id': %s}",
+ driver, addr, id));
+
+ if (extra_args) {
+ qdict_join(args, extra_args, true);
+ QDECREF(extra_args);
+ }
+
+ response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
+
g_assert(response);
g_assert(!qdict_haskey(response, "error"));
+ QDECREF(args);
QDECREF(response);
}
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index ed48061..c981061 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -109,6 +109,6 @@ void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
void qpci_plug_device_test(const char *driver, const char *id,
- uint8_t slot, const char *opts);
+ uint8_t slot, QDict *extra_args);
void qpci_unplug_acpi_device_test(const char *id, uint8_t slot);
#endif
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0576cb1..64a48f4 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -16,6 +16,7 @@
#include "libqos/virtio-pci.h"
#include "libqos/virtio-mmio.h"
#include "libqos/malloc-generic.h"
+#include "qapi/qmp/qjson.h"
#include "qemu/bswap.h"
#include "standard-headers/linux/virtio_ids.h"
#include "standard-headers/linux/virtio_config.h"
@@ -658,12 +659,13 @@ static void pci_hotplug(void)
QVirtioPCIDevice *dev;
QOSState *qs;
const char *arch = qtest_get_arch();
+ QObject *extra_args = qobject_from_jsonf("{ 'drive': 'drive1' }");
qs = pci_test_start();
/* plug secondary disk */
qpci_plug_device_test("virtio-blk-pci", "drv1", PCI_SLOT_HP,
- "'drive': 'drive1'");
+ qobject_to_qdict(extra_args));
dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT_HP);
g_assert(dev);
@@ -674,6 +676,7 @@ static void pci_hotplug(void)
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
}
+
qtest_shutdown(qs);
}
--
2.7.5
On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the commit before previous.
>
> The case in qpci_plug_device_test() is a bit complicated: it
> interpolates several JSON object members, not just a value. Clean it
> up by passing them in as QDict rather than string, so we can leave
> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>
> + response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
> +
I like this; in fact, in my earlier series attempting to remove
qobject_from_jsonf(), I heavily relied on %p being useful, to the point
that I even added a helper function to make it easier (off-hand, it was
something like qmp_execute(const char *command, QDict *arguments))
> @@ -674,6 +676,7 @@ static void pci_hotplug(void)
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
> }
> +
> qtest_shutdown(qs);
Spurious whitespace addition?
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the commit before previous.
>>
>> The case in qpci_plug_device_test() is a bit complicated: it
>> interpolates several JSON object members, not just a value. Clean it
>> up by passing them in as QDict rather than string, so we can leave
>> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>>
>> + response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
>> +
>
> I like this; in fact, in my earlier series attempting to remove
> qobject_from_jsonf(), I heavily relied on %p being useful, to the point
> that I even added a helper function to make it easier (off-hand, it was
> something like qmp_execute(const char *command, QDict *arguments))
>
>> @@ -674,6 +676,7 @@ static void pci_hotplug(void)
>> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
>> }
>> +
>> qtest_shutdown(qs);
>
> Spurious whitespace addition?
Is it spurious if the result looks better? ;)
There's a blank line after pci_test_start(), and that makes me want one
before the matching qtest_shutdown().
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
On Fri, Jul 21, 2017 at 03:53:23PM +0200, Markus Armbruster wrote: > Leaving interpolation into JSON to qmp() is more robust than building > QMP input manually, as explained in the commit before previous. > > The case in qpci_plug_device_test() is a bit complicated: it > interpolates several JSON object members, not just a value. Clean it > up by passing them in as QDict rather than string, so we can leave > interpolation to qmp() here and to qobject_from_jsonf() in callers. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/ivshmem-test.c | 10 +++++----- > tests/libqos/pci.c | 33 ++++++++++++++++++--------------- > tests/libqos/pci.h | 2 +- > tests/virtio-blk-test.c | 5 ++++- > 4 files changed, 28 insertions(+), 22 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 07/21/2017 08:53 AM, Markus Armbruster wrote:
> Leaving interpolation into JSON to qmp() is more robust than building
> QMP input manually, as explained in the commit before previous.
>
> The case in qpci_plug_device_test() is a bit complicated: it
> interpolates several JSON object members, not just a value. Clean it
> up by passing them in as QDict rather than string, so we can leave
> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> + QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
> + tmpshm);
>
> qtest_start("");
>
> - opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
> -
> - qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
Wait, 'size':'1M' works? I guess because it's a string, rather than a
JSON number. One of our intentional design choices in QMP was to
represent everything as bytes, rather than as suffixed numbers, since
machine-generated code can easily generate bytes anywhere that humans
prefer a suffixed number. But as you are not changing this interface,
but merely refactoring how it is tested, it's more of a side comment
than something that affects review.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the commit before previous.
>>
>> The case in qpci_plug_device_test() is a bit complicated: it
>> interpolates several JSON object members, not just a value. Clean it
>> up by passing them in as QDict rather than string, so we can leave
>> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> + QObject *extra_args = qobject_from_jsonf("{ 'shm': '%s', 'size': '1M' }",
>> + tmpshm);
>>
>> qtest_start("");
>>
>> - opts = g_strdup_printf("'shm': '%s', 'size': '1M'", tmpshm);
>> -
>> - qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts);
>
> Wait, 'size':'1M' works? I guess because it's a string, rather than a
> JSON number. One of our intentional design choices in QMP was to
> represent everything as bytes, rather than as suffixed numbers, since
> machine-generated code can easily generate bytes anywhere that humans
> prefer a suffixed number. But as you are not changing this interface,
> but merely refactoring how it is tested, it's more of a side comment
> than something that affects review.
Design mistake in legacy device "ivshmem". Fixed in "ivshmem-plain" and
"ivshmem-doorbell", see commit 5400c02.
© 2016 - 2025 Red Hat, Inc.