[Spice-devel] [PATCH spice-server] tests: Add a test for streaming devices

[Frediano Ziglio]

> 
> Frediano Ziglio writes:
> 
> > Currently create device, open it and pass some messages checking
> > they are handled.
> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/tests/Makefile.am          |   1 +
> >  server/tests/test-stream-device.c | 151
> >  ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 152 insertions(+)
> >  create mode 100644 server/tests/test-stream-device.c
> >
> > diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> > index 21e47c61..971575b5 100644
> > --- a/server/tests/Makefile.am
> > +++ b/server/tests/Makefile.am
> > @@ -58,6 +58,7 @@ check_PROGRAMS =				\
> >  	test-fail-on-null-core-interface	\
> >  	test-empty-success			\
> >  	test-channel				\
> > +	test-stream-device			\
> >  	$(NULL)
> >
> >  noinst_PROGRAMS =				\
> > diff --git a/server/tests/test-stream-device.c
> > b/server/tests/test-stream-device.c
> > new file mode 100644
> > index 00000000..92807eb0
> > --- /dev/null
> > +++ b/server/tests/test-stream-device.c
> > @@ -0,0 +1,151 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> 
> I don't like that too much. Could you at least put it at end of file?
> 

Most files start with this, I just copied the license.

> > +/*
> > +   Copyright (C) 2009-2017 Red Hat, Inc.
> > +
> > +   This library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   This library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with this library; if not, see
> > <http://www.gnu.org/licenses/>.
> > +*/
> > +/**
> > + * Test streaming device
> > + */
> > +
> > +#include <config.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include <spice/protocol.h>
> > +#include <spice/stream-device.h>
> > +
> > +#include "test-display-base.h"
> > +#include "test-glib-compat.h"
> > +
> > +#ifndef MIN
> > +#define MIN(a, b) ((a) > (b) ? (b) : (a))
> > +#endif
> > +
> > +static SpiceCharDeviceInstance vmc_instance;
> > +
> > +static uint8_t message[2048];
> 
> Where does the 2048 come from? Looks to me like you are using much less
> than that.
> 

Do you want to optimize test code? IMO we should reduce some buffering
and pre allocation on the server, not in the tests

> > +static unsigned pos = 0;
> > +static unsigned message_size;
> 
> Why is pos initialized and not message_size? Not that it matters, they
> are zero-initialized anyway, but it looks inconsistent and therefore
> puzzling.
> 

Make sense, this part is from other piece of codes, I'll remove the "pos = 0";

> > +
> > +static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> > +                     SPICE_GNUC_UNUSED const uint8_t *buf,
> > +                     int len)
> > +{
> > +    return len;
> > +}
> 
> By reading that code, I don't know if your test is expecting that
> function to be called. The test won't complain either way.
> 

Not currently important for the test, can be extended if needed.

> 
> > +
> > +static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> > +                    uint8_t *buf,
> > +                    int len)
> > +{
> > +    int ret;
> > +
> > +    if (pos == message_size) {
> > +        g_message("sent whole message");
> > +        pos++; /* Only print message once */
> > +    }
> > +    if (pos >= message_size) {
> > +        return 0;
> > +    }
> > +    ret = MIN(message_size - pos, len);
> 
> len >= message_size - pos is an important corner condition.
> Shouldn't you check for that condition more precisely?
> As written, the test won't complain if you go beyond message size.
> Can this legitimately happen the way you setup the test?
> 

I does not think it matters, IMO this would bound the test too much
on the implementation.

> > +    memcpy(buf, &message[pos], ret);
> > +    pos += ret;
> > +    // kick of next message read
> > +    spice_server_char_device_wakeup(&vmc_instance);
> 
> Not related to the test, but piqued my curiosity.
> In a real world environment, it is not the read itself that kicks the
> device wakeup, right?
> 

Currently Qemu keeps kicking spice-server so we need to do it
in some way.

> > +    return ret;
> > +}
> > +
> > +static void vmc_state(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
> > +                      SPICE_GNUC_UNUSED int connected)
> > +{
> > +}
> 
> Like for write, shouldn't the test check if the function is called?
> 

I think a fine follow up.

