Re: [Spice-devel] [PATCH spice-server] build: Rename spice-server-enums.tmpl.[ch] to spice-server-enums.[ch].tmpl

[Eduardo Lima (Etrunko)]

On 06/03/18 13:37, Christophe Fergeau wrote:
> On Tue, Mar 06, 2018 at 11:46:33AM -0300, Eduardo Lima (Etrunko) wrote:
>> This is a preparation for meson build, which has built-in support for
>> generating enums, but requires the template files to be renamed. It uses
>> the basename of template files to generate the output, and in this case
>> it would be the same file for both '.c' and '.h'.
>>
>> Reference http://mesonbuild.com/Gnome-module.html#gnomemkenums
> 
> Hmm the generated files which should have the same base name are
> currently spice-server-enums.c and spice-server-enums.h, so this is ok,
> and from the link you gave, 'spice-server-enums' would be the first arg
> to gnome.mkenums().
> Then the link you give lists 2 separate arguments for the templates, 
> c_template
> and h_template.
> So after reading the link, I'm not sure why this patch is needed?
> 

The documentation is definitively lacking, I will provide a patch for
that. I did not understand what was happening until I looked at the
source code [1]. Ideally, Meson should let us specify the name of the
output files, but this is not the case.

The declaration which failed was:

spice_server_enums = gnome.mkenums('spice-server-enums',
   sources : 'spice-server.h',
   c_template : 'spice-server-enums.tmpl.c',
   h_template : 'spice-server-enums.tmpl.h')


With the following error:

Meson encountered an error in file server/meson.build, line 30, column 0:
Tried to create target "spice-server-enums.tmpl", but a target of that
name already exists.

[1]
https://github.com/mesonbuild/meson/blob/master/mesonbuild/modules/gnome.py#L973

> 
>>
>> Signed-off-by: Eduardo Lima (Etrunko) 
>> ---
>>  server/Makefile.am   | 12 
>> ++--
>>  .../{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} |  0
>>  .../{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} |  0
>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>  rename server/{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} 
>> (100%)
>>  rename server/{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} 
>> (100%)
>>
>> diff --git a/server/Makefile.am b/server/Makefile.am
>> index 5d5590af..c1f241ac 100644
>> --- a/server/Makefile.am
>> +++ b/server/Makefile.am
>> @@ -201,11 +201,11 @@ endif
>>  libspice_server_la_LIBADD = libserver.la
>>  libspice_server_la_SOURCES =
>>  
>> -spice-server-enums.c: spice-server.h spice-server-enums.tmpl.c
>> -$(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.c 
>> $< > $@
>> +spice-server-enums.c: spice-server.h spice-server-enums.c.tmpl
>> +$(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.c.tmpl 
>> $< > $@
>>  
>> -spice-server-enums.h: spice-server.h spice-server-enums.tmpl.h
>> -$(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.h 
>> $< > $@
>> +spice-server-enums.h: spice-server.h spice-server-enums.h.tmpl
>> +$(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.h.tmpl 
>> $< > $@
>>  
>>  EXTRA_DIST =\
>>  spice-bitmap-utils.tmpl.c   \
>> @@ -213,8 +213,8 @@ EXTRA_DIST = \
>>  glz-encode-match.tmpl.c \
>>  glz-encode.tmpl.c   \
>>  spice-server.syms   \
>> -spice-server-enums.tmpl.h   \
>> -spice-server-enums.tmpl.c   \
>> +spice-server-enums.h.tmpl   \
>> +spice-server-enums.c.tmpl   \
>>  $(NULL)
>>  
>>  BUILT_SOURCES = $(spice_built_sources)
>> diff --git a/server/spice-server-enums.tmpl.c 
>> b/server/spice-server-enums.c.tmpl
>> similarity index 100%
>> rename from server/spice-server-enums.tmpl.c
>> rename to server/spice-server-enums.c.tmpl
>> diff --git a/server/spice-server-enums.tmpl.h 
>> b/server/spice-server-enums.h.tmpl
>> similarity index 100%
>> rename from server/spice-server-enums.tmpl.h
>> rename to server/spice-server-enums.h.tmpl
>> -- 
>> 2.14.3
>>
>> ___
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-streaming-agent PATCH] handle_stream_error: add comment for inheriting struct

[Frediano Ziglio]

> 
> Introduced in 548577dc8adae1a558
> 
> Signed-off-by: Uri Lublin 
> ---
>  src/spice-streaming-agent.cpp | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index b39782c..e25d47a 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -145,6 +145,11 @@ static void handle_stream_error(size_t len)
>   std::to_string(sizeof(StreamMsgNotifyError))
>   + ")");
>  }
>  
> +// This struct inherits StreamMsgNotifyError. Its memory layout is:
> +// offset 0: StreamMsgNotifyError.error_code (a uint32_t)
> +// offset 4: StreamMsgNotifyError.msg and also msg.msg (a
> uint8_t[1024]).
> +// Both StreamMsgNotifyError.msg and msg.msg point to the same
> +// memory location (practically local msg overrides inherited msg).
>  struct : StreamMsgNotifyError {
>  uint8_t msg[1024];
>  } msg;

When I sent my suggestion I didn't wanted to clash with inherited field,
just didn't invented a new name.
Maybe would be easier and more clear to rename to msg_buffer or similar
to avoid the confusion?

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

Re: [Spice-devel] [spice-server 2/8] build: Bump glib version

[Frediano Ziglio]

> 
> From spice-gtk b312ca08 commit:
> "At the moment:
> - Fedora 26 has 2.52
> - Fedora 25 has 2.50
> - Fedora 24 has 2.48
> - CentOS 7 has 2.46
> - Debian 9 has 2.50"
> 
> RHEL6 only have 2.28, but glib 2.32 is only used in a test case at the
> moment.

Well... or is supported or is not supported, in the
past we changed requirements for specific tests, I would
follow that style.

Honestly... if you send a patch saying that we stop RHEL6 support
I'll ack in 2 ms!

> ---
>  configure.ac | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 9eb9bb492..bcd4bb4d5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -161,8 +161,8 @@ SPICE_PROTOCOL_MIN_VER=0.12.14
>  PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >=
>  $SPICE_PROTOCOL_MIN_VER])
>  AC_SUBST([SPICE_PROTOCOL_MIN_VER])
>  
> -GLIB2_REQUIRED=2.28
> -GLIB2_ENCODED_VERSION="GLIB_VERSION_2_28"
> +GLIB2_REQUIRED=2.32
> +GLIB2_ENCODED_VERSION="GLIB_VERSION_2_32"
>  PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= $GLIB2_REQUIRED gio-2.0 >=
>  $GLIB2_REQUIRED])
>  GLIB2_CFLAGS="$GLIB2_CFLAGS
>  -DGLIB_VERSION_MIN_REQUIRED=$GLIB2_ENCODED_VERSION \
>-DGLIB_VERSION_MAX_ALLOWED=$GLIB2_ENCODED_VERSION"

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

Re: [Spice-devel] [spice-server 3/8] tests: basic-event-loop: Silence debug message

