Re: [Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

> > Hi > > On Tue, Feb 13, 2018 at 10:08 AM, Frediano Ziglio wrote: > >> > >> Hi > >> > >> On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio > >> wrote: > >> >> > >> >> From: Marc-André Lureau > >> >> > >> >> For some reason,

Re: [Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

Hi On Tue, Feb 13, 2018 at 10:08 AM, Frediano Ziglio wrote: >> >> Hi >> >> On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio wrote: >> >> >> >> From: Marc-André Lureau >> >> >> >> For some reason, the URIs test didn't include

Re: [Spice-devel] [RFC PATCH 1/1] separate and encapsulate the agent business code

Hi Lukas, You asked for it, so here is my TON (TON stands for TON of nits). Except for what’s just below this, nothing major, just sharing ideas. Top-comment: since your objective is to “change as little as possible”, maybe it would be possible to start with a patch doing just a file rename?

[Spice-devel] [PATCH spice-server v5 2/8] test-stream-device: Better Qemu emulation for data reading

Qemu does not trigger a new data read if we don't read all data in the buffer. Signed-off-by: Frediano Ziglio --- server/tests/test-stream-device.c | 43 +-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git

[Spice-devel] [PATCH spice-server v5 3/8] test-stream-device: Test batched multiple messages

Test all batched (send together) messages are handled correctly and device is not stuck. Signed-off-by: Frediano Ziglio --- server/tests/test-stream-device.c | 36 1 file changed, 36 insertions(+) diff --git

[Spice-devel] [PATCH spice-server v5 6/8] stream-device: Workaround Qemu bug closing device

Previous patch causes a bug into Qemu if the patch 46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault on disconnect") is not include in that version of Qemu. This crash happens when device is closed inside a write operation. For SPICE character device a

[Spice-devel] [PATCH spice-server v5 0/8] Better handling reset for streaming device

Changes since v4: - split reset patch; - add a workaround for a Qemu bug, seems easier to do it instead of detecting it; - add another problem with fix and test case. Changes since v3: - add a test case. Changes since v2: - split first patch between fix and test; - better wording (not code

[Spice-devel] [PATCH spice-server v5 4/8] stream-device: Implements properly device reset on open/close

Due to the way Qemu handle the device we must consume all pending data inside the callback to read data from the device. We need to consume all data from previous device dialog to avoid that next device usage is still seeing old data. This as Qemu return 0 if you call

[Spice-devel] [PATCH spice-server v5 8/8] test-stream-device: Check we don't read past data message

Test case for the issue fixed by previous commit. Signed-off-by: Frediano Ziglio --- server/tests/test-stream-device.c | 42 +++ 1 file changed, 42 insertions(+) diff --git a/server/tests/test-stream-device.c

[Spice-devel] [PATCH spice-server v5 7/8] stream-device: Do not read past data message

If data message is followed by another message was theoretically possible that device looses the sync with the guest. The actual Qemu and agent implementation avoids it but better to avoid it. Signed-off-by: Frediano Ziglio --- server/stream-device.c | 2 +- 1 file changed,

[Spice-devel] [PATCH spice-server v5 5/8] stream-device: Disable guest device on errors

To communicate the error and avoiding to have to read any data the guest want to write disable the device. Signed-off-by: Frediano Ziglio --- server/stream-device.c | 20 1 file changed, 20 insertions(+) diff --git a/server/stream-device.c

[Spice-devel] [PATCH spice-server v5 1/8] stream-device: Avoid device to get stuck if multiple messages are batched

If messages are sent together by the agent the device is reading only part of the data. This cause Qemu to not poll for new data and stream_device_read_msg_from_dev won't be called again. This can cause a stall. To avoid this continue handling data after a full message was processed.

Re: [Spice-devel] [PATCH spice-streaming-agent v2 1/3] mjpeg-fallback: a more C++ way of handling options

On Tue, 2018-02-13 at 09:18 -0500, Frediano Ziglio wrote: > I would use a more "polite": "a more high level way of handling options" Ok :) > > > > Use C++ standard library: > > - std::string > > - std::stoi() to parse the numbers (also note atoi() behavior is undefined > > in > > case of

Re: [Spice-devel] [PATCH spice-streaming-agent v2 1/3] mjpeg-fallback: a more C++ way of handling options

I would use a more "polite": "a more high level way of handling options" > > Use C++ standard library: > - std::string > - std::stoi() to parse the numbers (also note atoi() behavior is undefined in > case of errors) > - exceptions for errors, makes testing and potential future changes to >

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/3] src/unitests: add temporary files to .gitignore

On Tue, 2018-02-13 at 09:08 -0500, Frediano Ziglio wrote: > > > > Also, leading / is unnecessary. > > > > Signed-off-by: Lukáš Hrázký > > --- > > src/unittests/.gitignore | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/3] src/unitests: add temporary files to .gitignore

> > Also, leading / is unnecessary. > > Signed-off-by: Lukáš Hrázký > --- > src/unittests/.gitignore | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore > index 36548a1..e871e91 100644 > ---

[Spice-devel] [PATCH spice-streaming-agent v2 3/3] mjpeg-fallback: unittest for the options parsing

Introduce a unit test framework (Catch) to the codebase and a simple unit test for parsing the options of the mjpeg plugin. Signed-off-by: Lukáš Hrázký --- configure.ac | 3 ++ src/mjpeg-fallback.cpp| 5 +++ src/mjpeg-fallback.hpp

[Spice-devel] [PATCH spice-streaming-agent v2 2/3] src/unitests: add temporary files to .gitignore

Also, leading / is unnecessary. Signed-off-by: Lukáš Hrázký --- src/unittests/.gitignore | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore index 36548a1..e871e91 100644 --- a/src/unittests/.gitignore

[Spice-devel] [PATCH spice-streaming-agent v2 1/3] mjpeg-fallback: a more C++ way of handling options

Use C++ standard library: - std::string - std::stoi() to parse the numbers (also note atoi() behavior is undefined in case of errors) - exceptions for errors, makes testing and potential future changes to error handling easier. Signed-off-by: Lukáš Hrázký ---

[Spice-devel] [PATCH spice-streaming-agent v2 0/3] first unit test and options parsing improvements

This series introduces a C++ unit test framework called Catch to the codebase, adds a simple unit test for the options parsing for the mjpeg plugin and improves on the option parsing code. Since we more or less agreed we can solve the Catch package in RHEL one way or another, I suppose we can

[Spice-devel] [PATCH spice-server v3 1/2] stream-device: handle cursor from device

Signed-off-by: Frediano Ziglio --- server/stream-device.c | 165 ++--- 1 file changed, 158 insertions(+), 7 deletions(-) diff --git a/server/stream-device.c b/server/stream-device.c index b7553c67..f6fd9108 100644 ---

[Spice-devel] [PATCH spice-server v3 2/2] stream-device: Implement mouse movement

Signed-off-by: Frediano Ziglio --- server/stream-device.c | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/server/stream-device.c b/server/stream-device.c index f6fd9108..bf03efb6 100644 --- a/server/stream-device.c

[Spice-devel] [PATCH spice-server v3 0/2] Implement cursor for streaming device

Changes since v2: - shrink buffer after the message is handled. Frediano Ziglio (2): stream-device: handle cursor from device stream-device: Implement mouse movement server/stream-device.c | 201 +++-- 1 file changed, 194 insertions(+), 7

Re: [Spice-devel] [PATCH spice-streaming-agent v2] tests: Fix tests names

On Tue, 2018-02-13 at 13:48 +, Frediano Ziglio wrote: > The test-hexdump was more an utility to test hexdump function instead > of a proper test. > Rename hexdump.sh to test-hexdump as this is the real test executed > during "make check". > > Signed-off-by: Frediano Ziglio

[Spice-devel] [PATCH spice-streaming-agent v2] tests: Fix tests names

The test-hexdump was more an utility to test hexdump function instead of a proper test. Rename hexdump.sh to test-hexdump as this is the real test executed during "make check". Signed-off-by: Frediano Ziglio --- src/unittests/.gitignore | 2 +-

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

> On 13 Feb 2018, at 14:39, Frediano Ziglio wrote: > >>> >>> On 12 Feb 2018, at 17:58, Frediano Ziglio wrote: >>> >>> In SPICE, as in Glib, ERROR is more "strong" than CRITICAL >>> (I don't know where this came, even in Solaris CRITICAL is more >>>

Re: [Spice-devel] [PATCH spice-streaming-agent] tests: Fix tests names

On Tue, 2018-02-13 at 13:33 +, Frediano Ziglio wrote: > The test-hexdump was more an utility to test hexdump function instead > of a proper test. > Rename hexdump.sh to test-hexdump as this is the real test executed > during "make check". > > Signed-off-by: Frediano Ziglio

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

> On 12 Feb 2018, at 17:58, Frediano Ziglio wrote: > > In SPICE, as in Glib, ERROR is more "strong" than CRITICAL > (I don't know where this came, even in Solaris CRITICAL is more > strong like Linux, maybe an initial mistake). > By default CRITICAL is fatal (it calls

[Spice-devel] [PATCH spice-streaming-agent] tests: Fix tests names

The test-hexdump was more an utility to test hexdump function instead of a proper test. Rename hexdump.sh to test-hexdump as this is the real test executed during "make check". Signed-off-by: Frediano Ziglio --- src/unittests/.gitignore| 2 +-

Re: [Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

> > Hi > > On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio wrote: > >> > >> From: Marc-André Lureau > >> > >> For some reason, the URIs test didn't include spice+unix:// checks, > >> probably because they came about the same time. > >> > >>