> > +
> > +static SpiceCharDeviceInterface vmc_interface = {
> > +    .base = {
> > +        .type          = SPICE_INTERFACE_CHAR_DEVICE,
> > +        .description   = "test spice virtual channel char device",
> > +        .major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
> > +        .minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
> > +    },
> > +    .state              = vmc_state,
> > +    .write              = vmc_write,
> > +    .read               = vmc_read,
> > +};
> > +
> > +static SpiceCharDeviceInstance vmc_instance = {
> > +    .subtype = "port",
> > +    .portname = "com.redhat.stream.0",
> 
> What is the significance of that name? If the test does not depend on
> it, shouldn't use something like "dummy" to make that explicit?
> 

It matters as you are checking a specific device.

> > +};
> > +
> > +static uint8_t *add_stream_hdr(uint8_t *p, StreamMsgType type, uint32_t
> > size)
> > +{
> > +    StreamDevHeader hdr;
> > +    memset(&hdr, 0, sizeof(hdr));
> > +    hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > +    hdr.type = GUINT16_TO_LE(type);
> > +    hdr.size = GUINT32_TO_LE(size);
> 
> I would prefer:
>     StreamDevHeader hdr = {
>         .protocol = ...
>     };
> 
> Side comment. I have seen code with GUINT16_TO_LE and GUINT32_TO_LE
> all over the place. I don't like it because it adds distributed
> dependency / knowledge about the type of each field. Since C99, we can
> write a macro using _Generic and inline functions so that you could
> write
> 
>        LE_SET(hdr.type, type);
> 
> and have the compiler automatically pick up the right one for the size.
> To add to my list of patches to write.
> 

This is OT, this is a protocol, sizes are not going to change.

> 
> > +
> > +    memcpy(p, &hdr, sizeof(hdr));
> > +    return p + sizeof(hdr);
> > +}
> > +
> > +static uint8_t *add_format(uint8_t *p, uint32_t w, uint32_t h,
> > SpiceVideoCodecType codec)
> > +{
> > +    StreamMsgFormat fmt;
> > +    memset(&fmt, 0, sizeof(fmt));
> > +    fmt.width = GUINT32_TO_LE(w);
> > +    fmt.height = GUINT32_TO_LE(h);
> > +    fmt.codec = codec;
> 
> StreamMsgFormat fmt = {
>     .width =
>     .height =
>     .codec =
> };
> 
> > +
> > +    p = add_stream_hdr(p, STREAM_TYPE_FORMAT, sizeof(fmt));
> > +    memcpy(p, &fmt, sizeof(fmt));
> > +    return p + sizeof(fmt);
> > +}
> > +
> > +static void test_stream_device(void)
> > +{
> > +    SpiceCoreInterface *core = basic_event_loop_init();
> > +    Test *test = test_new(core);
> > +
> > +    // build some messages into device
> > +    // here we are testing the device is reading two consecutive
> > +    // format messages
> 
> I don't think you are testing exactly what the comment says. It looks to
> me like you are only testing that at least message_size data is read.
> Maybe that's all you wanted to test, but then I would rephrase the
> comment to say so.
> 
> Here are some alternatives that would be stronger tests without much
> change to your code:
> 
> - Check that you read exactly the right number of bytes, nothing more
> 

Why should stop? Did you run the test or try to change it?
Yes, the stop condition is a bit fragile in this test.
Maybe inserting an error at the end and check that it stops to read even
if kicked again is more robust.

> - Check if the number of bytes read is consistent with what's in the
>   headers.
> 

IMO is too implementation dependent. What about if implementation starts
buffering?

> - Check how many calls to vmc_read were made, and what size they expected
> 

Likewise

> - Check that no calls to vmc_read occur without data (i.e. before kicks)
> 

Yes, good follow up.

> I don't think these would add much to your code, but they would catch
> many more possible errors, e.g. some uninitialized size, etc.
> 
> 
> > +    uint8_t *p = message;
> > +    p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
> > +    p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_VP9);
> > +    message_size = p - message;
> 
> > +
> > +    g_test_expect_message(G_LOG_DOMAIN, G_LOG_LEVEL_MESSAGE, "sent whole
> > message");
> > +    vmc_instance.base.sif = &vmc_interface.base;
> > +    spice_server_add_interface(test->server, &vmc_instance.base);
> > +
> > +    // we need to open the device and kick the start
> > +    spice_server_port_event(&vmc_instance, SPICE_PORT_EVENT_OPENED);
> > +    spice_server_char_device_wakeup(&vmc_instance);
> > +
> > +    g_test_assert_expected_messages();
> > +    test_destroy(test);
> > +    basic_event_loop_destroy();
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +    g_test_init(&argc, &argv, NULL);
> > +
> > +    g_test_add_func("/server/stream-device", test_stream_device);
> > +
> > +    return g_test_run();
> > +}
> 

Frediano