[Frediano Ziglio]

> 
> There is currently a debug printf which is always shown when a mainloop
> event is triggered. This is unlikely to be useful unless one is
> debugging the event loop code.
> ---
>  server/tests/basic-event-loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/tests/basic-event-loop.c
> b/server/tests/basic-event-loop.c
> index b10ac9468..607a5a5ee 100644
> --- a/server/tests/basic-event-loop.c
> +++ b/server/tests/basic-event-loop.c
> @@ -46,7 +46,7 @@ GMainContext *basic_event_loop_get_context(void)
>  
>  static void event_loop_channel_event(int event, SpiceChannelEventInfo *info)
>  {
> -DPRINTF(0, "channel event con, type, id, event: %d, %d, %d, %d",
> +DPRINTF(1, "channel event con, type, id, event: %d, %d, %d, %d",
>  info->connection_id, info->type, info->id, event);
>  }
>  

Acked-by: Frediano Ziglio 

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 v5 2/2] Implement handling of error messages from the server

[Frediano Ziglio]

> 
> > On 3 Mar 2018, at 10:36, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 28 Feb 2018, at 13:53, Lukáš Hrázký  wrote:
> >>> 
> >>> Log error messages from the server to syslog (not aborting the agent).
> >>> Limits the messages to 1023 bytes, error messages from the server are
> >>> not expected to be longer. In the unlikely case the message is longer,
> >>> log the first 1023 bytes and throw an exception (because we don't read
> >>> out the rest of the message, causing desynchronization between the
> >>> server and the streaming agent).
> >>> 
> >>> Signed-off-by: Lukáš Hrázký 
> >>> ---
> >>> src/spice-streaming-agent.cpp | 26 +++---
> >>> 1 file changed, 23 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/src/spice-streaming-agent.cpp
> >>> b/src/spice-streaming-agent.cpp
> >>> index ee57920..954d1b3 100644
> >>> --- a/src/spice-streaming-agent.cpp
> >>> +++ b/src/spice-streaming-agent.cpp
> >>> @@ -137,10 +137,30 @@ static void handle_stream_capabilities(uint32_t
> >>> len)
> >>>}
> >>> }
> >>> 
> >>> -static void handle_stream_error(uint32_t len)
> >>> +static void handle_stream_error(size_t len)
> >>> {
> >>> -// TODO read message and use it
> >>> -throw std::runtime_error("got an error message from server");
> >>> +if (len < sizeof(StreamMsgNotifyError)) {
> >>> +throw std::runtime_error("Received NotifyError message size " +
> >>> std::to_string(len) +
> >>> + " is too small (smaller than " +
> >>> +
> >>> std::to_string(sizeof(StreamMsgNotifyError))
> >>> + ")");
> >>> +}
> >>> +
> >>> +struct : StreamMsgNotifyError {
> >> 
> >> :-O
> >> 
> >> I had t read that twice to make sure that was actually what was written…
> >> Barf.
> >> 
> >> 
> >>> +uint8_t msg[1024];
> >> 
> >> Hard-coded magic value. There should be a constant for it in
> >> stream-device.h.
> >> Please add it.
> >> 
> > 
> > Currently the protocol does not define a limit, this is a guest limit but
> > probably a safe and reasonable limit for the protocol can be decided.
> 
> Problem with this approach is that the guest cannot read anything past any
> error message that is too large for it. One could
> - define a protocol limit (the simplest)
> - skip the extra bytes
> - allocate a modest amount for the common case, and dynamically allocate
> otherwise
> - use dynamic storage in all cases
> 
> I don’t care either way, but killing the agent if the server sends an error
> that is too large opens a denial of service window.
> 

Yes, can be done (I'd suggest skipping the rest).
Currently errors are fatal.

> > 
> >>> +} msg;
> >> 
> >> So I was aggravated very recently regarding padding (having to initialize
> >> it…), but this patch gets nary a peep and the series is acked and merged
> >> right away?
> >> 
> > 
> > Perhaps you lost the mails saying that the protocol structure don't and
> > won't have internal padding.
> 
> Only on x86. It has padding on any ABI with a natural 64-bit alignment.
> 
> I don’t have an Itanium handy, but computing the offsetof(msg.msg) and
> offsetof(msg.StreamNotifyError::msg) on a Raspberry Pi using
> -mstructure-size-boundary=64 yields:
> 
> offsetof msg.msg=8
> offsetof msg.StreamNotifyError::msg=4
> 

Some compiler flag are used to break the ABI and should be used in
specific cases like some optimized code or writing a language library
if the language for some reason requires a different ABI, you won't
be able to call libc function safely if you use such options and as
you want to break the ABI you'll get an ABI breakage.

My suggestion on overriding the field was not intended.

> 
> >> When you inherit, the derived members are aligned, and there is potential
> >> padding. However, the code as merged relies on msg.msg being identical to
> >> msg.StreamMsgNotifyError::msg. In other words, this code implicitly relies
> >> on the lack of any padding. Add for example a ‘char c’ or ‘bool b’ after
> >> error_code in StreamMsgNotifyError, and suddenly,
> > 
> > Adding bool or char to StreamMsgNotifyError or a internal padding is
> > a bug writing protocol (as already discussed) and also being an ABI now
> > we can't change it so there isn't and there won't be any such field.
> 
> I was using that as an illustration of the problem.
> 
> > 
> >> msg.StreamMsgNotifyError::msg is at offset 5 while msg.msg is at offset 8
> >> on
> >> my machine, and all your messages will “mysteriously” be off by three
> >> bytes.
> >> 
> > 
> > Yes, if you don't know how to write a protocol structure, ignore the ABI
> > and ignore the notes on the protocol file describing it you have this
> > problem.
> 
> Why not just use the “packed” attribute?
> 

Already replied.

> > 
> >> Please fix it.
> >> 
> > 
> > Fix what ?
> 
> The non-portable, hard to read code that was introduced by this patch.
> 
> For example, use the original msg field, not the derived object’s msg 

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

[Christophe Fergeau]

On Fri, Mar 02, 2018 at 06:06:32PM +0100, Christophe de Dinechin wrote:
> > FWIW, I also find myself in agreement with Frediano and Lukas on this point.
> 
> OK. Maybe I misunderstood something.
> 
> Do we agree that the case Frediano raised is someone trying:
> 
>   throw Error(“The value of “ + std::to_string(2) + “ does not match “ + 
> std::to_string(1));
> 
> which yields the following error:
> 
>   spice-streaming-agent.cpp:165:93: error: no matching function for call 
> to ‘spice::streaming_agent::Error::Error(std::__cxx11::basic_string)’
> 
> So far, I think the interface was not “error prone”. It caught the error 
> correctly.
> 
> Therefore, in order to find the interface “error prone”, one has to accept a 
> second hypothesis, which is that whoever attempted that, after seeing that 
> error message, looks at the interface, which has a ‘const char *’ and a 
> comment that specifically states "It is recommended to only use static C 
> strings, since formatting of any dynamic argument is done by 
> ‘format_message’”, and after reading that comment, has a big “Aha” moment and 
> writes:
> 
>   throw Error((“The value of “ + std::to_string(2) + “ does not match “ + 
> std::to_string(1)).c_str());
> 
> I’m sorry, but I cannot accept that step of the reasoning as being even 
> remotely valid. Just looking at the code makes me want to barf.
> 
> So either I misunderstood Frediano’s point, or the reasoning is deeply flawed 
> in a not very subtle way.

It's not just about .c_str(), it's really about any API returning a
non-static string which will be freed before the exception is caught
(think RAII-like cases, wrappers around preexisting C APIs, ...).
I already mentioned it, but an API taking a const char *, and also
mandating that this const char * must be alive as long as the instance
I invoked the method from is really unusual/unexpected to me, and would
thus be error-prone.

Christophe


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

[Spice-devel] qdh displays

[Peter Carlson]

when I connect to my vm using my laptop I get a way different resolution 
on the guest than with my desktop.


My laptop has a resolution of 3840x2160.  A maximized remote-viewer 
client results in a guest resolution of 1920x991


My desktop has a resolution of 2560x1440.  A mxaimized remote-viewer 
client results in a guest resolution of 1560x1342


How can I control the scaling that remote-viewer is doing on qhd displays?

Peter

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

[Spice-devel] [spice-server 0/8] Add test-listen test case

[Christophe Fergeau]

While working on some bug/new feature for SPICE, I added a test case for
our spice_server_set_port/_set_tls/... API. While the work I did this
for still needs some work, this test case should be good enough on its
own.

Christophe

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

[Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

[Christophe Fergeau]

Currently if we fail to set up the watch waiting for accept() to be
called on the socket, we still keep the network socket open even if we
are not going to be able to use it. This commit makes sure it's closed a
set to -1 when such a failure occurs.
---
 server/reds.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index a31ed4e96..7867e8573 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2595,6 +2595,8 @@ static int reds_init_net(RedsState *reds)
  SPICE_WATCH_EVENT_READ,
  reds_accept, reds);
 if (reds->listen_watch == NULL) {
+close(reds->listen_socket);
+reds->listen_socket = -1;
 spice_warning("set fd handle failed");
 return -1;
 }
@@ -2610,6 +2612,8 @@ static int reds_init_net(RedsState *reds)
 SPICE_WATCH_EVENT_READ,
 
reds_accept_ssl_connection, reds);
 if (reds->secure_listen_watch == NULL) {
+close(reds->secure_listen_socket);
+reds->secure_listen_socket = -1;
 spice_warning("set fd handle failed");
 return -1;
 }
@@ -2621,6 +2625,7 @@ static int reds_init_net(RedsState *reds)
  SPICE_WATCH_EVENT_READ,
  reds_accept, reds);
 if (reds->listen_watch == NULL) {
+reds->listen_socket = -1;
 spice_warning("set fd handle failed");
 return -1;
 }
-- 
2.14.3

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

[Spice-devel] [spice-server 8/8] test-listen: Add Unix socket test

[Christophe Fergeau]

---
 configure.ac   |  3 +++
 server/tests/Makefile.am   |  4 +++-
 server/tests/test-listen.c | 38 +++---
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index bcd4bb4d5..863834343 100644
--- a/configure.ac
+++ b/configure.ac
@@ -171,6 +171,9 @@ AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 
$GLIB2_REQUIRED gio-2.0 >= $GLIB2
 PKG_CHECK_MODULES([GOBJECT2], [gobject-2.0 >= $GLIB2_REQUIRED])
 AS_VAR_APPEND([SPICE_REQUIRES], [" gobject-2.0 >= $GLIB2_REQUIRED"])
 
+#used only by tests
+PKG_CHECK_MODULES([GIO_UNIX], [gio-unix-2.0 >= $GLIB2_REQUIRED])
+
 PIXMAN_REQUIRED=0.17.7
 PKG_CHECK_MODULES(PIXMAN, pixman-1 >= $PIXMAN_REQUIRED)
 AC_SUBST(PIXMAN_CFLAGS)
diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 0aae1fd61..ffeb8fc04 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -16,8 +16,9 @@ AM_CPPFLAGS = \
-I$(top_builddir)/server\
-I$(top_srcdir)/server/tests\
$(COMMON_CFLAGS)\
+   $(GIO_UNIX_CFLAGS)  \
$(GLIB2_CFLAGS) \
-   $(GOBJECT2_CFLAGS)  \
+   $(GOBJECT2_CFLAGS)  \
$(SMARTCARD_CFLAGS) \
$(SPICE_NONPKGCONFIG_CFLAGS)\
$(SPICE_PROTOCOL_CFLAGS)\
@@ -39,6 +40,7 @@ LDADD =   
\
libtest.a   \
$(top_builddir)/spice-common/common/libspice-common.la  \
$(top_builddir)/server/libserver.la \
+   $(GIO_UNIX_LIBS)\
$(GLIB2_LIBS)   \
$(GOBJECT2_LIBS)\
$(SPICE_NONPKGCONFIG_LIBS)  \
diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
index 2a15df1ab..2658d61c5 100644
--- a/server/tests/test-listen.c
+++ b/server/tests/test-listen.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define PKI_DIR SPICE_TOP_SRCDIR "/server/tests/pki/"
 
