Re: [Spice-devel] [PATCH 2/3] SPICE port implementation

2016-09-22 Thread Jeremy White 
Hey Oliver,

Thanks for this patch; it broadly looks good to me, with one question:

On 09/20/2016 09:30 AM, Oliver Gutierrez wrote:
> +this.portName = new TextDecoder('utf-8').decode(new 
> Uint8Array(m.name));

TextDecoder is considered experimental, and is not supported by IE.
Could this be implemented a different way?

Cheers,

Jeremy


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

Re: [Spice-devel] [PATCH 1/3] Cleaning of trailing whitespaces

2016-09-22 Thread Jeremy White 
Acked-by:  Jeremy White 


On 09/20/2016 09:30 AM, Oliver Gutierrez wrote:
> ---
>  enums.js|  2 +-
>  main.js |  2 +-
>  spice.html  | 40 
>  spice_auto.html |  2 +-
>  spicemsg.js |  6 +++---
>  5 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/enums.js b/enums.js
> index 3ef36dc..301fea0 100644
> --- a/enums.js
> +++ b/enums.js
> @@ -264,7 +264,7 @@ var SPICE_MOUSE_BUTTON_MASK_LEFT = (1 << 0),
>  SPICE_MOUSE_BUTTON_MASK_MIDDLE = (1 << 1),
>  SPICE_MOUSE_BUTTON_MASK_RIGHT = (1 << 2),
>  SPICE_MOUSE_BUTTON_MASK_MASK = 0x7;
> -
> +
>  var SPICE_MOUSE_BUTTON_INVALID  = 0;
>  var SPICE_MOUSE_BUTTON_LEFT = 1;
>  var SPICE_MOUSE_BUTTON_MIDDLE   = 2;
> diff --git a/main.js b/main.js
> index afe69bf..874a038 100644
> --- a/main.js
> +++ b/main.js
> @@ -22,7 +22,7 @@
>  **  SpiceMainConn
>  **  This is the master Javascript class for establishing and
>  **  managing a connection to a Spice Server.
> -**  
> +**
>  **  Invocation:  You must pass an object with properties as follows:
>  **  uri (required)  Uri of a WebSocket listener that is
>  **  connected to a spice server.
> diff --git a/spice.html b/spice.html
> index f2f9ed0..c473678 100644
> --- a/spice.html
> +++ b/spice.html
> @@ -28,26 +28,26 @@
>  
>  
>  Spice Javascript client
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> - 
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>  
>  
>  
> - 
> +
>  
>  
>  
> @@ -71,8 +71,8 @@
>  {
>  var host, port, password, scheme = "ws://", uri;
>  
> -host = document.getElementById("host").value; 
> -port = document.getElementById("port").value; 
> +host = document.getElementById("host").value;
> +port = document.getElementById("port").value;
>  password = document.getElementById("password").value;
>  
>  
> @@ -92,7 +92,7 @@
>  
>  try
>  {
> -sc = new SpiceMainConn({uri: uri, screen_id: 
> "spice-screen", dump_id: "debug-div", 
> +sc = new SpiceMainConn({uri: uri, screen_id: 
> "spice-screen", dump_id: "debug-div",
>  message_id: "message-div", password: 
> password, onerror: spice_error, onagent: agent_connected });
>  }
>  catch (e)
> diff --git a/spice_auto.html b/spice_auto.html
> index 9aae118..1179ebe 100644
> --- a/spice_auto.html
> +++ b/spice_auto.html
> @@ -28,7 +28,7 @@
>  
>  
>  Spice Javascript client
> - 
> +
>  
>  
>  
> diff --git a/spicemsg.js b/spicemsg.js
> index db6625a..0321cc7 100644
> --- a/spicemsg.js
> +++ b/spicemsg.js
> @@ -21,7 +21,7 @@
>  
> /*
>  **  Spice messages
>  **  This file contains classes for passing messages to and from
> -**  a spice server.  This file should arguably be generated from 
> +**  a spice server.  This file should arguably be generated from
>  **  spice.proto, but it was instead put together by hand.
>  
> **--*/
>  function SpiceLinkHeader(a, at)
> @@ -63,7 +63,7 @@ SpiceLinkHeader.prototype =
>  dv.setUint32(at, this.size, true); at += 4;
>  },
>  buffer_size: function()
> -{ 
> +{
>  return 16;
>  },
>  }
> @@ -938,7 +938,7 @@ function SpiceMsgcMousePosition(sc, e)
>  this.x = e.clientX - 
> sc.display.surfaces[sc.display.primary_surface].canvas.offsetLeft + 
> scrollLeft;
>  this.y = e.clientY - 
> sc.display.surfaces[sc.display.primary_surface].canvas.offsetTop + scrollTop;
>  sc.mousex = this.x;
> -sc.mousey = this.y; 
> +sc.mousey = this.y;
>  }
>  else
>  {
> 

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

[Spice-devel] [PATCH 1/2] Split RedCharDeviceSmartcard and SmartCardChannelClient

2016-09-22 Thread Frediano Ziglio 
---
 server/Makefile.am|   2 +
 server/smartcard-channel-client.c | 300 +++
 server/smartcard-channel-client.h |  96 ++
 server/smartcard.c| 360 ++
 server/smartcard.h|  21 +++
 5 files changed, 471 insertions(+), 308 deletions(-)
 create mode 100644 server/smartcard-channel-client.c
 create mode 100644 server/smartcard-channel-client.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 9c5b3b6..abbec16 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -172,6 +172,8 @@ if HAVE_SMARTCARD
 libserver_la_SOURCES +=\
smartcard.c \
smartcard.h \
+   smartcard-channel-client.c  \
+   smartcard-channel-client.h  \
$(NULL)
 endif
 
diff --git a/server/smartcard-channel-client.c 
b/server/smartcard-channel-client.c
new file mode 100644
index 000..0622be5
--- /dev/null
+++ b/server/smartcard-channel-client.c
@@ -0,0 +1,300 @@
+/*
+Copyright (C) 2009-2015 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 "smartcard-channel-client.h"
+
+typedef struct RedErrorItem {
+RedPipeItem base;
+VSCMsgHeader vheader;
+VSCMsgError  error;
+} RedErrorItem;
+
+uint8_t *smartcard_channel_client_alloc_msg_rcv_buf(RedChannelClient *rcc,
+uint16_t type,
+uint32_t size)
+{
+SmartCardChannelClient *scc = SMARTCARD_CHANNEL_CLIENT(rcc);
+RedClient *client = red_channel_client_get_client(rcc);
+
+/* todo: only one reader is actually supported. When we fix the code to 
support
+ * multiple readers, we will porbably associate different devices to
+ * differenc channels */
+if (!scc->priv->smartcard) {
+scc->priv->msg_in_write_buf = FALSE;
+return spice_malloc(size);
+} else {
+RedCharDeviceSmartcard *smartcard;
+
+spice_assert(smartcard_get_n_readers() == 1);
+smartcard = scc->priv->smartcard;
+spice_assert(smartcard_char_device_get_client(smartcard) || 
scc->priv->smartcard);
+spice_assert(!scc->priv->write_buf);
+scc->priv->write_buf =
+red_char_device_write_buffer_get(RED_CHAR_DEVICE(smartcard), 
client,
+ size);
+
+if (!scc->priv->write_buf) {
+spice_error("failed to allocate write buffer");
+return NULL;
+}
+scc->priv->msg_in_write_buf = TRUE;
+return scc->priv->write_buf->buf;
+}
+}
+
+void smartcard_channel_client_release_msg_rcv_buf(RedChannelClient *rcc,
+  uint16_t type,
+  uint32_t size,
+  uint8_t *msg)
+{
+SmartCardChannelClient *scc = SMARTCARD_CHANNEL_CLIENT(rcc);
+
+/* todo: only one reader is actually supported. When we fix the code to 
support
+ * multiple readers, we will porbably associate different devices to
+ * differenc channels */
+
+if (!scc->priv->msg_in_write_buf) {
+spice_assert(!scc->priv->write_buf);
+free(msg);
+} else {
+if (scc->priv->write_buf) { /* msg hasn't been pushed to the guest */
+spice_assert(scc->priv->write_buf->buf == msg);
+
red_char_device_write_buffer_release(RED_CHAR_DEVICE(scc->priv->smartcard),
+ >priv->write_buf);
+}
+}
+}
+
+void smartcard_channel_client_on_disconnect(RedChannelClient *rcc)
+{
+SmartCardChannelClient *scc = SMARTCARD_CHANNEL_CLIENT(rcc);
+RedCharDeviceSmartcard *device = scc->priv->smartcard;
+
+if (device) {
+smartcard_char_device_detach_client(device, scc);
+smartcard_char_device_notify_reader_remove(device);
+}
+}
+
+void smartcard_channel_client_send_data(RedChannelClient *rcc,
+SpiceMarshaller *m,
+RedPipeItem *item,
+VSCMsgHeader *vheader)
+{
+spice_assert(rcc);
+spice_assert(vheader);
+

[Spice-devel] [PATCH 0/2] Split "Convert RedChannelClient hierarchy to GObject" patch

2016-09-22 Thread Frediano Ziglio 
Split SmartCard.
Probably some additional changes should be removed from second patch.

Frediano Ziglio (2):
  Split RedCharDeviceSmartcard and SmartCardChannelClient
  Convert RedChannelClient hierarchy to GObject

 configure.ac|   4 +-
 server/Makefile.am  |   4 +
 server/cursor-channel-client.c  |  74 ++--
 server/cursor-channel-client.h  |  34 +-
 server/dcc-private.h|  14 +-
 server/dcc.c| 202 --
 server/dcc.h|  37 +-
 server/display-channel.c|   4 +-
 server/dummy-channel-client.c   | 156 
 server/dummy-channel-client.h   |  59 +++
 server/inputs-channel-client.c  |  56 ++-
 server/inputs-channel-client.h  |  47 ++-
 server/main-channel-client.c| 146 +++-
 server/main-channel-client.h|  34 +-
 server/red-channel-client-private.h |   4 +-
 server/red-channel-client.c | 714 ++--
 server/red-channel-client.h |  80 +++-
 server/red-channel.h|  33 +-
 server/reds.h   |   1 +
 server/smartcard-channel-client.c   | 414 +
 server/smartcard-channel-client.h   | 108 ++
 server/smartcard.c  | 376 +++
 server/smartcard.h  |  21 ++
 server/sound.c  |   9 +-
 server/spice-server.h   |  16 +
 server/spicevmc.c   |   6 +-
 server/tests/test_display_base.c|   4 +-
 27 files changed, 1887 insertions(+), 770 deletions(-)
 create mode 100644 server/dummy-channel-client.c
 create mode 100644 server/dummy-channel-client.h
 create mode 100644 server/smartcard-channel-client.c
 create mode 100644 server/smartcard-channel-client.h

-- 
2.7.4

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

[Spice-devel] [PATCH 2/2] Convert RedChannelClient hierarchy to GObject

2016-09-22 Thread Frediano Ziglio 
Convert the RedChannelClient heirarchy into GObjects. Since the existing
constructors could fail and return NULL, I inherited the base channel
client from GInitable, which introduces a dependency on gio.

When using private structs with GObject, there's a maximum size of (I
think) 64k, which was exceeded by some of the private structs. To avoid
this limitation I changed some members to dynamically allocated.
---
 configure.ac|   4 +-
 server/Makefile.am  |   2 +
 server/cursor-channel-client.c  |  74 ++--
 server/cursor-channel-client.h  |  34 +-
 server/dcc-private.h|  14 +-
 server/dcc.c| 202 --
 server/dcc.h|  37 +-
 server/display-channel.c|   4 +-
 server/dummy-channel-client.c   | 156 
 server/dummy-channel-client.h   |  59 +++
 server/inputs-channel-client.c  |  56 ++-
 server/inputs-channel-client.h  |  47 ++-
 server/main-channel-client.c| 146 +++-
 server/main-channel-client.h|  34 +-
 server/red-channel-client-private.h |   4 +-
 server/red-channel-client.c | 714 ++--
 server/red-channel-client.h |  80 +++-
 server/red-channel.h|  33 +-
 server/reds.h   |   1 +
 server/smartcard-channel-client.c   | 120 +-
 server/smartcard-channel-client.h   |  42 ++-
 server/smartcard.c  |  30 +-
 server/sound.c  |   9 +-
 server/spice-server.h   |  16 +
 server/spicevmc.c   |   6 +-
 server/tests/test_display_base.c|   4 +-
 26 files changed, 1441 insertions(+), 487 deletions(-)
 create mode 100644 server/dummy-channel-client.c
 create mode 100644 server/dummy-channel-client.h

diff --git a/configure.ac b/configure.ac
index f8284f6..483f18b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -147,8 +147,8 @@ SPICE_PROTOCOL_MIN_VER=0.12.12
 PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 
$SPICE_PROTOCOL_MIN_VER])
 AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 
-PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.22])
-AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 2.22"])
+PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.22 gio-2.0 >= 2.22])
+AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 2.22 gio-2.0 >= 2.22"])
 
 PKG_CHECK_MODULES([GOBJECT2], [gobject-2.0 >= 2.22])
 AS_VAR_APPEND([SPICE_REQUIRES], [" gobject-2.0 >= 2.22"])
diff --git a/server/Makefile.am b/server/Makefile.am
index abbec16..f217399 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -100,6 +100,8 @@ libserver_la_SOURCES =  \
red-channel-client.c\
red-channel-client.h\
red-channel-client-private.h\
+   dummy-channel-client.c  \
+   dummy-channel-client.h  \
red-common.h\
dispatcher.c\
dispatcher.h\
diff --git a/server/cursor-channel-client.c b/server/cursor-channel-client.c
index 89c23a3..51fef19 100644
--- a/server/cursor-channel-client.c
+++ b/server/cursor-channel-client.c
@@ -40,7 +40,10 @@ enum {
 RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
 };
 
-typedef struct CursorChannelClientPrivate CursorChannelClientPrivate;
+G_DEFINE_TYPE(CursorChannelClient, cursor_channel_client, 
RED_TYPE_CHANNEL_CLIENT)
+
+#define CURSOR_CHANNEL_CLIENT_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o), 
TYPE_CURSOR_CHANNEL_CLIENT, CursorChannelClientPrivate))
+
 struct CursorChannelClientPrivate
 {
 RedCacheItem *cursor_cache[CURSOR_CACHE_HASH_SIZE];
@@ -49,12 +52,28 @@ struct CursorChannelClientPrivate
 uint32_t cursor_cache_items;
 };
 
-struct CursorChannelClient
+static void cursor_channel_client_constructed(GObject *object)
 {
-RedChannelClient base;
+G_OBJECT_CLASS(cursor_channel_client_parent_class)->constructed(object);
+}
 
-CursorChannelClientPrivate priv[1];
-};
+static void
+cursor_channel_client_class_init(CursorChannelClientClass *klass)
+{
+GObjectClass *object_class = G_OBJECT_CLASS(klass);
+
+g_type_class_add_private(klass, sizeof(CursorChannelClientPrivate));
+
+object_class->constructed = cursor_channel_client_constructed;
+}
+
+static void
+cursor_channel_client_init(CursorChannelClient *self)
+{
+self->priv = CURSOR_CHANNEL_CLIENT_PRIVATE(self);
+ring_init(>priv->cursor_cache_lru);
+self->priv->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
+}
 
 #define CLIENT_CURSOR_CACHE
 #include "cache-item.tmpl.c"
@@ -90,28 +109,35 @@ CursorChannelClient* 
cursor_channel_client_new(CursorChannel *cursor, RedClient
uint32_t *common_caps, int 
num_common_caps,
uint32_t *caps, int num_caps)
 {
-spice_return_val_if_fail(cursor, NULL);
-

Re: [Spice-devel] [PATCH v3] replay: Update pointer in allocated list

2016-09-22 Thread Jonathon Jongsma 
Not sure whether I should ACK since it's basically my patch, but

Acked-by: Jonathon Jongsma 



On Thu, 2016-09-22 at 09:29 +0100, Frediano Ziglio wrote:
> Avoid to free invalid pointer.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> Changes since v2:
> - better encapsulation (Jonathon)
> 
> I have the sensation that Quic code is broken.. but this is
> not related to this patch.
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index e95cf91..b5baded 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -113,6 +113,13 @@ static inline void replay_free(SpiceReplay
> *replay, void *mem)
>  free(mem);
>  }
>  
> +static inline void *replay_realloc(SpiceReplay *replay, void *mem,
> size_t n_bytes)
> +{
> +GList *elem = g_list_find(replay->allocated, mem);
> +elem->data = spice_realloc(mem, n_bytes);
> +return elem->data;
> +}
> +
>  static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
>  {
>  uint32_t newid = 0;
> @@ -486,8 +493,8 @@ static QXLImage *red_replay_image(SpiceReplay
> *replay, uint32_t flags)
>  if (replay->error) {
>  return NULL;
>  }
> -qxl = realloc(qxl, sizeof(QXLImageDescriptor) +
> sizeof(QXLQUICData) +
> -  qxl->quic.data_size);
> +qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
> + sizeof(QXLQUICData) +
> + qxl->quic.data_size);
>  size = red_replay_data_chunks(replay, "quic.data",
> (uint8_t**)>quic.data, 0);
>  spice_assert(size == qxl->quic.data_size);
>  break;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] SPICE android library

2016-09-22 Thread Sören Busse 
Hi,


is there a android library available?

I haven't found any it yet.


Regards

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

Re: [Spice-devel] [PATCH v2] Remove unused fields

2016-09-22 Thread Jonathon Jongsma 
Acked-by: Jonathon Jongsma 


On Thu, 2016-09-22 at 09:19 +0100, Frediano Ziglio wrote:
> These fields were added in a32e90257e834e340075e633132b52c612be4578
> as part of the multiple client support and were never used.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.h | 2 --
>  server/red-worker.h  | 1 -
>  2 files changed, 3 deletions(-)
> 
> Changes since v1:
> - added comment (Jonathon)
> 
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 3da2b5f..36633d5 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -55,8 +55,6 @@ struct Drawable {
>  RingItem list_link;
>  DrawItem tree_item;
>  GList *pipes;
> -RedPipeItem *pipe_item_rest;
> -uint32_t size_pipe_item_rest;
>  RedDrawable *red_drawable;
>  
>  GlzImageRetention glz_retention;
> diff --git a/server/red-worker.h b/server/red-worker.h
> index a0a327a..7d68678 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -35,7 +35,6 @@ typedef struct CommonGraphicsChannel {
>  
>  QXLInstance *qxl;
>  uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> -uint32_t id_alloc; // bitfield. TODO - use this instead of shift
> scheme.
>  int during_target_migrate; /* TRUE when the client that is
> associated with the channel
>    is during migration. Turned off
> when the vm is started.
>    The flag is used to avoid sending
> messages that are artifacts
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2] Small comment on structure checking

2016-09-22 Thread Jonathon Jongsma 
On Thu, 2016-09-22 at 09:55 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Changes since v1:
> - improved comment
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 8b949a2..a85c091 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1985,6 +1985,8 @@ void
> display_channel_process_surface_cmd(DisplayChannel *display,
> RedSurfaceCmd
>  }
>  data = surface->u.surface_create.data;
>  if (stride < 0) {
> +/* no worry for overflow here, command should already be
> validated
> + * when is readed, specifically red_get_surface_cmd */
>  data -= (int32_t)(stride * (height - 1));
>  }
>  display_channel_create_surface(display, surface_id, surface-
> >u.surface_create.width,


OK, but slight re-wording:

"No need to worry about overflow..."
"...when it is read, ..."

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

Re: [Spice-devel] [PATCH v2] Revert if to make code more readable

2016-09-22 Thread Uri Lublin 

On 09/20/2016 10:05 AM, Frediano Ziglio wrote:

Keep all code to send SPICE_MSG_LIST together.

Signed-off-by: Frediano Ziglio 


Ack.


---
 server/dcc-send.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

Changed since v1:
- moved comment

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 1d05c68..32c6f17 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -283,14 +283,15 @@ static void send_free_list(RedChannelClient *rcc)
 dcc_pixmap_cache_hit(dcc, dcc->priv->send_data.pixmap_cache_items[i], 
);
 }

-if (free_list->wait.header.wait_count) {
-red_channel_client_init_send_data(rcc, SPICE_MSG_LIST, NULL);
-} else { /* only one message, no need for a list */
+if (!free_list->wait.header.wait_count) {
+/* only one message, no need for a list */
 red_channel_client_init_send_data(rcc, SPICE_MSG_DISPLAY_INVAL_LIST, 
NULL);
 spice_marshall_msg_display_inval_list(urgent_marshaller, 
free_list->res);
 return;
 }

+red_channel_client_init_send_data(rcc, SPICE_MSG_LIST, NULL);
+
 inval_m = spice_marshaller_get_submarshaller(urgent_marshaller);
 marshal_sub_msg_inval_list(inval_m, free_list);




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

[Spice-devel] [PATCH v2] Remove unused fields

2016-09-22 Thread Frediano Ziglio 
These fields were added in a32e90257e834e340075e633132b52c612be4578
as part of the multiple client support and were never used.

Signed-off-by: Frediano Ziglio 
---
 server/display-channel.h | 2 --
 server/red-worker.h  | 1 -
 2 files changed, 3 deletions(-)

Changes since v1:
- added comment (Jonathon)

diff --git a/server/display-channel.h b/server/display-channel.h
index 3da2b5f..36633d5 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -55,8 +55,6 @@ struct Drawable {
 RingItem list_link;
 DrawItem tree_item;
 GList *pipes;
-RedPipeItem *pipe_item_rest;
-uint32_t size_pipe_item_rest;
 RedDrawable *red_drawable;
 
 GlzImageRetention glz_retention;
diff --git a/server/red-worker.h b/server/red-worker.h
index a0a327a..7d68678 100644
--- a/server/red-worker.h
+++ b/server/red-worker.h
@@ -35,7 +35,6 @@ typedef struct CommonGraphicsChannel {
 
 QXLInstance *qxl;
 uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
-uint32_t id_alloc; // bitfield. TODO - use this instead of shift scheme.
 int during_target_migrate; /* TRUE when the client that is associated with 
the channel
   is during migration. Turned off when the vm 
is started.
   The flag is used to avoid sending messages 
that are artifacts
-- 
2.7.4

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

Re: [Spice-devel] [PATCH 7/8] Remove unused fields

2016-09-22 Thread Jonathon Jongsma 
I'd like to add a comment to the commit log explaining some of the
history. It looks like these fields were added in
a32e90257e834e340075e633132b52c612be4578 as part of the multiple client
support and were never used. 


Acked-by: Jonathon Jongsma 



On Mon, 2016-09-19 at 09:30 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.h | 2 --
>  server/red-worker.h  | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 7b71480..cf4135d 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -55,8 +55,6 @@ struct Drawable {
>  RingItem list_link;
>  DrawItem tree_item;
>  Ring pipes;
> -RedPipeItem *pipe_item_rest;
> -uint32_t size_pipe_item_rest;
>  RedDrawable *red_drawable;
>  
>  GlzImageRetention glz_retention;
> diff --git a/server/red-worker.h b/server/red-worker.h
> index a0a327a..7d68678 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -35,7 +35,6 @@ typedef struct CommonGraphicsChannel {
>  
>  QXLInstance *qxl;
>  uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> -uint32_t id_alloc; // bitfield. TODO - use this instead of shift
> scheme.
>  int during_target_migrate; /* TRUE when the client that is
> associated with the channel
>    is during migration. Turned off
> when the vm is started.
>    The flag is used to avoid sending
> messages that are artifacts
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Capitalize option documentation

2016-09-22 Thread Uri Lublin 

On 09/21/2016 06:31 PM, Frediano Ziglio wrote:

All other options are documented using initial capital case letter.

Signed-off-by: Frediano Ziglio 


Ack.


---
 server/tests/replay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 86a7bab..467a344 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -322,7 +322,7 @@ int main(int argc, char **argv)
 { "port", 'p', 0, G_OPTION_ARG_INT, , "Server port (default 5000)", 
"PORT" },
 { "wait", 'w', 0, G_OPTION_ARG_NONE, , "Wait for client", NULL },
 { "slow", 's', 0, G_OPTION_ARG_INT, , "Slow down replay. Delays USEC 
microseconds before each command", "USEC" },
-{ "skip", 0, 0, G_OPTION_ARG_INT, , "skip 'slow' for the first n 
commands", NULL },
+{ "skip", 0, 0, G_OPTION_ARG_INT, , "Skip 'slow' for the first n 
commands", NULL },
 { "count", 0, 0, G_OPTION_ARG_NONE, _count, "Print the number of 
commands processed", NULL },
 { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, , "replay file", 
"FILE" },
 { NULL }



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

Re: [Spice-devel] [PATCH 6/8] worker: Do not check surface twice

2016-09-22 Thread Jonathon Jongsma 
Looks fine, but we could retain the warning with additional
explanation.  For example (proposed):

diff --git a/server/red-worker.c b/server/red-worker.c
index 62e5d86..46f562f 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -662,8 +662,10 @@ static void destroy_primary_surface(RedWorker
*worker, uint32_t surface_id)
 {
 DisplayChannel *display = worker->display_channel;
 
-if (!display_channel_validate_surface(display, surface_id))
+if (!display_channel_validate_surface(display, surface_id)) {
+spice_warning("double destroy of primary surface");
 return;
+}
 spice_warn_if_fail(surface_id == 0);
 
 spice_debug(NULL);



Acked-by: Jonathon Jongsma 



On Mon, 2016-09-19 at 09:30 +0100, Frediano Ziglio wrote:
> validate_surface already do the same checks.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-worker.c | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 590412b..e39bd84 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -693,12 +693,6 @@ static void destroy_primary_surface(RedWorker
> *worker, uint32_t surface_id)
>  return;
>  spice_warn_if_fail(surface_id == 0);
>  
> -spice_debug(NULL);
> -if (!display->surfaces[surface_id].context.canvas) {
> -spice_warning("double destroy of primary surface");
> -return;
> -}
> -
>  flush_all_qxl_commands(worker);
>  display_channel_destroy_surface_wait(display, 0);
>  display_channel_surface_unref(display, 0);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] Convert RedChannelClient heirarchy to GObject

2016-09-22 Thread Frediano Ziglio 
> 
> Convert the RedChannelClient heirarchy into GObjects. Since the existing
> constructors could fail and return NULL, I inherited the base channel
> client from GInitable, which introduces a dependency on gio.
> 
> When using private structs with GObject, there's a maximum size of (I
> think) 64k, which was exceeded by some of the private structs. To avoid
> this limitation I changed some members to dynamically allocated.

Why not handling the priv memory management manually (g_new/g_free)
instead? This would require less changes.

I think the first comment however is that this patch is too big
and do too much stuff.

The dummy channel is promoted as a new class. I was looking some
days ago (for other reasons) at the dummy and RedChannelClient
creation and it looks that mainly the difference between is that
in the dummy the RedStream parameter is NULL. I though that the 2
function could be merged quite easily and in this case the difference
between the two would just some couple of lines.
Why this "feature" is needed? Looks like only sound use it but what
does it means? Sound channels are not connected to a client? Perhaps
RedStream should have different implementations instead of RedChannel?

The smartchannel separation should be done in a separate patch.

red-channel-client-private.h should be included only by red-channel-client.c.

I'll try to split some stuff.

Frediano


> ---
>  configure.ac|   4 +-
>  server/Makefile.am  |   4 +
>  server/cursor-channel-client.c  |  74 ++--
>  server/cursor-channel-client.h  |  34 +-
>  server/dcc-private.h|  14 +-
>  server/dcc.c| 202 --
>  server/dcc.h|  37 +-
>  server/display-channel.c|   4 +-
>  server/dummy-channel-client.c   | 157 
>  server/dummy-channel-client.h   |  59 +++
>  server/inputs-channel-client.c  |  56 ++-
>  server/inputs-channel-client.h  |  47 ++-
>  server/main-channel-client.c| 146 +++-
>  server/main-channel-client.h|  34 +-
>  server/red-channel-client-private.h |   4 +-
>  server/red-channel-client.c | 717
>  ++--
>  server/red-channel-client.h |  80 +++-
>  server/red-channel.h|  33 +-
>  server/reds.h   |   1 +
>  server/smartcard-channel-client.c   | 414 +
>  server/smartcard-channel-client.h   | 114 ++
>  server/smartcard.c  | 376 +++
>  server/smartcard.h  |  21 ++
>  server/sound.c  |   9 +-
>  server/spice-server.h   |  16 +
>  server/spicevmc.c   |   6 +-
>  server/tests/test_display_base.c|   4 +-
>  27 files changed, 1895 insertions(+), 772 deletions(-)
>  create mode 100644 server/dummy-channel-client.c
>  create mode 100644 server/dummy-channel-client.h
>  create mode 100644 server/smartcard-channel-client.c
>  create mode 100644 server/smartcard-channel-client.h
> 


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

Re: [Spice-devel] [PATCH 5/8] Simplify serial sending packets

2016-09-22 Thread Jonathon Jongsma 
Acked-by: Jonathon Jongsma 


On Mon, 2016-09-19 at 09:30 +0100, Frediano Ziglio wrote:
> serial was the future serial to send while last_sent_serial was the
> last sent.
> serial sent started from 1.
> To make sure sequence variable is updated just before sending the
> message, not every message prepared.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client-private.h |  1 -
>  server/red-channel-client.c | 31 +
> --
>  2 files changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/server/red-channel-client-private.h b/server/red-
> channel-client-private.h
> index 532bfa3..c169f62 100644
> --- a/server/red-channel-client-private.h
> +++ b/server/red-channel-client-private.h
> @@ -43,7 +43,6 @@ struct RedChannelClientPrivate
>  uint32_t size;
>  RedPipeItem *item;
>  int blocked;
> -uint64_t serial;
>  uint64_t last_sent_serial;
>  
>  struct {
> diff --git a/server/red-channel-client.c b/server/red-channel-
> client.c
> index 4108be0..1c2fb15 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -166,26 +166,9 @@ static void
> red_channel_client_reset_send_data(RedChannelClient *rcc)
>  rcc->priv->send_data.header.set_msg_type(>priv-
> >send_data.header, 0);
>  rcc->priv->send_data.header.set_msg_size(>priv-
> >send_data.header, 0);
>  
> -/* Keeping the serial consecutive: resetting it if
> reset_send_data
> - * has been called before, but no message has been sent since
> then.
> - */
> -if (rcc->priv->send_data.last_sent_serial != rcc->priv-
> >send_data.serial) {
> -spice_assert(rcc->priv->send_data.serial - rcc->priv-
> >send_data.last_sent_serial == 1);
> -/*  When the urgent marshaller is active, the serial was
> incremented by
> - *  the call to reset_send_data that was made for the main
> marshaller.
> - *  The urgent msg receives this serial, and the main msg
> serial is
> - *  the following one. Thus, (rcc->priv->send_data.serial -
> rcc->priv->send_data.last_sent_serial)
> - *  should be 1 in this case*/
> -if (!red_channel_client_urgent_marshaller_is_active(rcc)) {
> -rcc->priv->send_data.serial = rcc->priv-
> >send_data.last_sent_serial;
> -}
> -}
> -rcc->priv->send_data.serial++;
> -
>  if (!rcc->priv->is_mini_header) {
>  spice_assert(rcc->priv->send_data.marshaller != rcc->priv-
> >send_data.urgent.marshaller);
>  rcc->priv->send_data.header.set_msg_sub_list(>priv-
> >send_data.header, 0);
> -rcc->priv->send_data.header.set_msg_serial(>priv-
> >send_data.header, rcc->priv->send_data.serial);
>  }
>  }
>  
> @@ -304,10 +287,6 @@ static void
> red_channel_client_restore_main_sender(RedChannelClient *rcc)
>  spice_marshaller_reset(rcc->priv->send_data.urgent.marshaller);
>  rcc->priv->send_data.marshaller = rcc->priv-
> >send_data.main.marshaller;
>  rcc->priv->send_data.header.data = rcc->priv-
> >send_data.main.header_data;
> -if (!rcc->priv->is_mini_header) {
> -rcc->priv->send_data.header.set_msg_serial(>priv-
> >send_data.header,
> -   rcc->priv-
> >send_data.serial);
> -}
>  rcc->priv->send_data.item = rcc->priv->send_data.main.item;
>  }
>  
> @@ -589,7 +568,7 @@ static void
> full_header_set_msg_serial(SpiceDataHeaderOpaque *header, uint64_t s
>  
>  static void mini_header_set_msg_serial(SpiceDataHeaderOpaque
> *header, uint64_t serial)
>  {
> -spice_error("attempt to set header serial on mini header");
> +/* ignore serial, not supported by mini header */
>  }
>  
>  static void full_header_set_msg_sub_list(SpiceDataHeaderOpaque
> *header, uint32_t sub_list)
> @@ -1254,8 +1233,9 @@ void
> red_channel_client_begin_send_message(RedChannelClient *rcc)
>  rcc->priv->send_data.header.set_msg_size(>priv-
> >send_data.header,
>   rcc->priv-
> >send_data.size -
>   rcc->priv-
> >send_data.header.header_size);
> +rcc->priv->send_data.header.set_msg_serial(>priv-
> >send_data.header,
> +   ++rcc->priv-
> >send_data.last_sent_serial);
>  rcc->priv->ack_data.messages_window++;
> -rcc->priv->send_data.last_sent_serial = rcc->priv-
> >send_data.serial;
>  rcc->priv->send_data.header.data = NULL; /* avoid writing to
> this until we have a new message */
>  red_channel_client_send(rcc);
>  }
> @@ -1275,13 +1255,12 @@ SpiceMarshaller
> *red_channel_client_switch_to_urgent_sender(RedChannelClient *rc
>  
>  uint64_t red_channel_client_get_message_serial(RedChannelClient
> *rcc)
>  {
> -return rcc->priv->send_data.serial;
> +return rcc->priv->send_data.last_sent_serial + 1;
>  }
>  
>  void

[Spice-devel] [PATCH v2] Small comment on structure checking

2016-09-22 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/display-channel.c | 2 ++
 1 file changed, 2 insertions(+)

Changes since v1:
- improved comment

diff --git a/server/display-channel.c b/server/display-channel.c
index 8b949a2..a85c091 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1985,6 +1985,8 @@ void display_channel_process_surface_cmd(DisplayChannel 
*display, RedSurfaceCmd
 }
 data = surface->u.surface_create.data;
 if (stride < 0) {
+/* no worry for overflow here, command should already be validated
+ * when is readed, specifically red_get_surface_cmd */
 data -= (int32_t)(stride * (height - 1));
 }
 display_channel_create_surface(display, surface_id, 
surface->u.surface_create.width,
-- 
2.7.4

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

Re: [Spice-devel] [PATCH 8/8] Small comment on structure checking

2016-09-22 Thread Jonathon Jongsma 
On Mon, 2016-09-19 at 09:30 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 108e69b..3290565 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1968,6 +1968,7 @@ void
> display_channel_process_surface_cmd(DisplayChannel *display,
> RedSurfaceCmd
>  }
>  data = surface->u.surface_create.data;
>  if (stride < 0) {
> +/* no worry for overflow here, command is already
> validated */
>  data -= (int32_t)(stride * (height - 1));
>  }
>  display_channel_create_surface(display, surface_id, surface-
> >u.surface_create.width,


Hmm, it doesn't look like the command has been validated in *this*
function. 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v3] replay: Update pointer in allocated list

2016-09-22 Thread Frediano Ziglio 
Avoid to free invalid pointer.

Signed-off-by: Frediano Ziglio 
---
 server/red-replay-qxl.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

Changes since v2:
- better encapsulation (Jonathon)

I have the sensation that Quic code is broken.. but this is
not related to this patch.

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index e95cf91..b5baded 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -113,6 +113,13 @@ static inline void replay_free(SpiceReplay *replay, void 
*mem)
 free(mem);
 }
 
+static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t 
n_bytes)
+{
+GList *elem = g_list_find(replay->allocated, mem);
+elem->data = spice_realloc(mem, n_bytes);
+return elem->data;
+}
+
 static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
 {
 uint32_t newid = 0;
@@ -486,8 +493,8 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 if (replay->error) {
 return NULL;
 }
-qxl = realloc(qxl, sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
-  qxl->quic.data_size);
+qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+ qxl->quic.data_size);
 size = red_replay_data_chunks(replay, "quic.data", 
(uint8_t**)>quic.data, 0);
 spice_assert(size == qxl->quic.data_size);
 break;
-- 
2.7.4

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

Re: [Spice-devel] [PATCH] Capitalize option documentation

2016-09-22 Thread Jonathon Jongsma 
Acked-by: Jonathon Jongsma 


On Wed, 2016-09-21 at 16:31 +0100, Frediano Ziglio wrote:
> All other options are documented using initial capital case letter.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/tests/replay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index 86a7bab..467a344 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -322,7 +322,7 @@ int main(int argc, char **argv)
>  { "port", 'p', 0, G_OPTION_ARG_INT, , "Server port
> (default 5000)", "PORT" },
>  { "wait", 'w', 0, G_OPTION_ARG_NONE, , "Wait for
> client", NULL },
>  { "slow", 's', 0, G_OPTION_ARG_INT, , "Slow down
> replay. Delays USEC microseconds before each command", "USEC" },
> -{ "skip", 0, 0, G_OPTION_ARG_INT, , "skip 'slow' for
> the first n commands", NULL },
> +{ "skip", 0, 0, G_OPTION_ARG_INT, , "Skip 'slow' for
> the first n commands", NULL },
>  { "count", 0, 0, G_OPTION_ARG_NONE, _count, "Print the
> number of commands processed", NULL },
>  { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY,
> , "replay file", "FILE" },
>  { NULL }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel