date:20180213

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

2018-02-13 Thread Frediano Ziglio

> 
> 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 spice+unix:// checks,
> >> >> probably because they came about the same time.
> >> >>
> >> >> Signed-off-by: Marc-André Lureau 
> >> >> ---
> >> >>  tests/session.c | 28 +---
> >> >>  1 file changed, 25 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/tests/session.c b/tests/session.c
> >> >> index 7ed4a41..413d812 100644
> >> >> --- a/tests/session.c
> >> >> +++ b/tests/session.c
> >> >> @@ -9,6 +9,7 @@ typedef struct {
> >> >>  const gchar *uri_input;
> >> >>  const gchar *uri_output;
> >> >>  const gchar *message;
> >> >> +const gchar *unix_path;
> >> >>  } TestCase;
> >> >>
> >> >>  static void test_session_uri_bad(void)
> >> >> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase
> >> >> *tests,
> >> >> const guint cases)
> >> >>
> >> >>  /* Set URI and check URI, port and tls_port */
> >> >>  for (i = 0; i < cases; i++) {
> >> >> -gchar *uri, *port, *tls_port, *host, *username, *password;
> >> >> +gchar *uri, *port, *tls_port, *host, *username, *password,
> >> >> *unix_path;
> >> >>
> >> >>  s = spice_session_new();
> >> >>  if (tests[i].message != NULL)
> >> >> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
> >> >> *tests, const guint cases)
> >> >>   "host", ,
> >> >>   "username", ,
> >> >>   "password", ,
> >> >> + "unix-path", _path,
> >> >>NULL);
> >> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> >> uri);
> >> >>  g_assert_cmpstr(tests[i].port, ==, port);
> >> >>  g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
> >> >>  g_assert_cmpstr(tests[i].host, ==, host);
> >> >>  g_assert_cmpstr(tests[i].username, ==, username);
> >> >>  g_assert_cmpstr(tests[i].password, ==, password);
> >> >>  g_test_assert_expected_messages();
> >> >> +g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
> >> >>  g_clear_pointer(, g_free);
> >> >>  g_clear_pointer(, g_free);
> >> >>  g_clear_pointer(_port, g_free);
> >> >>  g_clear_pointer(, g_free);
> >> >>  g_clear_pointer(, g_free);
> >> >>  g_clear_pointer(, g_free);
> >> >> +g_clear_pointer(_path, g_free);
> >> >>  g_object_unref(s);
> >> >>  }
> >> >>
> >> >> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase
> >> >> *tests,
> >> >> const guint cases)
> >> >>   "host", tests[i].host,
> >> >>   "username", tests[i].username,
> >> >>   "password", tests[i].password,
> >> >> + "unix-path", tests[i].unix_path,
> >> >>NULL);
> >> >>  g_object_get(s, "uri", , NULL);
> >> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> >> uri);
> >> >>  g_clear_pointer(, g_free);
> >> >>  g_object_unref(s);
> >> >>  }
> >> >> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
> >> >>  test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >> >>  }
> >> >>
> >> >> +static void test_session_uri_unix_good(void)
> >> >> +{
> >> >> +const TestCase tests[] = {
> >> >> +{ .uri_input = "spice+unix:///tmp/foo.sock",
> >> >> +  .unix_path = "/tmp/foo.sock" },
> >> >> +/* perhaps not very clever, but this doesn't raise an
> >> >> error/warning
> >> >> */
> >> >> +{ .uri_input = "spice+unix://",
> >> >> +  .unix_path = "" },
> >> >> +/* unix uri don't support passing password or other kind of
> >> >> options
> >> >> */
> >> >> +{ .uri_input =
> >> >> "spice+unix:///tmp/foo.sock?password=frobnicate",
> >> >> +  .unix_path = "/tmp/foo.sock?password=frobnicate" },
> >> >> +};
> >> >> +
> >> >> +test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >> >> +}
> >> >> +
> >> >>  int main(int argc, char* argv[])
> >> >>  {
> >> >>  g_test_init(, , NULL);
> >> >> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
> >> >>  g_test_add_func("/session/bad-uri", test_session_uri_bad);
> >> >>  g_test_add_func("/session/good-ipv4-uri",
> >> >>  test_session_uri_ipv4_good);
> >> >>  g_test_add_func("/session/good-ipv6-uri",
> >> >>  test_session_uri_ipv6_good);
> >> >> +g_test_add_func("/session/good-unix", test_session_uri_unix_good);
> >> >>
> >> >>

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

2018-02-13 Thread Marc-Andre Lureau

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 spice+unix:// checks,
>> >> probably because they came about the same time.
>> >>
>> >> Signed-off-by: Marc-André Lureau 
>> >> ---
>> >>  tests/session.c | 28 +---
>> >>  1 file changed, 25 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/tests/session.c b/tests/session.c
>> >> index 7ed4a41..413d812 100644
>> >> --- a/tests/session.c
>> >> +++ b/tests/session.c
>> >> @@ -9,6 +9,7 @@ typedef struct {
>> >>  const gchar *uri_input;
>> >>  const gchar *uri_output;
>> >>  const gchar *message;
>> >> +const gchar *unix_path;
>> >>  } TestCase;
>> >>
>> >>  static void test_session_uri_bad(void)
>> >> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase
>> >> *tests,
>> >> const guint cases)
>> >>
>> >>  /* Set URI and check URI, port and tls_port */
>> >>  for (i = 0; i < cases; i++) {
>> >> -gchar *uri, *port, *tls_port, *host, *username, *password;
>> >> +gchar *uri, *port, *tls_port, *host, *username, *password,
>> >> *unix_path;
>> >>
>> >>  s = spice_session_new();
>> >>  if (tests[i].message != NULL)
>> >> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
>> >> *tests, const guint cases)
>> >>   "host", ,
>> >>   "username", ,
>> >>   "password", ,
>> >> + "unix-path", _path,
>> >>NULL);
>> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
>> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
>> >> uri);
>> >>  g_assert_cmpstr(tests[i].port, ==, port);
>> >>  g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
>> >>  g_assert_cmpstr(tests[i].host, ==, host);
>> >>  g_assert_cmpstr(tests[i].username, ==, username);
>> >>  g_assert_cmpstr(tests[i].password, ==, password);
>> >>  g_test_assert_expected_messages();
>> >> +g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
>> >>  g_clear_pointer(, g_free);
>> >>  g_clear_pointer(, g_free);
>> >>  g_clear_pointer(_port, g_free);
>> >>  g_clear_pointer(, g_free);
>> >>  g_clear_pointer(, g_free);
>> >>  g_clear_pointer(, g_free);
>> >> +g_clear_pointer(_path, g_free);
>> >>  g_object_unref(s);
>> >>  }
>> >>
>> >> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase
>> >> *tests,
>> >> const guint cases)
>> >>   "host", tests[i].host,
>> >>   "username", tests[i].username,
>> >>   "password", tests[i].password,
>> >> + "unix-path", tests[i].unix_path,
>> >>NULL);
>> >>  g_object_get(s, "uri", , NULL);
>> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
>> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
>> >> uri);
>> >>  g_clear_pointer(, g_free);
>> >>  g_object_unref(s);
>> >>  }
>> >> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
>> >>  test_session_uri_good(tests, G_N_ELEMENTS(tests));
>> >>  }
>> >>
>> >> +static void test_session_uri_unix_good(void)
>> >> +{
>> >> +const TestCase tests[] = {
>> >> +{ .uri_input = "spice+unix:///tmp/foo.sock",
>> >> +  .unix_path = "/tmp/foo.sock" },
>> >> +/* perhaps not very clever, but this doesn't raise an
>> >> error/warning
>> >> */
>> >> +{ .uri_input = "spice+unix://",
>> >> +  .unix_path = "" },
>> >> +/* unix uri don't support passing password or other kind of
>> >> options
>> >> */
>> >> +{ .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
>> >> +  .unix_path = "/tmp/foo.sock?password=frobnicate" },
>> >> +};
>> >> +
>> >> +test_session_uri_good(tests, G_N_ELEMENTS(tests));
>> >> +}
>> >> +
>> >>  int main(int argc, char* argv[])
>> >>  {
>> >>  g_test_init(, , NULL);
>> >> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
>> >>  g_test_add_func("/session/bad-uri", test_session_uri_bad);
>> >>  g_test_add_func("/session/good-ipv4-uri",
>> >>  test_session_uri_ipv4_good);
>> >>  g_test_add_func("/session/good-ipv6-uri",
>> >>  test_session_uri_ipv6_good);
>> >> +g_test_add_func("/session/good-unix", test_session_uri_unix_good);
>> >>
>> >>  return g_test_run();
>> >>  }
>> >
>> > Looks good (still to review better).
>> > Can we consider this patch separate from the rest of the series
>> > (that is merge even separately) ?
>>
>> Sure, it was just in the same area of code, and thus added

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

