> > On 16 Nov 2017, at 10:46, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > > >> > >> This is currently unlikely to happen since we communicate over a pipe > >> and the pipe buffer is sufficiently large to avoid splitting the > >> message. But for completeness, we should handle this scenario. > >> > >> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > >> --- > >> Since v1: > >> - updates from Frediano's review > >> - bumps spice-protocol requirement for > >> STREAM_MSG_CAPABILITIES_MAX_BYTES symbol > >> - change msg_pos from uint8_t to uint32_t > >> - move reset of msg_pos to calling function > >> (stream_device_port_event()) > >> > >> configure.ac | 2 +- > >> server/stream-device.c | 31 ++++++++++++++++++++++--------- > >> 2 files changed, 23 insertions(+), 10 deletions(-) > >> > > > > I would propose a challenge: > > - found the bug; > > - write an automatic test to prove the bug; > > - fix the bug. > > (not in strictly order). > > > > Nothing personal here, just that would be great to have more > > automated testing and thinking with test in mind helps. > > +1. This is in-line with the consensus that we need better tests. > > Any idea how we could test the streaming agent? The test you put > in place with hexdump hints that you had some idea, but I don’t know > what it is. > Some images/video can help :-) - https://twitter.com/timbray/status/822470746773409794?lang=en - https://www.youtube.com/watch?v=0GypdsJulKE Writing tests is an hard job. Said that we have integration tests run by us and some other by our Q/A team. But usually integration tests are hard and long to run (for instance as they require some long setup or because they are manual so not good for a CI). Having more unit tests would help. How to write unit tests for C? Well, not easy either. There are ton of tools for it but in many case they get complicated. There are usually 2 kind of "facilities": 1) unit tests frameworks/libraries/whatever; 2) mock frameworks/tools/whatever. 1) are quite easy to find and basically they allows to define the tests in some way, helps run them, get some fancy output. Glib provides this for instance (and we use in spice-server). Personally I like the idea behind MinUnit framework (http://www.jera.com/techinfo/jtns/jtn002.html)! Is really 3 lines of code. Is not much important the unit tests framework, much more the presence of tests! 2) helps writing tests. Tests are mainly checking that doing something specific has some results. Technically this is usually defined having a set of input and output pairs and testing that a given function returns such outputs given the respective input. But if you starts considering and input and output include some internal states and the amount of possible cases you can have you can easily understand that the problem is impossible to define. So basically what you try to do is to limit the cases and check what's happens. For instance if you are trying to implement a strdup like function you want to check that a malloc function is called or similar. Here came the mocking, basically replacing some part for checking and to limit state bloating. But compared to high level languages (like Python) catching and altering object behaviour is easy while in C is quite hard. All solutions I found are invasive and depending on some external tools (like specific linker options). All that said I don't have a magic answer. automake offers the possibility to write tests basically calling some executables (can be compiled or scripts or a mix). Having inside the same repository make sure they continue to run and running "make check" that they continue to work. > > > > > I'll try to write a test (I've already made the other 2 points). > > > >> diff --git a/configure.ac b/configure.ac > >> index fb266ad4c..3401dba83 100644 > >> --- a/configure.ac > >> +++ b/configure.ac > >> @@ -156,7 +156,7 @@ AS_IF([test x"$have_smartcard" = "xyes"], [ > >> AS_VAR_APPEND([SPICE_REQUIRES], [" libcacard >= 0.1.2"]) > >> ]) > >> > >> -SPICE_PROTOCOL_MIN_VER=0.12.13 > >> +SPICE_PROTOCOL_MIN_VER=0.12.14 > >> PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= > >> $SPICE_PROTOCOL_MIN_VER]) > >> AC_SUBST([SPICE_PROTOCOL_MIN_VER]) > >> > >> diff --git a/server/stream-device.c b/server/stream-device.c > >> index fc5b50659..efa6d8db5 100644 > >> --- a/server/stream-device.c > >> +++ b/server/stream-device.c > >> @@ -42,6 +42,12 @@ struct StreamDevice { > >> > >> StreamDevHeader hdr; > >> uint8_t hdr_pos; > >> + union { > >> + StreamMsgFormat format; > >> + StreamMsgCapabilities capabilities; > >> + uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES]; > >> + } msg; > >> + uint32_t msg_pos; > >> bool has_error; > >> bool opened; > >> bool flow_stopped; > >> @@ -155,19 +161,25 @@ handle_msg_invalid(StreamDevice *dev, > >> SpiceCharDeviceInstance *sin, const char * > >> static bool > >> handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin) > >> { > >> - StreamMsgFormat fmt; > >> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin); > >> - int n = sif->read(sin, (uint8_t *) &fmt, sizeof(fmt)); > >> - if (n == 0) { > >> - return false; > >> - } > >> - if (n != sizeof(fmt)) { > >> + > >> + spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader)); > >> + spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT); > >> + > >> + int n = sif->read(sin, dev->msg.buf + dev->msg_pos, > >> sizeof(StreamMsgFormat) - dev->msg_pos); > >> + if (n < 0) { > >> return handle_msg_invalid(dev, sin, NULL); > >> } > >> - fmt.width = GUINT32_FROM_LE(fmt.width); > >> - fmt.height = GUINT32_FROM_LE(fmt.height); > >> - stream_channel_change_format(dev->stream_channel, &fmt); > >> > >> + dev->msg_pos += n; > >> + > >> + if (dev->msg_pos < sizeof(StreamMsgFormat)) { > >> + return false; > >> + } > >> + > >> + dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width); > >> + dev->msg.format.height = GUINT32_FROM_LE(dev->msg.format.height); > >> + stream_channel_change_format(dev->stream_channel, &dev->msg.format); > >> return true; > >> } > >> > >> @@ -334,6 +346,7 @@ stream_device_port_event(RedCharDevice *char_dev, > >> uint8_t > >> event) > >> allocate_channels(dev); > >> } > >> dev->hdr_pos = 0; > >> + dev->msg_pos = 0; > >> dev->has_error = false; > >> dev->flow_stopped = false; > >> red_char_device_reset(char_dev); > > In this specific case I sent a patch to add a test for this issue. The test does not fail on master but fails when this patch is applied. As a rule of thumb tests should not be too dependent on implementation as they will break easily. In case of spice server for instance trying to use the public API is a good idea, this is not expected to change. But obviously if you want to test for instance RedStream directly you need to test RedStream interface. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel