> 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. > > > + 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. For the test sake won't be better to put the entire test case on the following patch? > > > + //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? > > > + // 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. > 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