Re: [Qemu-devel] [PATCH] qtest: ask endianness of the target in qtest_init()

[Peter Maydell]

On 6 October 2016 at 11:56, Laurent Vivier <address@hidden> wrote:
> The target endianness is not deduced anymore from
> the architecture name but asked directly to the guest,
> using a new qtest command: "endianness". As it can't
> change (this is the value of TARGET_WORDS_BIGENDIAN),
> we store it to not have to ask every time we want to
> know if we have to byte-swap a value.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> CC: Greg Kurz <address@hidden>
> CC: David Gibson <address@hidden>
> CC: Peter Maydell <address@hidden>
> ---
> Note: this patch can be seen as a v2 of
> "qtest: evaluate endianness of the target in qtest_init()"
> from the patch series "tests: enable virtio tests on SPAPR"
> in which I have added the idea from Peter to ask the endianness
> directly to the guest using a new qtest command.
>
>  qtest.c                   |   7 ++
>  tests/libqos/virtio-pci.c |   2 +-
>  tests/libqtest.c          | 224 
> ++++++++++++++++++++--------------------------
>  tests/libqtest.h          |  16 +++-
>  tests/virtio-blk-test.c   |   2 +-
>  5 files changed, 118 insertions(+), 133 deletions(-)
>
> diff --git a/qtest.c b/qtest.c
> index 22482cc..b53b39c 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -537,6 +537,13 @@ static void qtest_process_command(CharDriverState *chr, 
> gchar **words)
>
>          qtest_send_prefix(chr);
>          qtest_send(chr, "OK\n");
> +    } else if (strcmp(words[0], "endianness") == 0) {
> +        qtest_send_prefix(chr);
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +        qtest_sendf(chr, "OK big\n");
> +#else
> +        qtest_sendf(chr, "OK little\n");
> +#endif
>  #ifdef TARGET_PPC64
>      } else if (strcmp(words[0], "rtas") == 0) {
>          uint64_t res, args, ret;
> diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c
> index 18b92b9..6e005c1 100644
> --- a/tests/libqos/virtio-pci.c
> +++ b/tests/libqos/virtio-pci.c
> @@ -86,7 +86,7 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, 
> uint64_t addr)
>      int i;
>      uint64_t u64 = 0;
>
> -    if (qtest_big_endian()) {
> +    if (target_big_endian()) {
>          for (i = 0; i < 8; ++i) {
>              u64 |= (uint64_t)qpci_io_readb(dev->pdev,
>                                  (void *)(uintptr_t)addr + i) << (7 - i) * 8;

Why rename the function? We're only changing its
implementation.

> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 6f6bdf1..27cf6b1 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -37,6 +37,7 @@ struct QTestState
>      bool irq_level[MAX_IRQ];
>      GString *rx;
>      pid_t qemu_pid;  /* our child QEMU process */
> +    bool big_endian;
>  };
>
>  static GHookList abrt_hooks;
> @@ -146,89 +147,6 @@ void qtest_add_abrt_handler(GHookFunc fn, const void 
> *data)
>      g_hook_prepend(&abrt_hooks, hook);
>  }
>
> -QTestState *qtest_init(const char *extra_args)
> -{
> -    QTestState *s;
> -    int sock, qmpsock, i;
> -    gchar *socket_path;
> -    gchar *qmp_socket_path;
> -    gchar *command;
> -    const char *qemu_binary;
> -
> -    qemu_binary = getenv("QTEST_QEMU_BINARY");
> -    g_assert(qemu_binary != NULL);

This diff arrangement makes the patch a bit hard to read;
what meant that the functions had to be moved around?

> +    /* ask endianness of the target */
> +
> +    qtest_sendf(s, "endianness\n");
> +    args = qtest_rsp(s, 1);
> +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> +    s->big_endian = strcmp(args[1], "big") == 0;
> +    g_strfreev(args);

This would be better in its own utility function, I think.

thanks
-- PMM