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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2018-05-16 at 05:43 -0400, Frediano Ziglio wrote:
> > On Tue, 2018-05-15 at 16:52 -0400, Frediano Ziglio wrote:
> > > A bit more comments.
> > > 
> > > > 
> > > > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > > > ---
> > > >  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".
> > 
> 
> Is src_size is just strlen(src_buf), you can have
> 
>           ssa::read_all(fd[1], buf, 3);
>           CHECK(std::string(buf, 3) == "bre");
>           ssa::read_all(fd[1], buf, 4);
>           CHECK(std::string(buf, 4) == "keke");
> 
> this will avoid confusion with the terminator.
> Obviously requires some adjustment in other code.

Ok, I'll do that.

> > > > +            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?
> > 
> 
> Why does this loop forever, I don't remember ?
> Always returning EAGAIN ?
> I remember you told me was hanging, not looping.

Actually I think the comment is wrong. In blocking mode, the write() to
a closed virtio port blocks indefinitely. In nonblocking mode, the
write() returns EAGAIN (and subsequent poll() returns SIGHUP). I
unfortunately forgot the details already so I'm pulling this from old
conversations :/

> For the test sake won't be better to put the entire test case on
> the following patch?

I included the test case for completeness, but didn't finish it since
it's broken at this stage. Want me to remove it completely then and add
it only when it's working?

> > > > +            //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...
> > 
> 
> Above you closed the socket so without fork() I expect i this test case to
> have the file descriptor already closed, the fact that close still returns
> 0 is weird, unless there is a fork in the middle or another way.
> Maybe the scenario is repeated every time for different cases?

Oh, yes, I understand now. Yes, it does run the whole scenario for each
test case independently (which is great and basically needed, otherwise
you get a mess when the tests start growing).

> > > > +            // 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...
> > 
> 
> Yes, would more also this test case to the next patch.

Same as above..

> > Thanks for the review!
> > Lukas
> > 
> > > > +        }
> > > > +
> > > > +        // clean up the descriptors in case they are still open
> > > > +        close(fd[0]);
> > > > +        close(fd[1]);
> > > > +    }
> > > > +}
> > > 
> > > Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]