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

[Frediano Ziglio]

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".

> +            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.

> +            //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() ?

> +            // 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.

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

Frediano