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 <address@hidden>
> ---
>  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 <address@hidden>