2018-02-13 Thread Christophe de Dinechin

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? And then 
change the renamed file? In many tools, that would make it easier to see what 
you changed and what you inherited.

The commit log does not explain what an AgentRunner is, why it’s a necessary 
abstraction, and what this is supposed to be helping with. As I see it:
- It does not cleanly encapsulate resources nor enforces / provides RAII for 
things like streamfd and the like.
- Because we throw hapazardly here and there and because of the previous 
comment, it actually makes the situation worse in case of error (at the very 
least, by making the flow of control harder to follow, but I suspect by ending 
in “terminate()” more often than not).

Also, a number of the changes could have their own individual patch set, which 
would make it easier to review and understand later.


> On 8 Feb 2018, at 17:20, Lukáš Hrázký  wrote:
> 
> Create an AgentRunner (TODO: needs a better name) class to encapsulate
> the streaming and communication with the server. The basic setup (cmd
> arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
> functions is moved to the AgentRunner class and modified as little as
> possible:
> - The cursor updating code is moved into a functor called CursorThread
> - Some initialization and cleanup is moved to AgentRunner's constructor
>  and destructor
> - Some error handling moved over to exceptions, mainly what was in
>  main() and do_capture()
> - A couple of variables gently renamed.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
> src/Makefile.am   |   2 +
> src/main.cpp  | 127 
> src/spice-streaming-agent.cpp | 455 +-
> src/spice-streaming-agent.hpp |  56 ++
> 4 files changed, 370 insertions(+), 270 deletions(-)
> create mode 100644 src/main.cpp
> create mode 100644 src/spice-streaming-agent.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8d5c5bd..3a6fee7 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
> 
> spice_streaming_agent_SOURCES = \
>   spice-streaming-agent.cpp \
> + spice-streaming-agent.hpp \
>   static-plugin.cpp \
>   static-plugin.hpp \
>   concrete-agent.cpp \
> @@ -56,4 +57,5 @@ spice_streaming_agent_SOURCES = \
>   mjpeg-fallback.cpp \
>   jpeg.cpp \
>   jpeg.hpp \
> + main.cpp \
>   $(NULL)
> diff --git a/src/main.cpp b/src/main.cpp
> new file mode 100644
> index 000..a309011
> --- /dev/null
> +++ b/src/main.cpp
> @@ -0,0 +1,127 @@
> +/* An implementation of a SPICE streaming agent
> + *
> + * \copyright
> + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include 

I’d write “config.h”. No reason to ever look config.h in system headers.

> +#include "spice-streaming-agent.hpp"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +using namespace std;
> +using namespace SpiceStreamingAgent;

Inherited? I thought we had decided not to use that.

> +
> +
> +static void usage(const char *progname)
> +{
> +printf("usage: %s \n", progname);
> +printf("options are:\n");
> +printf("\t-p portname  -- virtio-serial port to use\n");
> +printf("\t-i accept commands from stdin\n");
> +printf("\t-l file -- log frames to file\n");
> +printf("\t--log-binary -- log binary frames (following -l)\n");
> +printf("\t-d -- enable debug logs\n");
> +printf("\t-c variable=value -- change settings\n");
> +printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> +printf("\n");
> +printf("\t-h or --help -- print this help message\n");
> +
> +exit(1);
> +}

As you know, one of my patches splits that into separate sections. Might be 
worth doing in a subsequent patch.

> +
> +void handle_interrupt(int intr)
> +{
> +syslog(LOG_INFO, "Got signal %d, exiting", intr);
> +AgentRunner::quit = true;

nit: since it’s a state and not an action, I’d call it “quit_requested"

> +}
> +
> +void register_interrupts(void)
> +{
> +struct sigaction sa;
> +
> +memset(, 0, sizeof(sa));

Every time I see that, I tell Frediano to use C++-style init. Compilers are 
good at getting rid of memset calls, but still, it’s much shorter:

struct sigaction sa = { 0 };


> +sa.sa_handler = handle_interrupt;

or better yet, with that stuffed in the init

> +if ((sigaction(SIGINT, , NULL) != 0) &&
> +(sigaction(SIGTERM, , NULL) != 0)) {
> +syslog(LOG_WARNING, "failed to register signal handler %m");
> +}
> +}
> +
> +int main(int argc, char* argv[])
> +{
> +string stream_port = "/dev/virtio-ports/com.redhat.stream.0”;

1/

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

2018-02-13 Thread Frediano Ziglio

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 a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index 656bf56b..edd618e2 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -68,8 +69,12 @@ static int vmc_read(SPICE_GNUC_UNUSED 
SpiceCharDeviceInstance *sin,
 pos += ret;
 // kick off next message read
 // currently Qemu kicks the device so we need to do it manually
-// here
-spice_server_char_device_wakeup(_instance);
+// here. If not all data are read the device goes into blocking
+// state and we get the wake only when we read from the device
+// again
+if (pos >= *message_sizes_curr) {
+spice_server_char_device_wakeup(_instance);
+}
 return ret;
 }
 
@@ -177,11 +182,45 @@ static void test_stream_device(void)
 basic_event_loop_destroy();
 }
 
+// check if sending a partial message cause issues
+static void test_stream_device_unfinished(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// this long and not finished message should not cause an infinite loop
+p = add_stream_hdr(p, STREAM_TYPE_DATA, 10);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
 
 g_test_add_func("/server/stream-device", test_stream_device);
+g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
 
 return g_test_run();
 }
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index edd618e2..72b3f382 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -215,12 +215,48 @@ static void test_stream_device_unfinished(void)
 basic_event_loop_destroy();
 }
 
+// check if sending multiple messages cause stall
+static void test_stream_device_multiple(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// add some messages into device buffer
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
 
 g_test_add_func("/server/stream-device", test_stream_device);
 g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
+g_test_add_func("/server/stream-device-multiple", 
test_stream_device_multiple);
 
 return g_test_run();
 }
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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_server_char_device_wakeup is called
to write data which handles both read and write pending operations.
As we want to close the device but we can't do it inside the handler
operation schedule a timer that will close the guest device outside
this callback.
The proper solution would be to patch Qemu but making sure this
is not so easy so this workaround patch.
Code is marked with some comments to remember to remove this
hack in a safe future.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index b7553c67..869d8841 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -52,6 +52,7 @@ struct StreamDevice {
 bool opened;
 bool flow_stopped;
 StreamChannel *stream_channel;
+SpiceTimer *close_timer;
 };
 
 struct StreamDeviceClass {
@@ -63,6 +64,8 @@ static StreamDevice 
*stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
 
 G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
 
+static void char_device_set_state(RedCharDevice *char_dev, int state);
+
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
@@ -71,6 +74,16 @@ static StreamMsgHandler handle_msg_format, handle_msg_data;
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
 
+static void
+close_timer_func(void *opaque)
+{
+StreamDevice *dev = opaque;
+
+if (dev->opened && dev->has_error) {
+char_device_set_state(RED_CHAR_DEVICE(dev), 0);
+}
+}
+
 static bool
 stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 {
@@ -88,7 +101,17 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 while (sif->read(sin, buf, sizeof(buf)) > 0) {
 continue;
 }
-sif->state(sin, 0);
+
+// This code is a workaround for a Qemu bug, see patch
+// "stream-device: Workaround Qemu bug closing device"
+// as calling sif->state here can cause a crash schedule
+// a timer and do the call in it. Remove this code when
+// will be sure all Qemu versions has been patched.
+RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+if (!dev->close_timer) {
+dev->close_timer = reds_core_timer_add(reds, close_timer_func, 
dev);
+}
+reds_core_timer_start(reds, dev->close_timer, 0);
 return false;
 }
 
@@ -340,6 +363,9 @@ stream_device_dispose(GObject *object)
 {
 StreamDevice *dev = STREAM_DEVICE(object);
 
+RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+reds_core_timer_remove(reds, dev->close_timer);
+
 if (dev->stream_channel) {
 // close all current connections and drop the reference
 red_channel_destroy(RED_CHANNEL(dev->stream_channel));
@@ -376,7 +402,7 @@ reset_channels(StreamDevice *dev)
 }
 
 static void
-char_device_enable(RedCharDevice *char_dev)
+char_device_set_state(RedCharDevice *char_dev, int state)
 {
 SpiceCharDeviceInstance *sin = NULL;
 g_object_get(char_dev, "sin", , NULL);
@@ -384,7 +410,7 @@ char_device_enable(RedCharDevice *char_dev)
 
 SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
 if (sif->state) {
-sif->state(sin, 1);
+sif->state(sin, state);
 }
 }
 
@@ -413,7 +439,7 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
 // this could prevent a possible race condition where we open the
 // device from the guest and it take some time to enable causing
 // temporary writing issues.
-char_device_enable(char_dev);
+char_device_set_state(char_dev, 1);
 }
 
 static void
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 changes).

*** BLURB HERE ***

Frediano Ziglio (8):
  stream-device: Avoid device to get stuck if multiple messages are
batched
  test-stream-device: Better Qemu emulation for data reading
  test-stream-device: Test batched multiple messages
  stream-device: Implements properly device reset on open/close
  stream-device: Disable guest device on errors
  stream-device: Workaround Qemu bug closing device
  stream-device: Do not read past data message
  test-stream-device: Check we don't read past data message

 server/stream-device.c|  92 +++--
 server/tests/test-stream-device.c | 205 +-
 2 files changed, 263 insertions(+), 34 deletions(-)

-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 SpiceCharDeviceInterface::read
outside this callback (which is called by Qemu using
spice_server_char_device_wakeup).
On the test now we must test that we receive an error from the device.
This as previously we checked that last part of the data was not read
but now potentially all data are readed so we need another way to check
the device detected the error.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c|  15 +-
 server/tests/test-stream-device.c | 108 +-
 2 files changed, 85 insertions(+), 38 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index 3a7cb306..a5c39992 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -78,11 +78,22 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 int n;
 bool handled = false;
 
-if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
+sif = spice_char_device_get_interface(sin);
+
+// in order to get in sync every time we open the device we need
+// to discard data here. This as Qemu keeps a buffer of data which
+// is used only during spice_server_char_device_wakeup from Qemu
+if (G_UNLIKELY(dev->has_error)) {
+uint8_t buf[16 * 1024];
+while (sif->read(sin, buf, sizeof(buf)) > 0) {
+continue;
+}
 return false;
 }
 
-sif = spice_char_device_get_interface(sin);
+if (dev->flow_stopped || !dev->stream_channel) {
+return false;
+}
 
 /* read header */
 while (dev->hdr_pos < sizeof(dev->hdr)) {
diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index 72b3f382..a75fe83c 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -42,13 +43,20 @@ static unsigned pos;
 // then the size is reach we move on next one till exausted
 static unsigned message_sizes[16];
 static unsigned *message_sizes_end, *message_sizes_curr;
+static bool device_enabled = false;
+
+static unsigned vmc_write_pos;
+static uint8_t vmc_write_buf[2048];
 
 // handle writes to the device
 static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
  SPICE_GNUC_UNUSED const uint8_t *buf,
  int len)
 {
-// currently we don't test anything on write so reply we wrote it
+// just copy into the buffer
+unsigned copy = MIN(sizeof(vmc_write_buf) - vmc_write_pos, len);
+memcpy(vmc_write_buf+vmc_write_pos, buf, copy);
+vmc_write_pos += copy;
 return len;
 }
 
@@ -81,6 +89,7 @@ static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance 
*sin,
 static void vmc_state(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
   SPICE_GNUC_UNUSED int connected)
 {
+device_enabled = !!connected;
 }
 
 static SpiceCharDeviceInterface vmc_interface = {
@@ -126,57 +135,80 @@ static uint8_t *add_format(uint8_t *p, uint32_t w, 
uint32_t h, SpiceVideoCodecTy
 return p + sizeof(fmt);
 }
 
+// check we have an error message on the write buffer
+static void
+check_vmc_error_message(void)
+{
+StreamDevHeader hdr;
+
+g_assert(vmc_write_pos >= sizeof(hdr));
+
+memcpy(, vmc_write_buf, sizeof(hdr));
+g_assert_cmpint(hdr.protocol_version, ==, STREAM_DEVICE_PROTOCOL);
+g_assert_cmpint(GUINT16_FROM_LE(hdr.type), ==, STREAM_TYPE_NOTIFY_ERROR);
+g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos - 
sizeof(hdr));
+}
+
 static void test_stream_device(void)
 {
 uint8_t *p = message;
 SpiceCoreInterface *core = basic_event_loop_init();
 Test *test = test_new(core);
 
-message_sizes_curr = message_sizes;
-message_sizes_end = message_sizes;
+for (int test_num=0; test_num < 2; ++test_num) {
+pos = 0;
+vmc_write_pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
 
-// add some messages into device buffer
-// here we are testing the device is reading at least two
-// consecutive format messages
-// first message part has 2 extra bytes to check for header split
-p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
-*message_sizes_end = p - message + 2;
-++message_sizes_end;
+// add some messages into device buffer
+// here we are testing the device is reading at least two
+// consecutive format messages
+// first message part has 2 extra bytes to check for header split
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+*message_sizes_end = p - message + 2;
+

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

2018-02-13 Thread Frediano Ziglio

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 
b/server/tests/test-stream-device.c
index a75fe83c..db812d49 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -286,6 +286,47 @@ static void test_stream_device_multiple(void)
 basic_event_loop_destroy();
 }
 
+// check if data message consume even following message
+static void test_stream_device_format_after_data(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+vmc_write_pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// add some messages into device buffer
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_stream_hdr(p, STREAM_TYPE_DATA, 5);
+memcpy(p, "hello", 5);
+p += 5;
+p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+// we should have an error back
+check_vmc_error_message();
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
@@ -293,6 +334,7 @@ int main(int argc, char *argv[])
 g_test_add_func("/server/stream-device", test_stream_device);
 g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
 g_test_add_func("/server/stream-device-multiple", 
test_stream_device_multiple);
+g_test_add_func("/server/stream-device-format-after-data", 
test_stream_device_format_after_data);
 
 return g_test_run();
 }
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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, 1 insertion(+), 1 deletion(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index 869d8841..d125a877 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -257,7 +257,7 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance 
*sin)
 
 while (1) {
 uint8_t buf[16 * 1024];
-n = sif->read(sin, buf, sizeof(buf));
+n = sif->read(sin, buf, MIN(sizeof(buf), dev->hdr.size));
 if (n <= 0) {
 break;
 }
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 b/server/stream-device.c
index a5c39992..b7553c67 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -88,6 +88,7 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 while (sif->read(sin, buf, sizeof(buf)) > 0) {
 continue;
 }
+sif->state(sin, 0);
 return false;
 }
 
@@ -374,6 +375,19 @@ reset_channels(StreamDevice *dev)
 }
 }
 
