| Message ID | 1480535094-23831-18-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/30/2016 02:44 PM, Eric Blake wrote: > As argued elsewhere, it's less code to maintain if we convert > from a dynamic string passed to qobject_from_jsonv() to instead > use a hand-built QDict. > > Rather than build up a QDict by manual qdict_put*() calls, we > can let QAPI do the work for us. The result is more lines of > code to initialize the QAPI struct, but the result will force us > to track any changes to the qapi (whereas the dynamic JSON string > would not detect qapi changes until runtime). > Benefit of doubt that you're right. > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/ahci-test.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/tests/ahci-test.c b/tests/ahci-test.c > index ef17629..dfa9c52 100644 > --- a/tests/ahci-test.c > +++ b/tests/ahci-test.c > @@ -32,6 +32,8 @@ > > #include "qemu-common.h" > #include "qemu/host-utils.h" > +#include "qapi/qmp/qjson.h" > +#include "qapi/qobject-output-visitor.h" > > #include "hw/pci/pci_ids.h" > #include "hw/pci/pci_regs.h" > @@ -1576,6 +1578,7 @@ static void test_atapi_tray(void) > uint8_t port, sense, asc; > uint64_t iso_size = ATAPI_SECTOR_SIZE; > QDict *rsp; > + QObject *args; > > fd = prepare_iso(iso_size, &tx, &iso); > ahci = ahci_boot_and_enable("-drive if=none,id=drive0,file=%s,format=raw " > @@ -1607,11 +1610,24 @@ static void test_atapi_tray(void) > atapi_wait_tray(true); > > /* Re-insert media */ > - qmp_discard_response("{'execute': 'blockdev-add', " > - "'arguments': {'node-name': 'node0', " > - "'driver': 'raw', " > - "'file': { 'driver': 'file', " > - "'filename': %s }}}", iso); > + { > + BlockdevRef ref = { > + .type = QTYPE_QDICT, > + .u.definition = { > + .driver = BLOCKDEV_DRIVER_FILE, > + .u.file.filename = iso, > + }, > + }; > + BlockdevOptions opts = { > + .has_node_name = true, > + .node_name = (char *)"node0", > + .driver = BLOCKDEV_DRIVER_RAW, > + .u.raw.file = &ref, > + }; > + args = QAPI_TO_QOBJECT(BlockdevOptions, &opts, &error_abort); > + } > + > + qmp_cmd_discard_response("blockdev-add", qobject_to_qdict(args)); > qmp_discard_response("{'execute': 'x-blockdev-insert-medium'," > "'arguments': { 'device': 'drive0', " > "'node-name': 'node0' }}"); > I assume qmp_cmd_discard_response takes ownership of the object we just built? Assuming yes: Reviewed-by: John Snow <jsnow@redhat.com>
On 11/30/2016 03:02 PM, John Snow wrote: > > > On 11/30/2016 02:44 PM, Eric Blake wrote: >> As argued elsewhere, it's less code to maintain if we convert >> from a dynamic string passed to qobject_from_jsonv() to instead >> use a hand-built QDict. >> >> Rather than build up a QDict by manual qdict_put*() calls, we >> can let QAPI do the work for us. The result is more lines of >> code to initialize the QAPI struct, but the result will force us >> to track any changes to the qapi (whereas the dynamic JSON string >> would not detect qapi changes until runtime). >> > > Benefit of doubt that you're right. > >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> tests/ahci-test.c | 26 +++++++++++++++++++++----- >> 1 file changed, 21 insertions(+), 5 deletions(-) >> + args = QAPI_TO_QOBJECT(BlockdevOptions, &opts, &error_abort); >> + } >> + >> + qmp_cmd_discard_response("blockdev-add", qobject_to_qdict(args)); >> qmp_discard_response("{'execute': 'x-blockdev-insert-medium'," >> "'arguments': { 'device': 'drive0', " >> "'node-name': 'node0' }}"); >> > > I assume qmp_cmd_discard_response takes ownership of the object we just > built? Yes. I had to track that down myself, which is why in patch 9, I documented it: /** + * qmp_cmd_discard_response: + * @cmd: Command name to send + * @args: Arguments to transfer to the command, or NULL. + * + * Sends a QMP message to QEMU and consumes the response. Calling this will + * reduce the reference count of @args. + */ +void qmp_cmd_discard_response(const char *cmd, QDict *args); > > Assuming yes: > Reviewed-by: John Snow <jsnow@redhat.com> >
diff --git a/tests/ahci-test.c b/tests/ahci-test.c index ef17629..dfa9c52 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -32,6 +32,8 @@ #include "qemu-common.h" #include "qemu/host-utils.h" +#include "qapi/qmp/qjson.h" +#include "qapi/qobject-output-visitor.h" #include "hw/pci/pci_ids.h" #include "hw/pci/pci_regs.h" @@ -1576,6 +1578,7 @@ static void test_atapi_tray(void) uint8_t port, sense, asc; uint64_t iso_size = ATAPI_SECTOR_SIZE; QDict *rsp; + QObject *args; fd = prepare_iso(iso_size, &tx, &iso); ahci = ahci_boot_and_enable("-drive if=none,id=drive0,file=%s,format=raw " @@ -1607,11 +1610,24 @@ static void test_atapi_tray(void) atapi_wait_tray(true); /* Re-insert media */ - qmp_discard_response("{'execute': 'blockdev-add', " - "'arguments': {'node-name': 'node0', " - "'driver': 'raw', " - "'file': { 'driver': 'file', " - "'filename': %s }}}", iso); + { + BlockdevRef ref = { + .type = QTYPE_QDICT, + .u.definition = { + .driver = BLOCKDEV_DRIVER_FILE, + .u.file.filename = iso, + }, + }; + BlockdevOptions opts = { + .has_node_name = true, + .node_name = (char *)"node0", + .driver = BLOCKDEV_DRIVER_RAW, + .u.raw.file = &ref, + }; + args = QAPI_TO_QOBJECT(BlockdevOptions, &opts, &error_abort); + } + + qmp_cmd_discard_response("blockdev-add", qobject_to_qdict(args)); qmp_discard_response("{'execute': 'x-blockdev-insert-medium'," "'arguments': { 'device': 'drive0', " "'node-name': 'node0' }}");
As argued elsewhere, it's less code to maintain if we convert from a dynamic string passed to qobject_from_jsonv() to instead use a hand-built QDict. Rather than build up a QDict by manual qdict_put*() calls, we can let QAPI do the work for us. The result is more lines of code to initialize the QAPI struct, but the result will force us to track any changes to the qapi (whereas the dynamic JSON string would not detect qapi changes until runtime). Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/ahci-test.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)