Re: [Qemu-devel] [PATCH 05/12] spapr-vty: add qtest test case

[Alexander Graf]

On 19.06.2013, at 23:43, Anthony Liguori wrote:

> Alexander Graf <address@hidden> writes:
> 
>> On 19.06.2013, at 22:40, Anthony Liguori wrote:
>> 
>>> Pretty basic for the moment but the interface is pretty simple.
>>> 
>>> Signed-off-by: Anthony Liguori <address@hidden>
>>> ---
>>> tests/Makefile         |  3 ++
>>> tests/spapr-vty-test.c | 89 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 92 insertions(+)
>>> create mode 100644 tests/spapr-vty-test.c
>>> 
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index c107489..7c0ce1a 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -67,6 +67,8 @@ gcov-files-sparc64-y += hw/m48t59.c
>>> check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>>> gcov-files-arm-y += hw/tmp105.c
>>> 
>>> +check-qtest-ppc64-y = tests/spapr-vty-test$(EXESUF)
>>> +
>>> GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h 
>>> tests/test-qmp-commands.h
>>> 
>>> test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>>> @@ -133,6 +135,7 @@ tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>>> tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>>> +tests/spapr-vty-test$(EXESUF): tests/spapr-vty-test.o
>>> 
>>> # QTest rules
>>> 
>>> diff --git a/tests/spapr-vty-test.c b/tests/spapr-vty-test.c
>>> new file mode 100644
>>> index 0000000..8e3908b
>>> --- /dev/null
>>> +++ b/tests/spapr-vty-test.c
>>> @@ -0,0 +1,89 @@
>>> +/*
>>> + * QTest testcase for the sPAPR VTY
>>> + *
>>> + * Copyright IBM, Corp. 2013
>>> + *
>>> + * Authors:
>>> + *  Anthony Liguori   <address@hidden>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +#include "libqtest.h"
>>> +
>>> +#include <glib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +
>>> +#define H_GET_TERM_CHAR         0x54
>>> +#define H_PUT_TERM_CHAR         0x58
>>> +
>>> +static char *qmp_ringbuf_read(const char *device, int max_len)
>>> +{
>>> +    size_t len;
>>> +    char *ret;
>>> +    char *ptr;
>>> +
>>> +    ret = qtest_qmp(global_qtest,
>>> +                    "{ 'execute': 'ringbuf-read', "
>>> +                    "  'arguments': { 'device': '%s', 'size': %d } }",
>>> +                    device, max_len);
>>> +    len = strlen(ret);
>>> +
>>> +    /* Skip pass {"return" */
>>> +    ptr = strchr(ret, '"');
>>> +    g_assert(ptr != NULL);
>>> +    ptr = strchr(ptr + 1, '"');
>>> +    g_assert(ptr != NULL);
>>> +
>>> +    /* Start of data */
>>> +    ptr = strchr(ptr + 1, '"');
>>> +    g_assert(ptr != NULL);
>>> +    ptr += 1;
>>> +
>>> +    len -= ptr - ret;
>>> +    memmove(ret, ptr, len);
>>> +    ret[len] = 0;
>>> +
>>> +    ptr = strrchr(ret, '"');
>>> +    g_assert(ptr != NULL);
>>> +    *ptr = 0;
>> 
>> Can't this be moved to a more generic function? Something along the lines of
>> 
>>  char *qmp_execute(const char *device, int max_len, const char *func, const 
>> char *args)
>> 
>> and then call it like:
>> 
>>  return qmp_execute(device, max_len, "ringbuf-read", "");
> 
> Yeah, it's not that simple.  We need to pull in the JSON parsing code
> etc too.
> 
> I'm going to look into that but I think it's outside the scope of this series.
> 
>> here? Or let the caller do the argument gathering completely. But a
>> function like surely occurs in other places too, no?
> 
> qmp support is pretty new in qtest and not quite baked yet.  My
> expectation is that this function will disappear over time.

Ok, works for me :). There really should be a more clever logic here 
eventually. Maybe you could construct a QMP call object in several steps until 
you send it out, to reflect the tree structure of JSON.

> 
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void vty_ping(void)
>>> +{
>>> +    const char *greeting = "Hello, world!";
>>> +    char *data;
>>> +    int i;
>>> +
>>> +    for (i = 0; greeting[i]; i++) {
>>> +        spapr_hcall3(H_PUT_TERM_CHAR, 0, 1, (uint64_t)greeting[i] << 56);
>> 
>> This looks like endianness breakage? Hypercall arguments should go in
>> native endianness. Registers don't have endianness. Only memory
>> accesses do.
> 
> Yes, you are correct re: hcall args but it's not a breakage.
> 
> VTY is weird.  The characters are packed into the arguments.  So sending
> two chars would look like:
> 
> spapr_hcall3(H_PUT_TERM_CHAR, 0, 2,
>            (uint64_t)greeting[0] << 56 |
>            (uint64_t)greeting[1] << 48);
> 
> It can take an additional argument too for the next 8 bytes.
> 
> So even within the Linux kernel or SLOF on big endian, the code would
> look like this too.

I only realized that later in the series. I think I got confused on this one 
when it got introduced already. What a terrific interface ;).


Alex