[Spice-devel] [PATCH spice-streaming-agent 6/9] Add a unit test for the stream port

[Lukáš Hrázký]

On Tue, 2018-05-15 at 16:52 -0400, Frediano Ziglio wrote:
> A bit more comments.
> 
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  src/unittests/.gitignore           |  1 +
> >  src/unittests/Makefile.am          |  9 ++++-
> >  src/unittests/test-stream-port.cpp | 69
> >  ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 78 insertions(+), 1 deletion(-)
> >  create mode 100644 src/unittests/test-stream-port.cpp
> > 
> > diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> > index 22f1335..ef9e31b 100644
> > --- a/src/unittests/.gitignore
> > +++ b/src/unittests/.gitignore
> > @@ -2,4 +2,5 @@
> >  /test-*.log
> >  /test-*.trs
> >  /test-mjpeg-fallback
> > +/test-stream-port
> >  /test-suite.log
> > diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> > index 0fc6d50..67f9c42 100644
> > --- a/src/unittests/Makefile.am
> > +++ b/src/unittests/Makefile.am
> > @@ -16,11 +16,12 @@ AM_CFLAGS = \
> >  check_PROGRAMS = \
> >  	hexdump \
> >  	test-mjpeg-fallback \
> > +	test-stream-port \
> >  	$(NULL)
> >  
> >  TESTS = \
> >  	test-hexdump.sh \
> > -	test-mjpeg-fallback \
> > +	test-stream-port \
> >  	$(NULL)
> >  
> >  noinst_PROGRAMS = \
> > @@ -46,6 +47,12 @@ test_mjpeg_fallback_LDADD = \
> >  	$(JPEG_LIBS) \
> >  	$(NULL)
> >  
> > +test_stream_port_SOURCES = \
> > +	test-stream-port.cpp \
> > +	../stream-port.cpp \
> > +	../error.cpp \
> > +	$(NULL)
> > +
> >  EXTRA_DIST = \
> >  	test-hexdump.sh \
> >  	hexdump1.in \
> > diff --git a/src/unittests/test-stream-port.cpp
> > b/src/unittests/test-stream-port.cpp
> > new file mode 100644
> > index 0000000..9added3
> > --- /dev/null
> > +++ b/src/unittests/test-stream-port.cpp
> > @@ -0,0 +1,69 @@
> > +#define CATCH_CONFIG_MAIN
> > +#include <catch/catch.hpp>
> > +#include <sys/socket.h>
> > +
> > +#include "stream-port.hpp"
> > +
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +/*
> > + * Note the semantics of a socketpair may be different from the virtio port
> > + * that is actually used for the real interface.
> > + */
> > +SCENARIO("test basic IO on the stream port", "[port][io]") {
> > +    GIVEN("An open port (socketpair)") {
> > +        int fd[2];
> > +        const char *src_buf = "brekeke";
> > +        const size_t src_size = strlen(src_buf) + 1;
> > +
> > +        socketpair(AF_LOCAL, SOCK_STREAM, 0, fd);
> > +
> > +        WHEN("reading data in one go") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, src_size);
> > +            CHECK(std::string(buf) == src_buf);
> > +        }
> > +
> > +        WHEN("reading data in two steps") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, 3);
> > +            CHECK(std::string(buf, 3) == "bre");
> > +            ssa::read_all(fd[1], buf, 5);
> 
> The fact that src_buf contains a terminator is not helpful, in
> this case is converting the string from C stripping the terminator
> but the code read "keke\0" (5 bytes), here seems it read just "keke".

Not sure what you want to say here... The code is slightly confusing
but correct, right? Would you suggest how to improve it?

The thing is the \0 gets written to the socket and then read from it
too. So "keke\0" is terminated and you can construct the std::string
with the line below. Can't do that with "bre".

> > +            CHECK(std::string(buf) == "keke");
> > +        }
> > +
> > +        WHEN("writing data") {
> > +            ssa::write_all(fd[1], src_buf, src_size);
> > +            char buf[10];
> > +            CHECK(read(fd[0], buf, src_size) == src_size);
> > +            CHECK(std::string(buf) == src_buf);
> > +        }
> > +
> > +        WHEN("closing the remote end and trying to read") {
> > +            CHECK(write(fd[0], src_buf, src_size) == src_size);
> > +            char buf[10];
> > +            ssa::read_all(fd[1], buf, 3);
> > +            CHECK(std::string(buf, 3) == "bre");
> > +            CHECK(close(fd[0]) == 0);
> > +            ssa::read_all(fd[1], buf, 5);
> > +            CHECK(std::string(buf) == "keke");
> > +            // TODO loops infinitely, we should recognize the remote end is
> > closed
> 
> This looks a bug to me.

Well, when thinking about it some days later after sending the patches
I kinda think so too. But at the time we discussed it you didn't say
you think it's a bug and just thought it's a bit weird (IIRC), so...

Shall we report a bug to qemu?

> > +            //ssa::read_all(fd[1], buf, 1);
> > +        }
> > +
> > +        WHEN("closing the remote end and trying to write") {
> > +            ssa::write_all(fd[1], src_buf, src_size);
> > +            char buf[10];
> > +            CHECK(close(fd[0]) == 0);
> 
> This file descriptor is not already closed? Does catch calls fork() ?

I don't think Catch (the unit test framework) calls fork. But no clue
what you mean... I'm closing the fd here as part of the test case...

> > +            // TODO causes a SIGPIPE
> > +            //ssa::write_all(fd[1], src_buf, src_size);
> 
> A signal(SIGPIPE, SIG_IGN) should fix this. The device does not raise a signal apparently.

Got that in a later patch, where the test case is fixed... For a reason
I forgot I couldn't make it work here I think...

Thanks for the review!
Lukas

> > +        }
> > +
> > +        // clean up the descriptors in case they are still open
> > +        close(fd[0]);
> > +        close(fd[1]);
> > +    }
> > +}
> 
> Frediano