Frediano Ziglio writes: > Currently create device, open it and pass some messages checking > they are handled. > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > --- > 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? > +/* > + 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. > +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. > + > +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. > + > +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? > + 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? > + 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? > + > +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? > +}; > + > +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. > + > + 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 - Check if the number of bytes read is consistent with what's in the headers. - Check how many calls to vmc_read were made, and what size they expected - Check that no calls to vmc_read occur without data (i.e. before kicks) 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(); > +} -- Cheers, Christophe de Dinechin (IRC c3d) _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel