On Thu, 2018-05-17 at 09:26 -0400, Frediano Ziglio wrote:
> >
> > 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.
I was talking about a different issue here, while reading from a closed
socket/virtio port (as opposed to writing to one), this line:
//ssa::read_all(fd[1], buf, 1);
It actually does loop indefinitely before the change to the nonblocking
implementation, which is a bug in spice streaming agent, not hard to
fix. You can try it out too, try to checkout the commit, uncomment the
line and run the test. I've pushed the branch here for an easier pull:
https://gitlab.com/lhrazky/spice-streaming-agent/commits/read_write_refactor-v2
> > 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