Re: [Qemu-devel] [PATCH 17/36] qtest: Avoid dynamic JSON in ahci-test

[Eric Blake]

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 
>> ---
>>  tests/ahci-test.c | 26 +-
>>  1 file changed, 21 insertions(+), 5 deletions(-)

>> +args = QAPI_TO_QOBJECT(BlockdevOptions, , _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 
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 17/36] qtest: Avoid dynamic JSON in ahci-test

[John Snow]

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 
> ---
>  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, , );
>  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 = ,
> +};
> +args = QAPI_TO_QOBJECT(BlockdevOptions, , _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 

[Qemu-devel] [PATCH 17/36] qtest: Avoid dynamic JSON in ahci-test

[Eric Blake]

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 
---
 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, , );
 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 = ,
+};
+args = QAPI_TO_QOBJECT(BlockdevOptions, , _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' }}");
-- 
2.7.4