>
> On Thu, 2018-05-17 at 05:24 -0400, Frediano Ziglio wrote:
> > >
> > > On Wed, 2018-05-16 at 12:47 -0400, Frediano Ziglio wrote:
> > > > >
> > > > > 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)
> > > > >
> > > >
> > > > Why did you remove test-mjpeg-fallback ?
> > >
> > > By accident! I'll put it back, don't know how that happened.
> > >
> > > > > 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..3f9dadf
> > > > > --- /dev/null
> > > > > +++ b/src/unittests/test-stream-port.cpp
> > > > > @@ -0,0 +1,69 @@
> > > >
> > > > Didn't notice previously.
> > > > You should add the copyright/file header.
> > >
> > > Yeah, I forgot it here, will add it.
> > >
> > > > > +#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);
> > > > > +
> > > > > + 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_size) == 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, 4);
> > > > > + CHECK(std::string(buf, 4) == "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_size) == 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, 4);
> > > > > + CHECK(std::string(buf, 4) == "keke");
> > > > > + // TODO blocks infinitely, we should recognize the
> > > > > remote
> > > > > end is
> > > > > closed
> > > > > + //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);
> > > > > + // TODO causes a SIGPIPE
> > > > > + //ssa::write_all(fd[1], src_buf, src_size);
> > > > > + }
> > > >
> > > > This is testing nothing actually, I would add the complete scenario
> > > > on following patch
> > >
> > > Well, it documents the failing test case, if it wasn't for the
> > > discrepancy in behaviour of the virtio port and the socketpair, it
> > > could also be used to investigate the bug (before it was fixed) easily
> > > in the test case. But if you insist, I'll remove it.
> > >
> >
> > SIGPIPE or not SIGPIPE is not a bug but just a difference in the file
> > behaviour.
>
> Yes, but I couldn't actually make the test case work in the blocking
> mode for reasons I unfortunately forgot...
>
> > The behaviour in read is more of a bug.
>
> Yes, bug in the streaming agent with the blocking implementation, I
No, the bug, if is a bug, is in the kernel. When a socket/pipe are closed,
for instance, while in a read/write you get an error, system call does
not get stuck. I think this device should behave like that.
> think. I also realized the original comment was probably right, the
> read was actually looping (it was the write that was blocking), it's
> been some time and I forgot the details :( sorry for the confusion.
>
Mumble mumble mumble... I'll give it a try.
> > Is write still blocking (like read) writing to a device that is closed?
>
> In blocking mode (i.e. in the state of this patch), yes, the write is
> blocking after the device is closed.
>
> > > Cheers,
> > > Lukas
> > >
> >
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel