[Spice-devel] [PATCH win-vdagent v2] Don't exit when receiving unknown messages

[Jonathon Jongsma]

In 8251fa25, a check on the minimum size of a message was introduced.
For unsupported messages, the vdagent simply exited. This makes it
inconsistent with previous behavior and inconsistent with the behavior
of the linux vdagent.  Instead, just print a warning indicating that an
unsupported message was received and ignore it.

Signed-off-by: Jonathon Jongsma 
---
changes in v2:
 - change rationale in commit log

 vdagent/vdagent.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index 89019bb..177e663 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -1288,8 +1288,7 @@ void VDAgent::dispatch_message(VDAgentMessage* msg, 
uint32_t port)
 break;
 }
 if (min_size < 0) {
-vd_printf("Unsupported message type %u size %u", msg->type, msg->size);
-_running = false;
+vd_printf("Unsupported message type %u size %u, ignoring", msg->type, 
msg->size);
 return;
 }
 if (msg->size < (unsigned) min_size) {
-- 
2.17.2

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

[Spice-devel] [PATCH spice-server v2 3/3] Switch some boolean fields to 'bool' type

[Jonathon Jongsma]

For coding style consistency, use 'bool' when we want to represent a
boolean value.

Signed-off-by: Jonathon Jongsma 
---
Changes in v2:
 - new patch


 server/reds.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 07562b555..db93acd16 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -231,9 +231,9 @@ typedef enum {
 } VDIPortReadStates;
 
 struct RedCharDeviceVDIPortPrivate {
-gboolean agent_attached;
+bool agent_attached;
 uint32_t plug_generation;
-int client_agent_started;
+bool client_agent_started;
 bool agent_supports_graphics_device_info;
 
 /* write to agent */
@@ -486,7 +486,7 @@ static void reds_reset_vdp(RedsState *reds)
  * to be sent from the client. This TODO will require server, protocol, 
and client changes */
 dev->priv->write_filter.result = AGENT_MSG_FILTER_DISCARD;
 dev->priv->write_filter.discard_all = TRUE;
-dev->priv->client_agent_started = FALSE;
+dev->priv->client_agent_started = false;
 dev->priv->agent_supports_graphics_device_info = false;
 
 /*  The client's tokens are set once when the main channel is initialized
@@ -1151,7 +1151,7 @@ void reds_on_main_agent_start(RedsState *reds, 
MainChannelClient *mcc, uint32_t
 spice_assert(reds->vdagent->st && reds->vdagent->st == dev_state);
 rcc = RED_CHANNEL_CLIENT(mcc);
 client = red_channel_client_get_client(rcc);
-reds->agent_dev->priv->client_agent_started = TRUE;
+reds->agent_dev->priv->client_agent_started = true;
 /*
  * Note that in older releases, send_tokens were set to ~0 on both client
  * and server. The server ignored the client given tokens.
@@ -3156,7 +3156,7 @@ static RedCharDevice *attach_to_red_agent(RedsState 
*reds, SpiceCharDeviceInstan
 RedCharDeviceVDIPort *dev = reds->agent_dev;
 SpiceCharDeviceInterface *sif;
 
-dev->priv->agent_attached = TRUE;
+dev->priv->agent_attached = true;
 red_char_device_reset_dev_instance(RED_CHAR_DEVICE(dev), sin);
 
 reds->vdagent = sin;
-- 
2.17.2

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

[Spice-devel] [PATCH spice-server v2 1/3] Refactor agent_adjust_capabilities() function

[Jonathon Jongsma]

Make this a RedsState member function rather than a standalone function.
This means that we simply pass RedsState* as an argument rather than the
internal member variables of RedsState. This enables the following
commit which handles the VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability to
avoid sending graphics device info to agents that do not support it.

Signed-off-by: Jonathon Jongsma 
---
 server/reds.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 2e5c69e60..63bfadb22 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -793,8 +793,9 @@ static void vdi_port_read_buf_free(RedPipeItem *base)
 g_free(buf);
 }
 
-static void agent_adjust_capabilities(VDAgentMessage *message,
-  bool clipboard_enabled, bool 
xfer_enabled)
+/* certain agent capabilities can be overridden and disabled in the server. In 
these cases, unset
+ * these capabilities before sending them on to the client */
+static void reds_adjust_agent_capabilities(RedsState *reds, VDAgentMessage 
*message)
 {
 VDAgentAnnounceCapabilities *capabilities;
 
@@ -803,13 +804,13 @@ static void agent_adjust_capabilities(VDAgentMessage 
*message,
 }
 capabilities = (VDAgentAnnounceCapabilities *) message->data;
 
-if (!clipboard_enabled) {
+if (!reds->config->agent_copypaste) {
 VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, VD_AGENT_CAP_CLIPBOARD);
 VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, 
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND);
 VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, 
VD_AGENT_CAP_CLIPBOARD_SELECTION);
 }
 
-if (!xfer_enabled) {
+if (!reds->config->agent_file_xfer) {
 VD_AGENT_SET_CAPABILITY(capabilities->caps, 
VD_AGENT_CAP_FILE_XFER_DISABLED);
 }
 }
@@ -879,9 +880,7 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(RedCharDevice *self,
 }
 switch (vdi_port_read_buf_process(dev, dispatch_buf)) {
 case AGENT_MSG_FILTER_OK:
-agent_adjust_capabilities((VDAgentMessage *) 
dispatch_buf->data,
-  reds->config->agent_copypaste,
-  reds->config->agent_file_xfer);
+reds_adjust_agent_capabilities(reds, (VDAgentMessage *) 
dispatch_buf->data);
 return _buf->base;
 case AGENT_MSG_FILTER_PROTO_ERROR:
 reds_agent_remove(reds);
@@ -1380,9 +1379,7 @@ void reds_on_main_channel_migrate(RedsState *reds, 
MainChannelClient *mcc)
 read_buf->len = read_data_len;
 switch (vdi_port_read_buf_process(agent_dev, read_buf)) {
 case AGENT_MSG_FILTER_OK:
-agent_adjust_capabilities((VDAgentMessage *)read_buf->data,
-  reds->config->agent_copypaste,
-  reds->config->agent_file_xfer);
+reds_adjust_agent_capabilities(reds, (VDAgentMessage 
*)read_buf->data);
 main_channel_client_push_agent_data(mcc,
 read_buf->data,
 read_buf->len,
-- 
2.17.2

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

[Spice-devel] [PATCH spice-server v2 2/3] Only send device display info to supported agents

[Jonathon Jongsma]

Only send the graphics device display info to agents that advertise the
VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability

Signed-off-by: Jonathon Jongsma 
---
Changes in v2:
 - renamed member variable
 - moved variable to RedCharDeviceVDIPortPrivate
 - reset variable when agent disconnects
 - make sure the message is sent immediately when we receive the capabilities.

NOTE: I did not change the function name as requested to 
filter_agent_capabilities because there is
already a agent message filter which determines whether a message should be 
handled or discarded,
etc. Adding another 'filter' concept would confuse things. I think "adjust_" 
seems fine to me.

 server/reds.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index 63bfadb22..07562b555 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -234,6 +234,7 @@ struct RedCharDeviceVDIPortPrivate {
 gboolean agent_attached;
 uint32_t plug_generation;
 int client_agent_started;
+bool agent_supports_graphics_device_info;
 
 /* write to agent */
 RedCharDeviceWriteBuffer *recv_from_client_buf;
@@ -486,6 +487,7 @@ static void reds_reset_vdp(RedsState *reds)
 dev->priv->write_filter.result = AGENT_MSG_FILTER_DISCARD;
 dev->priv->write_filter.discard_all = TRUE;
 dev->priv->client_agent_started = FALSE;
+dev->priv->agent_supports_graphics_device_info = false;
 
 /*  The client's tokens are set once when the main channel is initialized
  *  and once upon agent's connection with 
SPICE_MSG_MAIN_AGENT_CONNECTED_TOKENS.
@@ -813,6 +815,11 @@ static void reds_adjust_agent_capabilities(RedsState 
*reds, VDAgentMessage *mess
 if (!reds->config->agent_file_xfer) {
 VD_AGENT_SET_CAPABILITY(capabilities->caps, 
VD_AGENT_CAP_FILE_XFER_DISABLED);
 }
+
+size_t caps_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message->size);
+reds->agent_dev->priv->agent_supports_graphics_device_info =
+VD_AGENT_HAS_CAPABILITY(capabilities->caps, caps_size, 
VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
+reds_send_device_display_info(reds);
 }
 
 /* reads from the device till completes reading a message that is addressed to 
the client,
@@ -965,6 +972,10 @@ void reds_send_device_display_info(RedsState *reds)
 if (!reds->agent_dev->priv->agent_attached) {
 return;
 }
+if (!reds->agent_dev->priv->agent_supports_graphics_device_info) {
+return;
+}
+
 g_debug("Sending device display info to the agent:");
 
 SpiceMarshaller *m = spice_marshaller_new();
-- 
2.17.2

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

Re: [Spice-devel] [PATCH spice-server v4 01/20] Use proper format strings for spice_log

[Marc-André Lureau]

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Formatting string should be compatible with GLib.
> GLib uses formatting types compatible with GNU.
> For Linux this is not an issue as both systems (like a printf) and
> GLib one uses the same formatting type.  However on Windows they
> differs potentially causing issues.
> This is also make worse as GLib 2.58 changed format attribute from
> __printf__ to gnu_printf (Microsoft compatibility formats like %I64d
> are still supported but you'll get warnings using GCC/Clang
> compilers).
>
> Signed-off-by: Frediano Ziglio 

Reviewed-by: Marc-André Lureau 


> ---
>  server/char-device.c | 3 ++-
>  server/gstreamer-encoder.c   | 4 ++--
>  server/main-channel-client.c | 6 +++---
>  server/memslot.c | 2 +-
>  server/mjpeg-encoder.c   | 6 --
>  server/red-channel.c | 6 --
>  server/red-client.c  | 6 --
>  server/red-replay-qxl.c  | 9 +
>  server/reds.c| 4 ++--
>  9 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 64b41a94..c00e96ef 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -786,7 +786,8 @@ void red_char_device_client_remove(RedCharDevice *dev,
>  }
>
>  if (dev->priv->clients == NULL) {
> -spice_debug("client removed, memory pool will be freed (%"PRIu64" 
> bytes)", dev->priv->cur_pool_size);
> +spice_debug("client removed, memory pool will be freed 
> (%"G_GUINT64_FORMAT" bytes)",
> +dev->priv->cur_pool_size);
>  write_buffers_queue_free(>priv->write_bufs_pool);
>  dev->priv->cur_pool_size = 0;
>  }
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 04f0c02f..972c86e6 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1080,10 +1080,10 @@ static void set_gstenc_bitrate(SpiceGstEncoder 
> *encoder)
>  break;
>  }
>  default:
> -spice_warning("the %s property has an unsupported type %zu",
> +spice_warning("the %s property has an unsupported type %" 
> G_GSIZE_FORMAT,
>prop, param->value_type);
>  }
> -spice_debug("setting the GStreamer %s to %"PRIu64, prop, gst_bit_rate);
> +spice_debug("setting the GStreamer %s to %"G_GUINT64_FORMAT, prop, 
> gst_bit_rate);
>  }
>
>  /* A helper for spice_gst_encoder_encode_frame() */
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 54be9934..3b6ae269 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -529,8 +529,8 @@ void main_channel_client_handle_pong(MainChannelClient 
> *mcc, SpiceMsgPing *ping,
>  if (roundtrip <= mcc->priv->latency) {
>  // probably high load on client or server result with incorrect 
> values
>  red_channel_debug(red_channel_client_get_channel(rcc),
> -  "net test: invalid values, latency %" PRIu64
> -  " roundtrip %" PRIu64 ". assuming high"
> +  "net test: invalid values, latency %" 
> G_GUINT64_FORMAT
> +  " roundtrip %"G_GUINT64_FORMAT". assuming high"
>"bandwidth", mcc->priv->latency, roundtrip);
>  mcc->priv->latency = 0;
>  mcc->priv->net_test_stage = NET_TEST_STAGE_INVALID;
> @@ -542,7 +542,7 @@ void main_channel_client_handle_pong(MainChannelClient 
> *mcc, SpiceMsgPing *ping,
>  / (roundtrip - mcc->priv->latency);
>  mcc->priv->net_test_stage = NET_TEST_STAGE_COMPLETE;
>  red_channel_debug(red_channel_client_get_channel(rcc),
> -  "net test: latency %f ms, bitrate %"PRIu64" bps 
> (%f Mbps)%s",
> +  "net test: latency %f ms, bitrate 
> %"G_GUINT64_FORMAT" bps (%f Mbps)%s",
>(double)mcc->priv->latency / 1000,
>mcc->priv->bitrate_per_sec,
>(double)mcc->priv->bitrate_per_sec / 1024 / 1024,
> diff --git a/server/memslot.c b/server/memslot.c
> index 97311b2e..91ea53bd 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -105,7 +105,7 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL 
> addr, uint32_t add_size
>  slot_id = memslot_get_id(info, addr);
>  if (slot_id >= info->num_memslots) {
>  print_memslots(info);
> -spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr);
> +spice_critical("slot_id %d too big, addr=%" G_GINT64_MODIFIER "x", 
> slot_id, addr);
>  return NULL;
>  }
>
> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> index d4b5c6fc..d64fcfe0 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -614,7 +614,8 @@ static void 
> mjpeg_encoder_adjust_params_to_bit_rate(MJpegEncoder 

Re: [Spice-devel] [PATCH spice-server v4 02/20] build: Detect Windows build and change some definitions

[Marc-André Lureau]

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Windows needs some specific setting to use network.
>
> Signed-off-by: Frediano Ziglio 

autotools only?

Reviewed-by: Marc-André Lureau 


> ---
>  configure.ac | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 604a41b2..f8b41f37 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -68,6 +68,20 @@ case $host_cpu in
>  SPICE_WARNING([spice-server on non-x86_64 architectures has not been 
> extensively tested])
>  esac
>
> +AC_MSG_CHECKING([for native Win32])
> +case "$host_os" in
> + *mingw*|*cygwin*)
> +os_win32=yes
> +dnl AI_ADDRCONFIG and possibly some other code require at least Vista
> +AC_DEFINE([_WIN32_WINNT], [0x600], [Minimal Win32 version])]
> +;;
> + *)
> +os_win32=no
> +;;
> +esac
> +AC_MSG_RESULT([$os_win32])
> +AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"])
> +
>  dnl =
>  dnl Check optional features
>  SPICE_CHECK_SMARTCARD
> @@ -154,6 +168,9 @@ AC_CHECK_LIB(rt, clock_gettime, LIBRT="-lrt")
>  AC_SUBST(LIBRT)
>
>  AS_VAR_APPEND([SPICE_NONPKGCONFIG_LIBS], [" -pthread $LIBM $LIBRT"])
> +AS_IF([test "x$os_win32" = "xyes"], [
> +AS_VAR_APPEND([SPICE_NONPKGCONFIG_LIBS], [" -lws2_32"])
> +])
>
>  SPICE_REQUIRES=""
>
> @@ -176,7 +193,8 @@ 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])
> +AS_IF([test "x$os_win32" != "xyes"],
> +  PKG_CHECK_MODULES([GIO_UNIX], [gio-unix-2.0 >= $GLIB2_REQUIRED]))
>
>  PIXMAN_REQUIRED=0.17.7
>  PKG_CHECK_MODULES(PIXMAN, pixman-1 >= $PIXMAN_REQUIRED)
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-server v4 09/20] net-utils: Use socket compatibility layer

[Marc-André Lureau]

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Signed-off-by: Frediano Ziglio 

You could easily improve the commit message, as it doesn't seem to do
what it says.

> ---
>  server/net-utils.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/server/net-utils.c b/server/net-utils.c
> index 802509a4..ad66a328 100644
> --- a/server/net-utils.c
> +++ b/server/net-utils.c
> @@ -35,6 +35,7 @@
>  #include 
>
>  #include "net-utils.h"
> +#include "sys-socket.h"
>
>  /**
>   * red_socket_set_keepalive:
> @@ -101,6 +102,7 @@ bool red_socket_set_no_delay(int fd, bool no_delay)
>   */
>  bool red_socket_set_non_blocking(int fd, bool non_blocking)
>  {
> +#ifndef _WIN32
>  int flags;
>
>  if ((flags = fcntl(fd, F_GETFL)) == -1) {
> @@ -120,6 +122,15 @@ bool red_socket_set_non_blocking(int fd, bool 
> non_blocking)
>  }
>
>  return true;
> +#else

Please invert the condition blocks. The code added is specific to WIN32.

> +u_long ioctl_nonblocking = 1;
> +
> +if (ioctlsocket(fd, FIONBIO, _nonblocking) != 0) {
> +spice_warning("ioctlsocket(FIONBIO) failed, %d", WSAGetLastError());
> +return false;
> +}
> +return true;
> +#endif
>  }
>
>  /**
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-server v4 08/20] sys-socket: Add socket_newpair utility

[Marc-André Lureau]

Hi

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Allows to easier port socketpair.
> Windows does not have this function, we need to create a pair
> using 2 internet sockets and connecting one to the other.
>
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sys-socket.c | 75 +
>  server/sys-socket.h |  3 ++
>  2 files changed, 78 insertions(+)
>
> diff --git a/server/sys-socket.c b/server/sys-socket.c
> index 7ce5dab1..837da18e 100644
> --- a/server/sys-socket.c
> +++ b/server/sys-socket.c
> @@ -209,4 +209,79 @@ SPICE_CONSTRUCTOR_FUNC(socket_win32_init)
>  WSADATA wsaData;
>  WSAStartup(MAKEWORD(2, 2), );
>  }
> +
> +int socket_newpair(int type, int protocol, int sv[2])

overall, looks good.

Since it's limited to inet protocols, why not remove "protocol"
argument, and call it socket_new_inet_pair() ?

If you are using this for worker thread communication, a windows pipe
would be a better fit. (and GIO provides stream abstraction, wing has
a namedpipe implementation - based on the one we used to have in
spice-gtk for controller communication)

> +{
> +struct sockaddr_in sa, sa2;
> +socklen_t addrlen;
> +SOCKET s, pairs[2];
> +
> +if (!sv) {
> +return -1;
> +}
> +
> +/* create a listener */
> +s = socket(AF_INET, type, 0);
> +if (s == INVALID_SOCKET) {
> +return -1;
> +}
> +
> +pairs[1] = INVALID_SOCKET;
> +
> +pairs[0] = socket(AF_INET, type, 0);
> +if (pairs[0] == INVALID_SOCKET) {
> +goto cleanup;
> +}
> +
> +/* bind to a random port */
> +sa.sin_family = AF_INET;
> +sa.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> +sa.sin_port = 0;
> +if (bind(s, (struct sockaddr*) , sizeof(sa)) < 0) {
> +goto cleanup;
> +}
> +if (listen(s, 1) < 0) {
> +goto cleanup;
> +}
> +
> +/* connect to kernel choosen port */
> +addrlen = sizeof(sa);
> +if (getsockname(s, (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +if (connect(pairs[0], (struct sockaddr*) , sizeof(sa)) < 0) {
> +goto cleanup;
> +}
> +addrlen = sizeof(sa2);
> +pairs[1] = accept(s, (struct sockaddr*) , );
> +if (pairs[1] == INVALID_SOCKET) {
> +goto cleanup;
> +}
> +
> +/* check proper connection */
> +addrlen = sizeof(sa);
> +if (getsockname(pairs[0], (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +addrlen = sizeof(sa2);
> +if (getpeername(pairs[1], (struct sockaddr*) , ) < 0) {
> +goto cleanup;
> +}
> +if (sa.sin_family != sa2.sin_family || sa.sin_port != sa2.sin_port
> +|| sa.sin_addr.s_addr != sa2.sin_addr.s_addr) {
> +goto cleanup;
> +}
> +
> +closesocket(s);
> +sv[0] = pairs[0];
> +sv[1] = pairs[1];
> +return 0;
> +
> +cleanup:
> +socket_win32_set_errno();
> +closesocket(s);
> +closesocket(pairs[0]);
> +closesocket(pairs[1]);
> +return -1;
> +}
>  #endif
> diff --git a/server/sys-socket.h b/server/sys-socket.h
> index 65062571..3a3b7878 100644
> --- a/server/sys-socket.h
> +++ b/server/sys-socket.h
> @@ -134,6 +134,9 @@ socket_accept(int sock, struct sockaddr *addr, int 
> *addrlen)
>  }
>  #undef accept
>  #define accept socket_accept
> +
> +int socket_newpair(int type, int protocol, int sv[2]);
> +#define socketpair(family, type, protocol, sv) socket_newpair(type, 
> protocol, sv)
>  #endif
>
>  #endif // RED_SYS_SOCKET_H_
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-server v4 07/20] sys-socket: Introduce some utility to make sockets more portable

[Marc-André Lureau]

Hi
On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Between Unix and Windows socket are quite different:
> - on Windows sockets have a different namespace from C file
>   descriptors so you can't use read/write/close or similar functions;
> - errors are not stored in errno but you must be read/write the
>   errors with specific function;
> - sometimes sockets are put in non-blocking mode automatically
>   calling some functions;
> - SOCKET type is 64 bit on Windows 64 which does not fit technically
>   in an int. Is however safe to assume them to fit in an int.
>
> So encapsulate the socket APIs in some definition to make easier
> and more safe to deal with them.
> Where the portability to Windows would make to code more offuscated a Unix
> style was preferred. For instance if errors are detected errno is set from
> Windows socket error instead of changing all code handling.
> Fortunately on Windows Qemu core interface accepts socket (but not
> other types like C file descriptors!).
>
> Signed-off-by: Frediano Ziglio 

Reviewed-by: Marc-André Lureau 

> ---
>  server/Makefile.am  |   2 +
>  server/sys-socket.c | 212 
>  server/sys-socket.h | 139 +
>  3 files changed, 353 insertions(+)
>  create mode 100644 server/sys-socket.c
>  create mode 100644 server/sys-socket.h
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 34ec22ad..7f260612 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -166,6 +166,8 @@ libserver_la_SOURCES =  \
> stat.h  \
> stream-channel.c\
> stream-channel.h\
> +   sys-socket.h\
> +   sys-socket.c\
> red-stream-device.c \
> red-stream-device.h \
> sw-canvas.c \
> diff --git a/server/sys-socket.c b/server/sys-socket.c
> new file mode 100644
> index ..7ce5dab1
> --- /dev/null
> +++ b/server/sys-socket.c
> @@ -0,0 +1,212 @@
> +/* -*- 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 
> .
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include 
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#ifndef _WIN32
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#endif
> +
> +#include 
> +
> +#include "sys-socket.h"
> +
> +#ifdef _WIN32
> +// Map Windows socket errors to standard C ones
> +// See 
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
> +void socket_win32_set_errno(void)
> +{
> +int err = EPIPE; // default
> +switch (WSAGetLastError()) {
> +case WSAEWOULDBLOCK:
> +case WSAEINPROGRESS:
> +err = EAGAIN;
> +break;
> +case WSAEINTR:
> +err = EINTR;
> +break;
> +case WSAEBADF:
> +err = EBADF;
> +break;
> +case WSA_INVALID_HANDLE:
> +case WSA_INVALID_PARAMETER:
> +case WSAEINVAL:
> +err = EINVAL;
> +break;
> +case WSAENOTSOCK:
> +err = ENOTSOCK;
> +break;
> +case WSA_NOT_ENOUGH_MEMORY:
> +err = ENOMEM;
> +break;
> +case WSAEPROTONOSUPPORT:
> +case WSAESOCKTNOSUPPORT:
> +case WSAEOPNOTSUPP:
> +case WSAEPFNOSUPPORT:
> +case WSAEAFNOSUPPORT:
> +case WSAVERNOTSUPPORTED:
> +err = ENOTSUP;
> +break;
> +case WSAEFAULT:
> +err = EFAULT;
> +break;
> +case WSAEACCES:
> +err = EACCES;
> +break;
> +case WSAEMFILE:
> +err = EMFILE;
> +break;
> +case WSAENAMETOOLONG:
> +err = ENAMETOOLONG;
> +break;
> +case WSAENOTEMPTY:
> +err = ENOTEMPTY;
> +break;
> +case WSA_OPERATION_ABORTED:
> +case WSAECANCELLED:
> +case WSA_E_CANCELLED:
> +err = ECANCELED;
> +break;
> +case WSAEADDRINUSE:
> +err = EADDRINUSE;
> +break;
> +case WSAENETDOWN:
> +err = ENETDOWN;
> +break;
> +case WSAENETUNREACH:
> +err = ENETUNREACH;
> +break;
> +case 

Re: [Spice-devel] [PATCH spice-server v4 06/20] test-stat: Adjust delay checks

[Marc-André Lureau]

Hi

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> usleep under Windows does not seem to have the required precision.
> Use milliseconds and adjust check times according.
>
> Signed-off-by: Frediano Ziglio 

As discussed previously, g_usleep() would also be a good fit. I would
rather use it over the whole code base.

> ---
>  server/tests/stat-test.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/server/tests/stat-test.c b/server/tests/stat-test.c
> index e4a83f4f..444ff7e3 100644
> --- a/server/tests/stat-test.c
> +++ b/server/tests/stat-test.c
> @@ -57,13 +57,13 @@ void TEST_NAME(void)
>
>  stat_init(, "test", CLOCK_MONOTONIC);
>  stat_start_time_init(_time, );
> -usleep(2);
> +usleep(2000);
>  stat_add(, start_time);
>
>  #ifdef RED_WORKER_STAT
>  g_assert_cmpuint(info.count, ==, 1);
>  g_assert_cmpuint(info.min, ==, info.max);
> -g_assert_cmpuint(info.min, >=, 2000);
> +g_assert_cmpuint(info.min, >=, 200);
>  g_assert_cmpuint(info.min, <, 1);
>  #endif
>
> @@ -71,17 +71,17 @@ void TEST_NAME(void)
>
>  stat_compress_init(, "test", CLOCK_MONOTONIC);
>  stat_start_time_init(_time, );
> -usleep(2);
> +usleep(2000);
>  stat_compress_add(, start_time, 100, 50);
> -usleep(1);
> +usleep(1000);
>  stat_compress_add(, start_time, 1000, 500);
>
>  #ifdef COMPRESS_STAT
>  g_assert_cmpuint(info.count, ==, 2);
>  g_assert_cmpuint(info.min, !=, info.max);
> -g_assert_cmpuint(info.min, >=, 2000);
> +g_assert_cmpuint(info.min, >=, 200);
>  g_assert_cmpuint(info.min, <, 1);
> -g_assert_cmpuint(info.total, >=, 5000);
> +g_assert_cmpuint(info.total, >=, 500);
>  g_assert_cmpuint(info.orig_size, ==, 1100);
>  g_assert_cmpuint(info.comp_size, ==, 550);
>  #endif
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-server v4 04/20] windows: Undefine some conflicting preprocessor macros

[Marc-André Lureau]

Hi

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> "interface" and "MAX_MONITORS" are defined in some Windows system
> headers causing garbage code to be fed to the compiler.
>
> Signed-off-by: Frediano Ziglio 

To avoid the conflict, I suggest renaming those instead.

> ---
>  server/red-qxl.c | 4 
>  server/reds.c| 6 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 0dd26b22..9b9b8c17 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -40,6 +40,10 @@
>
>  #include "red-qxl.h"
>
> +#ifdef _WIN32
> +// undefine conflicting preprocessor macros
> +#undef interface
> +#endif
>
>  #define MAX_MONITORS_COUNT 16
>
> diff --git a/server/reds.c b/server/reds.c
> index 8c1c10dc..97023b38 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -79,6 +79,12 @@
>  #include "net-utils.h"
>  #include "red-stream-device.h"
>
> +#ifdef _WIN32
> +// undefine conflicting preprocessor macros
> +#undef MAX_MONITORS
> +#undef interface
> +#endif
> +
>  #define REDS_MAX_STAT_NODES 100
>
>  static void reds_client_monitors_config(RedsState *reds, 
> VDAgentMonitorsConfig *monitors_config);
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-server v4 03/20] Avoids %m in formatting for Windows

[Marc-André Lureau]

Hi

On Wed, Feb 6, 2019 at 2:59 PM Frediano Ziglio  wrote:
>
> Not supported, %m is a GNU extension of sscanf.
>
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index d3f73d8e..8c1c10dc 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3698,8 +3698,7 @@ static const int video_codec_caps[] = {
>   * @codec: a location to return the parsed codec
>   * @return the position of the next codec in the string
>   */
> -static const char* parse_next_video_codec(const char *codecs, char **encoder,
> -  char **codec)
> +static char* parse_next_video_codec(char *codecs, char **encoder, char 
> **codec)
>  {
>  if (!codecs) {
>  return NULL;
> @@ -3708,14 +3707,15 @@ static const char* parse_next_video_codec(const char 
> *codecs, char **encoder,
>  if (!*codecs) {
>  return NULL;
>  }
> -int n;
> +int end_encoder, end_codec = -1;
>  *encoder = *codec = NULL;
> -if (sscanf(codecs, "%m[0-9a-zA-Z_]:%m[0-9a-zA-Z_]%n", encoder, codec, 
> ) == 2) {
> -// this avoids accepting "encoder:codec" followed by garbage like 
> "$%*"
> -if (codecs[n] != ';' && codecs[n] != '\0') {
> -free(*codec);
> -*codec = NULL;
> -}
> +if (sscanf(codecs, "%*[0-9a-zA-Z_]:%n%*[0-9a-zA-Z_];%n", _encoder, 
> _codec) == 0
> +&& end_codec > 0) {
> +codecs[end_encoder - 1] = '\0';
> +codecs[end_codec - 1] = '\0';
> +*encoder = codecs;
> +*codec = codecs + end_encoder;
> +return codecs + end_codec;
>  }
>  return codecs + strcspn(codecs, ";");
>  }
> @@ -3732,7 +3732,8 @@ static void reds_set_video_codecs_from_string(RedsState 
> *reds, const char *codec
>  }
>
>  video_codecs = g_array_new(FALSE, FALSE, sizeof(RedVideoCodec));
> -const char *c = codecs;
> +char *codecs_copy = g_strdup_printf("%s;", codecs);
> +char *c = codecs_copy;
>  while ( (c = parse_next_video_codec(c, _name, _name)) ) {
>  uint32_t encoder_index, codec_index;
>  if (!encoder_name || !codec_name) {
> @@ -3755,11 +3756,9 @@ static void 
> reds_set_video_codecs_from_string(RedsState *reds, const char *codec
>  g_array_append_val(video_codecs, new_codec);
>  }
>
> -/* these are allocated by sscanf, do not use g_free */
> -free(encoder_name);
> -free(codec_name);
>  codecs = c;
>  }
> +g_free(codecs_copy);
>

codecs may refer to free memory after this, I believe.

Would be nice to factor this parsing code out of RedsState and make
some unit tests.


>  if (video_codecs->len == 0) {
>  spice_warning("Failed to set video codecs, input string: '%s'", 
> codecs);
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



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

Re: [Spice-devel] [PATCH spice-common 8/9] protocol: Add a dummy TunnelChannel

[Uri Lublin]

On 3/3/19 9:10 PM, Frediano Ziglio wrote:

The removal of the channel definition will cause the enumeration
to miss this old channel.
Add a dummy channel (empty) to avoid having to update spice-gtk
and spice-server and possibly breaking other software.


Hi Freidano,

This patch should be applied earlier, such that nothing
breaks when previous patches are applied.

Looks good to me.

Uri.



Signed-off-by: Frediano Ziglio 
---
  spice.proto | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/spice.proto b/spice.proto
index de80b27..1f9f57b 100644
--- a/spice.proto
+++ b/spice.proto
@@ -1235,6 +1235,9 @@ channel RecordChannel : BaseChannel {
  } @declare start_mark;
  };
  
+channel TunnelChannel {

+};
+
  enum32 vsc_message_type {
  Init = 1,
  Error,
@@ -1357,8 +1360,8 @@ protocol Spice {
  CursorChannel cursor;
  PlaybackChannel playback;
  RecordChannel record;
-// there used to be a TunnelChannel
-SmartcardChannel smartcard = 8;
+TunnelChannel tunnel;
+SmartcardChannel smartcard;
  UsbredirChannel usbredir;
  PortChannel port;
  WebDAVChannel webdav;



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

Re: [Spice-devel] [spice-gtk] usb-redir: use persistent libusb device on Windows

[Christophe Fergeau]

On Mon, Feb 25, 2019 at 04:07:03PM +0200, Yuri Benditovich wrote:
> On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau  
> wrote:
> > I don't think we should have a hard limit on the number of lines in a
> > patch, however there should be a maximum of 1 logical change per commit,
> > see 
> > https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/
> > for example for a rationale about this.
> > https://gitlab.freedesktop.org/teuf/spice-gtk/tree/gudev would be a
> > rough attempt at such a split (but authorship needs to be set back to
> > you, commit logs needs to be more verbose, ...).
> >
> 
> IMO, this is excellent example of why this approach is wrong.
> For example, there is definitely wrong commit
> https://gitlab.freedesktop.org/teuf/spice-gtk/commit/0135d831bc8929e45bac065f47daa1b4470ab7b0
> But nobody notice it as the problem in it fixed toward end of chain.

When I said "rough attempt at such a split", I meant it to be read as
"this is an untested proof of concept". So yes, there are bugs ;)

> Which tests are expected after _each_ commit?

Really depends on what the commit is doing. Each commit should build,
and basic functionality should still be working. I'd usually keep
extensive testing for the final patch, if some things are broken, I'd
move the fix to the right place, and possibly do more testing for the
commit which was broken.
In an ideal world, >95% of the testing would be automated, which would
make it trivial to test each commit..

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 spice-gtk 0/3] clipboard grab race

[Victor Toso]

Hi Jakub,

On Thu, Feb 28, 2019 at 08:12:41PM +0100, Jakub Janků wrote:
> Hi,
> 
> this is another try to solve the grab race.
> Intention of these patches is to make spice-gtk
> behave reasonably well with older agents.
> 
> The next step would be to fix the protocol itself.
> But that will require updating spice-gtk as well as vdagents.
> 
> Cheers,
> Jakub
> 
> Jakub Janků (3):
>   clipboard: accept grab only from the side with keyboard focus
>   clipboard: release on both sides if race occurs
>   clipboard: invalidate targets request when needed
> 
>  src/spice-gtk-session.c | 65 ++---
>  1 file changed, 54 insertions(+), 11 deletions(-)

I'm cc'ing elmarco for his thoughts on this series.

Cheers,
Victor


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