+static void
+char_device_enable(RedCharDevice *char_dev)
+{
+SpiceCharDeviceInstance *sin = NULL;
+g_object_get(char_dev, "sin", , NULL);
+spice_assert(sin != NULL);
+
+SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+if (sif->state) {
+sif->state(sin, 1);
+}
+}
+
 static void
 stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
 {
@@ -394,6 +408,12 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
 dev->flow_stopped = false;
 red_char_device_reset(char_dev);
 reset_channels(dev);
+
+// enable the device again. We try to enable it even on close as
+// this could prevent a possible race condition where we open the
+// device from the guest and it take some time to enable causing
+// temporary writing issues.
+char_device_enable(char_dev);
 }
 
 static void
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 29 -
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index 4eaa959b..3a7cb306 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -71,16 +71,15 @@ static StreamMsgHandler handle_msg_format, handle_msg_data;
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
 
-static RedPipeItem *
-stream_device_read_msg_from_dev(RedCharDevice *self, SpiceCharDeviceInstance 
*sin)
+static bool
+stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 {
-StreamDevice *dev = STREAM_DEVICE(self);
 SpiceCharDeviceInterface *sif;
 int n;
 bool handled = false;
 
 if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
-return NULL;
+return false;
 }
 
 sif = spice_char_device_get_interface(sin);
@@ -89,7 +88,7 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
SpiceCharDeviceInstance *si
 while (dev->hdr_pos < sizeof(dev->hdr)) {
 n = sif->read(sin, (uint8_t *) >hdr + dev->hdr_pos, 
sizeof(dev->hdr) - dev->hdr_pos);
 if (n <= 0) {
-return NULL;
+return false;
 }
 dev->hdr_pos += n;
 if (dev->hdr_pos >= sizeof(dev->hdr)) {
@@ -123,6 +122,26 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
SpiceCharDeviceInstance *si
 dev->hdr_pos = 0;
 }
 
+if (handled || dev->has_error) {
+// Qemu put the device on blocking state if we don't read all data
+// so schedule another read.
+// We arrive here if we processed that entire message or we
+// got an error, try to read another message or discard the
+// wrong data
+return true;
+}
+
+return false;
+}
+
+static RedPipeItem *
+stream_device_read_msg_from_dev(RedCharDevice *self, SpiceCharDeviceInstance 
*sin)
+{
+StreamDevice *dev = STREAM_DEVICE(self);
+
+while (stream_device_partial_read(dev, sin)) {
+continue;
+}
 return NULL;
 }
 
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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 errors)
> > - exceptions for errors, makes testing and potential future changes to
> >   error handling easier.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  src/mjpeg-fallback.cpp | 41 -
> >  1 file changed, 20 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 74682f3..1b51ee0 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -159,29 +159,24 @@ unsigned MjpegPlugin::Rank()
> >  
> >  void MjpegPlugin::ParseOptions(const ConfigureOption *options)
> >  {
> > -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> > -
> >  for (; options->name; ++options) {
> > -const char *name = options->name;
> > -const char *value = options->value;
> > -
> > -if (strcmp(name, "framerate") == 0) {
> > -int val = atoi(value);
> > -if (val > 0) {
> > -settings.fps = val;
> > +const std::string name = options->name;
> > +const std::string value = options->value;
> > +
> > +if (name == "framerate") {
> > +try {
> > +settings.fps = stoi(value);
> > +} catch (const std::exception ) {
> 
> would be better to catch just std::logic_error instead?
> (same below)

Wow, the exceptions thrown from the function really inherit from
logic_error... This seems totally wrong, because logic_error should
represent errors in the logic of the program, but here the outcome
depends on 'user input'.

If it weren't for this, I'd be inclined to catch the common base class
of the exception, but I feel really reluctant to catch logic_error in
this way...

In any case, the function should not be throwing other exceptions, so
this should be more of a formality.

> > +throw std::runtime_error("Invalid value '" + value + "' for
> > option 'framerate'.");
> >  }
> > -else {
> > -arg_error("wrong framerate arg %s\n", value);
> > -}
> > -}
> > -if (strcmp(name, "mjpeg.quality") == 0) {
> > -int val = atoi(value);
> > -if (val > 0) {
> > -settings.quality = val;
> > -}
> > -else {
> > -arg_error("wrong mjpeg.quality arg %s\n", value);
> > +} else if (name == "mjpeg.quality") {
> > +try {
> > +settings.quality = stoi(value);
> > +} catch (const std::exception ) {
> > +throw std::runtime_error("Invalid value '" + value + "' for
> > option 'mjpeg.quality'.");
> >  }
> > +} else {
> > +throw std::runtime_error("Invalid option '" + name + "'.");
> >  }
> >  }
> >  }
> > @@ -198,7 +193,11 @@ mjpeg_plugin_init(Agent* agent)
> >  
> >  std::unique_ptr plugin(new MjpegPlugin());
> >  
> > -plugin->ParseOptions(agent->Options());
> > +try {
> > +plugin->ParseOptions(agent->Options());
> > +} catch (const std::exception ) {
> > +syslog(LOG_ERR, "Error parsing plugin option: %s\n", e.what());
> 
> I think in this case "options" is better.
> Isn't std::exception catching too much (not much strong about it)?

The only other exception I can see the method throwing would be
std::bad_alloc (from the std::string constructors). Since we are lazy
to define our own exception classes and are using runtime_errors, I'd
keep it this way. If we wanted to differentiate, we'd have to go to the
"define your exceptions" land :) I don't think it matters much here?

The way I'm looking at it is: "Do we want to fail harder than an error
message if we get some more serious exceptions than a parsing error?"
It probably doesn't matter.

Lukas

> > +}
> >  
> >  agent->Register(*plugin.release());
> >  
> 
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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
>   error handling easier.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
>  src/mjpeg-fallback.cpp | 41 -
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 74682f3..1b51ee0 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -159,29 +159,24 @@ unsigned MjpegPlugin::Rank()
>  
>  void MjpegPlugin::ParseOptions(const ConfigureOption *options)
>  {
> -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> -
>  for (; options->name; ++options) {
> -const char *name = options->name;
> -const char *value = options->value;
> -
> -if (strcmp(name, "framerate") == 0) {
> -int val = atoi(value);
> -if (val > 0) {
> -settings.fps = val;
> +const std::string name = options->name;
> +const std::string value = options->value;
> +
> +if (name == "framerate") {
> +try {
> +settings.fps = stoi(value);
> +} catch (const std::exception ) {

would be better to catch just std::logic_error instead?
(same below)

> +throw std::runtime_error("Invalid value '" + value + "' for
> option 'framerate'.");
>  }
> -else {
> -arg_error("wrong framerate arg %s\n", value);
> -}
> -}
> -if (strcmp(name, "mjpeg.quality") == 0) {
> -int val = atoi(value);
> -if (val > 0) {
> -settings.quality = val;
> -}
> -else {
> -arg_error("wrong mjpeg.quality arg %s\n", value);
> +} else if (name == "mjpeg.quality") {
> +try {
> +settings.quality = stoi(value);
> +} catch (const std::exception ) {
> +throw std::runtime_error("Invalid value '" + value + "' for
> option 'mjpeg.quality'.");
>  }
> +} else {
> +throw std::runtime_error("Invalid option '" + name + "'.");
>  }
>  }
>  }
> @@ -198,7 +193,11 @@ mjpeg_plugin_init(Agent* agent)
>  
>  std::unique_ptr plugin(new MjpegPlugin());
>  
> -plugin->ParseOptions(agent->Options());
> +try {
> +plugin->ParseOptions(agent->Options());
> +} catch (const std::exception ) {
> +syslog(LOG_ERR, "Error parsing plugin option: %s\n", e.what());

I think in this case "options" is better.
Isn't std::exception catching too much (not much strong about it)?

> +}
>  
>  agent->Register(*plugin.release());
>  

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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 a/src/unittests/.gitignore b/src/unittests/.gitignore
> > index 36548a1..e871e91 100644
> > --- a/src/unittests/.gitignore
> > +++ b/src/unittests/.gitignore
> > @@ -1 +1,4 @@
> > -/test-hexdump
> > +hexdump.sh.log
> > +hexdump.sh.trs
> > +test-hexdump
> > +test-suite.log
> 
> Would not be better, if we decide to name all tests test-, to
> exclude test-*.log and test-*.trs?

Sounds good.

> The leading / is to avoid to catch files in subdirectories, in
> this case I don't think we are going to exclude more files than
> expected ones but doesn't hurt to reduce the scope.

I see. Will add it again.

I just realized this is outdated by your renaming patch. I'll update
it.

Lukas
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

> 
> 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
> +++ b/src/unittests/.gitignore
> @@ -1 +1,4 @@
> -/test-hexdump
> +hexdump.sh.log
> +hexdump.sh.trs
> +test-hexdump
> +test-suite.log

Would not be better, if we decide to name all tests test-, to
exclude test-*.log and test-*.trs?
The leading / is to avoid to catch files in subdirectories, in
this case I don't think we are going to exclude more files than
expected ones but doesn't hurt to reduce the scope.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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|  1 +
 src/unittests/.gitignore  |  3 ++
 src/unittests/Makefile.am | 14 +
 src/unittests/test-mjpeg-fallback.cpp | 58 +++
 6 files changed, 84 insertions(+)
 create mode 100644 src/unittests/test-mjpeg-fallback.cpp

diff --git a/configure.ac b/configure.ac
index 8795dae..5aab662 100644
--- a/configure.ac
+++ b/configure.ac
@@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
 AC_MSG_ERROR([C99 compiler is required.])
 fi
 AC_PROG_CXX
+AC_LANG(C++)
 AX_CXX_COMPILE_STDCXX_11
 AC_PROG_INSTALL
 AC_CANONICAL_HOST
@@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
 AC_MSG_ERROR([libjpeg not found]))
 AC_SUBST(JPEG_LIBS)
 
+AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch 
dependency header (catch/catch.hpp)])])
+
 dnl ===
 dnl check compiler flags
 
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 1b51ee0..ae1777d 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -181,6 +181,11 @@ void MjpegPlugin::ParseOptions(const ConfigureOption 
*options)
 }
 }
 
+MjpegSettings MjpegPlugin::Options() const
+{
+return settings;
+}
+
 SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
 return SPICE_VIDEO_CODEC_TYPE_MJPEG;
 }
diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
index 04fa2eb..8496763 100644
--- a/src/mjpeg-fallback.hpp
+++ b/src/mjpeg-fallback.hpp
@@ -25,6 +25,7 @@ public:
 FrameCapture *CreateCapture() override;
 unsigned Rank() override;
 void ParseOptions(const ConfigureOption *options);
+MjpegSettings Options() const;  // TODO unify on Settings vs Options
 SpiceVideoCodecType VideoCodecType() const;
 private:
 MjpegSettings settings = { 10, 80 };
diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
index e871e91..027ab83 100644
--- a/src/unittests/.gitignore
+++ b/src/unittests/.gitignore
@@ -1,4 +1,7 @@
 hexdump.sh.log
 hexdump.sh.trs
 test-hexdump
+test-mjpeg-fallback
+test-mjpeg-fallback.log
+test-mjpeg-fallback.trs
 test-suite.log
diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
index ef6c253..acadeb1 100644
--- a/src/unittests/Makefile.am
+++ b/src/unittests/Makefile.am
@@ -2,6 +2,7 @@ NULL =
 
 AM_CPPFLAGS = \
-DRH_TOP_SRCDIR=\"$(abs_top_srcdir)\" \
+   -I$(top_srcdir)/include \
-I$(top_srcdir)/src \
-I$(top_srcdir)/src/unittests \
$(SPICE_PROTOCOL_CFLAGS) \
@@ -14,10 +15,12 @@ AM_CFLAGS = \
 
 check_PROGRAMS = \
test-hexdump \
+   test-mjpeg-fallback \
$(NULL)
 
 TESTS = \
hexdump.sh \
+   test-mjpeg-fallback \
$(NULL)
 
 noinst_PROGRAMS = \
@@ -32,6 +35,17 @@ test_hexdump_LDADD = \
../libstreaming-utils.a \
$(NULL)
 
+test_mjpeg_fallback_SOURCES = \
+   test-mjpeg-fallback.cpp \
+   ../mjpeg-fallback.cpp \
+   ../jpeg.cpp \
+   $(NULL)
+
+test_mjpeg_fallback_LDADD = \
+   $(X11_LIBS) \
+   $(JPEG_LIBS) \
+   $(NULL)
+
 EXTRA_DIST = \
hexdump.sh \
hexdump1.in \
diff --git a/src/unittests/test-mjpeg-fallback.cpp 
b/src/unittests/test-mjpeg-fallback.cpp
new file mode 100644
index 000..cb2cb68
--- /dev/null
+++ b/src/unittests/test-mjpeg-fallback.cpp
@@ -0,0 +1,58 @@
+#define CATCH_CONFIG_MAIN
+#include "catch/catch.hpp"
+
+#include "mjpeg-fallback.hpp"
+
+namespace ssa = SpiceStreamingAgent;
+
+
+SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
+GIVEN("A new mjpeg plugin") {
+ssa::MjpegPlugin plugin;
+
+WHEN("passing correct options") {
+std::vector options = {
+{"framerate", "20"},
+{"mjpeg.quality", "90"},
+{NULL, NULL}
+};
+
+plugin.ParseOptions(options.data());
+ssa::MjpegSettings new_options = plugin.Options();
+
+THEN("the options are set in the plugin") {
+CHECK(new_options.fps == 20);
+CHECK(new_options.quality == 90);
+}
+}
+
+WHEN("passing an unknown option") {
+std::vector options = {
+{"wakaka", "10"},
+{NULL, NULL}
+};
+
+THEN("ParseOptions throws an exception") {
+REQUIRE_THROWS_WITH(
+plugin.ParseOptions(options.data()),
+"Invalid option 'wakaka'."
+);
+}
+}
+
+WHEN("passing an invalid option value") {
+std::vector options = {
+{"framerate", "40"},
+

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

2018-02-13 Thread Lukáš Hrázký

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
+++ b/src/unittests/.gitignore
@@ -1 +1,4 @@
-/test-hexdump
+hexdump.sh.log
+hexdump.sh.trs
+test-hexdump
+test-suite.log
-- 
2.16.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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ý 
---
 src/mjpeg-fallback.cpp | 41 -
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 74682f3..1b51ee0 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -159,29 +159,24 @@ unsigned MjpegPlugin::Rank()
 
 void MjpegPlugin::ParseOptions(const ConfigureOption *options)
 {
-#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
-
 for (; options->name; ++options) {
-const char *name = options->name;
-const char *value = options->value;
-
-if (strcmp(name, "framerate") == 0) {
-int val = atoi(value);
-if (val > 0) {
-settings.fps = val;
+const std::string name = options->name;
+const std::string value = options->value;
+
+if (name == "framerate") {
+try {
+settings.fps = stoi(value);
+} catch (const std::exception ) {
+throw std::runtime_error("Invalid value '" + value + "' for 
option 'framerate'.");
 }
-else {
-arg_error("wrong framerate arg %s\n", value);
-}
-}
-if (strcmp(name, "mjpeg.quality") == 0) {
-int val = atoi(value);
-if (val > 0) {
-settings.quality = val;
-}
-else {
-arg_error("wrong mjpeg.quality arg %s\n", value);
+} else if (name == "mjpeg.quality") {
+try {
+settings.quality = stoi(value);
+} catch (const std::exception ) {
+throw std::runtime_error("Invalid value '" + value + "' for 
option 'mjpeg.quality'.");
 }
+} else {
+throw std::runtime_error("Invalid option '" + name + "'.");
 }
 }
 }
@@ -198,7 +193,11 @@ mjpeg_plugin_init(Agent* agent)
 
 std::unique_ptr plugin(new MjpegPlugin());
 
-plugin->ParseOptions(agent->Options());
+try {
+plugin->ParseOptions(agent->Options());
+} catch (const std::exception ) {
+syslog(LOG_ERR, "Error parsing plugin option: %s\n", e.what());
+}
 
 agent->Register(*plugin.release());
 
-- 
2.16.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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 proceed here.

Changes since v1:
- squash the two test patches and keep the BDD version
- update for the explicit namespaces
- use syslog instead of std::cerr for logging the error message
- add temporary files to .gitignore

Lukáš Hrázký (3):
  mjpeg-fallback: a more C++ way of handling options
  src/unitests: add temporary files to .gitignore
  mjpeg-fallback: unittest for the options parsing

 configure.ac  |  3 ++
 src/mjpeg-fallback.cpp| 46 ++-
 src/mjpeg-fallback.hpp|  1 +
 src/unittests/.gitignore  |  8 -
 src/unittests/Makefile.am | 14 +
 src/unittests/test-mjpeg-fallback.cpp | 58 +++
 6 files changed, 108 insertions(+), 22 deletions(-)
 create mode 100644 src/unittests/test-mjpeg-fallback.cpp

-- 
2.16.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -23,6 +23,7 @@
 
 #include "char-device.h"
 #include "stream-channel.h"
+#include "cursor-channel.h"
 #include "reds.h"
 
 #define TYPE_STREAM_DEVICE stream_device_get_type()
@@ -45,13 +46,16 @@ struct StreamDevice {
 union {
 StreamMsgFormat format;
 StreamMsgCapabilities capabilities;
+StreamMsgCursorSet cursor_set;
 uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
-} msg;
+} *msg;
 uint32_t msg_pos;
+uint32_t msg_len;
 bool has_error;
 bool opened;
 bool flow_stopped;
 StreamChannel *stream_channel;
+CursorChannel *cursor_channel;
 };
 
 struct StreamDeviceClass {
@@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device, 
RED_TYPE_CHAR_DEVICE)
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
-static StreamMsgHandler handle_msg_format, handle_msg_data;
+static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_set;
 
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
@@ -121,6 +125,9 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 case STREAM_TYPE_DATA:
 handled = handle_msg_data(dev, sin);
 break;
+case STREAM_TYPE_CURSOR_SET:
+handled = handle_msg_cursor_set(dev, sin);
+break;
 case STREAM_TYPE_CAPABILITIES:
 /* FIXME */
 default:
@@ -132,6 +139,14 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
  * the next message */
 if (handled) {
 dev->hdr_pos = 0;
+
+// Reallocate message buffer to the minimum.
+// Currently the only message that requires resizing is the cursor 
shape,
+// which is not expected to be sent so often.
+if (dev->msg_len > sizeof(*dev->msg)) {
+dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
+dev->msg_len = sizeof(*dev->msg);
+}
 }
 
 if (handled || dev->has_error) {
@@ -204,7 +219,7 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
 }
 
-int n = sif->read(sin, dev->msg.buf + dev->msg_pos, 
sizeof(StreamMsgFormat) - dev->msg_pos);
+int n = sif->read(sin, dev->msg->buf + dev->msg_pos, 
sizeof(StreamMsgFormat) - dev->msg_pos);
 if (n < 0) {
 return handle_msg_invalid(dev, sin, NULL);
 }
@@ -215,9 +230,9 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return false;
 }
 
-dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width);
-dev->msg.format.height = GUINT32_FROM_LE(dev->msg.format.height);
-stream_channel_change_format(dev->stream_channel, >msg.format);
+dev->msg->format.width = GUINT32_FROM_LE(dev->msg->format.width);
+dev->msg->format.height = GUINT32_FROM_LE(dev->msg->format.height);
+stream_channel_change_format(dev->stream_channel, >msg->format);
 return true;
 }
 
@@ -248,6 +263,114 @@ handle_msg_data(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return dev->hdr.size == 0;
 }
 
+/*
+ * Returns number of bits required for a pixel of a given cursor type.
+ *
+ * Take into account mask bits.
+ * Returns 0 on error.
+ */
+static unsigned int
+get_cursor_type_bits(unsigned int cursor_type)
+{
+switch (cursor_type) {
+case SPICE_CURSOR_TYPE_ALPHA:
+// RGBA
+return 32;
+case SPICE_CURSOR_TYPE_COLOR24:
+// RGB + bitmask
+return 24 + 1;
+case SPICE_CURSOR_TYPE_COLOR32:
+// RGBx + bitmask
+return 32 + 1;
+default:
+return 0;
+}
+}
+
+static RedCursorCmd *
+stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg, size_t 
msg_size)
+{
+RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
+cmd->type = QXL_CURSOR_SET;
+cmd->u.set.position.x = 0; // TODO
+cmd->u.set.position.y = 0; // TODO
+cmd->u.set.visible = 1; // TODO
+SpiceCursor *cursor = >u.set.shape;
+cursor->header.unique = 0;
+cursor->header.type = msg->type;
+cursor->header.width = GUINT16_FROM_LE(msg->width);
+cursor->header.height = GUINT16_FROM_LE(msg->height);
+cursor->header.hot_spot_x = GUINT16_FROM_LE(msg->hot_spot_x);
+cursor->header.hot_spot_y = GUINT16_FROM_LE(msg->hot_spot_y);
+
+/* Limit cursor size to prevent DoS */
+if (cursor->header.width > STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
+cursor->header.height > STREAM_MSG_CURSOR_SET_MAX_HEIGHT) {
+g_free(cmd);
+

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

2018-02-13 Thread Frediano Ziglio

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
+++ b/server/stream-device.c
@@ -47,6 +47,7 @@ struct StreamDevice {
 StreamMsgFormat format;
 StreamMsgCapabilities capabilities;
 StreamMsgCursorSet cursor_set;
+StreamMsgCursorMove cursor_move;
 uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
 } *msg;
 uint32_t msg_pos;
@@ -70,7 +71,8 @@ G_DEFINE_TYPE(StreamDevice, stream_device, 
RED_TYPE_CHAR_DEVICE)
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
-static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_set;
+static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_set,
+handle_msg_cursor_move;
 
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
@@ -128,6 +130,13 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 case STREAM_TYPE_CURSOR_SET:
 handled = handle_msg_cursor_set(dev, sin);
 break;
+case STREAM_TYPE_CURSOR_MOVE:
+if (dev->hdr.size != sizeof(StreamMsgCursorMove)) {
+handled = handle_msg_invalid(dev, sin, "Wrong size for 
StreamMsgCursorMove");
+} else {
+handled = handle_msg_cursor_move(dev, sin);
+}
+break;
 case STREAM_TYPE_CAPABILITIES:
 /* FIXME */
 default:
@@ -371,6 +380,33 @@ handle_msg_cursor_set(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return true;
 }
 
+static bool
+handle_msg_cursor_move(StreamDevice *dev, SpiceCharDeviceInstance *sin)
+{
+SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size - 
dev->msg_pos);
+if (n <= 0) {
+return false;
+}
+dev->msg_pos += n;
+if (dev->msg_pos != dev->hdr.size) {
+return false;
+}
+
+StreamMsgCursorMove *move = >msg->cursor_move;
+move->x = GINT32_FROM_LE(move->x);
+move->y = GINT32_FROM_LE(move->y);
+
+RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
+cmd->type = QXL_CURSOR_MOVE;
+cmd->u.position.x = move->x;
+cmd->u.position.y = move->y;
+
+cursor_channel_process_cmd(dev->cursor_channel, cmd);
+
+return true;
+}
+
 static void
 stream_device_send_msg_to_client(RedCharDevice *self, RedPipeItem *msg, 
RedClient *client)
 {
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 deletions(-)

-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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 
> ---
>  src/unittests/.gitignore  |  2 +-
>  src/unittests/Makefile.am | 12 ++--
>  src/unittests/{test-hexdump.c => hexdump.c}   |  0
>  src/unittests/{hexdump.sh => test-hexdump.sh} |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
>  rename src/unittests/{test-hexdump.c => hexdump.c} (100%)
>  rename src/unittests/{hexdump.sh => test-hexdump.sh} (91%)
> 
> Changes since v1:
> - rename test-hexdump to test-hexdump.sh;
> - added again wrongly removed target.
> 
> diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> index 36548a1..bb238d7 100644
> --- a/src/unittests/.gitignore
> +++ b/src/unittests/.gitignore
> @@ -1 +1 @@
> -/test-hexdump
> +/hexdump
> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> index ef6c253..a70a4b4 100644
> --- a/src/unittests/Makefile.am
> +++ b/src/unittests/Makefile.am
> @@ -13,27 +13,27 @@ AM_CFLAGS = \
>   $(NULL)
>  
>  check_PROGRAMS = \
> - test-hexdump \
> + hexdump \
>   $(NULL)
>  
>  TESTS = \
> - hexdump.sh \
> + test-hexdump.sh \
>   $(NULL)
>  
>  noinst_PROGRAMS = \
>   $(check_PROGRAMS) \
>   $(NULL)
>  
> -test_hexdump_SOURCES = \
> - test-hexdump.c \
> +hexdump_SOURCES = \
> + hexdump.c \
>   $(NULL)
>  
> -test_hexdump_LDADD = \
> +hexdump_LDADD = \
>   ../libstreaming-utils.a \
>   $(NULL)
>  
>  EXTRA_DIST = \
> - hexdump.sh \
> + test-hexdump.sh \
>   hexdump1.in \
>   hexdump1.out \
>   hexdump2.in \
> diff --git a/src/unittests/test-hexdump.c b/src/unittests/hexdump.c
> similarity index 100%
> rename from src/unittests/test-hexdump.c
> rename to src/unittests/hexdump.c
> diff --git a/src/unittests/hexdump.sh b/src/unittests/test-hexdump.sh
> similarity index 91%
> rename from src/unittests/hexdump.sh
> rename to src/unittests/test-hexdump.sh
> index 691b264..69fc7aa 100755
> --- a/src/unittests/hexdump.sh
> +++ b/src/unittests/test-hexdump.sh
> @@ -12,7 +12,7 @@ for f in "$SRCDIR"/hexdump*.in; do
>  reference=`echo $f | sed 's,\.in,.out,'`
>  out=`basename $reference`.test
>  rm -f $out
> -./test-hexdump $out < $f
> +./hexdump $out < $f
>  cmp $out $reference
>  rm -f $out
>  done

Acked-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 +-
 src/unittests/Makefile.am | 12 ++--
 src/unittests/{test-hexdump.c => hexdump.c}   |  0
 src/unittests/{hexdump.sh => test-hexdump.sh} |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
 rename src/unittests/{test-hexdump.c => hexdump.c} (100%)
 rename src/unittests/{hexdump.sh => test-hexdump.sh} (91%)

Changes since v1:
- rename test-hexdump to test-hexdump.sh;
- added again wrongly removed target.

diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
index 36548a1..bb238d7 100644
--- a/src/unittests/.gitignore
+++ b/src/unittests/.gitignore
@@ -1 +1 @@
-/test-hexdump
+/hexdump
diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
index ef6c253..a70a4b4 100644
--- a/src/unittests/Makefile.am
+++ b/src/unittests/Makefile.am
@@ -13,27 +13,27 @@ AM_CFLAGS = \
$(NULL)
 
 check_PROGRAMS = \
-   test-hexdump \
+   hexdump \
$(NULL)
 
 TESTS = \
-   hexdump.sh \
+   test-hexdump.sh \
$(NULL)
 
 noinst_PROGRAMS = \
$(check_PROGRAMS) \
$(NULL)
 
-test_hexdump_SOURCES = \
-   test-hexdump.c \
+hexdump_SOURCES = \
+   hexdump.c \
$(NULL)
 
-test_hexdump_LDADD = \
+hexdump_LDADD = \
../libstreaming-utils.a \
$(NULL)
 
 EXTRA_DIST = \
-   hexdump.sh \
+   test-hexdump.sh \
hexdump1.in \
hexdump1.out \
hexdump2.in \
diff --git a/src/unittests/test-hexdump.c b/src/unittests/hexdump.c
similarity index 100%
rename from src/unittests/test-hexdump.c
rename to src/unittests/hexdump.c
diff --git a/src/unittests/hexdump.sh b/src/unittests/test-hexdump.sh
similarity index 91%
rename from src/unittests/hexdump.sh
rename to src/unittests/test-hexdump.sh
index 691b264..69fc7aa 100755
--- a/src/unittests/hexdump.sh
+++ b/src/unittests/test-hexdump.sh
@@ -12,7 +12,7 @@ for f in "$SRCDIR"/hexdump*.in; do
 reference=`echo $f | sed 's,\.in,.out,'`
 out=`basename $reference`.test
 rm -f $out
-./test-hexdump $out < $f
+./hexdump $out < $f
 cmp $out $reference
 rm -f $out
 done
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Christophe de Dinechin



> 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
>>> strong like Linux, maybe an initial mistake).
>>> By default CRITICAL is fatal (it calls abort) so ERROR is too.
>> 
>> I’ve not tested with the agent, but I know that for spicy, I regularly get
>> “CRITICAL” about assertions without an abort:
>> 
>> (spicy:53020): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT
>> (object)' failed
>> (spicy:53020): GLib-GObject-CRITICAL **: g_object_unref: assertion
>> 'G_IS_OBJECT (object)' failed
>> 
>> If the desired behavior is an abort, we are not configuring it properly.
>> 
>> 
>> Christophe
>> 
> 
> But this is a CRITICAL in spicy, not a ERROR in the server.
> By default g_critical is not fatal, spice_critical yes.
> Both g_error and spice_error are fatal.

My point was that an assertion failure was not aborting.

Do you feel that it’s logical and consistent? For me, it’s remarkably confusing 
;-)


Christophe

> 
> Frediano
> 
>>> 
> I agree, asserts should not be used as needed runtime checks.
 
 I’ll add that to the next writeup.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Lukáš Hrázký

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 
> ---
>  src/unittests/.gitignore|  2 +-
>  src/unittests/Makefile.am   | 16 ++--
>  src/unittests/{test-hexdump.c => hexdump.c} |  0
>  src/unittests/{hexdump.sh => test-hexdump}  |  2 +-
>  4 files changed, 8 insertions(+), 12 deletions(-)
>  rename src/unittests/{test-hexdump.c => hexdump.c} (100%)
>  rename src/unittests/{hexdump.sh => test-hexdump} (91%)

I would prefer the name to be test-hexdump.sh if it's a shell script.

> diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
> index 36548a1..bb238d7 100644
> --- a/src/unittests/.gitignore
> +++ b/src/unittests/.gitignore
> @@ -1 +1 @@
> -/test-hexdump
> +/hexdump
> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
> index ef6c253..1dbdb11 100644
> --- a/src/unittests/Makefile.am
> +++ b/src/unittests/Makefile.am
> @@ -13,27 +13,23 @@ AM_CFLAGS = \
>   $(NULL)
>  
>  check_PROGRAMS = \
> - test-hexdump \
> + hexdump \
>   $(NULL)
>  
>  TESTS = \
> - hexdump.sh \
> - $(NULL)
> -
> -noinst_PROGRAMS = \
> - $(check_PROGRAMS) \

I take it noinst_PROGRAMS is not needed here, if you removed it? You
didn't mention it in the commit message. It's a minor thing, just
making sure this was intentional.

> + test-hexdump \
>   $(NULL)
>  
> -test_hexdump_SOURCES = \
> - test-hexdump.c \
> +hexdump_SOURCES = \
> + hexdump.c \
>   $(NULL)
>  
> -test_hexdump_LDADD = \
> +hexdump_LDADD = \
>   ../libstreaming-utils.a \
>   $(NULL)
>  
>  EXTRA_DIST = \
> - hexdump.sh \
> + test-hexdump \
>   hexdump1.in \
>   hexdump1.out \
>   hexdump2.in \
> diff --git a/src/unittests/test-hexdump.c b/src/unittests/hexdump.c
> similarity index 100%
> rename from src/unittests/test-hexdump.c
> rename to src/unittests/hexdump.c
> diff --git a/src/unittests/hexdump.sh b/src/unittests/test-hexdump
> similarity index 91%
> rename from src/unittests/hexdump.sh
> rename to src/unittests/test-hexdump
> index 691b264..69fc7aa 100755
> --- a/src/unittests/hexdump.sh
> +++ b/src/unittests/test-hexdump
> @@ -12,7 +12,7 @@ for f in "$SRCDIR"/hexdump*.in; do
>  reference=`echo $f | sed 's,\.in,.out,'`
>  out=`basename $reference`.test
>  rm -f $out
> -./test-hexdump $out < $f
> +./hexdump $out < $f
>  cmp $out $reference
>  rm -f $out
>  done
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Christophe de Dinechin



> 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 abort) so ERROR is too.

I’ve not tested with the agent, but I know that for spicy, I regularly get 
“CRITICAL” about assertions without an abort:

(spicy:53020): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT 
(object)' failed
(spicy:53020): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT 
(object)' failed

If the desired behavior is an abort, we are not configuring it properly.


Christophe

> 
>>> I agree, asserts should not be used as needed runtime checks.
>> 
>> I’ll add that to the next writeup.
>> 
> 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

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 +-
 src/unittests/Makefile.am   | 16 ++--
 src/unittests/{test-hexdump.c => hexdump.c} |  0
 src/unittests/{hexdump.sh => test-hexdump}  |  2 +-
 4 files changed, 8 insertions(+), 12 deletions(-)
 rename src/unittests/{test-hexdump.c => hexdump.c} (100%)
 rename src/unittests/{hexdump.sh => test-hexdump} (91%)

diff --git a/src/unittests/.gitignore b/src/unittests/.gitignore
index 36548a1..bb238d7 100644
--- a/src/unittests/.gitignore
+++ b/src/unittests/.gitignore
@@ -1 +1 @@
-/test-hexdump
+/hexdump
diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
index ef6c253..1dbdb11 100644
--- a/src/unittests/Makefile.am
+++ b/src/unittests/Makefile.am
@@ -13,27 +13,23 @@ AM_CFLAGS = \
$(NULL)
 
 check_PROGRAMS = \
-   test-hexdump \
+   hexdump \
$(NULL)
 
 TESTS = \
-   hexdump.sh \
-   $(NULL)
-
-noinst_PROGRAMS = \
-   $(check_PROGRAMS) \
+   test-hexdump \
$(NULL)
 
-test_hexdump_SOURCES = \
-   test-hexdump.c \
+hexdump_SOURCES = \
+   hexdump.c \
$(NULL)
 
-test_hexdump_LDADD = \
+hexdump_LDADD = \
../libstreaming-utils.a \
$(NULL)
 
 EXTRA_DIST = \
-   hexdump.sh \
+   test-hexdump \
hexdump1.in \
hexdump1.out \
hexdump2.in \
diff --git a/src/unittests/test-hexdump.c b/src/unittests/hexdump.c
similarity index 100%
rename from src/unittests/test-hexdump.c
rename to src/unittests/hexdump.c
diff --git a/src/unittests/hexdump.sh b/src/unittests/test-hexdump
similarity index 91%
rename from src/unittests/hexdump.sh
rename to src/unittests/test-hexdump
index 691b264..69fc7aa 100755
--- a/src/unittests/hexdump.sh
+++ b/src/unittests/test-hexdump
@@ -12,7 +12,7 @@ for f in "$SRCDIR"/hexdump*.in; do
 reference=`echo $f | sed 's,\.in,.out,'`
 out=`basename $reference`.test
 rm -f $out
-./test-hexdump $out < $f
+./hexdump $out < $f
 cmp $out $reference
 rm -f $out
 done
-- 
2.14.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-13 Thread Frediano Ziglio

> 
> 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.
> >>
> >> Signed-off-by: Marc-André Lureau 
> >> ---
> >>  tests/session.c | 28 +---
> >>  1 file changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/session.c b/tests/session.c
> >> index 7ed4a41..413d812 100644
> >> --- a/tests/session.c
> >> +++ b/tests/session.c
> >> @@ -9,6 +9,7 @@ typedef struct {
> >>  const gchar *uri_input;
> >>  const gchar *uri_output;
> >>  const gchar *message;
> >> +const gchar *unix_path;
> >>  } TestCase;
> >>
> >>  static void test_session_uri_bad(void)
> >> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase
> >> *tests,
> >> const guint cases)
> >>
> >>  /* Set URI and check URI, port and tls_port */
> >>  for (i = 0; i < cases; i++) {
> >> -gchar *uri, *port, *tls_port, *host, *username, *password;
> >> +gchar *uri, *port, *tls_port, *host, *username, *password,
> >> *unix_path;
> >>
> >>  s = spice_session_new();
> >>  if (tests[i].message != NULL)
> >> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
> >> *tests, const guint cases)
> >>   "host", ,
> >>   "username", ,
> >>   "password", ,
> >> + "unix-path", _path,
> >>NULL);
> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> uri);
> >>  g_assert_cmpstr(tests[i].port, ==, port);
> >>  g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
> >>  g_assert_cmpstr(tests[i].host, ==, host);
> >>  g_assert_cmpstr(tests[i].username, ==, username);
> >>  g_assert_cmpstr(tests[i].password, ==, password);
> >>  g_test_assert_expected_messages();
> >> +g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
> >>  g_clear_pointer(, g_free);
> >>  g_clear_pointer(, g_free);
> >>  g_clear_pointer(_port, g_free);
> >>  g_clear_pointer(, g_free);
> >>  g_clear_pointer(, g_free);
> >>  g_clear_pointer(, g_free);
> >> +g_clear_pointer(_path, g_free);
> >>  g_object_unref(s);
> >>  }
> >>
> >> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase
> >> *tests,
> >> const guint cases)
> >>   "host", tests[i].host,
> >>   "username", tests[i].username,
> >>   "password", tests[i].password,
> >> + "unix-path", tests[i].unix_path,
> >>NULL);
> >>  g_object_get(s, "uri", , NULL);
> >> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> >> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==,
> >> uri);
> >>  g_clear_pointer(, g_free);
> >>  g_object_unref(s);
> >>  }
> >> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
> >>  test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >>  }
> >>
> >> +static void test_session_uri_unix_good(void)
> >> +{
> >> +const TestCase tests[] = {
> >> +{ .uri_input = "spice+unix:///tmp/foo.sock",
> >> +  .unix_path = "/tmp/foo.sock" },
> >> +/* perhaps not very clever, but this doesn't raise an
> >> error/warning
> >> */
> >> +{ .uri_input = "spice+unix://",
> >> +  .unix_path = "" },
> >> +/* unix uri don't support passing password or other kind of
> >> options
> >> */
> >> +{ .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
> >> +  .unix_path = "/tmp/foo.sock?password=frobnicate" },
> >> +};
> >> +
> >> +test_session_uri_good(tests, G_N_ELEMENTS(tests));
> >> +}
> >> +
> >>  int main(int argc, char* argv[])
> >>  {
> >>  g_test_init(, , NULL);
> >> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
> >>  g_test_add_func("/session/bad-uri", test_session_uri_bad);
> >>  g_test_add_func("/session/good-ipv4-uri",
> >>  test_session_uri_ipv4_good);
> >>  g_test_add_func("/session/good-ipv6-uri",
> >>  test_session_uri_ipv6_good);
> >> +g_test_add_func("/session/good-unix", test_session_uri_unix_good);
> >>
> >>  return g_test_run();
> >>  }
> >
> > Looks good (still to review better).
> > Can we consider this patch separate from the rest of the series
> > (that is merge even separately) ?
> 
> Sure, it was just in the same area of code, and thus added dependency,
> but we can review & merge this one right away I think.
> thanks
> 

Acked-by: Frediano Ziglio 

Follow ups:

The "spice://" syntax is weird, maybe we should refuse it.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

30 matches

Site Navigation

Mail list logo

Footer information