@@ -207,9 +208,13 @@ static GThread *fake_client_new(GThreadFunc thread_func,
 {
 ThreadData *thread_data = g_new0(ThreadData, 1);
 
-g_assert_cmpuint(port, >, 0);
-g_assert_cmpuint(port, <, 65536);
-thread_data->connectable = g_network_address_new(hostname, port);
+if (port == -1) {
+thread_data->connectable = 
G_SOCKET_CONNECTABLE(g_unix_socket_address_new(hostname));
+} else {
+g_assert_cmpuint(port, >, 0);
+g_assert_cmpuint(port, <, 65536);
+thread_data->connectable = g_network_address_new(hostname, port);
+}
 thread_data->use_tls = use_tls;
 thread_data->user_data = user_data;
 
@@ -310,6 +315,32 @@ static void test_connect_both(void)
 spice_server_destroy(server);
 }
 
+static void test_connect_unix(void)
+{
+GThread *thread;
+int result;
+
+TestEventLoop event_loop = { 0, };
+
+test_event_loop_init(_loop);
+
+/* server */
+SpiceServer *server = spice_server_new();
+spice_server_set_name(server, "SPICE listen test");
+spice_server_set_noauth(server);
+spice_server_set_addr(server, "test-listen.unix", 
SPICE_ADDR_FLAG_UNIX_ONLY);
+result = spice_server_init(server, event_loop.core);
+g_assert_cmpint(result, ==, 0);
+
+/* fake client */
+thread = fake_client_new(check_magic_thread, "test-listen.unix", -1, 
false, _loop);
+test_event_loop_run(_loop);
+g_assert_null(g_thread_join(thread));
+
+test_event_loop_destroy(_loop);
+spice_server_destroy(server);
+}
+
 static void test_connect_ko(void)
 {
 GThread *thread;
@@ -332,6 +363,7 @@ int main(int argc, char **argv)
 g_test_add_func("/server/listen/connect_plain", test_connect_plain);
 g_test_add_func("/server/listen/connect_tls", test_connect_tls);
 g_test_add_func("/server/listen/connect_both", test_connect_both);
+g_test_add_func("/server/listen/connect_unix", test_connect_unix);
 g_test_add_func("/server/listen/connect_ko", test_connect_ko);
 
 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] [spice-server 4/8] test-listen: Add test case port/address configuration

[Christophe Fergeau]

This test case will be testing the external spice-server API to
configure the address/port it's listening on. For now it sets up a
listening server, spawns a thread which is going to connect to that
port, and check it gets the REDQ magic upon connection. It will be
extended to test for Unix sockets, TLS sockets, ...
---
 server/tests/Makefile.am   |   1 +
 server/tests/test-listen.c | 146 +
 2 files changed, 147 insertions(+)
 create mode 100644 server/tests/test-listen.c

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 2ff06d7a5..0aae1fd61 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -59,6 +59,7 @@ check_PROGRAMS =  \
test-empty-success  \
test-channel\
test-stream-device  \
+   test-listen \
$(NULL)
 
 noinst_PROGRAMS =  \
diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
new file mode 100644
index 0..052dc0b8f
--- /dev/null
+++ b/server/tests/test-listen.c
@@ -0,0 +1,146 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2018 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see .
+*/
+/*
+ * This tests the external API entry points to configure the address/port
+ * spice-server is listening on
+ */
+#include 
+
+#include "basic-event-loop.h"
+
+#include 
+#include 
+#include 
+#include 
+
+static SpiceCoreInterface *core;
+
+static bool error_is_set(GError **error)
+{
+return ((error != NULL) && (*error != NULL));
+}
+
+static GIOStream *fake_client_connect(GSocketConnectable *connectable, GError 
**error)
+{
+GSocketClient *client;
+GSocketConnection *connection;
+
+client = g_socket_client_new();
+connection = g_socket_client_connect(client, connectable, NULL, error);
+
+g_object_unref(client);
+
+return G_IO_STREAM(connection);
+}
+
+static void check_magic(GIOStream *io_stream, GError **error)
+{
+uint8_t buffer[4];
+gsize bytes_read;
+gsize bytes_written;
+GInputStream *input_stream;
+GOutputStream *output_stream;
+
+/* send dummy data to trigger a response from the server */
+output_stream = g_io_stream_get_output_stream(io_stream);
+memset(buffer, 0xa5, G_N_ELEMENTS(buffer));
+g_output_stream_write_all(output_stream, buffer, G_N_ELEMENTS(buffer), 
_written, NULL, error);
+if (error_is_set(error)) {
+return;
+}
+
+input_stream = g_io_stream_get_input_stream(io_stream);
+g_input_stream_read_all(input_stream, buffer, G_N_ELEMENTS(buffer), 
_read, NULL, error);
+if (error_is_set(error)) {
+return;
+}
+g_assert_cmpuint(bytes_read, ==, G_N_ELEMENTS(buffer));
+g_assert_cmpint(memcmp(buffer, "REDQ", 4), ==, 0);
+}
+
+static void exit_mainloop_cb(SPICE_GNUC_UNUSED void *opaque)
+{
+basic_event_loop_quit();
+}
+
+static gpointer check_magic_thread(gpointer data)
+{
+GError *error = NULL;
+GSocketConnectable *connectable = G_SOCKET_CONNECTABLE(data);
+GIOStream *stream;
+SpiceTimer *exit_mainloop_timer;
+
+stream = fake_client_connect(connectable, );
+g_assert_no_error(error);
+check_magic(stream, );
+g_assert_no_error(error);
+
+g_object_unref(stream);
+g_object_unref(connectable);
+exit_mainloop_timer = core->timer_add(exit_mainloop_cb, NULL);
+core->timer_start(exit_mainloop_timer, 0);
+
+return NULL;
+}
+
+static GThread *fake_client_new(const char *hostname, int port)
+{
+GSocketConnectable *connectable;
+
+g_assert_cmpuint(port, >, 0);
+g_assert_cmpuint(port, <, 65536);
+connectable = g_network_address_new(hostname, port);
+
+/* check_magic_thread will assume ownership of 'connectable' */
+return g_thread_new("fake-client-thread", check_magic_thread, connectable);
+}
+
+static void test_connect_plain(void)
+{
+GThread *thread;
+int result;
+
+/* server */
+SpiceServer *server = spice_server_new();
+core = basic_event_loop_init();
+spice_server_set_name(server, "SPICE listen test");
+spice_server_set_noauth(server);
+spice_server_set_port(server, 5701);
+result = spice_server_init(server, core);
+

[Spice-devel] [spice-server 2/8] build: Bump glib version

[Christophe Fergeau]

From spice-gtk b312ca08 commit:
"At the moment:
- Fedora 26 has 2.52
- Fedora 25 has 2.50
- Fedora 24 has 2.48
- CentOS 7 has 2.46
- Debian 9 has 2.50"

RHEL6 only have 2.28, but glib 2.32 is only used in a test case at the
moment.
---
 configure.ac | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9eb9bb492..bcd4bb4d5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -161,8 +161,8 @@ SPICE_PROTOCOL_MIN_VER=0.12.14
 PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 
$SPICE_PROTOCOL_MIN_VER])
 AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 
-GLIB2_REQUIRED=2.28
-GLIB2_ENCODED_VERSION="GLIB_VERSION_2_28"
+GLIB2_REQUIRED=2.32
+GLIB2_ENCODED_VERSION="GLIB_VERSION_2_32"
 PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= $GLIB2_REQUIRED gio-2.0 >= 
$GLIB2_REQUIRED])
 GLIB2_CFLAGS="$GLIB2_CFLAGS -DGLIB_VERSION_MIN_REQUIRED=$GLIB2_ENCODED_VERSION 
\
   -DGLIB_VERSION_MAX_ALLOWED=$GLIB2_ENCODED_VERSION"
-- 
2.14.3

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

[Spice-devel] [spice-server 5/8] test-listen: Add connection attempt to non-open port

[Christophe Fergeau]

---
 server/tests/test-listen.c | 46 +++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
index 052dc0b8f..562f07487 100644
--- a/server/tests/test-listen.c
+++ b/server/tests/test-listen.c
@@ -98,7 +98,27 @@ static gpointer check_magic_thread(gpointer data)
 return NULL;
 }
 
-static GThread *fake_client_new(const char *hostname, int port)
+static gpointer check_no_connect_thread(gpointer data)
+{
+GError *error = NULL;
+GSocketConnectable *connectable = G_SOCKET_CONNECTABLE(data);
+GIOStream *stream;
+SpiceTimer *exit_mainloop_timer;
+
+stream = fake_client_connect(connectable, );
+g_assert(error != NULL);
+g_assert(stream == NULL);
+g_clear_error();
+
+g_object_unref(connectable);
+exit_mainloop_timer = core->timer_add(exit_mainloop_cb, NULL);
+core->timer_start(exit_mainloop_timer, 0);
+
+return NULL;
+}
+
+
+static GThread *fake_client_new(GThreadFunc thread_func, const char *hostname, 
int port)
 {
 GSocketConnectable *connectable;
 
@@ -107,7 +127,7 @@ static GThread *fake_client_new(const char *hostname, int 
port)
 connectable = g_network_address_new(hostname, port);
 
 /* check_magic_thread will assume ownership of 'connectable' */
-return g_thread_new("fake-client-thread", check_magic_thread, connectable);
+return g_thread_new("fake-client-thread", thread_func, connectable);
 }
 
 static void test_connect_plain(void)
@@ -125,7 +145,7 @@ static void test_connect_plain(void)
 g_assert_cmpint(result, ==, 0);
 
 /* fake client */
-thread = fake_client_new("localhost", 5701);
+thread = fake_client_new(check_magic_thread, "localhost", 5701);
 
 basic_event_loop_mainloop();
 
@@ -137,10 +157,30 @@ static void test_connect_plain(void)
 spice_server_destroy(server);
 }
 
+static void test_connect_ko(void)
+{
+GThread *thread;
+
+core = basic_event_loop_init();
+
+/* fake client */
+thread = fake_client_new(check_no_connect_thread, "localhost", 5701);
+
+basic_event_loop_mainloop();
+
+g_assert_null(g_thread_join(thread));
+
+g_thread_unref(thread);
+basic_event_loop_destroy();
+core = NULL;
+}
+
 int main(int argc, char **argv)
 {
 g_test_init(, , NULL);
+
 g_test_add_func("/server/listen/connect_plain", test_connect_plain);
+g_test_add_func("/server/listen/connect_ko", test_connect_ko);
 
 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] [spice-server 3/8] tests: basic-event-loop: Silence debug message

[Christophe Fergeau]

There is currently a debug printf which is always shown when a mainloop
event is triggered. This is unlikely to be useful unless one is
debugging the event loop code.
---
 server/tests/basic-event-loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/tests/basic-event-loop.c b/server/tests/basic-event-loop.c
index b10ac9468..607a5a5ee 100644
--- a/server/tests/basic-event-loop.c
+++ b/server/tests/basic-event-loop.c
@@ -46,7 +46,7 @@ GMainContext *basic_event_loop_get_context(void)
 
 static void event_loop_channel_event(int event, SpiceChannelEventInfo *info)
 {
-DPRINTF(0, "channel event con, type, id, event: %d, %d, %d, %d",
+DPRINTF(1, "channel event con, type, id, event: %d, %d, %d, %d",
 info->connection_id, info->type, info->id, event);
 }
 
-- 
2.14.3

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

[Spice-devel] [spice-server 6/8] test-listen: Add event loop helpers

[Christophe Fergeau]

These factor a bit of common code, and more importantly, help with
freeing all event loop related data at the end of each test.
---
 server/tests/test-listen.c | 120 -
 1 file changed, 86 insertions(+), 34 deletions(-)

diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
index 562f07487..e88105eea 100644
--- a/server/tests/test-listen.c
+++ b/server/tests/test-listen.c
@@ -28,13 +28,62 @@
 #include 
 #include 
 
-static SpiceCoreInterface *core;
 
 static bool error_is_set(GError **error)
 {
 return ((error != NULL) && (*error != NULL));
 }
 
+
+typedef struct {
+SpiceCoreInterface *core;
+SpiceTimer *exit_mainloop_timer;
+SpiceTimer *timeout_timer;
+} TestEventLoop;
+
+static void timeout_cb(SPICE_GNUC_UNUSED void *opaque)
+{
+g_assert_not_reached();
+}
+
+static void exit_mainloop_cb(SPICE_GNUC_UNUSED void *opaque)
+{
+basic_event_loop_quit();
+}
+
+static void test_event_loop_quit(TestEventLoop *event_loop)
+{
+event_loop->core->timer_start(event_loop->exit_mainloop_timer, 0);
+}
+
+static void test_event_loop_init(TestEventLoop *event_loop)
+{
+event_loop->core = basic_event_loop_init();
+event_loop->timeout_timer = event_loop->core->timer_add(timeout_cb, NULL);
+event_loop->exit_mainloop_timer = 
event_loop->core->timer_add(exit_mainloop_cb, NULL);
+}
+
+static void test_event_loop_destroy(TestEventLoop *event_loop)
+{
+if (event_loop->timeout_timer != NULL) {
+event_loop->core->timer_remove(event_loop->timeout_timer);
+event_loop->timeout_timer = NULL;
+}
+if (event_loop->exit_mainloop_timer != NULL) {
+event_loop->core->timer_remove(event_loop->exit_mainloop_timer);
+event_loop->exit_mainloop_timer = NULL;
+}
+basic_event_loop_destroy();
+event_loop->core = NULL;
+}
+
+static void test_event_loop_run(TestEventLoop *event_loop)
+{
+event_loop->core->timer_start(event_loop->timeout_timer, 5000);
+basic_event_loop_mainloop();
+}
+
+
 static GIOStream *fake_client_connect(GSocketConnectable *connectable, GError 
**error)
 {
 GSocketClient *client;
@@ -73,17 +122,19 @@ static void check_magic(GIOStream *io_stream, GError 
**error)
 g_assert_cmpint(memcmp(buffer, "REDQ", 4), ==, 0);
 }
 
-static void exit_mainloop_cb(SPICE_GNUC_UNUSED void *opaque)
+typedef struct
 {
-basic_event_loop_quit();
-}
+GSocketConnectable *connectable;
+gpointer user_data;
+} ThreadData;
 
 static gpointer check_magic_thread(gpointer data)
 {
 GError *error = NULL;
-GSocketConnectable *connectable = G_SOCKET_CONNECTABLE(data);
+ThreadData *thread_data = data;
+GSocketConnectable *connectable = 
G_SOCKET_CONNECTABLE(thread_data->connectable);
+TestEventLoop *event_loop = thread_data->user_data;
 GIOStream *stream;
-SpiceTimer *exit_mainloop_timer;
 
 stream = fake_client_connect(connectable, );
 g_assert_no_error(error);
@@ -92,8 +143,9 @@ static gpointer check_magic_thread(gpointer data)
 
 g_object_unref(stream);
 g_object_unref(connectable);
-exit_mainloop_timer = core->timer_add(exit_mainloop_cb, NULL);
-core->timer_start(exit_mainloop_timer, 0);
+g_free(thread_data);
+
+test_event_loop_quit(event_loop);
 
 return NULL;
 }
@@ -101,9 +153,10 @@ static gpointer check_magic_thread(gpointer data)
 static gpointer check_no_connect_thread(gpointer data)
 {
 GError *error = NULL;
-GSocketConnectable *connectable = G_SOCKET_CONNECTABLE(data);
+ThreadData *thread_data = data;
+GSocketConnectable *connectable = 
G_SOCKET_CONNECTABLE(thread_data->connectable);
+TestEventLoop *event_loop = thread_data->user_data;
 GIOStream *stream;
-SpiceTimer *exit_mainloop_timer;
 
 stream = fake_client_connect(connectable, );
 g_assert(error != NULL);
@@ -111,23 +164,26 @@ static gpointer check_no_connect_thread(gpointer data)
 g_clear_error();
 
 g_object_unref(connectable);
-exit_mainloop_timer = core->timer_add(exit_mainloop_cb, NULL);
-core->timer_start(exit_mainloop_timer, 0);
+g_free(thread_data);
+
+test_event_loop_quit(event_loop);
 
 return NULL;
 }
 
-
-static GThread *fake_client_new(GThreadFunc thread_func, const char *hostname, 
int port)
+static GThread *fake_client_new(GThreadFunc thread_func,
+const char *hostname, int port,
+gpointer user_data)
 {
-GSocketConnectable *connectable;
+ThreadData *thread_data = g_new0(ThreadData, 1);
 
 g_assert_cmpuint(port, >, 0);
 g_assert_cmpuint(port, <, 65536);
-connectable = g_network_address_new(hostname, port);
+thread_data->connectable = g_network_address_new(hostname, port);
+thread_data->user_data = user_data;
 
 /* check_magic_thread will assume ownership of 'connectable' */
-return g_thread_new("fake-client-thread", thread_func, connectable);
+return 

[Spice-devel] [spice-server 7/8] test-listen: Add TLS test

[Christophe Fergeau]

---
 server/tests/test-listen.c | 106 +++--
 1 file changed, 103 insertions(+), 3 deletions(-)

diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c
index e88105eea..2a15df1ab 100644
--- a/server/tests/test-listen.c
+++ b/server/tests/test-listen.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 
+#define PKI_DIR SPICE_TOP_SRCDIR "/server/tests/pki/"
 
 static bool error_is_set(GError **error)
 {
@@ -97,6 +98,29 @@ static GIOStream *fake_client_connect(GSocketConnectable 
*connectable, GError **
 return G_IO_STREAM(connection);
 }
 
+static GIOStream *fake_client_connect_tls(GSocketConnectable *connectable, 
GError **error)
+{
+GSocketClient *client;
+GSocketConnection *connection;
+GIOStream *tls_connection;
+
+client = g_socket_client_new();
+connection = g_socket_client_connect(client, connectable, NULL, error);
+g_assert_no_error(*error);
+tls_connection = g_tls_client_connection_new(G_IO_STREAM(connection),
+ connectable,
+ error);
+g_assert_no_error(*error);
+/* Disable all certificate checks as our test setup is known to be invalid 
*/
+
g_tls_client_connection_set_validation_flags(G_TLS_CLIENT_CONNECTION(tls_connection),
 0);
+
+g_object_unref(connection);
+g_object_unref(client);
+
+return tls_connection;
+}
+
+
 static void check_magic(GIOStream *io_stream, GError **error)
 {
 uint8_t buffer[4];
@@ -125,6 +149,7 @@ static void check_magic(GIOStream *io_stream, GError 
**error)
 typedef struct
 {
 GSocketConnectable *connectable;
+bool use_tls;
 gpointer user_data;
 } ThreadData;
 
@@ -136,7 +161,11 @@ static gpointer check_magic_thread(gpointer data)
 TestEventLoop *event_loop = thread_data->user_data;
 GIOStream *stream;
 
-stream = fake_client_connect(connectable, );
+if (thread_data->use_tls) {
+stream = fake_client_connect_tls(connectable, );
+} else {
+stream = fake_client_connect(connectable, );
+}
 g_assert_no_error(error);
 check_magic(stream, );
 g_assert_no_error(error);
@@ -173,6 +202,7 @@ static gpointer check_no_connect_thread(gpointer data)
 
 static GThread *fake_client_new(GThreadFunc thread_func,
 const char *hostname, int port,
+bool use_tls,
 gpointer user_data)
 {
 ThreadData *thread_data = g_new0(ThreadData, 1);
@@ -180,6 +210,7 @@ static GThread *fake_client_new(GThreadFunc thread_func,
 g_assert_cmpuint(port, >, 0);
 g_assert_cmpuint(port, <, 65536);
 thread_data->connectable = g_network_address_new(hostname, port);
+thread_data->use_tls = use_tls;
 thread_data->user_data = user_data;
 
 /* check_magic_thread will assume ownership of 'connectable' */
@@ -204,7 +235,74 @@ static void test_connect_plain(void)
 g_assert_cmpint(result, ==, 0);
 
 /* fake client */
-thread = fake_client_new(check_magic_thread, "localhost", 5701, 
_loop);
+thread = fake_client_new(check_magic_thread, "localhost", 5701, false, 
_loop);
+test_event_loop_run(_loop);
+g_assert_null(g_thread_join(thread));
+
+test_event_loop_destroy(_loop);
+spice_server_destroy(server);
+}
+
+static void test_connect_tls(void)
+{
+GThread *thread;
+int result;
+
+TestEventLoop event_loop = { 0, };
+
+test_event_loop_init(_loop);
+
+/* server */
+SpiceServer *server = spice_server_new();
+spice_server_set_name(server, "SPICE listen test");
+spice_server_set_noauth(server);
+result = spice_server_set_tls(server, 5701,
+  PKI_DIR "ca-cert.pem",
+  PKI_DIR "server-cert.pem",
+  PKI_DIR "server-key.pem",
+  NULL, NULL, NULL);
+g_assert_cmpint(result, ==, 0);
+result = spice_server_init(server, event_loop.core);
+g_assert_cmpint(result, ==, 0);
+
+/* fake client */
+thread = fake_client_new(check_magic_thread, "localhost", 5701, true, 
_loop);
+test_event_loop_run(_loop);
+g_assert_null(g_thread_join(thread));
+
+test_event_loop_destroy(_loop);
+spice_server_destroy(server);
+}
+
+static void test_connect_both(void)
+{
+GThread *thread;
+int result;
+
+TestEventLoop event_loop = { 0, };
+
+test_event_loop_init(_loop);
+
+/* server */
+SpiceServer *server = spice_server_new();
+spice_server_set_name(server, "SPICE listen test");
+spice_server_set_noauth(server);
+spice_server_set_port(server, 5701);
+result = spice_server_set_tls(server, 5702,
+  PKI_DIR "ca-cert.pem",
+  PKI_DIR "server-cert.pem",
+  PKI_DIR "server-key.pem",
+  NULL, NULL, 

Re: [Spice-devel] [spice-streaming-agent v2] build: Don't use -fvisibility when building the agent

[Frediano Ziglio]

> 
> In my testing (x86_64/gcc), this had no impact on the resulting binary,
> building with/without it gives the same stripped binary save for its
> buildid.
> The check for -fvisibility in configure.ac is kept as this is going to
> be useful when we ship dlopen'ed plugins.
> 
> Signed-off-by: Christophe Fergeau 
> ---
> Here is what the patch would look like if we keep the configure.ac check
> for future usage.
> 
> Christophe
> 
> 
>  src/Makefile.am | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3717b5c..857d763 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -16,12 +16,10 @@ AM_CPPFLAGS = \
>   $(NULL)
>  
>  AM_CFLAGS = \
> - $(VISIBILITY_HIDDEN_CFLAGS) \
>   $(WARN_CFLAGS) \
>   $(NULL)
>  
>  AM_CXXFLAGS = \
> - $(VISIBILITY_HIDDEN_CFLAGS) \
>   $(WARN_CXXFLAGS) \
>   $(NULL)
>  

Acked-by: Frediano Ziglio 

Not sure if with -fPIE and x86 this won't change slightly.

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

[Spice-devel] [PATCH spice-server] build: Rename spice-server-enums.tmpl.[ch] to spice-server-enums.[ch].tmpl

[Eduardo Lima (Etrunko)]

This is a preparation for meson build, which has built-in support for
generating enums, but requires the template files to be renamed. It uses
the basename of template files to generate the output, and in this case
it would be the same file for both '.c' and '.h'.

Reference http://mesonbuild.com/Gnome-module.html#gnomemkenums

Signed-off-by: Eduardo Lima (Etrunko) 
---
 server/Makefile.am   | 12 ++--
 .../{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} |  0
 .../{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} |  0
 3 files changed, 6 insertions(+), 6 deletions(-)
 rename server/{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} (100%)
 rename server/{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} (100%)

diff --git a/server/Makefile.am b/server/Makefile.am
index 5d5590af..c1f241ac 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -201,11 +201,11 @@ endif
 libspice_server_la_LIBADD = libserver.la
 libspice_server_la_SOURCES =
 
-spice-server-enums.c: spice-server.h spice-server-enums.tmpl.c
-   $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.c 
$< > $@
+spice-server-enums.c: spice-server.h spice-server-enums.c.tmpl
+   $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.c.tmpl 
$< > $@
 
-spice-server-enums.h: spice-server.h spice-server-enums.tmpl.h
-   $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.h 
$< > $@
+spice-server-enums.h: spice-server.h spice-server-enums.h.tmpl
+   $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.h.tmpl 
$< > $@
 
 EXTRA_DIST =   \
spice-bitmap-utils.tmpl.c   \
@@ -213,8 +213,8 @@ EXTRA_DIST =\
glz-encode-match.tmpl.c \
glz-encode.tmpl.c   \
spice-server.syms   \
-   spice-server-enums.tmpl.h   \
-   spice-server-enums.tmpl.c   \
+   spice-server-enums.h.tmpl   \
+   spice-server-enums.c.tmpl   \
$(NULL)
 
 BUILT_SOURCES = $(spice_built_sources)
diff --git a/server/spice-server-enums.tmpl.c b/server/spice-server-enums.c.tmpl
similarity index 100%
rename from server/spice-server-enums.tmpl.c
rename to server/spice-server-enums.c.tmpl
diff --git a/server/spice-server-enums.tmpl.h b/server/spice-server-enums.h.tmpl
similarity index 100%
rename from server/spice-server-enums.tmpl.h
rename to server/spice-server-enums.h.tmpl
-- 
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-server] build: Rename spice-server-enums.tmpl.[ch] to spice-server-enums.[ch].tmpl

[Christophe Fergeau]

On Tue, Mar 06, 2018 at 11:46:33AM -0300, Eduardo Lima (Etrunko) wrote:
> This is a preparation for meson build, which has built-in support for
> generating enums, but requires the template files to be renamed. It uses
> the basename of template files to generate the output, and in this case
> it would be the same file for both '.c' and '.h'.
> 
> Reference http://mesonbuild.com/Gnome-module.html#gnomemkenums

Hmm the generated files which should have the same base name are
currently spice-server-enums.c and spice-server-enums.h, so this is ok,
and from the link you gave, 'spice-server-enums' would be the first arg
to gnome.mkenums().
Then the link you give lists 2 separate arguments for the templates, c_template
and h_template.
So after reading the link, I'm not sure why this patch is needed?

Christophe

> 
> Signed-off-by: Eduardo Lima (Etrunko) 
> ---
>  server/Makefile.am   | 12 
> ++--
>  .../{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} |  0
>  .../{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} |  0
>  3 files changed, 6 insertions(+), 6 deletions(-)
>  rename server/{spice-server-enums.tmpl.c => spice-server-enums.c.tmpl} (100%)
>  rename server/{spice-server-enums.tmpl.h => spice-server-enums.h.tmpl} (100%)
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 5d5590af..c1f241ac 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -201,11 +201,11 @@ endif
>  libspice_server_la_LIBADD = libserver.la
>  libspice_server_la_SOURCES =
>  
> -spice-server-enums.c: spice-server.h spice-server-enums.tmpl.c
> - $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.c 
> $< > $@
> +spice-server-enums.c: spice-server.h spice-server-enums.c.tmpl
> + $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.c.tmpl 
> $< > $@
>  
> -spice-server-enums.h: spice-server.h spice-server-enums.tmpl.h
> - $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.tmpl.h 
> $< > $@
> +spice-server-enums.h: spice-server.h spice-server-enums.h.tmpl
> + $(AM_V_GEN)glib-mkenums --template $(srcdir)/spice-server-enums.h.tmpl 
> $< > $@
>  
>  EXTRA_DIST = \
>   spice-bitmap-utils.tmpl.c   \
> @@ -213,8 +213,8 @@ EXTRA_DIST =  \
>   glz-encode-match.tmpl.c \
>   glz-encode.tmpl.c   \
>   spice-server.syms   \
> - spice-server-enums.tmpl.h   \
> - spice-server-enums.tmpl.c   \
> + spice-server-enums.h.tmpl   \
> + spice-server-enums.c.tmpl   \
>   $(NULL)
>  
>  BUILT_SOURCES = $(spice_built_sources)
> diff --git a/server/spice-server-enums.tmpl.c 
> b/server/spice-server-enums.c.tmpl
> similarity index 100%
> rename from server/spice-server-enums.tmpl.c
> rename to server/spice-server-enums.c.tmpl
> diff --git a/server/spice-server-enums.tmpl.h 
> b/server/spice-server-enums.h.tmpl
> similarity index 100%
> rename from server/spice-server-enums.tmpl.h
> rename to server/spice-server-enums.h.tmpl
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

[Christophe Fergeau]

On Tue, Mar 06, 2018 at 11:38:15AM -0500, Frediano Ziglio wrote:
> Looks fine.
> Actually when this function returns -1 spice_server_init returns -1 and Qemu
> calls exit so there's no much difference at runtime.
> But I suppose this is necessary for your tests.

I did not try the tests without this patch, it's something I noticed
before starting writing them. Maybe the tests are fine without this.

Christophe


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

Re: [Spice-devel] [PATCH 06/22] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe Fergeau]

On Thu, Mar 01, 2018 at 09:01:37PM +0100, Christophe de Dinechin wrote:
> > On 28 Feb 2018, at 17:36, Christophe Fergeau  wrote:
> > 
> > My understanding is that the previous iteration was quite controversial,
> > I would just drop it from the series unless you get acks from everyone
> > involved this time.
> 
> It’s a bit difficult to drop that from the series, as it is a core element of 
> the next steps if you look carefully.

I only looked at the code with the full series applied, but it really seems 
like both way would
be possible?

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index fe8564f..2e1472a 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -140,7 +140,10 @@ public:
 }
 void write_message_body(Stream , unsigned w, unsigned h, uint8_t c)
 {
-StreamMsgFormat msg = { .width = w, .height = h, .codec = c, .padding1 
= {} };
+StreamMsgFormat msg;
+msg.width = w;
+msg.height = h;
+msg.codec = c;
 stream.write_all("format", , sizeof(msg));
 }
 };
diff --git a/src/message.hpp b/src/message.hpp
index fd69033..674e122 100644
--- a/src/message.hpp
+++ b/src/message.hpp
@@ -21,13 +21,12 @@ class Message
 public:
 template 
 Message(PayloadArgs... payload_args)
-: hdr(StreamDevHeader {
-  .protocol_version = STREAM_DEVICE_PROTOCOL,
-  .padding = 0, // Workaround GCC bug "sorry: not implemented"
-  .type = Type,
-  .size = (uint32_t) Info::size(payload_args...)
-  })
-{ }
+{
+hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
+hdr.padding = 0;
+hdr.type = Type;
+hdr.size = (uint32_t) Info::size(payload_args...);
+}
 void write_header(Stream )
 {
 stream.write_all("header", , sizeof(hdr));

Not strongly advocating for that change to be made just now, I was just
a bit surprised by how you dismissed this ;)

Christophe


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

Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

[Frediano Ziglio]

> 
> Currently if we fail to set up the watch waiting for accept() to be
> called on the socket, we still keep the network socket open even if we
> are not going to be able to use it. This commit makes sure it's closed a
> set to -1 when such a failure occurs.
> ---
>  server/reds.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index a31ed4e96..7867e8573 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2595,6 +2595,8 @@ static int reds_init_net(RedsState *reds)
>   SPICE_WATCH_EVENT_READ,
>   reds_accept, reds);
>  if (reds->listen_watch == NULL) {
> +close(reds->listen_socket);
> +reds->listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }
> @@ -2610,6 +2612,8 @@ static int reds_init_net(RedsState *reds)
>  
> SPICE_WATCH_EVENT_READ,
>  
> reds_accept_ssl_connection,
>  reds);
>  if (reds->secure_listen_watch == NULL) {
> +close(reds->secure_listen_socket);
> +reds->secure_listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }
> @@ -2621,6 +2625,7 @@ static int reds_init_net(RedsState *reds)
>   SPICE_WATCH_EVENT_READ,
>   reds_accept, reds);
>  if (reds->listen_watch == NULL) {
> +reds->listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }

Looks fine.
Actually when this function returns -1 spice_server_init returns -1 and Qemu
calls exit so there's no much difference at runtime.
But I suppose this is necessary for your tests.

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

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

[Jonathon Jongsma]

On Tue, 2018-03-06 at 11:02 +0100, Christophe Fergeau wrote:
> On Fri, Mar 02, 2018 at 06:06:32PM +0100, Christophe de Dinechin
> wrote:
> > > FWIW, I also find myself in agreement with Frediano and Lukas on
> > > this point.
> > 
> > OK. Maybe I misunderstood something.
> > 
> > Do we agree that the case Frediano raised is someone trying:
> > 
> > throw Error(“The value of “ + std::to_string(2) + “ does not
> > match “ + std::to_string(1));
> > 
> > which yields the following error:
> > 
> > spice-streaming-agent.cpp:165:93: error: no matching function
> > for call to
> > ‘spice::streaming_agent::Error::Error(std::__cxx11::basic_string > ar>)’
> > 
> > So far, I think the interface was not “error prone”. It caught the
> > error correctly.
> > 
> > Therefore, in order to find the interface “error prone”, one has to
> > accept a second hypothesis, which is that whoever attempted that,
> > after seeing that error message, looks at the interface, which has
> > a ‘const char *’ and a comment that specifically states "It is
> > recommended to only use static C strings, since formatting of any
> > dynamic argument is done by ‘format_message’”, and after reading
> > that comment, has a big “Aha” moment and writes:
> > 
> > throw Error((“The value of “ + std::to_string(2) + “ does not
> > match “ + std::to_string(1)).c_str());
> > 
> > I’m sorry, but I cannot accept that step of the reasoning as being
> > even remotely valid. Just looking at the code makes me want to
> > barf.
> > 
> > So either I misunderstood Frediano’s point, or the reasoning is
> > deeply flawed in a not very subtle way.
> 
> It's not just about .c_str(), it's really about any API returning a
> non-static string which will be freed before the exception is caught
> (think RAII-like cases, wrappers around preexisting C APIs, ...).
> I already mentioned it, but an API taking a const char *, and also
> mandating that this const char * must be alive as long as the
> instance
> I invoked the method from is really unusual/unexpected to me, and
> would
> thus be error-prone.
> 
> Christophe


A simple concrete example:
--

#include 
#include 
#include 

class Error final : public std::exception
{
public:
Error(const char *message): exception(), message(message) { }
virtual const char *what() const noexcept override { return
message; }
protected:
const char *message;
};


void do_it(int x)
{
std::string foo("Hello, world");
// do some other stuff...
if (x == 1)
throw Error(foo.c_str()); // dangerous
else if (x == 2)
throw std::runtime_error(foo.c_str()); // fine

std::cout << "run without any arguments for Error, or with 1
argument for runtime_error";
}

int main(int argc, char** argv)
{
try {
do_it(argc);
} catch (Error& ex) {
std::cout << "Error: " << ex.what() << std::endl;
} catch (std::runtime_error& ex) {
std::cout << "runtime_error: " << ex.what() << std::endl;
}

return 0;
}


--

The fact that the interface is exactly the same as runtime_error but it
behaves differently is almost the textbook definition of error-prone,
IMO.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent v5 2/2] Implement handling of error messages from the server

[Christophe Fergeau]

On Mon, Mar 05, 2018 at 05:58:15PM +0100, Christophe de Dinechin wrote:
> > Perhaps you lost the mails saying that the protocol structure don't and 
> > won't have internal padding.
> 
> Only on x86. It has padding on any ABI with a natural 64-bit alignment.
> 
> I don’t have an Itanium handy, but computing the offsetof(msg.msg) and
> offsetof(msg.StreamNotifyError::msg) on a Raspberry Pi using
> -mstructure-size-boundary=64 yields:
> 
> offsetof msg.msg=8
> offsetof msg.StreamNotifyError::msg=4

For what it's worth, at the moment stream-device.h expects that its
struct members are going to be "naturally aligned", ie that struct
members are aligned to a multiple of their size in byte within the
struct. This seems to be the case with the ABIs/arch we are interested
in. If there is a real-world ABI which does not match this, then we'll
indeed need to adapt stream-device.h assumptions.
The rest of spice-protocol is using __attribute__("packed"). The
disadvantage with that is that you have to do some manual handling of
unaligned accesses on some arches (older 32 bits ARM come to mind).

Christophe


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