> > 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? > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel