[Spice-devel] [PATCH 3/7] Move InputsChannelClient to a separate file

[Jonathon Jongsma]

Preparation for converting to GObject
---
 server/Makefile.am |  2 +
 server/inputs-channel-client.c | 87 ++
 server/inputs-channel-client.h | 48 +++
 server/inputs-channel.c| 80 +++---
 server/inputs-channel.h|  4 +-
 5 files changed, 163 insertions(+), 58 deletions(-)
 create mode 100644 server/inputs-channel-client.c
 create mode 100644 server/inputs-channel-client.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 5a5c7e8..cca3b9b 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -80,6 +80,8 @@ libserver_la_SOURCES =\
glz-encoder-priv.h  \
inputs-channel.c\
inputs-channel.h\
+   inputs-channel-client.c \
+   inputs-channel-client.h \
jpeg-encoder.c  \
jpeg-encoder.h  \
lz4-encoder.c   \
diff --git a/server/inputs-channel-client.c b/server/inputs-channel-client.c
new file mode 100644
index 000..f9dd6b2
--- /dev/null
+++ b/server/inputs-channel-client.c
@@ -0,0 +1,87 @@
+/*
+   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 "inputs-channel-client.h"
+#include "inputs-channel.h"
+#include "migration-protocol.h"
+
+struct InputsChannelClient {
+RedChannelClient base;
+uint16_t motion_count;
+};
+
+RedChannelClient* inputs_channel_client_create(RedChannel *channel,
+   RedClient *client,
+   RedsStream *stream,
+   int monitor_latency,
+   int num_common_caps,
+   uint32_t *common_caps,
+   int num_caps,
+   uint32_t *caps)
+{
+InputsChannelClient* icc =
+
(InputsChannelClient*)red_channel_client_create(sizeof(InputsChannelClient),
+channel, client,
+stream,
+monitor_latency,
+num_common_caps,
+common_caps, num_caps,
+caps);
+if (icc)
+icc->motion_count = 0;
+return >base;
+}
+
+void inputs_channel_client_send_migrate_data(RedChannelClient *rcc,
+ SpiceMarshaller *m,
+ RedPipeItem *item)
+{
+InputsChannelClient *icc = SPICE_CONTAINEROF(rcc, InputsChannelClient, 
base);
+InputsChannel *inputs = (InputsChannel*)rcc->channel;
+
+inputs_channel_set_src_during_migrate(inputs, FALSE);
+red_channel_client_init_send_data(rcc, SPICE_MSG_MIGRATE_DATA, item);
+
+spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_MAGIC);
+spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_VERSION);
+spice_marshaller_add_uint16(m, icc->motion_count);
+}
+
+void inputs_channel_client_handle_migrate_data(InputsChannelClient *icc,
+   uint16_t motion_count)
+{
+icc->motion_count = motion_count;
+
+for (; icc->motion_count >= SPICE_INPUT_MOTION_ACK_BUNCH;
+   icc->motion_count -= SPICE_INPUT_MOTION_ACK_BUNCH) {
+red_channel_client_pipe_add_type(>base, 
RED_PIPE_ITEM_MOUSE_MOTION_ACK);
+}
+}
+
+void inputs_channel_client_on_mouse_motion(InputsChannelClient *icc)
+{
+InputsChannel *inputs_channel = (InputsChannel *)icc->base.channel;
+
+if (++icc->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0 &&
+!inputs_channel_is_src_during_migrate(inputs_channel)) {
+red_channel_client_pipe_add_type(>base, 
RED_PIPE_ITEM_MOUSE_MOTION_ACK);
+icc->motion_count = 0;
+}
+}
diff --git a/server/inputs-channel-client.h b/server/inputs-channel-client.h
new file mode 100644
index 

[Spice-devel] [PATCH 5/7] RedChannel: Add FOREACH_CLIENT and use it to iterate

[Jonathon Jongsma]

Remove the custom FOREACH_DCC macro and use the more generic
FOREACH_CLIENT macro and use it for all channels.
---
 server/display-channel.c | 20 ++--
 server/display-channel.h |  9 -
 server/main-channel.c| 32 +++-
 server/red-channel.c | 40 +++-
 server/red-channel.h | 10 ++
 server/red-worker.c  |  4 ++--
 server/stream.c  | 12 ++--
 7 files changed, 58 insertions(+), 69 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index c94bc26..615cac5 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -243,7 +243,7 @@ void display_channel_surface_unref(DisplayChannel *display, 
uint32_t surface_id)
 
 region_destroy(>draw_dirty_region);
 surface->context.canvas = NULL;
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 dcc_destroy_surface(dcc, surface_id);
 }
 
@@ -278,7 +278,7 @@ static void streams_update_visible_region(DisplayChannel 
*display, Drawable *dra
 continue;
 }
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 agent = >stream_agents[get_stream_id(display, stream)];
 
 if (region_intersects(>vis_region, 
>tree_item.base.rgn)) {
@@ -296,7 +296,7 @@ static void pipes_add_drawable(DisplayChannel *display, 
Drawable *drawable)
 GList *link, *next;
 
 spice_warn_if_fail(ring_is_empty(>pipes));
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 dcc_prepend_drawable(dcc, drawable);
 }
 }
@@ -320,7 +320,7 @@ static void pipes_add_drawable_after(DisplayChannel 
*display,
 if (num_other_linked != display->common.base.clients_num) {
 GList *link, *next;
 spice_debug("TODO: not O(n^2)");
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 int sent = 0;
 DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next, 
dpi_pos_after) {
 if (dpi_pos_after->dcc == dcc) {
@@ -1209,7 +1209,7 @@ void 
display_channel_free_glz_drawables_to_free(DisplayChannel *display)
 
 spice_return_if_fail(display);
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 dcc_free_glz_drawables_to_free(dcc);
 }
 }
@@ -1221,7 +1221,7 @@ void display_channel_free_glz_drawables(DisplayChannel 
*display)
 
 spice_return_if_fail(display);
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 dcc_free_glz_drawables(dcc);
 }
 }
@@ -1268,7 +1268,7 @@ void display_channel_free_some(DisplayChannel *display)
 
 spice_debug("#draw=%d, #glz_draw=%d", display->drawable_count,
 display->glz_drawable_count);
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
 
 if (glz_dict) {
@@ -1283,7 +1283,7 @@ void display_channel_free_some(DisplayChannel *display)
 free_one_drawable(display, TRUE);
 }
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 GlzSharedDictionary *glz_dict = dcc ? dcc->glz_dict : NULL;
 
 if (glz_dict) {
@@ -1758,7 +1758,7 @@ static void 
clear_surface_drawables_from_pipes(DisplayChannel *display, int surf
 GList *link, *next;
 DisplayChannelClient *dcc;
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 if (!dcc_clear_surface_drawables_from_pipe(dcc, surface_id, 
wait_if_used)) {
 red_channel_client_disconnect(RED_CHANNEL_CLIENT(dcc));
 }
@@ -1824,7 +1824,7 @@ static void send_create_surface(DisplayChannel *display, 
int surface_id, int ima
 DisplayChannelClient *dcc;
 GList *link, *next;
 
-FOREACH_DCC(display, link, next, dcc) {
+FOREACH_CLIENT(display, link, next, dcc) {
 dcc_create_surface(dcc, surface_id);
 if (image_ready)
 dcc_push_surface_image(dcc, surface_id);
diff --git a/server/display-channel.h b/server/display-channel.h
index cec8c8e..7c1bfde 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -226,15 +226,6 @@ struct DisplayChannel {
 stat_info_t lz4_stat;
 };
 
-#define FOREACH_DCC(channel, _link, _next, _data)   \
-for (_link = (channel ? RED_CHANNEL(channel)->clients : NULL), \
- _next = (_link ? _link->next : NULL), \
- _data = (_link ? _link->data : NULL); \
- _link; \
- _link = _next, \
- _next = (_link ? _link->next : NULL), \
- _data = (_link ? _link->data : NULL))
-
 static inline int get_stream_id(DisplayChannel *display, Stream *stream)
 {
 return (int)(stream - 

[Spice-devel] [PATCH 2/7] change main_channel_marshall_mouse_mode call style

[Jonathon Jongsma]

From: Frediano Ziglio 

Make the call more similar to other marshall function calls in the
same function.

Signed-off-by: Frediano Ziglio 
---
 server/main-channel-client.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index 02b701f..19510e3 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -866,12 +866,9 @@ void main_channel_client_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 SPICE_CONTAINEROF(base, RedPingPipeItem, base));
 break;
 case RED_PIPE_ITEM_TYPE_MAIN_MOUSE_MODE:
-{
-RedMouseModePipeItem *item =
-SPICE_CONTAINEROF(base, RedMouseModePipeItem, base);
-main_channel_marshall_mouse_mode(rcc, m, item);
-break;
-}
+main_channel_marshall_mouse_mode(rcc, m,
+SPICE_CONTAINEROF(base, RedMouseModePipeItem, base));
+break;
 case RED_PIPE_ITEM_TYPE_MAIN_AGENT_DISCONNECTED:
 main_channel_marshall_agent_disconnected(rcc, m, base);
 break;
-- 
2.4.11

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

[Spice-devel] [PATCH 1/7] move all item creation in main-channel-client.c

[Jonathon Jongsma]

From: Frediano Ziglio 

Move all core to create and destroy MainChannel pipe items in a single
place.

Signed-off-by: Frediano Ziglio 
---
 server/main-channel-client.c | 34 ++
 server/main-channel-client.h | 15 +--
 server/main-channel.c| 29 +
 3 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index a8e8632..02b701f 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -103,6 +103,17 @@ typedef struct RedNotifyPipeItem {
 char *msg;
 } RedNotifyPipeItem;
 
+typedef struct RedMouseModePipeItem {
+RedPipeItem base;
+int current_mode;
+int is_client_mouse_allowed;
+} RedMouseModePipeItem;
+
+typedef struct RedMultiMediaTimePipeItem {
+RedPipeItem base;
+int time;
+} RedMultiMediaTimePipeItem;
+
 #define ZERO_BUF_SIZE 4096
 
 static const uint8_t zero_page[ZERO_BUF_SIZE] = {0};
@@ -296,6 +307,29 @@ void main_channel_client_push_notify(MainChannelClient 
*mcc, const char *msg)
 red_channel_client_pipe_add_push(>base, item);
 }
 
+RedPipeItem *main_mouse_mode_item_new(RedChannelClient *rcc, void *data, int 
num)
+{
+RedMouseModePipeItem *item = spice_malloc(sizeof(RedMouseModePipeItem));
+MainMouseModeItemInfo *info = data;
+
+red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_MAIN_MOUSE_MODE);
+item->current_mode = info->current_mode;
+item->is_client_mouse_allowed = info->is_client_mouse_allowed;
+return >base;
+}
+
+RedPipeItem *main_multi_media_time_item_new(RedChannelClient *rcc,
+void *data, int num)
+{
+MainMultiMediaTimeItemInfo *info = data;
+RedMultiMediaTimePipeItem *item;
+
+item = spice_malloc(sizeof(RedMultiMediaTimePipeItem));
+red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_MAIN_MULTI_MEDIA_TIME);
+item->time = info->time;
+return >base;
+}
+
 void main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
   int success,
   int seamless)
diff --git a/server/main-channel-client.h b/server/main-channel-client.h
index e65a2fb..c74f847 100644
--- a/server/main-channel-client.h
+++ b/server/main-channel-client.h
@@ -93,15 +93,18 @@ enum {
 RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS,
 };
 
-typedef struct RedMouseModePipeItem {
-RedPipeItem base;
+typedef struct MainMouseModeItemInfo {
 int current_mode;
 int is_client_mouse_allowed;
-} RedMouseModePipeItem;
+} MainMouseModeItemInfo;
 
-typedef struct RedMultiMediaTimePipeItem {
-RedPipeItem base;
+RedPipeItem *main_mouse_mode_item_new(RedChannelClient *rcc, void *data, int 
num);
+
+typedef struct MainMultiMediaTimeItemInfo {
 int time;
-} RedMultiMediaTimePipeItem;
+} MainMultiMediaTimeItemInfo;
+
+RedPipeItem *main_multi_media_time_item_new(RedChannelClient *rcc,
+void *data, int num);
 
 #endif /* __MAIN_CHANNEL_CLIENT_H__ */
diff --git a/server/main-channel.c b/server/main-channel.c
index 7b33346..9b1bd22 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -56,33 +56,6 @@ RedClient *main_channel_get_client_by_link_id(MainChannel 
*main_chan, uint32_t c
 return NULL;
 }
 
-typedef struct MainMouseModeItemInfo {
-int current_mode;
-int is_client_mouse_allowed;
-} MainMouseModeItemInfo;
-
-static RedPipeItem *main_mouse_mode_item_new(RedChannelClient *rcc, void 
*data, int num)
-{
-RedMouseModePipeItem *item = spice_malloc(sizeof(RedMouseModePipeItem));
-MainMouseModeItemInfo *info = data;
-
-red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_MAIN_MOUSE_MODE);
-item->current_mode = info->current_mode;
-item->is_client_mouse_allowed = info->is_client_mouse_allowed;
-return >base;
-}
-
-static RedPipeItem *main_multi_media_time_item_new(RedChannelClient *rcc,
-   void *data, int num)
-{
-RedMultiMediaTimePipeItem *item, *info = data;
-
-item = spice_malloc(sizeof(RedMultiMediaTimePipeItem));
-red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_MAIN_MULTI_MEDIA_TIME);
-item->time = info->time;
-return >base;
-}
-
 static void main_channel_push_channels(MainChannelClient *mcc)
 {
 if 
(red_client_during_migrate_at_target((main_channel_client_get_base(mcc))->client))
 {
@@ -148,7 +121,7 @@ static int 
main_channel_handle_migrate_data(RedChannelClient *rcc,
 
 void main_channel_push_multi_media_time(MainChannel *main_chan, int time)
 {
-RedMultiMediaTimePipeItem info = {
+MainMultiMediaTimeItemInfo info = {
 .time = time,
 };
 
-- 
2.4.11

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

[Spice-devel] [PATCH 0/7] Rebased patches from refactory

[Jonathon Jongsma]

Just a small set of patches incorporating some comments from previous review.
The new "Add FOREACH_CLIENT" commit was in response to Frediano's request to
use a macro for looping through clients in his review of "Replace
RedChannel::clients with GList". This new commit could be squashed into that
one if necessary.

Frediano Ziglio (2):
  move all item creation in main-channel-client.c
  change main_channel_marshall_mouse_mode call style

Jonathon Jongsma (5):
  Move InputsChannelClient to a separate file
  Replace RedChannel::clients with GList
  RedChannel: Add FOREACH_CLIENT and use it to iterate
  Replace RedClient::channels with GList
  Limit direct access to DisplayChannelClient

 server/Makefile.am |   3 +
 server/dcc-encoders.c  |  11 +-
 server/dcc-encoders.h  |   4 +-
 server/dcc-private.h   |  87 +++
 server/dcc-send.c  |   2 +-
 server/dcc.c   |  59 --
 server/dcc.h   |  93 
 server/display-channel.c   |  84 +++
 server/display-channel.h   |  12 +--
 server/image-cache.h   |   1 -
 server/inputs-channel-client.c |  87 +++
 server/inputs-channel-client.h |  48 +
 server/inputs-channel.c|  80 --
 server/inputs-channel.h|   4 +-
 server/main-channel-client.c   |  43 ++--
 server/main-channel-client.h   |  15 +--
 server/main-channel.c  |  73 -
 server/red-channel.c   | 238 ++---
 server/red-channel.h   |  18 +++-
 server/red-worker.c|   8 +-
 server/red-worker.h|  15 ++-
 server/stream.c|  79 +++---
 server/stream.h|   2 +-
 23 files changed, 608 insertions(+), 458 deletions(-)
 create mode 100644 server/dcc-private.h
 create mode 100644 server/inputs-channel-client.c
 create mode 100644 server/inputs-channel-client.h

-- 
2.4.11

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

[Spice-devel] [PATCH 6/7] Replace RedClient::channels with GList

[Jonathon Jongsma]

Allows us to not expose the client_link in RedChannelClient.

Acked-by: Pavel Grunt 
---
 server/red-channel.c | 51 ---
 server/red-channel.h |  5 +
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index e4dc4ee..8b4752a 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -1787,8 +1787,7 @@ static void red_channel_remove_client(RedChannelClient 
*rcc)
 static void red_client_remove_channel(RedChannelClient *rcc)
 {
 pthread_mutex_lock(>client->lock);
-ring_remove(>client_link);
-rcc->client->channels_num--;
+rcc->client->channels = g_list_remove(rcc->client->channels, rcc);
 pthread_mutex_unlock(>client->lock);
 }
 
@@ -2003,7 +2002,6 @@ RedClient *red_client_new(RedsState *reds, int migrated)
 
 client = spice_malloc0(sizeof(RedClient));
 client->reds = reds;
-ring_init(>channels);
 pthread_mutex_init(>lock, NULL);
 client->thread_id = pthread_self();
 client->during_target_migrate = migrated;
@@ -2047,15 +2045,14 @@ static gboolean 
red_channel_client_set_migration_seamless(RedChannelClient *rcc)
 
 void red_client_set_migration_seamless(RedClient *client) // dest
 {
-RingItem *link;
+GList *link;
 spice_assert(client->during_target_migrate);
 pthread_mutex_lock(>lock);
 client->seamless_migrate = TRUE;
 /* update channel clients that got connected before the migration
  * type was set. red_client_add_channel will handle newer channel clients 
*/
-RING_FOREACH(link, >channels) {
-RedChannelClient *rcc = SPICE_CONTAINEROF(link, RedChannelClient, 
client_link);
-if (red_channel_client_set_migration_seamless(rcc))
+for (link = client->channels; link != NULL; link = link->next) {
+if (red_channel_client_set_migration_seamless(link->data))
 client->num_migrated_channels++;
 }
 pthread_mutex_unlock(>lock);
@@ -2063,30 +2060,33 @@ void red_client_set_migration_seamless(RedClient 
*client) // dest
 
 void red_client_migrate(RedClient *client)
 {
-RingItem *link, *next;
+GList *link, *next;
 RedChannelClient *rcc;
 
-spice_printerr("migrate client with #channels %d", client->channels_num);
+spice_printerr("migrate client with #channels %d", 
g_list_length(client->channels));
 if (!pthread_equal(pthread_self(), client->thread_id)) {
 spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
   "If one of the threads is != io-thread && != 
vcpu-thread,"
   " this might be a BUG",
   client->thread_id, pthread_self());
 }
-RING_FOREACH_SAFE(link, next, >channels) {
-rcc = SPICE_CONTAINEROF(link, RedChannelClient, client_link);
+link = client->channels;
+while (link) {
+next = link->next;
+rcc = link->data;
 if (red_channel_client_is_connected(rcc)) {
 rcc->channel->client_cbs.migrate(rcc);
 }
+link = next;
 }
 }
 
 void red_client_destroy(RedClient *client)
 {
-RingItem *link, *next;
+GList *link, *next;
 RedChannelClient *rcc;
 
-spice_printerr("destroy client %p with #channels=%d", client, 
client->channels_num);
+spice_printerr("destroy client %p with #channels=%d", client, 
g_list_length(client->channels));
 if (!pthread_equal(pthread_self(), client->thread_id)) {
 spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
   "If one of the threads is != io-thread && != 
vcpu-thread,"
@@ -2094,10 +2094,12 @@ void red_client_destroy(RedClient *client)
   client->thread_id,
   pthread_self());
 }
-RING_FOREACH_SAFE(link, next, >channels) {
+link = client->channels;
+while (link) {
+next = link->next;
 // some channels may be in other threads, so disconnection
 // is not synchronous.
-rcc = SPICE_CONTAINEROF(link, RedChannelClient, client_link);
+rcc = link->data;
 rcc->destroying = 1;
 // some channels may be in other threads. However we currently
 // assume disconnect is synchronous (we changed the dispatcher
@@ -2109,6 +2111,7 @@ void red_client_destroy(RedClient *client)
 spice_assert(rcc->pipe_size == 0);
 spice_assert(rcc->send_data.size == 0);
 red_channel_client_destroy(rcc);
+link = next;
 }
 red_client_unref(client);
 }
@@ -2116,12 +2119,12 @@ void red_client_destroy(RedClient *client)
 /* client->lock should be locked */
 static RedChannelClient *red_client_get_channel(RedClient *client, int type, 
int id)
 {
-RingItem *link;
+GList *link;
 RedChannelClient *rcc;
 RedChannelClient *ret = NULL;
 
-RING_FOREACH(link, >channels) {
-rcc = SPICE_CONTAINEROF(link, RedChannelClient, client_link);
+for (link = 

[Spice-devel] [PATCH 4/7] Replace RedChannel::clients with GList

[Jonathon Jongsma]

Instead of using a Ring, use a GList to store the list of channel
clients. This allows us to iterate the clients without poking inside of
the client struct to get the channel_link. This is required in order to
make the RedChannelClient struct private.
---
 server/display-channel.c |  64 
 server/display-channel.h |  16 ++--
 server/main-channel.c|  42 +--
 server/red-channel.c | 193 ---
 server/red-channel.h |   3 +-
 server/red-worker.c  |   6 +-
 server/red-worker.h  |  15 ++--
 server/stream.c  |  23 +++---
 8 files changed, 162 insertions(+), 200 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 3f414fd..c94bc26 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -221,7 +221,7 @@ void display_channel_surface_unref(DisplayChannel *display, 
uint32_t surface_id)
 RedSurface *surface = >surfaces[surface_id];
 QXLInstance *qxl = display->common.qxl;
 DisplayChannelClient *dcc;
-RingItem *link, *next;
+GList *link, *next;
 
 if (--surface->refs != 0) {
 return;
@@ -254,7 +254,7 @@ static void streams_update_visible_region(DisplayChannel 
*display, Drawable *dra
 {
 Ring *ring;
 RingItem *item;
-RingItem *dcc_ring_item, *next;
+GList *link, *next;
 DisplayChannelClient *dcc;
 
 if (!red_channel_is_connected(RED_CHANNEL(display))) {
@@ -278,7 +278,7 @@ static void streams_update_visible_region(DisplayChannel 
*display, Drawable *dra
 continue;
 }
 
-FOREACH_DCC(display, dcc_ring_item, next, dcc) {
+FOREACH_DCC(display, link, next, dcc) {
 agent = >stream_agents[get_stream_id(display, stream)];
 
 if (region_intersects(>vis_region, 
>tree_item.base.rgn)) {
@@ -293,10 +293,10 @@ static void streams_update_visible_region(DisplayChannel 
*display, Drawable *dra
 static void pipes_add_drawable(DisplayChannel *display, Drawable *drawable)
 {
 DisplayChannelClient *dcc;
-RingItem *dcc_ring_item, *next;
+GList *link, *next;
 
 spice_warn_if_fail(ring_is_empty(>pipes));
-FOREACH_DCC(display, dcc_ring_item, next, dcc) {
+FOREACH_DCC(display, link, next, dcc) {
 dcc_prepend_drawable(dcc, drawable);
 }
 }
@@ -318,9 +318,9 @@ static void pipes_add_drawable_after(DisplayChannel 
*display,
 return;
 }
 if (num_other_linked != display->common.base.clients_num) {
-RingItem *item, *next;
+GList *link, *next;
 spice_debug("TODO: not O(n^2)");
-FOREACH_DCC(display, item, next, dcc) {
+FOREACH_DCC(display, link, next, dcc) {
 int sent = 0;
 DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next, 
dpi_pos_after) {
 if (dpi_pos_after->dcc == dcc) {
@@ -465,34 +465,32 @@ static int current_add_equal(DisplayChannel *display, 
DrawItem *item, TreeItem *
 
 DisplayChannelClient *dcc;
 RedDrawablePipeItem *dpi;
-RingItem *worker_ring_item, *dpi_ring_item;
+RingItem *dpi_ring_item;
+GList *link;
 
 other_drawable->refs++;
 current_remove_drawable(display, other_drawable);
 
 /* sending the drawable to clients that already received
  * (or will receive) other_drawable */
-worker_ring_item = ring_get_head(_CHANNEL(display)->clients);
+link = RED_CHANNEL(display)->clients;
 dpi_ring_item = ring_get_head(_drawable->pipes);
 /* dpi contains a sublist of dcc's, ordered the same */
-while (worker_ring_item) {
-dcc = SPICE_CONTAINEROF(worker_ring_item, DisplayChannelClient,
-common.base.channel_link);
+while (link) {
+dcc = link->data;
 dpi = SPICE_CONTAINEROF(dpi_ring_item, RedDrawablePipeItem, 
base);
-while (worker_ring_item && (!dpi || dcc != dpi->dcc)) {
+while (link && (!dpi || dcc != dpi->dcc)) {
 dcc_prepend_drawable(dcc, drawable);
-worker_ring_item = 
ring_next(_CHANNEL(display)->clients,
- worker_ring_item);
-dcc = SPICE_CONTAINEROF(worker_ring_item, 
DisplayChannelClient,
-common.base.channel_link);
+link = link->next;
+if (link)
+dcc = link->data;
 }
 
 if (dpi_ring_item) {
 dpi_ring_item = ring_next(_drawable->pipes, 
dpi_ring_item);
 }
-if (worker_ring_item) {
-worker_ring_item = 
ring_next(_CHANNEL(display)->clients,
- worker_ring_item);
+if (link) {
+   

Re: [Spice-devel] [PATCH 11/11] Get code more typesafe

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Scan remaining code searching for problems with structure
> layout assumptions in the code.
> Where code required some restructuring put some verify checks
> to make sure code won't compile if these assumptions are not
> in place anymore.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cache-item.tmpl.c |  1 +
>  server/cursor-channel.c  |  4 ++--
>  server/dcc-encoders.c|  2 +-
>  server/dcc-send.c| 14 ++
>  server/dcc.c |  5 +++--
>  server/dispatcher.c  |  3 +--
>  server/display-channel.c | 26 ++
>  server/image-cache.c |  7 +--
>  server/main-channel.c|  5 ++---
>  server/pixmap-cache.c|  3 ++-
>  server/red-replay-qxl.c  |  2 +-
>  server/smartcard.c   |  4 ++--
>  server/tree.c| 12 +++-
>  server/tree.h|  8 
>  14 files changed, 51 insertions(+), 45 deletions(-)
> 
> diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
> index 0617bea..0f22b35 100644
> --- a/server/cache-item.tmpl.c
> +++ b/server/cache-item.tmpl.c
> @@ -93,6 +93,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT *channel_client,
> uint64_t id, size_t siz
>  item = spice_new(RedCacheItem, 1);
>  
>  channel_client->VAR_NAME(available) -= size;
> +verify(SPICE_OFFSETOF(RedCacheItem, u.cache_data.lru_link) == 0);
>  while (channel_client->VAR_NAME(available) < 0) {
>  RedCacheItem *tail = (RedCacheItem *)ring_get_tail(_client
> ->VAR_NAME(lru));
>  if (!tail) {
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index c257654..23a8cb8 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -321,10 +321,10 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *pipe_it
>  cursor_marshall(rcc, m, SPICE_CONTAINEROF(pipe_item,
> RedCursorPipeItem, base));
>  break;
>  case RED_PIPE_ITEM_TYPE_INVAL_ONE:
> -red_marshall_inval(rcc, m, (RedCacheItem *)pipe_item);
> +red_marshall_inval(rcc, m, SPICE_CONTAINEROF(pipe_item, RedCacheItem,
> u.pipe_data));
>  break;
>  case RED_PIPE_ITEM_TYPE_VERB:
> -red_marshall_verb(rcc, (RedVerbItem*)pipe_item);
> +red_marshall_verb(rcc, SPICE_CONTAINEROF(pipe_item, RedVerbItem,
> base));
>  break;
>  case RED_PIPE_ITEM_TYPE_CURSOR_INIT:
>  red_reset_cursor_cache(rcc);
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index f1dd1bb..65f5a17 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -624,7 +624,7 @@ static GlzSharedDictionary *find_glz_dictionary(RedClient
> *client, uint8_t dict_
>  
>  now = _dictionary_list;
>  while ((now = ring_next(_dictionary_list, now))) {
> -GlzSharedDictionary *dict = (GlzSharedDictionary *)now;
> +GlzSharedDictionary *dict = SPICE_CONTAINEROF(now,
> GlzSharedDictionary, base);
>  if ((dict->client == client) && (dict->id == dict_id)) {
>  ret = dict;
>  break;
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 44b8448..91adbd7 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -2381,34 +2381,32 @@ void dcc_send_item(RedChannelClient *rcc, RedPipeItem
> *pipe_item)
>  break;
>  }
>  case RED_PIPE_ITEM_TYPE_INVAL_ONE:
> -marshall_inval_palette(rcc, m, (RedCacheItem *)pipe_item);
> +marshall_inval_palette(rcc, m, SPICE_CONTAINEROF(pipe_item,
> RedCacheItem, u.pipe_data));
>  break;
>  case RED_PIPE_ITEM_TYPE_STREAM_CREATE: {
>  StreamCreateDestroyItem *item = SPICE_CONTAINEROF(pipe_item,
> StreamCreateDestroyItem, base);
>  marshall_stream_start(rcc, m, item->agent);
>  break;
>  }
> -case RED_PIPE_ITEM_TYPE_STREAM_CLIP: {
> -RedStreamClipItem* clip_item = (RedStreamClipItem *)pipe_item;
> -marshall_stream_clip(rcc, m, clip_item);
> +case RED_PIPE_ITEM_TYPE_STREAM_CLIP:
> +marshall_stream_clip(rcc, m, SPICE_CONTAINEROF(pipe_item,
> RedStreamClipItem, base));
>  break;
> -}
>  case RED_PIPE_ITEM_TYPE_STREAM_DESTROY: {
>  StreamCreateDestroyItem *item = SPICE_CONTAINEROF(pipe_item,
> StreamCreateDestroyItem, base);
>  marshall_stream_end(rcc, m, item->agent);
>  break;
>  }
>  case RED_PIPE_ITEM_TYPE_UPGRADE:
> -marshall_upgrade(rcc, m, (RedUpgradeItem *)pipe_item);
> +marshall_upgrade(rcc, m, SPICE_CONTAINEROF(pipe_item, RedUpgradeItem,
> base));
>  break;
>  case RED_PIPE_ITEM_TYPE_VERB:
> -red_marshall_verb(rcc, (RedVerbItem*)pipe_item);
> +red_marshall_verb(rcc, SPICE_CONTAINEROF(pipe_item, RedVerbItem,
> base));
>  break;
>  case RED_PIPE_ITEM_TYPE_MIGRATE_DATA:
>  display_channel_marshall_migrate_data(rcc, m);
>  

Re: [Spice-devel] [PATCH 10/11] Make sure link in RedPipeItem can be not the first field

[Jonathon Jongsma]

In general, this is a good idea, but I question why you're doing it now. There
are patches already in the refactory branch which change the PipeItem ring into
a GList (although perhaps GQueue might be more appropriate). That patch would
also fix the underlying issue you're solving here. So having both patches just
creates more work for no benefit.

Jonathon


On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> There are many line of code were this assumption is in place.
> For instance RedPipeItem* is converted to RingItem* or
> viceversa.
> Use SPICE_CONTAINEROF to make sure link is in RedPipeItem and the
> offset is computed correctly.
> Also check that RingItem pointer is checked before SPICE_CONTAINEROF.
> On the other direction use >link instead of (RingItem*)item.
> This will also make sure that code won't compile if link is
> removed from RedPipeItem.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dcc-send.c  | 23 ---
>  server/dcc.c   | 17 ++---
>  server/red-channel.c   |  9 ++---
>  server/red-pipe-item.h |  2 ++
>  server/reds.c  |  3 ++-
>  5 files changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 8ec22c8..44b8448 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -188,7 +188,11 @@ static int is_brush_lossy(RedChannelClient *rcc,
> SpiceBrush *brush,
>  
>  static RedPipeItem *dcc_get_tail(DisplayChannelClient *dcc)
>  {
> -return (RedPipeItem*)ring_get_tail(_CHANNEL_CLIENT(dcc)->pipe);
> +RingItem *ring_item = ring_get_tail(_CHANNEL_CLIENT(dcc)->pipe);
> +if (!ring_item) {
> +return NULL;
> +}
> +return SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
>  }
>  
>  static void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> @@ -596,16 +600,14 @@ static int
> pipe_rendered_drawables_intersect_with_areas(DisplayChannelClient *dc
>  SpiceRect
> *surface_areas[],
>  int num_surfaces)
>  {
> -RedPipeItem *pipe_item;
> +RingItem *ring_item;
>  Ring *pipe;
>  
>  spice_assert(num_surfaces);
>  pipe = _CHANNEL_CLIENT(dcc)->pipe;
>  
> -for (pipe_item = (RedPipeItem *)ring_get_head(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_next(pipe, _item->link))
> -{
> +RING_FOREACH(ring_item, pipe) {
> +RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem,
> link);
>  Drawable *drawable;
>  
>  if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
> @@ -685,7 +687,7 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>  int resent_surface_ids[MAX_PIPE_SIZE];
>  SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawables
> may be released
>  int num_resent;
> -RedPipeItem *pipe_item;
> +RingItem *ring_item;
>  Ring *pipe;
>  
>  resent_surface_ids[0] = first_surface_id;
> @@ -695,9 +697,8 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>  pipe = _CHANNEL_CLIENT(dcc)->pipe;
>  
>  // going from the oldest to the newest
> -for (pipe_item = (RedPipeItem *)ring_get_tail(pipe);
> - pipe_item;
> - pipe_item = (RedPipeItem *)ring_prev(pipe, _item->link)) {
> +RING_FOREACH(ring_item, pipe) {
> +RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem,
> link);
>  Drawable *drawable;
>  RedDrawablePipeItem *dpi;
>  RedImageItem *image;
> @@ -728,7 +729,7 @@ static void
> red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
>  
>  spice_assert(image);
>  red_channel_client_pipe_remove_and_release(RED_CHANNEL_CLIENT(dcc),
> >dpi_pipe_item);
> -pipe_item = >base;
> +ring_item = >base.link;
>  }
>  }
>  
> diff --git a/server/dcc.c b/server/dcc.c
> index 20a8f11..df5123b 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -67,6 +67,7 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
>int wait_if_used)
>  {
>  Ring *ring;
> +RingItem *ring_item;
>  RedPipeItem *item;
>  int x;
>  RedChannelClient *rcc;
> @@ -76,13 +77,15 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
> no other drawable depends on them */
>  
>  rcc = RED_CHANNEL_CLIENT(dcc);
> -ring = >pipe;
> -item = (RedPipeItem *) ring;
> -while ((item = (RedPipeItem *)ring_next(ring, >link))) {
> +ring_item = ring = >pipe;
> +item = NULL;
> +while ((ring_item = ring_next(ring, ring_item))) {
>  Drawable *drawable;
>  RedDrawablePipeItem *dpi = NULL;
>  int depend_found = FALSE;
>  
> +item = SPICE_CONTAINEROF(ring_item, RedPipeItem, 

Re: [Spice-devel] [PATCH 09/11] reduce casts to RedPipeItem and RingItem

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Make code more type safe. This allow to move or delete structure
> fields more safely
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dcc.c   | 6 +++---
>  server/reds.c  | 2 +-
>  server/smartcard.c | 4 ++--
>  server/spicevmc.c  | 5 ++---
>  server/stream.c| 2 +-
>  5 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index 33357cd..20a8f11 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -78,7 +78,7 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
>  rcc = RED_CHANNEL_CLIENT(dcc);
>  ring = >pipe;
>  item = (RedPipeItem *) ring;
> -while ((item = (RedPipeItem *)ring_next(ring, (RingItem *)item))) {
> +while ((item = (RedPipeItem *)ring_next(ring, >link))) {
>  Drawable *drawable;
>  RedDrawablePipeItem *dpi = NULL;
>  int depend_found = FALSE;
> @@ -94,7 +94,7 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
>  
>  if (drawable->surface_id == surface_id) {
>  RedPipeItem *tmp_item = item;
> -item = (RedPipeItem *)ring_prev(ring, (RingItem *)item);
> +item = (RedPipeItem *)ring_prev(ring, >link);
>  red_channel_client_pipe_remove_and_release(rcc, tmp_item);
>  if (!item) {
>  item = (RedPipeItem *)ring;
> @@ -514,7 +514,7 @@ void dcc_stream_agent_clip(DisplayChannelClient* dcc,
> StreamAgent *agent)
>  item->rects->num_rects = n_rects;
>  region_ret_rects(>clip, item->rects->rects, n_rects);
>  
> -red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), (RedPipeItem
> *)item);
> +red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), >base);
>  }
>  
>  static void red_monitors_config_item_free(RedPipeItem *base)
> diff --git a/server/reds.c b/server/reds.c
> index fa9a79e..90911b4 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -888,7 +888,7 @@ static RedPipeItem
> *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
>  dev->priv->read_state = VDI_PORT_READ_STATE_GET_BUFF;
>  }
>  if (vdi_port_read_buf_process(reds->agent_dev, dispatch_buf,
> )) {
> -return (RedPipeItem *)dispatch_buf;
> +return _buf->base;
>  } else {
>  if (error) {
>  reds_agent_remove(reds);
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 2acddcf..9280038 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -160,7 +160,7 @@ static RedPipeItem
> *smartcard_read_msg_from_device(SpiceCharDeviceInstance *sin,
>  dev->priv->buf_pos = dev->priv->buf;
>  dev->priv->buf_used = remaining;
>  if (msg_to_client) {
> -return (RedPipeItem *)msg_to_client;
> +return _to_client->base;
>  }
>  }
>  return NULL;
> @@ -172,7 +172,7 @@ static void smartcard_send_msg_to_client(RedPipeItem *msg,
>  {
>  RedCharDeviceSmartcard *dev = opaque;
>  spice_assert(dev->priv->scc && dev->priv->scc->base.client == client);
> -smartcard_channel_client_pipe_add_push(>priv->scc->base,
> (RedPipeItem *)msg);
> +smartcard_channel_client_pipe_add_push(>priv->scc->base, msg);
>  
>  }
>  
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index f46b9e5..54c4f42 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -133,7 +133,7 @@ static RedPipeItem
> *spicevmc_chardev_read_msg_from_dev(SpiceCharDeviceInstance *
>  if (n > 0) {
>  spice_debug("read from dev %d", n);
>  msg_item->buf_used = n;
> -return (RedPipeItem *)msg_item;
> +return _item->base;
>  } else {
>  state->pipe_item = msg_item;
>  return NULL;
> @@ -145,11 +145,10 @@ static void
> spicevmc_chardev_send_msg_to_client(RedPipeItem *msg,
>  void *opaque)
>  {
>  SpiceVmcState *state = opaque;
> -RedVmcPipeItem *vmc_msg = SPICE_CONTAINEROF(msg, RedVmcPipeItem, base);
>  
>  spice_assert(state->rcc->client == client);
>  red_pipe_item_ref(msg);
> -red_channel_client_pipe_add_push(state->rcc, (RedPipeItem *)vmc_msg);
> +red_channel_client_pipe_add_push(state->rcc, msg);
>  }
>  
>  static SpiceVmcState *spicevmc_red_channel_client_get_state(RedChannelClient
> *rcc)
> diff --git a/server/stream.c b/server/stream.c
> index 74df254..be92289 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -177,7 +177,7 @@ void red_stream_clip_item_free(RedPipeItem *base)
>  RedStreamClipItem *red_stream_clip_item_new(StreamAgent *agent)
>  {
>  RedStreamClipItem *item = spice_new(RedStreamClipItem, 1);
> -red_pipe_item_init_full((RedPipeItem *)item,
> RED_PIPE_ITEM_TYPE_STREAM_CLIP,
> +red_pipe_item_init_full(>base, 

Re: [Spice-devel] [PATCH 08/11] rename RedVDIReadBug::parent to base

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> All other classes using RedPipeItem as base use base as parent name
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 74de7d5..fa9a79e 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -208,7 +208,7 @@ struct ChannelSecurityOptions {
>  };
>  
>  typedef struct RedVDIReadBuf {
> -RedPipeItem parent;
> +RedPipeItem base;
>  RedCharDeviceVDIPort *dev;
>  
>  int len;
> @@ -523,7 +523,7 @@ static void reds_reset_vdp(RedsState *reds)
>  dev->priv->receive_len = sizeof(dev->priv->vdi_chunk_header);
>  dev->priv->message_receive_len = 0;
>  if (dev->priv->current_read_buf) {
> -red_pipe_item_unref(>priv->current_read_buf->parent);
> +red_pipe_item_unref(>priv->current_read_buf->base);
>  dev->priv->current_read_buf = NULL;
>  }
>  /* Reset read filter to start with clean state when the agent reconnects
> */
> @@ -787,7 +787,7 @@ static void vdi_read_buf_init(RedVDIReadBuf *buf)
>  /* Bogus pipe item type, we only need the RingItem and refcounting
>   * from the base class and are not going to use the type
>   */
> -red_pipe_item_init_full(>parent, -1,
> +red_pipe_item_init_full(>base, -1,
>  vdi_port_read_buf_free);
>  }
>  
> @@ -801,9 +801,9 @@ static RedVDIReadBuf
> *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
>  }
>  
>  ring_remove(item);
> -buf = SPICE_CONTAINEROF(item, RedVDIReadBuf, parent.link);
> +buf = SPICE_CONTAINEROF(item, RedVDIReadBuf, base.link);
>  
> -g_warn_if_fail(buf->parent.refcount == 0);
> +g_warn_if_fail(buf->base.refcount == 0);
>  vdi_read_buf_init(buf);
>  
>  return buf;
> @@ -811,10 +811,10 @@ static RedVDIReadBuf
> *vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
>  
>  static void vdi_port_read_buf_free(RedPipeItem *base)
>  {
> -RedVDIReadBuf *buf = SPICE_CONTAINEROF(base, RedVDIReadBuf, parent);
> +RedVDIReadBuf *buf = SPICE_CONTAINEROF(base, RedVDIReadBuf, base);
>  
> -g_warn_if_fail(buf->parent.refcount == 0);
> -ring_add(>dev->priv->read_bufs, (RingItem *)buf);
> +g_warn_if_fail(buf->base.refcount == 0);
> +ring_add(>dev->priv->read_bufs, >base.link);
>  
>  /* read_one_msg_from_vdi_port may have never completed because the
> read_bufs
> ring was empty. So we call it again so it can complete its work if
> @@ -893,7 +893,7 @@ static RedPipeItem
> *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
>  if (error) {
>  reds_agent_remove(reds);
>  }
> -red_pipe_item_unref(_buf->parent);
> +red_pipe_item_unref(_buf->base);
>  }
>  }
>  } /* END switch */
> @@ -1271,7 +1271,7 @@ void reds_on_main_channel_migrate(RedsState *reds,
> MainChannelClient *mcc)
>  if (error) {
> reds_agent_remove(reds);
>  }
> -red_pipe_item_unref(_buf->parent);
> +red_pipe_item_unref(_buf->base);
>  }
>  
>  spice_assert(agent_dev->priv->receive_len);
> @@ -4351,7 +4351,7 @@ red_char_device_vdi_port_init(RedCharDeviceVDIPort
> *self)
>  /* This ensures the newly created buffer is placed in the
>   * RedCharDeviceVDIPort::read_bufs queue ready to be reused
>   */
> -red_pipe_item_unref(>parent);
> +red_pipe_item_unref(>base);
>  }
>  }
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 07/11] make red_pipe_item_unref more typesafe

[Jonathon Jongsma]

ok, I'll take some added typesafety over a having to pass the ->parent
occasionally

Acked-by: Jonathon Jongsma 


On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/char-device.c   |  2 +-
>  server/red-pipe-item.c |  4 +---
>  server/red-pipe-item.h |  2 +-
>  server/reds.c  | 12 +---
>  4 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index b67e192..4a6e4c8 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -189,7 +189,7 @@ static void red_char_device_client_free(RedCharDevice
> *dev,
>  dev_client->wait_for_tokens_timer = NULL;
>  }
>  
> -g_queue_free_full(dev_client->send_queue, red_pipe_item_unref);
> +g_queue_free_full(dev_client->send_queue,
> (GDestroyNotify)red_pipe_item_unref);
>  
>  /* remove write buffers that are associated with the client */
>  spice_debug("write_queue_is_empty %d", ring_is_empty(>priv
> ->write_queue) && !dev->priv->cur_write_buf);
> diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
> index 74bf267..31262fa 100644
> --- a/server/red-pipe-item.c
> +++ b/server/red-pipe-item.c
> @@ -29,10 +29,8 @@ RedPipeItem *red_pipe_item_ref(RedPipeItem *item)
>  return item;
>  }
>  
> -void red_pipe_item_unref(gpointer object)
> +void red_pipe_item_unref(RedPipeItem *item)
>  {
> -RedPipeItem *item = object;
> -
>  g_return_if_fail(item->refcount > 0);
>  
>  if (g_atomic_int_dec_and_test(>refcount)) {
> diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> index 0138216..bf13b0b 100644
> --- a/server/red-pipe-item.h
> +++ b/server/red-pipe-item.h
> @@ -37,7 +37,7 @@ typedef struct RedPipeItem {
>  
>  void red_pipe_item_init_full(RedPipeItem *item, int type,
> red_pipe_item_free_t free_func);
>  RedPipeItem *red_pipe_item_ref(RedPipeItem *item);
> -void red_pipe_item_unref(gpointer item);
> +void red_pipe_item_unref(RedPipeItem *item);
>  
>  static inline int red_pipe_item_is_linked(RedPipeItem *item)
>  {
> diff --git a/server/reds.c b/server/reds.c
> index 8ea6098..74de7d5 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -523,7 +523,7 @@ static void reds_reset_vdp(RedsState *reds)
>  dev->priv->receive_len = sizeof(dev->priv->vdi_chunk_header);
>  dev->priv->message_receive_len = 0;
>  if (dev->priv->current_read_buf) {
> -red_pipe_item_unref(dev->priv->current_read_buf);
> +red_pipe_item_unref(>priv->current_read_buf->parent);
>  dev->priv->current_read_buf = NULL;
>  }
>  /* Reset read filter to start with clean state when the agent reconnects
> */
> @@ -747,9 +747,7 @@ static void reds_agent_remove(RedsState *reds)
>  
>  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
>  {
> -RedVDIReadBuf *buf = (RedVDIReadBuf *)opaque;
> -
> -red_pipe_item_unref(buf);
> +red_pipe_item_unref((RedPipeItem *)opaque);
>  }
>  
>  /* returns TRUE if the buffer can be forwarded */
> @@ -895,7 +893,7 @@ static RedPipeItem
> *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
>  if (error) {
>  reds_agent_remove(reds);
>  }
> -red_pipe_item_unref(dispatch_buf);
> +red_pipe_item_unref(_buf->parent);
>  }
>  }
>  } /* END switch */
> @@ -1273,7 +1271,7 @@ void reds_on_main_channel_migrate(RedsState *reds,
> MainChannelClient *mcc)
>  if (error) {
> reds_agent_remove(reds);
>  }
> -red_pipe_item_unref(read_buf);
> +red_pipe_item_unref(_buf->parent);
>  }
>  
>  spice_assert(agent_dev->priv->receive_len);
> @@ -4353,7 +4351,7 @@ red_char_device_vdi_port_init(RedCharDeviceVDIPort
> *self)
>  /* This ensures the newly created buffer is placed in the
>   * RedCharDeviceVDIPort::read_bufs queue ready to be reused
>   */
> -red_pipe_item_unref(buf);
> +red_pipe_item_unref(>parent);
>  }
>  }
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 06/11] make red_pipe_item_ref more typesafe

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-pipe-item.c | 4 +---
>  server/red-pipe-item.h | 2 +-
>  server/reds.c  | 2 +-
>  server/spicevmc.c  | 4 ++--
>  4 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
> index cc0ab5a..74bf267 100644
> --- a/server/red-pipe-item.c
> +++ b/server/red-pipe-item.c
> @@ -20,10 +20,8 @@
>  #include "red-channel.h"
>  #include "red-pipe-item.h"
>  
> -RedPipeItem *red_pipe_item_ref(gpointer object)
> +RedPipeItem *red_pipe_item_ref(RedPipeItem *item)
>  {
> -RedPipeItem *item = object;
> -
>  g_return_val_if_fail(item->refcount > 0, NULL);
>  
>  g_atomic_int_inc(>refcount);
> diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> index 4b3d892..0138216 100644
> --- a/server/red-pipe-item.h
> +++ b/server/red-pipe-item.h
> @@ -36,7 +36,7 @@ typedef struct RedPipeItem {
>  } RedPipeItem;
>  
>  void red_pipe_item_init_full(RedPipeItem *item, int type,
> red_pipe_item_free_t free_func);
> -RedPipeItem *red_pipe_item_ref(gpointer item);
> +RedPipeItem *red_pipe_item_ref(RedPipeItem *item);
>  void red_pipe_item_unref(gpointer item);
>  
>  static inline int red_pipe_item_is_linked(RedPipeItem *item)
> diff --git a/server/reds.c b/server/reds.c
> index 8a903b7..8ea6098 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -910,7 +910,7 @@ static void vdi_port_send_msg_to_client(RedPipeItem *msg,
>  {
>  RedVDIReadBuf *agent_data_buf = (RedVDIReadBuf *)msg;
>  
> -red_pipe_item_ref(agent_data_buf);
> +red_pipe_item_ref(msg);
>  main_channel_client_push_agent_data(red_client_get_main(client),
>  agent_data_buf->data,
>  agent_data_buf->len,
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 1e68909..f46b9e5 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -145,10 +145,10 @@ static void
> spicevmc_chardev_send_msg_to_client(RedPipeItem *msg,
>  void *opaque)
>  {
>  SpiceVmcState *state = opaque;
> -RedVmcPipeItem *vmc_msg = (RedVmcPipeItem *)msg;
> +RedVmcPipeItem *vmc_msg = SPICE_CONTAINEROF(msg, RedVmcPipeItem, base);
>  
>  spice_assert(state->rcc->client == client);
> -red_pipe_item_ref(vmc_msg);
> +red_pipe_item_ref(msg);
>  red_channel_client_pipe_add_push(state->rcc, (RedPipeItem *)vmc_msg);
>  }
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 05/11] make red_pipe_item_init_full more typesafe

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Use a proper type for free callback
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel.c | 10 ++
>  server/dcc.c|  8 +---
>  server/main-channel.c   | 10 ++
>  server/red-channel.c|  6 --
>  server/red-pipe-item.c  |  4 ++--
>  server/red-pipe-item.h  | 10 +++---
>  server/reds.c   |  8 +---
>  server/smartcard.c  |  5 +++--
>  server/stream.c | 15 +--
>  9 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 949cbc4..c257654 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -81,7 +81,7 @@ struct CursorChannelClient {
>  #include "cache-item.tmpl.c"
>  #undef CLIENT_CURSOR_CACHE
>  
> -static void cursor_pipe_item_free(RedCursorPipeItem *pipe_item);
> +static red_pipe_item_free_t cursor_pipe_item_free;
>  
>  static CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd)
>  {
> @@ -138,7 +138,7 @@ static RedPipeItem *new_cursor_pipe_item(RedChannelClient
> *rcc, void *data, int
>  RedCursorPipeItem *item = spice_malloc0(sizeof(RedCursorPipeItem));
>  
>  red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_CURSOR,
> -(GDestroyNotify)cursor_pipe_item_free);
> +cursor_pipe_item_free);
>  item->cursor_item = data;
>  item->cursor_item->refs++;
>  return >base;
> @@ -204,9 +204,11 @@ void cursor_channel_disconnect(CursorChannel
> *cursor_channel)
>  }
>  
>  
> -static void cursor_pipe_item_free(RedCursorPipeItem *pipe_item)
> +static void cursor_pipe_item_free(RedPipeItem *base)
>  {
> -spice_return_if_fail(pipe_item);
> +spice_return_if_fail(base);
> +
> +RedCursorPipeItem *pipe_item = SPICE_CONTAINEROF(base, RedCursorPipeItem,
> base);
>  
>  spice_assert(!red_pipe_item_is_linked(_item->base));
>  
> diff --git a/server/dcc.c b/server/dcc.c
> index b9e4eb3..33357cd 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -306,7 +306,7 @@ static RedDrawablePipeItem
> *red_drawable_pipe_item_new(DisplayChannelClient *dcc
>  ring_item_init(>base);
>  ring_add(>pipes, >base);
>  red_pipe_item_init_full(>dpi_pipe_item, RED_PIPE_ITEM_TYPE_DRAW,
> -(GDestroyNotify)red_drawable_pipe_item_free);
> +red_drawable_pipe_item_free);
>  drawable->refs++;
>  return dpi;
>  }
> @@ -517,8 +517,10 @@ void dcc_stream_agent_clip(DisplayChannelClient* dcc,
> StreamAgent *agent)
>  red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), (RedPipeItem
> *)item);
>  }
>  
> -static void red_monitors_config_item_free(RedMonitorsConfigItem *item)
> +static void red_monitors_config_item_free(RedPipeItem *base)
>  {
> +RedMonitorsConfigItem *item = SPICE_CONTAINEROF(base,
> RedMonitorsConfigItem, pipe_item);
> +
>  monitors_config_unref(item->monitors_config);
>  free(item);
>  }
> @@ -532,7 +534,7 @@ static RedMonitorsConfigItem
> *red_monitors_config_item_new(RedChannel* channel,
>  mci->monitors_config = monitors_config;
>  
>  red_pipe_item_init_full(>pipe_item,
> RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
> -(GDestroyNotify)red_monitors_config_item_free);
> +red_monitors_config_item_free);
>  return mci;
>  }
>  
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 70aaff7..e89af97 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -247,8 +247,9 @@ static RedPipeItem
> *main_agent_tokens_item_new(RedChannelClient *rcc, uint32_t n
>  return >base;
>  }
>  
> -static void main_agent_data_item_free(RedAgentDataPipeItem *item)
> +static void main_agent_data_item_free(RedPipeItem *base)
>  {
> +RedAgentDataPipeItem *item = SPICE_CONTAINEROF(base,
> RedAgentDataPipeItem, base);
>  item->free_data(item->data, item->opaque);
>  free(item);
>  }
> @@ -260,7 +261,7 @@ static RedPipeItem
> *main_agent_data_item_new(RedChannelClient *rcc, uint8_t* dat
>  RedAgentDataPipeItem *item = spice_malloc(sizeof(RedAgentDataPipeItem));
>  
>  red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_MAIN_AGENT_DATA,
> -(GDestroyNotify)main_agent_data_item_free);
> +main_agent_data_item_free);
>  item->data = data;
>  item->len = len;
>  item->free_data = free_data;
> @@ -306,8 +307,9 @@ static RedPipeItem *main_uuid_item_new(MainChannelClient
> *mcc, const uint8_t uui
>  return >base;
>  }
>  
> -static void main_notify_item_free(RedNotifyPipeItem *data)
> +static void main_notify_item_free(RedPipeItem *base)
>  {
> +RedNotifyPipeItem *data = SPICE_CONTAINEROF(base, RedNotifyPipeItem,
> base);
>  free(data->msg);
>  free(data);
>  }
> @@ -318,7 +320,7 @@ 

Re: [Spice-devel] [PATCH 04/11] Use a marker instead of checking a RedPipeItem presence

[Jonathon Jongsma]

I would prefer a more elegant long-term solution, but this is OK as a short-term
solution

Acked-by: Jonathon Jongsma 


On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> This avoids having to retain a pointer just to check item is still in
> the queue with ring_item_is_linked(>link).
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel.c | 30 ++
>  server/red-channel.h |  1 +
>  2 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 23defc0..c0a9501 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -49,6 +49,11 @@ typedef struct RedEmptyMsgPipeItem {
>  int msg;
>  } RedEmptyMsgPipeItem;
>  
> +typedef struct MarkerPipeItem {
> +RedPipeItem base;
> +gboolean *item_in_pipe;
> +} MarkerPipeItem;
> +
>  #define PING_TEST_TIMEOUT_MS (MSEC_PER_SEC * 15)
>  #define PING_TEST_IDLE_NET_TIMEOUT_MS (MSEC_PER_SEC / 10)
>  
> @@ -574,6 +579,8 @@ static void red_channel_client_send_item(RedChannelClient
> *rcc, RedPipeItem *ite
>  case RED_PIPE_ITEM_TYPE_PING:
>  red_channel_client_send_ping(rcc);
>  break;
> +case RED_PIPE_ITEM_TYPE_MARKER:
> +break;
>  default:
>  rcc->channel->channel_cbs.send_item(rcc, item);
>  break;
> @@ -2358,13 +2365,21 @@ int
> red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
>  }
>  }
>  
> +static void marker_pipe_item_free(MarkerPipeItem *item)
> +{
> +if (item->item_in_pipe) {
> +*item->item_in_pipe = FALSE;
> +}
> +free(item);
> +}
> +
>  /* TODO: more evil sync stuff. anything with the word wait in it's name. */
>  int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
> RedPipeItem *item,
> int64_t timeout)
>  {
>  uint64_t end_time;
> -int item_in_pipe;
> +gboolean item_in_pipe;
>  
>  spice_info(NULL);
>  
> @@ -2374,7 +2389,13 @@ int
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  end_time = UINT64_MAX;
>  }
>  
> -red_pipe_item_ref(item);
> +MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
> +
> +red_pipe_item_init_full(_item->base, RED_PIPE_ITEM_TYPE_MARKER,
> +(GDestroyNotify)marker_pipe_item_free);
> +item_in_pipe = TRUE;
> +mark_item->item_in_pipe = _in_pipe;
> +red_channel_client_pipe_add_after(rcc, _item->base, item);
>  
>  if (red_channel_client_blocked(rcc)) {
>  red_channel_client_receive(rcc);
> @@ -2382,7 +2403,7 @@ int
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  }
>  red_channel_client_push(rcc);
>  
> -while((item_in_pipe = ring_item_is_linked(>link)) &&
> +while(item_in_pipe &&
>(timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
>  usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
>  red_channel_client_receive(rcc);
> @@ -2390,8 +2411,9 @@ int
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  red_channel_client_push(rcc);
>  }
>  
> -red_pipe_item_unref(item);
>  if (item_in_pipe) {
> +// still on the queue, make sure won't overwrite the stack variable
> +mark_item->item_in_pipe = NULL;
>  spice_warning("timeout");
>  return FALSE;
>  } else {
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 8e8845e..c4eb436 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -143,6 +143,7 @@ enum {
>  RED_PIPE_ITEM_TYPE_MIGRATE,
>  RED_PIPE_ITEM_TYPE_EMPTY_MSG,
>  RED_PIPE_ITEM_TYPE_PING,
> +RED_PIPE_ITEM_TYPE_MARKER,
>  
>  RED_PIPE_ITEM_TYPE_CHANNEL_BASE=101,
>  };
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 03/11] Call dcc_send_item directly

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 


On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dcc-send.c| 4 ++--
>  server/dcc.h | 2 +-
>  server/display-channel.c | 7 +--
>  3 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 05da07f..8ec22c8 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -2367,9 +2367,9 @@ static void reset_send_data(DisplayChannelClient *dcc)
>  memset(dcc->send_data.free_list.sync, 0, sizeof(dcc
> ->send_data.free_list.sync));
>  }
>  
> -void dcc_send_item(DisplayChannelClient *dcc, RedPipeItem *pipe_item)
> +void dcc_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
>  {
> -RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
> +DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
>  SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
>  
>  reset_send_data(dcc);
> diff --git a/server/dcc.h b/server/dcc.h
> index 864a768..a11d25a 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -203,7 +203,7 @@ void   dcc_append_drawable
>(DisplayCha
>  void   dcc_add_drawable_after   
>  (DisplayChannelClient *dcc,
>   
>  Drawable *drawable,
>   
>  RedPipeItem *pos);
> -void   dcc_send_item
>  (DisplayChannelClient *dcc,
> +void   dcc_send_item
>  (RedChannelClient *dcc,
>   
>  RedPipeItem *item);
>  intdcc_clear_surface_drawables_from_pipe
>  (DisplayChannelClient *dcc,
>int
> surface_id,
> diff --git a/server/display-channel.c b/server/display-channel.c
> index b9ed285..9f97911 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1925,11 +1925,6 @@ static void on_disconnect(RedChannelClient *rcc)
>  display->glz_drawable_count);
>  }
>  
> -static void send_item(RedChannelClient *rcc, RedPipeItem *item)
> -{
> -dcc_send_item(RCC_TO_DCC(rcc), item);
> -}
> -
>  static int handle_migrate_flush_mark(RedChannelClient *rcc)
>  {
>  DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel,
> DisplayChannel, common.base);
> @@ -1969,7 +1964,7 @@ DisplayChannel* display_channel_new(SpiceServer *reds,
> RedWorker *worker,
>  DisplayChannel *display;
>  ChannelCbs cbs = {
>  .on_disconnect = on_disconnect,
> -.send_item = send_item,
> +.send_item = dcc_send_item,
>  .handle_migrate_flush_mark = handle_migrate_flush_mark,
>  .handle_migrate_data = handle_migrate_data,
>  .handle_migrate_data_get_serial = handle_migrate_data_get_serial
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 02/11] Handle reference for RedPipeItem in RedChannel

[Jonathon Jongsma]

Better.

Acked-by: Jonathon Jongsma 


On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> Remove the need to release the item inside send_item callbacks.
> This looks like a partial rollback of previous patch but is
> to make clear the intention of the change.
> The lifetime of items could extend a bit further but there
> are no cases this small lag should cause problems.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel.c | 1 -
>  server/dcc-send.c   | 2 --
>  server/inputs-channel.c | 1 -
>  server/main-channel.c   | 2 --
>  server/red-channel.c| 2 +-
>  server/smartcard.c  | 2 --
>  server/spicevmc.c   | 2 --
>  7 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 104bf51..949cbc4 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -336,7 +336,6 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *pipe_it
>  spice_error("invalid pipe item type");
>  }
>  
> -red_pipe_item_unref(pipe_item);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index f0f2e16..05da07f 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -2455,8 +2455,6 @@ void dcc_send_item(DisplayChannelClient *dcc,
> RedPipeItem *pipe_item)
>  spice_warn_if_reached();
>  }
>  
> -red_pipe_item_unref(pipe_item);
> -
>  // a message is pending
>  if (red_channel_client_send_message_pending(rcc)) {
>  begin_send_message(rcc);
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 3218a48..4b5813f 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -289,7 +289,6 @@ static void inputs_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *base)
>  spice_warning("invalid pipe iten %d", base->type);
>  break;
>  }
> -red_pipe_item_unref(base);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 219212c..70aaff7 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -723,7 +723,6 @@ static void main_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
>  spice_printerr("Init msg for client %p was not sent yet "
> "(client is probably during semi-seamless migration).
> Ignoring msg type %d",
> rcc->client, base->type);
> -red_pipe_item_unref(base);
>  return;
>  }
>  switch (base->type) {
> @@ -789,7 +788,6 @@ static void main_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
>  main_channel_marshall_agent_connected(m, rcc, base);
>  break;
>  };
> -red_pipe_item_unref(base);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 8226bc4..23defc0 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -576,7 +576,7 @@ static void red_channel_client_send_item(RedChannelClient
> *rcc, RedPipeItem *ite
>  break;
>  default:
>  rcc->channel->channel_cbs.send_item(rcc, item);
> -return;
> +break;
>  }
>  red_pipe_item_unref(item);
>  }
> diff --git a/server/smartcard.c b/server/smartcard.c
> index e68ccdc..c39aeae 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -498,10 +498,8 @@ static void smartcard_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *item
>  break;
>  default:
>  spice_error("bad pipe item %d", item->type);
> -red_pipe_item_unref(item);
>  return;
>  }
> -red_pipe_item_unref(item);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index be51d54..1e68909 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -425,10 +425,8 @@ static void
> spicevmc_red_channel_send_item(RedChannelClient *rcc,
>  break;
>  default:
>  spice_error("bad pipe item %d", item->type);
> -red_pipe_item_unref(item);
>  return;
>  }
> -red_pipe_item_unref(item);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 01/11] Remove RedChannel::hold_item callback

[Jonathon Jongsma]

On Fri, 2016-05-20 at 14:01 +0100, Frediano Ziglio wrote:
> This is quite confusing and prone to errors.
> Use RedPipeItem reference counting instead.
> To compensate for the additional reference due to red_pipe_item_ref
> in RedChannel sub class with empty hold_item have to add a
> red_pipe_item_unref call in send_item.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel.c  | 8 
>  server/display-channel.c | 8 
>  server/inputs-channel.c  | 6 +-
>  server/main-channel.c| 8 +---
>  server/red-channel.c | 4 ++--
>  server/red-channel.h | 2 --
>  server/smartcard.c   | 9 ++---
>  server/spicevmc.c| 8 +---
>  8 files changed, 7 insertions(+), 46 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index fa462c5..104bf51 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -340,13 +340,6 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *pipe_it
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> -static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
> -{
> -spice_return_if_fail(item);
> -
> -red_pipe_item_ref(item);
> -}
> -
>  CursorChannel* cursor_channel_new(RedWorker *worker)
>  {
>  CursorChannel *cursor_channel;
> @@ -354,7 +347,6 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
>  ChannelCbs cbs = {
>  .on_disconnect =  cursor_channel_client_on_disconnect,
>  .send_item = cursor_channel_send_item,
> -.hold_item = cursor_channel_hold_pipe_item,
>  };
>  
>  spice_info("create cursor channel");
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 3f414fd..b9ed285 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1930,13 +1930,6 @@ static void send_item(RedChannelClient *rcc,
> RedPipeItem *item)
>  dcc_send_item(RCC_TO_DCC(rcc), item);
>  }
>  
> -static void hold_item(RedChannelClient *rcc, RedPipeItem *item)
> -{
> -spice_return_if_fail(item);
> -
> -red_pipe_item_ref(item);
> -}
> -
>  static int handle_migrate_flush_mark(RedChannelClient *rcc)
>  {
>  DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel,
> DisplayChannel, common.base);
> @@ -1977,7 +1970,6 @@ DisplayChannel* display_channel_new(SpiceServer *reds,
> RedWorker *worker,
>  ChannelCbs cbs = {
>  .on_disconnect = on_disconnect,
>  .send_item = send_item,
> -.hold_item = hold_item,
>  .handle_migrate_flush_mark = handle_migrate_flush_mark,
>  .handle_migrate_data = handle_migrate_data,
>  .handle_migrate_data_get_serial = handle_migrate_data_get_serial
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index a3c9fb2..3218a48 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -289,6 +289,7 @@ static void inputs_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *base)
>  spice_warning("invalid pipe iten %d", base->type);
>  break;
>  }
> +red_pipe_item_unref(base);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -518,10 +519,6 @@ static int inputs_channel_config_socket(RedChannelClient
> *rcc)
>  return TRUE;
>  }
>  
> -static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
> -{
> -}
> -
>  static void inputs_connect(RedChannel *channel, RedClient *client,
> RedsStream *stream, int migration,
> int num_common_caps, uint32_t *common_caps,
> @@ -620,7 +617,6 @@ InputsChannel* inputs_channel_new(RedsState *reds)
>  channel_cbs.config_socket = inputs_channel_config_socket;
>  channel_cbs.on_disconnect = inputs_channel_on_disconnect;
>  channel_cbs.send_item = inputs_channel_send_item;
> -channel_cbs.hold_item = inputs_channel_hold_pipe_item;
>  channel_cbs.alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
>  channel_cbs.release_recv_buf = inputs_channel_release_msg_rcv_buf;
>  channel_cbs.handle_migrate_data = inputs_channel_handle_migrate_data;
> diff --git a/server/main-channel.c b/server/main-channel.c
> index d540796..219212c 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -788,9 +788,8 @@ static void main_channel_send_item(RedChannelClient *rcc,
> RedPipeItem *base)
>  case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
>  main_channel_marshall_agent_connected(m, rcc, base);
>  break;
> -default:
> -break;

this is unrelated to the patch. Consider splitting?

>  };
> +red_pipe_item_unref(base);
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> @@ -1013,10 +1012,6 @@ static int main_channel_config_socket(RedChannelClient
> *rcc)
>  return TRUE;
>  }
>  
> -static void main_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> 

Re: [Spice-devel] [PATCH spice-gtk 1/7] util: remove STATIC_MUTEX macros

[Frediano Ziglio]

> 
> They are no longer needed since 0a9ec4ec0.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  src/channel-usbredir.c   | 10 +-
>  src/spice-channel-priv.h |  2 +-
>  src/spice-channel.c  | 28 ++--
>  src/spice-util-priv.h|  6 --
>  4 files changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index c8a2da9..2c5feae 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -78,7 +78,7 @@ struct _SpiceUsbredirChannelPrivate {
>  GTask *task;
>  SpiceUsbAclHelper *acl_helper;
>  #endif
> -STATIC_MUTEX device_connect_mutex;
> +GMutex device_connect_mutex;
>  };
>  
>  static void channel_set_handlers(SpiceChannelClass *klass);
> @@ -110,7 +110,7 @@ static void
> spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
>  {
>  #ifdef USE_USBREDIR
>  channel->priv = SPICE_USBREDIR_CHANNEL_GET_PRIVATE(channel);
> -STATIC_MUTEX_INIT(channel->priv->device_connect_mutex);
> +g_mutex_init(>priv->device_connect_mutex);
>  #endif
>  }
>  
> @@ -221,7 +221,7 @@ static void spice_usbredir_channel_finalize(GObject *obj)
>  if (channel->priv->host)
>  usbredirhost_close(channel->priv->host);
>  #ifdef USE_USBREDIR
> -STATIC_MUTEX_CLEAR(channel->priv->device_connect_mutex);
> +g_mutex_clear(>priv->device_connect_mutex);
>  #endif
>  
>  /* Chain up to the parent class */
> @@ -657,12 +657,12 @@ static void *usbredir_alloc_lock(void) {
>  
>  void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel)
>  {
> -STATIC_MUTEX_LOCK(channel->priv->device_connect_mutex);
> +g_mutex_lock(>priv->device_connect_mutex);
>  }
>  
>  void spice_usbredir_channel_unlock(SpiceUsbredirChannel *channel)
>  {
> -STATIC_MUTEX_UNLOCK(channel->priv->device_connect_mutex);
> +g_mutex_unlock(>priv->device_connect_mutex);
>  }
>  
>  static void usbredir_lock_lock(void *user_data) {
> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> index 526b661..50aca5c 100644
> --- a/src/spice-channel-priv.h
> +++ b/src/spice-channel-priv.h
> @@ -109,7 +109,7 @@ struct _SpiceChannelPrivate {
>  
>  GQueue  xmit_queue;
>  gbooleanxmit_queue_blocked;
> -STATIC_MUTEXxmit_queue_lock;
> +GMutex  xmit_queue_lock;
>  guint   xmit_queue_wakeup_id;
>  guint64 xmit_queue_size;
>  
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index e81034f..35a2cae 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -122,7 +122,7 @@ static void spice_channel_init(SpiceChannel *channel)
>  spice_channel_set_common_capability(channel,
>  SPICE_COMMON_CAP_AUTH_SASL);
>  #endif
>  g_queue_init(>xmit_queue);
> -STATIC_MUTEX_INIT(c->xmit_queue_lock);
> +g_mutex_init(>xmit_queue_lock);
>  }
>  
>  static void spice_channel_constructed(GObject *gobject)
> @@ -173,7 +173,7 @@ static void spice_channel_finalize(GObject *gobject)
>  
>  g_idle_remove_by_data(gobject);
>  
> -STATIC_MUTEX_CLEAR(c->xmit_queue_lock);
> +g_mutex_clear(>xmit_queue_lock);
>  
>  if (c->caps)
>  g_array_free(c->caps, TRUE);
> @@ -681,9 +681,9 @@ static gboolean spice_channel_idle_wakeup(gpointer
> user_data)
>   *   5) xmit_queue_wakeup_id now says there is a wakeup pending which is
>   *  false
>   */
> -STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> +g_mutex_lock(>xmit_queue_lock);
>  c->xmit_queue_wakeup_id = 0;
> -STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> +g_mutex_unlock(>xmit_queue_lock);
>  
>  spice_channel_wakeup(channel, FALSE);
>  
> @@ -703,7 +703,7 @@ void spice_msg_out_send(SpiceMsgOut *out)
>  c = out->channel->priv;
>  size = spice_marshaller_get_total_size(out->marshaller);
>  
> -STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> +g_mutex_lock(>xmit_queue_lock);
>  if (c->xmit_queue_blocked) {
>  g_warning("message queue is blocked, dropping message");
>  goto end;
> @@ -724,7 +724,7 @@ void spice_msg_out_send(SpiceMsgOut *out)
>  }
>  
>  end:
> -STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> +g_mutex_unlock(>xmit_queue_lock);
>  }
>  
>  /* coroutine context */
> @@ -2200,9 +2200,9 @@ static void spice_channel_iterate_write(SpiceChannel
> *channel)
>  SpiceMsgOut *out;
>  
>  do {
> -STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> +g_mutex_lock(>xmit_queue_lock);
>  out = g_queue_pop_head(>xmit_queue);
> -STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> +g_mutex_unlock(>xmit_queue_lock);
>  if (out) {
>  guint32 size = spice_marshaller_get_total_size(out->marshaller);
>  c->xmit_queue_size = (c->xmit_queue_size < size) ? 0 :
>  c->xmit_queue_size - size;
> @@ -2723,7 +2723,7 @@ static void channel_reset(SpiceChannel 

Re: [Spice-devel] [PATCH spice-gtk 5/7] egl: fix multiple gl display flickering

[Marc-André Lureau]

On Fri, May 20, 2016 at 5:16 PM, Marc-André Lureau
 wrote:
> For some unclear reason, when multiple display use gl in the same
> process (with virt-manager for ex), the texture content can be
> overwritten by another display on update.
>
> Although the gl contexts are differents when using multiple widgets,
> even when sharing a texture ID, it shouldn't overwrite the other context
> texture, there is something fishy.
>
> Set the image texture target before drawing to solve this for now.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  src/spice-widget-egl.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 210b834..5c24524 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -548,8 +548,12 @@ void spice_egl_update_display(SpiceDisplay *display)
>  }
>  SPICE_DEBUG("update %f +%d+%d %dx%d +%f+%f %fx%f", s, x, y, w, h,
>  tx, ty, tw, th);
> -
>  glBindTexture(GL_TEXTURE_2D, d->egl.tex_id);
> +/* FIXME: this should be only necessary on scanout update */

actually, it's the same issue as with other multiple display bugs, we
need to make context current in spice_egl_update_scanout(), otherwise
it may manipulate a texture from a different context, depending on the
last display to have changed it.

However, there doesn't seem to be an easy way to do that with
gtkglarea < 3.16, thus this patch is still valid. A further patch can
move it back to update_scanout() for 3.16 and x11, but it will stil
need this here for gtkglarea < 3.16.

> +glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> +glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> +glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, (GLeglImageOES)d->egl.image);
> +
>  glDisable(GL_BLEND);
>  glGetIntegerv(GL_CURRENT_PROGRAM, );
>  glUseProgram(d->egl.prog);
> @@ -627,10 +631,6 @@ gboolean spice_egl_update_scanout(SpiceDisplay *display,
> (EGLClientBuffer)NULL,
> attrs);
>
> -glBindTexture(GL_TEXTURE_2D, d->egl.tex_id);
> -glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> -glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> -glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, (GLeglImageOES)d->egl.image);
>  d->egl.scanout = *scanout;
>
>  return TRUE;
> --
> 2.7.4
>



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

[Spice-devel] [PATCH spice-gtk 5/7] egl: fix multiple gl display flickering

[Marc-André Lureau]

For some unclear reason, when multiple display use gl in the same
process (with virt-manager for ex), the texture content can be
overwritten by another display on update.

Although the gl contexts are differents when using multiple widgets,
even when sharing a texture ID, it shouldn't overwrite the other context
texture, there is something fishy.

Set the image texture target before drawing to solve this for now.

Signed-off-by: Marc-André Lureau 
---
 src/spice-widget-egl.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index 210b834..5c24524 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -548,8 +548,12 @@ void spice_egl_update_display(SpiceDisplay *display)
 }
 SPICE_DEBUG("update %f +%d+%d %dx%d +%f+%f %fx%f", s, x, y, w, h,
 tx, ty, tw, th);
-
 glBindTexture(GL_TEXTURE_2D, d->egl.tex_id);
+/* FIXME: this should be only necessary on scanout update */
+glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, (GLeglImageOES)d->egl.image);
+
 glDisable(GL_BLEND);
 glGetIntegerv(GL_CURRENT_PROGRAM, );
 glUseProgram(d->egl.prog);
@@ -627,10 +631,6 @@ gboolean spice_egl_update_scanout(SpiceDisplay *display,
(EGLClientBuffer)NULL,
attrs);
 
-glBindTexture(GL_TEXTURE_2D, d->egl.tex_id);
-glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
-glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
-glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, (GLeglImageOES)d->egl.image);
 d->egl.scanout = *scanout;
 
 return TRUE;
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 3/7] egl: don't destroy wayland egl context

[Marc-André Lureau]

The egl context is from Gtk on Wayland. Destroy it only on X11.

Signed-off-by: Marc-André Lureau 
---
 src/spice-widget-egl.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index 7611b0b..fff2831 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -364,10 +364,6 @@ void spice_egl_unrealize_display(SpiceDisplay *display)
 d->egl.tex_pointer_id = 0;
 }
 
-if (d->egl.surface != EGL_NO_SURFACE) {
-eglDestroySurface(d->egl.display, d->egl.surface);
-d->egl.surface = EGL_NO_SURFACE;
-}
 if (d->egl.vbuf_id) {
 glDeleteBuffers(1, >egl.vbuf_id);
 d->egl.vbuf_id = 0;
@@ -378,14 +374,21 @@ void spice_egl_unrealize_display(SpiceDisplay *display)
 d->egl.prog = 0;
 }
 
-if (d->egl.ctx) {
-eglDestroyContext(d->egl.display, d->egl.ctx);
-d->egl.ctx = 0;
-}
+if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
+if (d->egl.surface != EGL_NO_SURFACE) {
+eglDestroySurface(d->egl.display, d->egl.surface);
+d->egl.surface = EGL_NO_SURFACE;
+}
 
-eglMakeCurrent(d->egl.display, EGL_NO_SURFACE, EGL_NO_SURFACE,
-   EGL_NO_CONTEXT);
-eglTerminate(d->egl.display);
+if (d->egl.ctx) {
+eglDestroyContext(d->egl.display, d->egl.ctx);
+d->egl.ctx = 0;
+}
+
+eglMakeCurrent(d->egl.display, EGL_NO_SURFACE, EGL_NO_SURFACE,
+   EGL_NO_CONTEXT);
+eglTerminate(d->egl.display);
+}
 }
 
 G_GNUC_INTERNAL
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 6/7] egl: don't terminate display

[Marc-André Lureau]

This is global to the display connection: all egl resources will be
released, including those from other widgets or from the application.

Fix spice/virgl display being rendered black after another widget
display is destroyed.

Signed-off-by: Marc-André Lureau 
---
 src/spice-widget-egl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index 5c24524..754fc20 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -387,7 +387,6 @@ void spice_egl_unrealize_display(SpiceDisplay *display)
 
 eglMakeCurrent(d->egl.display, EGL_NO_SURFACE, EGL_NO_SURFACE,
EGL_NO_CONTEXT);
-eglTerminate(d->egl.display);
 }
 }
 
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 4/7] egl: only swap buffers on x11

[Marc-André Lureau]

Gtk does it for us already with GtkGlArea.

Signed-off-by: Marc-André Lureau 
---
 src/spice-widget-egl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
index fff2831..210b834 100644
--- a/src/spice-widget-egl.c
+++ b/src/spice-widget-egl.c
@@ -575,7 +575,9 @@ void spice_egl_update_display(SpiceDisplay *display)
  0, 0, 1, 1);
 }
 
-eglSwapBuffers(d->egl.display, d->egl.surface);
+if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
+eglSwapBuffers(d->egl.display, d->egl.surface);
+}
 
 glUseProgram(prog);
 }
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 2/7] channel: check if channel has a session

[Marc-André Lureau]

Since 8943d2329, the channel may be disconnected from the session
before it's destroyed. In this case, session is NULL.

Fixes some critical with virt-manager when closing a display:

(virt-manager:20451): GSpice-CRITICAL **: spice_session_is_for_migration: 
assertion 'SPICE_IS_SESSION(session)' failed

Signed-off-by: Marc-André Lureau 
---
 src/spice-channel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/spice-channel.c b/src/spice-channel.c
index 35a2cae..c555f75 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -2307,7 +2307,7 @@ static gboolean spice_channel_delayed_unref(gpointer data)
 c->state = SPICE_CHANNEL_STATE_UNCONNECTED;
 
 session = spice_channel_get_session(channel);
-if (spice_session_is_for_migration(session)) {
+if (session && spice_session_is_for_migration(session)) {
 /* error during migration - abort migration */
 spice_session_abort_migration(session);
 return FALSE;
-- 
2.7.4

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

[Spice-devel] [PATCH spice-gtk 1/7] util: remove STATIC_MUTEX macros

[Marc-André Lureau]

They are no longer needed since 0a9ec4ec0.

Signed-off-by: Marc-André Lureau 
---
 src/channel-usbredir.c   | 10 +-
 src/spice-channel-priv.h |  2 +-
 src/spice-channel.c  | 28 ++--
 src/spice-util-priv.h|  6 --
 4 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index c8a2da9..2c5feae 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -78,7 +78,7 @@ struct _SpiceUsbredirChannelPrivate {
 GTask *task;
 SpiceUsbAclHelper *acl_helper;
 #endif
-STATIC_MUTEX device_connect_mutex;
+GMutex device_connect_mutex;
 };
 
 static void channel_set_handlers(SpiceChannelClass *klass);
@@ -110,7 +110,7 @@ static void 
spice_usbredir_channel_init(SpiceUsbredirChannel *channel)
 {
 #ifdef USE_USBREDIR
 channel->priv = SPICE_USBREDIR_CHANNEL_GET_PRIVATE(channel);
-STATIC_MUTEX_INIT(channel->priv->device_connect_mutex);
+g_mutex_init(>priv->device_connect_mutex);
 #endif
 }
 
@@ -221,7 +221,7 @@ static void spice_usbredir_channel_finalize(GObject *obj)
 if (channel->priv->host)
 usbredirhost_close(channel->priv->host);
 #ifdef USE_USBREDIR
-STATIC_MUTEX_CLEAR(channel->priv->device_connect_mutex);
+g_mutex_clear(>priv->device_connect_mutex);
 #endif
 
 /* Chain up to the parent class */
@@ -657,12 +657,12 @@ static void *usbredir_alloc_lock(void) {
 
 void spice_usbredir_channel_lock(SpiceUsbredirChannel *channel)
 {
-STATIC_MUTEX_LOCK(channel->priv->device_connect_mutex);
+g_mutex_lock(>priv->device_connect_mutex);
 }
 
 void spice_usbredir_channel_unlock(SpiceUsbredirChannel *channel)
 {
-STATIC_MUTEX_UNLOCK(channel->priv->device_connect_mutex);
+g_mutex_unlock(>priv->device_connect_mutex);
 }
 
 static void usbredir_lock_lock(void *user_data) {
diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
index 526b661..50aca5c 100644
--- a/src/spice-channel-priv.h
+++ b/src/spice-channel-priv.h
@@ -109,7 +109,7 @@ struct _SpiceChannelPrivate {
 
 GQueue  xmit_queue;
 gbooleanxmit_queue_blocked;
-STATIC_MUTEXxmit_queue_lock;
+GMutex  xmit_queue_lock;
 guint   xmit_queue_wakeup_id;
 guint64 xmit_queue_size;
 
diff --git a/src/spice-channel.c b/src/spice-channel.c
index e81034f..35a2cae 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -122,7 +122,7 @@ static void spice_channel_init(SpiceChannel *channel)
 spice_channel_set_common_capability(channel, SPICE_COMMON_CAP_AUTH_SASL);
 #endif
 g_queue_init(>xmit_queue);
-STATIC_MUTEX_INIT(c->xmit_queue_lock);
+g_mutex_init(>xmit_queue_lock);
 }
 
 static void spice_channel_constructed(GObject *gobject)
@@ -173,7 +173,7 @@ static void spice_channel_finalize(GObject *gobject)
 
 g_idle_remove_by_data(gobject);
 
-STATIC_MUTEX_CLEAR(c->xmit_queue_lock);
+g_mutex_clear(>xmit_queue_lock);
 
 if (c->caps)
 g_array_free(c->caps, TRUE);
@@ -681,9 +681,9 @@ static gboolean spice_channel_idle_wakeup(gpointer 
user_data)
  *   5) xmit_queue_wakeup_id now says there is a wakeup pending which is
  *  false
  */
-STATIC_MUTEX_LOCK(c->xmit_queue_lock);
+g_mutex_lock(>xmit_queue_lock);
 c->xmit_queue_wakeup_id = 0;
-STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
+g_mutex_unlock(>xmit_queue_lock);
 
 spice_channel_wakeup(channel, FALSE);
 
@@ -703,7 +703,7 @@ void spice_msg_out_send(SpiceMsgOut *out)
 c = out->channel->priv;
 size = spice_marshaller_get_total_size(out->marshaller);
 
-STATIC_MUTEX_LOCK(c->xmit_queue_lock);
+g_mutex_lock(>xmit_queue_lock);
 if (c->xmit_queue_blocked) {
 g_warning("message queue is blocked, dropping message");
 goto end;
@@ -724,7 +724,7 @@ void spice_msg_out_send(SpiceMsgOut *out)
 }
 
 end:
-STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
+g_mutex_unlock(>xmit_queue_lock);
 }
 
 /* coroutine context */
@@ -2200,9 +2200,9 @@ static void spice_channel_iterate_write(SpiceChannel 
*channel)
 SpiceMsgOut *out;
 
 do {
-STATIC_MUTEX_LOCK(c->xmit_queue_lock);
+g_mutex_lock(>xmit_queue_lock);
 out = g_queue_pop_head(>xmit_queue);
-STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
+g_mutex_unlock(>xmit_queue_lock);
 if (out) {
 guint32 size = spice_marshaller_get_total_size(out->marshaller);
 c->xmit_queue_size = (c->xmit_queue_size < size) ? 0 : 
c->xmit_queue_size - size;
@@ -2723,7 +2723,7 @@ static void channel_reset(SpiceChannel *channel, gboolean 
migrating)
 g_clear_pointer(>peer_msg, g_free);
 c->peer_pos = 0;
 
-STATIC_MUTEX_LOCK(c->xmit_queue_lock);
+g_mutex_lock(>xmit_queue_lock);
 c->xmit_queue_blocked = TRUE; /* Disallow queuing new messages */
 gboolean was_empty = 

[Spice-devel] [spice-gtk] RFC Sync only on focus change

[Frediano Ziglio]

Limit the virtual keystrokes sent to the remote machine.
The modifiers are synced only when the application receive or lose
the focus. This reduce a lot the possible virtual keystrokes sent
to the guest to synchronize the modifiers.
This affect the situations where modifiers are configured
differently in client and guest.
When the application receive the focus the synchronization is
attempted from client to guest while when the application lose
focus is attempted guest to client (basically is moved following
user moving).

This patch is actually not complete but more an RFC:
- only X11 is currently supported and there are some
  embedded constants which should be changed even for X11
  (from commit 063c1b9c0627c87eb7f5369c4a6b9776a22e5c7d);
  I think to move the function in a separate file (you will
  understand the reason with Windows...);
- what happen with multimonitors? I don't think this patch
  it's causing regressions anyway;
- instead of calling spice_inputs_set_key_locks to send
  a message to spice-server which will insert the keystrokes
  based on spice-server knowledge of the modifiers use
  client knowledge, this will also help when some more knowledge
  of the guest status will be implemented in the guest;
- there are some possible changes in behavior for
  keymap_modifiers_changed;
- one possible regression is that if you are using virt-viewer
  and the guest is booted it's possible the boot process will switch
  modifiers status. Honestly I consider this more of an improvement
  than a regression.

Signed-off-by: Frediano Ziglio 
---
 src/spice-gtk-session-priv.h |   2 +-
 src/spice-gtk-session.c  | 104 +++
 src/spice-widget.c   |   5 ++-
 3 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/src/spice-gtk-session-priv.h b/src/spice-gtk-session-priv.h
index d7fe313..abf90d6 100644
--- a/src/spice-gtk-session-priv.h
+++ b/src/spice-gtk-session-priv.h
@@ -38,13 +38,13 @@ struct _SpiceGtkSessionClass
 void spice_gtk_session_request_auto_usbredir(SpiceGtkSession *self,
  gboolean state);
 gboolean spice_gtk_session_get_read_only(SpiceGtkSession *self);
-void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self);
 void spice_gtk_session_set_pointer_grabbed(SpiceGtkSession *self, gboolean 
grabbed);
 gboolean spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self);
 void spice_gtk_session_set_keyboard_has_focus(SpiceGtkSession *self, gboolean 
keyboard_has_focus);
 void spice_gtk_session_set_mouse_has_pointer(SpiceGtkSession *self, gboolean  
mouse_has_pointer);
 gboolean spice_gtk_session_get_keyboard_has_focus(SpiceGtkSession *self);
 gboolean spice_gtk_session_get_mouse_has_pointer(SpiceGtkSession *self);
+void spice_gtk_session_set_focus(SpiceGtkSession *self, gboolean focus);
 
 G_END_DECLS
 
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index bbcbeeb..476098e 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -66,6 +66,8 @@ struct _SpiceGtkSessionPrivate {
 gbooleankeyboard_has_focus;
 gbooleanmouse_has_pointer;
 gbooleansync_modifiers;
+/** the session has a window with the focus */
+gbooleanhas_focus;
 };
 
 /**
@@ -103,6 +105,13 @@ static void channel_new(SpiceSession *session, 
SpiceChannel *channel,
 static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
 gpointer user_data);
 static gboolean read_only(SpiceGtkSession *self);
+typedef enum {
+from_guest_to_client,
+from_client_to_guest
+} keyboard_sync_dir;
+static void spice_gtk_session_sync_keyboard_modifiers(SpiceGtkSession *self,
+  gboolean force,
+  keyboard_sync_dir dir);
 
 /* -- */
 /* gobject glue   */
@@ -179,9 +188,45 @@ static guint32 get_keyboard_lock_modifiers(void)
 return modifiers;
 }
 
+static void set_keyboard_lock_modifiers(guint32 modifiers)
+{
+#ifdef HAVE_X11_XKBLIB_H
+// TODO fixed ??
+enum {
+XKB_CAPS_LOCK = 2,
+XKB_NUM_LOCK = 0x10,
+XKB_SCROLL_LOCK = 0 // TODO
+};
+Display *x_display = NULL;
+int x_modifiers = 0;
+
+GdkScreen *screen = gdk_screen_get_default();
+if (!GDK_IS_X11_DISPLAY(gdk_screen_get_display(screen))) {
+SPICE_DEBUG("FIXME: gtk backend is not X11");
+return;
+}
+
+x_display = GDK_SCREEN_XDISPLAY(screen);
+
+if ((modifiers & SPICE_INPUTS_CAPS_LOCK) != 0) {
+x_modifiers |= XKB_CAPS_LOCK;
+}
+if ((modifiers & SPICE_INPUTS_NUM_LOCK) != 0) {
+x_modifiers |= XKB_NUM_LOCK;
+}
+if ((modifiers & SPICE_INPUTS_SCROLL_LOCK) != 0) {
+x_modifiers |= 

Re: [Spice-devel] RedPipeItem lifespan, past, present and ... bug

[Frediano Ziglio]

> 
> On Thu, 2016-05-19 at 18:25 +0200, Christophe Fergeau wrote:
> > Hey,
> > 
> > On Thu, May 19, 2016 at 06:14:52AM -0400, Frediano Ziglio wrote:
> > > Possible future changes (please comment):
> > > - would be good if red_channel_client_wait_pipe_item_sent could work even
> > >   without the hold_item, one possible implementation is adding a fake
> > > RedPipeItem
> > >   which when removed will set a flag causing the loop to exit (better
> > >   would
> > > be
> > >   to remove the function and implement this stuff in another way, there
> > >   are
> > > only
> > >   a single call to make sure there are no pending drawing on a surface);
> > 
> > Yeah, I was wondering too if a fake pipe item could be used to be
> > notified when the preceding pipe item has been sent.
> 
> With GObject, we could potentially add an "item-sent" signal that is emitted
> whenever an item is sent. Then wait_pipe_item_sent() could run a loop while
> handling this signal to and quit the loop when the appropriate item is sent?
> 

I think GObject is overkilling, this path is really cold, this code is
executed only when a surface is destroyed and not even for each destroy.
Adding a single bit just to support this is a waste of resources.
I posted a patch that implement this using a "marker" item.

> > 
> > > - remove the RingItem and make possible to add the item to multiple
> > > clients,
> > > this
> > >   IMHO would make stuff much easier to understand;
> 
> yes.
> 

Posted some patches for type safety in order to be able to remove
the link field safely without code crashing in unexplored paths.

> > > - remove hold_item and use always red_pipe_item_ref;
> > 
> > Yup, I would do that, hold_item really looks like a convoluted _ref()
> > used when not all pipe items had a refcount.
> 
> agreed
> 

Posted a patch for this.

> > 
> > > - add another callback to RedPipeItem to send the item to make all
> > > send_item
> > >   callbacks really small;
> > 
> > It probably makes sense too, maybe longer term. I don't think this
> > is strictly related to the bug you discussed earlier though.
> 
> hmm, what would this new callback do? marshall the pipe item?
> 

Yes, but it's future work I think.

> > 
> > 
> > > - remove atomic operation on RedPipeItem reference counter, I think was
> > > added
> > >   to make sure there was no thread issues but it's just slowing down big
> > > servers
> > >   with many cores.
> > 
> > Imo this would need to go with annotations saying in which thread the code
> > is running (thinking about the display thread). Probably not a huge
> > issue right now.
> > 

Obviously I have a patch even for this (not that hard...)

Have no idea how to solve the CharDevice issue (not a big issue till we don't
support multiple clients). If you look at the code 
(main_channel_client_push_agent_data)
there are still RedPipeItem which points to RedPipeItem, I think this is
mainly wrong. CharDevice expect a RedPipeItem for messages but only uses for
reference counting (which is even wrong!).
I was thinking about either:
- add a RedDataBuffer to pass data to CharDevice, something like
  struct RedDataBuffer {
 int refcount;
 void (*free)(struct RedDataBuffer);
 void *data;
 uint32_t data_len;
  }
- removing link from RedPipeItem (to make possible to add the same item to
  multiple pipes). And of course rename RedPipeItem to RedMessage or something
  like that. In the future I think this is the right direction however
  requires some analysis also on the best structures to use (I would
  like to understand the order of the data structure we are using but
  this depends also on the different pipes length based on different
  usages and statistics, mostly Ring functions are O(1) which is good)

I think I'll move to other stuff for now.

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

[Spice-devel] [PATCH 05/11] make red_pipe_item_init_full more typesafe

[Frediano Ziglio]

Use a proper type for free callback

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c | 10 ++
 server/dcc.c|  8 +---
 server/main-channel.c   | 10 ++
 server/red-channel.c|  6 --
 server/red-pipe-item.c  |  4 ++--
 server/red-pipe-item.h  | 10 +++---
 server/reds.c   |  8 +---
 server/smartcard.c  |  5 +++--
 server/stream.c | 15 +--
 9 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 949cbc4..c257654 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -81,7 +81,7 @@ struct CursorChannelClient {
 #include "cache-item.tmpl.c"
 #undef CLIENT_CURSOR_CACHE
 
-static void cursor_pipe_item_free(RedCursorPipeItem *pipe_item);
+static red_pipe_item_free_t cursor_pipe_item_free;
 
 static CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd)
 {
@@ -138,7 +138,7 @@ static RedPipeItem *new_cursor_pipe_item(RedChannelClient 
*rcc, void *data, int
 RedCursorPipeItem *item = spice_malloc0(sizeof(RedCursorPipeItem));
 
 red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_CURSOR,
-(GDestroyNotify)cursor_pipe_item_free);
+cursor_pipe_item_free);
 item->cursor_item = data;
 item->cursor_item->refs++;
 return >base;
@@ -204,9 +204,11 @@ void cursor_channel_disconnect(CursorChannel 
*cursor_channel)
 }
 
 
-static void cursor_pipe_item_free(RedCursorPipeItem *pipe_item)
+static void cursor_pipe_item_free(RedPipeItem *base)
 {
-spice_return_if_fail(pipe_item);
+spice_return_if_fail(base);
+
+RedCursorPipeItem *pipe_item = SPICE_CONTAINEROF(base, RedCursorPipeItem, 
base);
 
 spice_assert(!red_pipe_item_is_linked(_item->base));
 
diff --git a/server/dcc.c b/server/dcc.c
index b9e4eb3..33357cd 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -306,7 +306,7 @@ static RedDrawablePipeItem 
*red_drawable_pipe_item_new(DisplayChannelClient *dcc
 ring_item_init(>base);
 ring_add(>pipes, >base);
 red_pipe_item_init_full(>dpi_pipe_item, RED_PIPE_ITEM_TYPE_DRAW,
-(GDestroyNotify)red_drawable_pipe_item_free);
+red_drawable_pipe_item_free);
 drawable->refs++;
 return dpi;
 }
@@ -517,8 +517,10 @@ void dcc_stream_agent_clip(DisplayChannelClient* dcc, 
StreamAgent *agent)
 red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), (RedPipeItem *)item);
 }
 
-static void red_monitors_config_item_free(RedMonitorsConfigItem *item)
+static void red_monitors_config_item_free(RedPipeItem *base)
 {
+RedMonitorsConfigItem *item = SPICE_CONTAINEROF(base, 
RedMonitorsConfigItem, pipe_item);
+
 monitors_config_unref(item->monitors_config);
 free(item);
 }
@@ -532,7 +534,7 @@ static RedMonitorsConfigItem 
*red_monitors_config_item_new(RedChannel* channel,
 mci->monitors_config = monitors_config;
 
 red_pipe_item_init_full(>pipe_item, 
RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
-(GDestroyNotify)red_monitors_config_item_free);
+red_monitors_config_item_free);
 return mci;
 }
 
diff --git a/server/main-channel.c b/server/main-channel.c
index 70aaff7..e89af97 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -247,8 +247,9 @@ static RedPipeItem 
*main_agent_tokens_item_new(RedChannelClient *rcc, uint32_t n
 return >base;
 }
 
-static void main_agent_data_item_free(RedAgentDataPipeItem *item)
+static void main_agent_data_item_free(RedPipeItem *base)
 {
+RedAgentDataPipeItem *item = SPICE_CONTAINEROF(base, RedAgentDataPipeItem, 
base);
 item->free_data(item->data, item->opaque);
 free(item);
 }
@@ -260,7 +261,7 @@ static RedPipeItem 
*main_agent_data_item_new(RedChannelClient *rcc, uint8_t* dat
 RedAgentDataPipeItem *item = spice_malloc(sizeof(RedAgentDataPipeItem));
 
 red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_MAIN_AGENT_DATA,
-(GDestroyNotify)main_agent_data_item_free);
+main_agent_data_item_free);
 item->data = data;
 item->len = len;
 item->free_data = free_data;
@@ -306,8 +307,9 @@ static RedPipeItem *main_uuid_item_new(MainChannelClient 
*mcc, const uint8_t uui
 return >base;
 }
 
-static void main_notify_item_free(RedNotifyPipeItem *data)
+static void main_notify_item_free(RedPipeItem *base)
 {
+RedNotifyPipeItem *data = SPICE_CONTAINEROF(base, RedNotifyPipeItem, base);
 free(data->msg);
 free(data);
 }
@@ -318,7 +320,7 @@ static RedPipeItem *main_notify_item_new(RedChannelClient 
*rcc, void *data, int
 const char *msg = data;
 
 red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_MAIN_NOTIFY,
-(GDestroyNotify)main_notify_item_free);
+main_notify_item_free);
 item->msg = spice_strdup(msg);
 return >base;
 }
diff --git 

[Spice-devel] [PATCH 04/11] Use a marker instead of checking a RedPipeItem presence

[Frediano Ziglio]

This avoids having to retain a pointer just to check item is still in
the queue with ring_item_is_linked(>link).

Signed-off-by: Frediano Ziglio 
---
 server/red-channel.c | 30 ++
 server/red-channel.h |  1 +
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 23defc0..c0a9501 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -49,6 +49,11 @@ typedef struct RedEmptyMsgPipeItem {
 int msg;
 } RedEmptyMsgPipeItem;
 
+typedef struct MarkerPipeItem {
+RedPipeItem base;
+gboolean *item_in_pipe;
+} MarkerPipeItem;
+
 #define PING_TEST_TIMEOUT_MS (MSEC_PER_SEC * 15)
 #define PING_TEST_IDLE_NET_TIMEOUT_MS (MSEC_PER_SEC / 10)
 
@@ -574,6 +579,8 @@ static void red_channel_client_send_item(RedChannelClient 
*rcc, RedPipeItem *ite
 case RED_PIPE_ITEM_TYPE_PING:
 red_channel_client_send_ping(rcc);
 break;
+case RED_PIPE_ITEM_TYPE_MARKER:
+break;
 default:
 rcc->channel->channel_cbs.send_item(rcc, item);
 break;
@@ -2358,13 +2365,21 @@ int 
red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
 }
 }
 
+static void marker_pipe_item_free(MarkerPipeItem *item)
+{
+if (item->item_in_pipe) {
+*item->item_in_pipe = FALSE;
+}
+free(item);
+}
+
 /* TODO: more evil sync stuff. anything with the word wait in it's name. */
 int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
RedPipeItem *item,
int64_t timeout)
 {
 uint64_t end_time;
-int item_in_pipe;
+gboolean item_in_pipe;
 
 spice_info(NULL);
 
@@ -2374,7 +2389,13 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 end_time = UINT64_MAX;
 }
 
-red_pipe_item_ref(item);
+MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
+
+red_pipe_item_init_full(_item->base, RED_PIPE_ITEM_TYPE_MARKER,
+(GDestroyNotify)marker_pipe_item_free);
+item_in_pipe = TRUE;
+mark_item->item_in_pipe = _in_pipe;
+red_channel_client_pipe_add_after(rcc, _item->base, item);
 
 if (red_channel_client_blocked(rcc)) {
 red_channel_client_receive(rcc);
@@ -2382,7 +2403,7 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 }
 red_channel_client_push(rcc);
 
-while((item_in_pipe = ring_item_is_linked(>link)) &&
+while(item_in_pipe &&
   (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
 usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
 red_channel_client_receive(rcc);
@@ -2390,8 +2411,9 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 red_channel_client_push(rcc);
 }
 
-red_pipe_item_unref(item);
 if (item_in_pipe) {
+// still on the queue, make sure won't overwrite the stack variable
+mark_item->item_in_pipe = NULL;
 spice_warning("timeout");
 return FALSE;
 } else {
diff --git a/server/red-channel.h b/server/red-channel.h
index 8e8845e..c4eb436 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -143,6 +143,7 @@ enum {
 RED_PIPE_ITEM_TYPE_MIGRATE,
 RED_PIPE_ITEM_TYPE_EMPTY_MSG,
 RED_PIPE_ITEM_TYPE_PING,
+RED_PIPE_ITEM_TYPE_MARKER,
 
 RED_PIPE_ITEM_TYPE_CHANNEL_BASE=101,
 };
-- 
2.7.4

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

[Spice-devel] [PATCH 00/11] RedChannel refcounting and type safety

[Frediano Ziglio]

This patchset collect 2 different changes:
- work on RedChannel for reference counting changes
  and cleanup;
- different type safety improvements to avoid structure
  layout assumption and make easier to move/remove fields
  on different structures.

Frediano Ziglio (11):
  Remove RedChannel::hold_item callback
  Handle reference for RedPipeItem in RedChannel
  Call dcc_send_item directly
  Use a marker instead of checking a RedPipeItem presence
  make red_pipe_item_init_full more typesafe
  make red_pipe_item_ref more typesafe
  make red_pipe_item_unref more typesafe
  rename RedVDIReadBug::parent to base
  reduce casts to RedPipeItem and RingItem
  Make sure link in RedPipeItem can be not the first field
  Get code more typesafe

 server/cache-item.tmpl.c |  1 +
 server/char-device.c |  2 +-
 server/cursor-channel.c  | 23 ---
 server/dcc-encoders.c|  2 +-
 server/dcc-send.c| 43 ---
 server/dcc.c | 32 +++-
 server/dcc.h |  2 +-
 server/dispatcher.c  |  3 +--
 server/display-channel.c | 41 +++--
 server/image-cache.c |  7 +--
 server/inputs-channel.c  |  5 -
 server/main-channel.c| 23 ---
 server/pixmap-cache.c|  3 ++-
 server/red-channel.c | 45 -
 server/red-channel.h |  3 +--
 server/red-pipe-item.c   | 12 
 server/red-pipe-item.h   | 16 +++-
 server/red-replay-qxl.c  |  2 +-
 server/reds.c| 37 +++--
 server/smartcard.c   | 20 +++-
 server/spicevmc.c| 15 +++
 server/stream.c  | 17 ++---
 server/tree.c| 12 +++-
 server/tree.h|  8 
 24 files changed, 185 insertions(+), 189 deletions(-)

-- 
2.7.4

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

[Spice-devel] [PATCH 07/11] make red_pipe_item_unref more typesafe

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/char-device.c   |  2 +-
 server/red-pipe-item.c |  4 +---
 server/red-pipe-item.h |  2 +-
 server/reds.c  | 12 +---
 4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/server/char-device.c b/server/char-device.c
index b67e192..4a6e4c8 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -189,7 +189,7 @@ static void red_char_device_client_free(RedCharDevice *dev,
 dev_client->wait_for_tokens_timer = NULL;
 }
 
-g_queue_free_full(dev_client->send_queue, red_pipe_item_unref);
+g_queue_free_full(dev_client->send_queue, 
(GDestroyNotify)red_pipe_item_unref);
 
 /* remove write buffers that are associated with the client */
 spice_debug("write_queue_is_empty %d", 
ring_is_empty(>priv->write_queue) && !dev->priv->cur_write_buf);
diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
index 74bf267..31262fa 100644
--- a/server/red-pipe-item.c
+++ b/server/red-pipe-item.c
@@ -29,10 +29,8 @@ RedPipeItem *red_pipe_item_ref(RedPipeItem *item)
 return item;
 }
 
-void red_pipe_item_unref(gpointer object)
+void red_pipe_item_unref(RedPipeItem *item)
 {
-RedPipeItem *item = object;
-
 g_return_if_fail(item->refcount > 0);
 
 if (g_atomic_int_dec_and_test(>refcount)) {
diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
index 0138216..bf13b0b 100644
--- a/server/red-pipe-item.h
+++ b/server/red-pipe-item.h
@@ -37,7 +37,7 @@ typedef struct RedPipeItem {
 
 void red_pipe_item_init_full(RedPipeItem *item, int type, red_pipe_item_free_t 
free_func);
 RedPipeItem *red_pipe_item_ref(RedPipeItem *item);
-void red_pipe_item_unref(gpointer item);
+void red_pipe_item_unref(RedPipeItem *item);
 
 static inline int red_pipe_item_is_linked(RedPipeItem *item)
 {
diff --git a/server/reds.c b/server/reds.c
index 8ea6098..74de7d5 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -523,7 +523,7 @@ static void reds_reset_vdp(RedsState *reds)
 dev->priv->receive_len = sizeof(dev->priv->vdi_chunk_header);
 dev->priv->message_receive_len = 0;
 if (dev->priv->current_read_buf) {
-red_pipe_item_unref(dev->priv->current_read_buf);
+red_pipe_item_unref(>priv->current_read_buf->parent);
 dev->priv->current_read_buf = NULL;
 }
 /* Reset read filter to start with clean state when the agent reconnects */
@@ -747,9 +747,7 @@ static void reds_agent_remove(RedsState *reds)
 
 static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
 {
-RedVDIReadBuf *buf = (RedVDIReadBuf *)opaque;
-
-red_pipe_item_unref(buf);
+red_pipe_item_unref((RedPipeItem *)opaque);
 }
 
 /* returns TRUE if the buffer can be forwarded */
@@ -895,7 +893,7 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
 if (error) {
 reds_agent_remove(reds);
 }
-red_pipe_item_unref(dispatch_buf);
+red_pipe_item_unref(_buf->parent);
 }
 }
 } /* END switch */
@@ -1273,7 +1271,7 @@ void reds_on_main_channel_migrate(RedsState *reds, 
MainChannelClient *mcc)
 if (error) {
reds_agent_remove(reds);
 }
-red_pipe_item_unref(read_buf);
+red_pipe_item_unref(_buf->parent);
 }
 
 spice_assert(agent_dev->priv->receive_len);
@@ -4353,7 +4351,7 @@ red_char_device_vdi_port_init(RedCharDeviceVDIPort *self)
 /* This ensures the newly created buffer is placed in the
  * RedCharDeviceVDIPort::read_bufs queue ready to be reused
  */
-red_pipe_item_unref(buf);
+red_pipe_item_unref(>parent);
 }
 }
 
-- 
2.7.4

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

[Spice-devel] [PATCH 10/11] Make sure link in RedPipeItem can be not the first field

[Frediano Ziglio]

There are many line of code were this assumption is in place.
For instance RedPipeItem* is converted to RingItem* or
viceversa.
Use SPICE_CONTAINEROF to make sure link is in RedPipeItem and the
offset is computed correctly.
Also check that RingItem pointer is checked before SPICE_CONTAINEROF.
On the other direction use >link instead of (RingItem*)item.
This will also make sure that code won't compile if link is
removed from RedPipeItem.

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c  | 23 ---
 server/dcc.c   | 17 ++---
 server/red-channel.c   |  9 ++---
 server/red-pipe-item.h |  2 ++
 server/reds.c  |  3 ++-
 5 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 8ec22c8..44b8448 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -188,7 +188,11 @@ static int is_brush_lossy(RedChannelClient *rcc, 
SpiceBrush *brush,
 
 static RedPipeItem *dcc_get_tail(DisplayChannelClient *dcc)
 {
-return (RedPipeItem*)ring_get_tail(_CHANNEL_CLIENT(dcc)->pipe);
+RingItem *ring_item = ring_get_tail(_CHANNEL_CLIENT(dcc)->pipe);
+if (!ring_item) {
+return NULL;
+}
+return SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
 }
 
 static void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
@@ -596,16 +600,14 @@ static int 
pipe_rendered_drawables_intersect_with_areas(DisplayChannelClient *dc
 SpiceRect 
*surface_areas[],
 int num_surfaces)
 {
-RedPipeItem *pipe_item;
+RingItem *ring_item;
 Ring *pipe;
 
 spice_assert(num_surfaces);
 pipe = _CHANNEL_CLIENT(dcc)->pipe;
 
-for (pipe_item = (RedPipeItem *)ring_get_head(pipe);
- pipe_item;
- pipe_item = (RedPipeItem *)ring_next(pipe, _item->link))
-{
+RING_FOREACH(ring_item, pipe) {
+RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem, 
link);
 Drawable *drawable;
 
 if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
@@ -685,7 +687,7 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 int resent_surface_ids[MAX_PIPE_SIZE];
 SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawables may 
be released
 int num_resent;
-RedPipeItem *pipe_item;
+RingItem *ring_item;
 Ring *pipe;
 
 resent_surface_ids[0] = first_surface_id;
@@ -695,9 +697,8 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 pipe = _CHANNEL_CLIENT(dcc)->pipe;
 
 // going from the oldest to the newest
-for (pipe_item = (RedPipeItem *)ring_get_tail(pipe);
- pipe_item;
- pipe_item = (RedPipeItem *)ring_prev(pipe, _item->link)) {
+RING_FOREACH(ring_item, pipe) {
+RedPipeItem *pipe_item = SPICE_CONTAINEROF(ring_item, RedPipeItem, 
link);
 Drawable *drawable;
 RedDrawablePipeItem *dpi;
 RedImageItem *image;
@@ -728,7 +729,7 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 
 spice_assert(image);
 red_channel_client_pipe_remove_and_release(RED_CHANNEL_CLIENT(dcc), 
>dpi_pipe_item);
-pipe_item = >base;
+ring_item = >base.link;
 }
 }
 
diff --git a/server/dcc.c b/server/dcc.c
index 20a8f11..df5123b 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -67,6 +67,7 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
   int wait_if_used)
 {
 Ring *ring;
+RingItem *ring_item;
 RedPipeItem *item;
 int x;
 RedChannelClient *rcc;
@@ -76,13 +77,15 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
no other drawable depends on them */
 
 rcc = RED_CHANNEL_CLIENT(dcc);
-ring = >pipe;
-item = (RedPipeItem *) ring;
-while ((item = (RedPipeItem *)ring_next(ring, >link))) {
+ring_item = ring = >pipe;
+item = NULL;
+while ((ring_item = ring_next(ring, ring_item))) {
 Drawable *drawable;
 RedDrawablePipeItem *dpi = NULL;
 int depend_found = FALSE;
 
+item = SPICE_CONTAINEROF(ring_item, RedPipeItem, link);
+
 if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
 dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem, dpi_pipe_item);
 drawable = dpi->drawable;
@@ -94,10 +97,10 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
 
 if (drawable->surface_id == surface_id) {
 RedPipeItem *tmp_item = item;
-item = (RedPipeItem *)ring_prev(ring, >link);
+ring_item = ring_prev(ring, >link);
 red_channel_client_pipe_remove_and_release(rcc, tmp_item);
-if (!item) {
-item = (RedPipeItem *)ring;
+if (!ring_item) {
+ 

[Spice-devel] [PATCH 01/11] Remove RedChannel::hold_item callback

[Frediano Ziglio]

This is quite confusing and prone to errors.
Use RedPipeItem reference counting instead.
To compensate for the additional reference due to red_pipe_item_ref
in RedChannel sub class with empty hold_item have to add a
red_pipe_item_unref call in send_item.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c  | 8 
 server/display-channel.c | 8 
 server/inputs-channel.c  | 6 +-
 server/main-channel.c| 8 +---
 server/red-channel.c | 4 ++--
 server/red-channel.h | 2 --
 server/smartcard.c   | 9 ++---
 server/spicevmc.c| 8 +---
 8 files changed, 7 insertions(+), 46 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index fa462c5..104bf51 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -340,13 +340,6 @@ static void cursor_channel_send_item(RedChannelClient 
*rcc, RedPipeItem *pipe_it
 red_channel_client_begin_send_message(rcc);
 }
 
-static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-spice_return_if_fail(item);
-
-red_pipe_item_ref(item);
-}
-
 CursorChannel* cursor_channel_new(RedWorker *worker)
 {
 CursorChannel *cursor_channel;
@@ -354,7 +347,6 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
 ChannelCbs cbs = {
 .on_disconnect =  cursor_channel_client_on_disconnect,
 .send_item = cursor_channel_send_item,
-.hold_item = cursor_channel_hold_pipe_item,
 };
 
 spice_info("create cursor channel");
diff --git a/server/display-channel.c b/server/display-channel.c
index 3f414fd..b9ed285 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1930,13 +1930,6 @@ static void send_item(RedChannelClient *rcc, RedPipeItem 
*item)
 dcc_send_item(RCC_TO_DCC(rcc), item);
 }
 
-static void hold_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-spice_return_if_fail(item);
-
-red_pipe_item_ref(item);
-}
-
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, 
DisplayChannel, common.base);
@@ -1977,7 +1970,6 @@ DisplayChannel* display_channel_new(SpiceServer *reds, 
RedWorker *worker,
 ChannelCbs cbs = {
 .on_disconnect = on_disconnect,
 .send_item = send_item,
-.hold_item = hold_item,
 .handle_migrate_flush_mark = handle_migrate_flush_mark,
 .handle_migrate_data = handle_migrate_data,
 .handle_migrate_data_get_serial = handle_migrate_data_get_serial
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index a3c9fb2..3218a48 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -289,6 +289,7 @@ static void inputs_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_warning("invalid pipe iten %d", base->type);
 break;
 }
+red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
@@ -518,10 +519,6 @@ static int inputs_channel_config_socket(RedChannelClient 
*rcc)
 return TRUE;
 }
 
-static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-}
-
 static void inputs_connect(RedChannel *channel, RedClient *client,
RedsStream *stream, int migration,
int num_common_caps, uint32_t *common_caps,
@@ -620,7 +617,6 @@ InputsChannel* inputs_channel_new(RedsState *reds)
 channel_cbs.config_socket = inputs_channel_config_socket;
 channel_cbs.on_disconnect = inputs_channel_on_disconnect;
 channel_cbs.send_item = inputs_channel_send_item;
-channel_cbs.hold_item = inputs_channel_hold_pipe_item;
 channel_cbs.alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
 channel_cbs.release_recv_buf = inputs_channel_release_msg_rcv_buf;
 channel_cbs.handle_migrate_data = inputs_channel_handle_migrate_data;
diff --git a/server/main-channel.c b/server/main-channel.c
index d540796..219212c 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -788,9 +788,8 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
 main_channel_marshall_agent_connected(m, rcc, base);
 break;
-default:
-break;
 };
+red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
@@ -1013,10 +1012,6 @@ static int main_channel_config_socket(RedChannelClient 
*rcc)
 return TRUE;
 }
 
-static void main_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-}
-
 static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 spice_debug(NULL);
@@ -1154,7 +1149,6 @@ MainChannel* main_channel_new(RedsState *reds)
 channel_cbs.config_socket = main_channel_config_socket;
 channel_cbs.on_disconnect = main_channel_client_on_disconnect;
 channel_cbs.send_item = main_channel_send_item;
-   

[Spice-devel] [PATCH 06/11] make red_pipe_item_ref more typesafe

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-pipe-item.c | 4 +---
 server/red-pipe-item.h | 2 +-
 server/reds.c  | 2 +-
 server/spicevmc.c  | 4 ++--
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
index cc0ab5a..74bf267 100644
--- a/server/red-pipe-item.c
+++ b/server/red-pipe-item.c
@@ -20,10 +20,8 @@
 #include "red-channel.h"
 #include "red-pipe-item.h"
 
-RedPipeItem *red_pipe_item_ref(gpointer object)
+RedPipeItem *red_pipe_item_ref(RedPipeItem *item)
 {
-RedPipeItem *item = object;
-
 g_return_val_if_fail(item->refcount > 0, NULL);
 
 g_atomic_int_inc(>refcount);
diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
index 4b3d892..0138216 100644
--- a/server/red-pipe-item.h
+++ b/server/red-pipe-item.h
@@ -36,7 +36,7 @@ typedef struct RedPipeItem {
 } RedPipeItem;
 
 void red_pipe_item_init_full(RedPipeItem *item, int type, red_pipe_item_free_t 
free_func);
-RedPipeItem *red_pipe_item_ref(gpointer item);
+RedPipeItem *red_pipe_item_ref(RedPipeItem *item);
 void red_pipe_item_unref(gpointer item);
 
 static inline int red_pipe_item_is_linked(RedPipeItem *item)
diff --git a/server/reds.c b/server/reds.c
index 8a903b7..8ea6098 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -910,7 +910,7 @@ static void vdi_port_send_msg_to_client(RedPipeItem *msg,
 {
 RedVDIReadBuf *agent_data_buf = (RedVDIReadBuf *)msg;
 
-red_pipe_item_ref(agent_data_buf);
+red_pipe_item_ref(msg);
 main_channel_client_push_agent_data(red_client_get_main(client),
 agent_data_buf->data,
 agent_data_buf->len,
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 1e68909..f46b9e5 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -145,10 +145,10 @@ static void 
spicevmc_chardev_send_msg_to_client(RedPipeItem *msg,
 void *opaque)
 {
 SpiceVmcState *state = opaque;
-RedVmcPipeItem *vmc_msg = (RedVmcPipeItem *)msg;
+RedVmcPipeItem *vmc_msg = SPICE_CONTAINEROF(msg, RedVmcPipeItem, base);
 
 spice_assert(state->rcc->client == client);
-red_pipe_item_ref(vmc_msg);
+red_pipe_item_ref(msg);
 red_channel_client_pipe_add_push(state->rcc, (RedPipeItem *)vmc_msg);
 }
 
-- 
2.7.4

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

[Spice-devel] [PATCH 09/11] reduce casts to RedPipeItem and RingItem

[Frediano Ziglio]

Make code more type safe. This allow to move or delete structure
fields more safely

Signed-off-by: Frediano Ziglio 
---
 server/dcc.c   | 6 +++---
 server/reds.c  | 2 +-
 server/smartcard.c | 4 ++--
 server/spicevmc.c  | 5 ++---
 server/stream.c| 2 +-
 5 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 33357cd..20a8f11 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -78,7 +78,7 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
 rcc = RED_CHANNEL_CLIENT(dcc);
 ring = >pipe;
 item = (RedPipeItem *) ring;
-while ((item = (RedPipeItem *)ring_next(ring, (RingItem *)item))) {
+while ((item = (RedPipeItem *)ring_next(ring, >link))) {
 Drawable *drawable;
 RedDrawablePipeItem *dpi = NULL;
 int depend_found = FALSE;
@@ -94,7 +94,7 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
 
 if (drawable->surface_id == surface_id) {
 RedPipeItem *tmp_item = item;
-item = (RedPipeItem *)ring_prev(ring, (RingItem *)item);
+item = (RedPipeItem *)ring_prev(ring, >link);
 red_channel_client_pipe_remove_and_release(rcc, tmp_item);
 if (!item) {
 item = (RedPipeItem *)ring;
@@ -514,7 +514,7 @@ void dcc_stream_agent_clip(DisplayChannelClient* dcc, 
StreamAgent *agent)
 item->rects->num_rects = n_rects;
 region_ret_rects(>clip, item->rects->rects, n_rects);
 
-red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), (RedPipeItem *)item);
+red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), >base);
 }
 
 static void red_monitors_config_item_free(RedPipeItem *base)
diff --git a/server/reds.c b/server/reds.c
index fa9a79e..90911b4 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -888,7 +888,7 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
 dev->priv->read_state = VDI_PORT_READ_STATE_GET_BUFF;
 }
 if (vdi_port_read_buf_process(reds->agent_dev, dispatch_buf, 
)) {
-return (RedPipeItem *)dispatch_buf;
+return _buf->base;
 } else {
 if (error) {
 reds_agent_remove(reds);
diff --git a/server/smartcard.c b/server/smartcard.c
index 2acddcf..9280038 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -160,7 +160,7 @@ static RedPipeItem 
*smartcard_read_msg_from_device(SpiceCharDeviceInstance *sin,
 dev->priv->buf_pos = dev->priv->buf;
 dev->priv->buf_used = remaining;
 if (msg_to_client) {
-return (RedPipeItem *)msg_to_client;
+return _to_client->base;
 }
 }
 return NULL;
@@ -172,7 +172,7 @@ static void smartcard_send_msg_to_client(RedPipeItem *msg,
 {
 RedCharDeviceSmartcard *dev = opaque;
 spice_assert(dev->priv->scc && dev->priv->scc->base.client == client);
-smartcard_channel_client_pipe_add_push(>priv->scc->base, (RedPipeItem 
*)msg);
+smartcard_channel_client_pipe_add_push(>priv->scc->base, msg);
 
 }
 
diff --git a/server/spicevmc.c b/server/spicevmc.c
index f46b9e5..54c4f42 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -133,7 +133,7 @@ static RedPipeItem 
*spicevmc_chardev_read_msg_from_dev(SpiceCharDeviceInstance *
 if (n > 0) {
 spice_debug("read from dev %d", n);
 msg_item->buf_used = n;
-return (RedPipeItem *)msg_item;
+return _item->base;
 } else {
 state->pipe_item = msg_item;
 return NULL;
@@ -145,11 +145,10 @@ static void 
spicevmc_chardev_send_msg_to_client(RedPipeItem *msg,
 void *opaque)
 {
 SpiceVmcState *state = opaque;
-RedVmcPipeItem *vmc_msg = SPICE_CONTAINEROF(msg, RedVmcPipeItem, base);
 
 spice_assert(state->rcc->client == client);
 red_pipe_item_ref(msg);
-red_channel_client_pipe_add_push(state->rcc, (RedPipeItem *)vmc_msg);
+red_channel_client_pipe_add_push(state->rcc, msg);
 }
 
 static SpiceVmcState *spicevmc_red_channel_client_get_state(RedChannelClient 
*rcc)
diff --git a/server/stream.c b/server/stream.c
index 74df254..be92289 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -177,7 +177,7 @@ void red_stream_clip_item_free(RedPipeItem *base)
 RedStreamClipItem *red_stream_clip_item_new(StreamAgent *agent)
 {
 RedStreamClipItem *item = spice_new(RedStreamClipItem, 1);
-red_pipe_item_init_full((RedPipeItem *)item, 
RED_PIPE_ITEM_TYPE_STREAM_CLIP,
+red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_STREAM_CLIP,
 red_stream_clip_item_free);
 
 item->stream_agent = agent;
-- 
2.7.4

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

[Spice-devel] [PATCH 08/11] rename RedVDIReadBug::parent to base

[Frediano Ziglio]

All other classes using RedPipeItem as base use base as parent name

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 74de7d5..fa9a79e 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -208,7 +208,7 @@ struct ChannelSecurityOptions {
 };
 
 typedef struct RedVDIReadBuf {
-RedPipeItem parent;
+RedPipeItem base;
 RedCharDeviceVDIPort *dev;
 
 int len;
@@ -523,7 +523,7 @@ static void reds_reset_vdp(RedsState *reds)
 dev->priv->receive_len = sizeof(dev->priv->vdi_chunk_header);
 dev->priv->message_receive_len = 0;
 if (dev->priv->current_read_buf) {
-red_pipe_item_unref(>priv->current_read_buf->parent);
+red_pipe_item_unref(>priv->current_read_buf->base);
 dev->priv->current_read_buf = NULL;
 }
 /* Reset read filter to start with clean state when the agent reconnects */
@@ -787,7 +787,7 @@ static void vdi_read_buf_init(RedVDIReadBuf *buf)
 /* Bogus pipe item type, we only need the RingItem and refcounting
  * from the base class and are not going to use the type
  */
-red_pipe_item_init_full(>parent, -1,
+red_pipe_item_init_full(>base, -1,
 vdi_port_read_buf_free);
 }
 
@@ -801,9 +801,9 @@ static RedVDIReadBuf 
*vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
 }
 
 ring_remove(item);
-buf = SPICE_CONTAINEROF(item, RedVDIReadBuf, parent.link);
+buf = SPICE_CONTAINEROF(item, RedVDIReadBuf, base.link);
 
-g_warn_if_fail(buf->parent.refcount == 0);
+g_warn_if_fail(buf->base.refcount == 0);
 vdi_read_buf_init(buf);
 
 return buf;
@@ -811,10 +811,10 @@ static RedVDIReadBuf 
*vdi_port_get_read_buf(RedCharDeviceVDIPort *dev)
 
 static void vdi_port_read_buf_free(RedPipeItem *base)
 {
-RedVDIReadBuf *buf = SPICE_CONTAINEROF(base, RedVDIReadBuf, parent);
+RedVDIReadBuf *buf = SPICE_CONTAINEROF(base, RedVDIReadBuf, base);
 
-g_warn_if_fail(buf->parent.refcount == 0);
-ring_add(>dev->priv->read_bufs, (RingItem *)buf);
+g_warn_if_fail(buf->base.refcount == 0);
+ring_add(>dev->priv->read_bufs, >base.link);
 
 /* read_one_msg_from_vdi_port may have never completed because the 
read_bufs
ring was empty. So we call it again so it can complete its work if
@@ -893,7 +893,7 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
 if (error) {
 reds_agent_remove(reds);
 }
-red_pipe_item_unref(_buf->parent);
+red_pipe_item_unref(_buf->base);
 }
 }
 } /* END switch */
@@ -1271,7 +1271,7 @@ void reds_on_main_channel_migrate(RedsState *reds, 
MainChannelClient *mcc)
 if (error) {
reds_agent_remove(reds);
 }
-red_pipe_item_unref(_buf->parent);
+red_pipe_item_unref(_buf->base);
 }
 
 spice_assert(agent_dev->priv->receive_len);
@@ -4351,7 +4351,7 @@ red_char_device_vdi_port_init(RedCharDeviceVDIPort *self)
 /* This ensures the newly created buffer is placed in the
  * RedCharDeviceVDIPort::read_bufs queue ready to be reused
  */
-red_pipe_item_unref(>parent);
+red_pipe_item_unref(>base);
 }
 }
 
-- 
2.7.4

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

[Spice-devel] [PATCH 11/11] Get code more typesafe

[Frediano Ziglio]

Scan remaining code searching for problems with structure
layout assumptions in the code.
Where code required some restructuring put some verify checks
to make sure code won't compile if these assumptions are not
in place anymore.

Signed-off-by: Frediano Ziglio 
---
 server/cache-item.tmpl.c |  1 +
 server/cursor-channel.c  |  4 ++--
 server/dcc-encoders.c|  2 +-
 server/dcc-send.c| 14 ++
 server/dcc.c |  5 +++--
 server/dispatcher.c  |  3 +--
 server/display-channel.c | 26 ++
 server/image-cache.c |  7 +--
 server/main-channel.c|  5 ++---
 server/pixmap-cache.c|  3 ++-
 server/red-replay-qxl.c  |  2 +-
 server/smartcard.c   |  4 ++--
 server/tree.c| 12 +++-
 server/tree.h|  8 
 14 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
index 0617bea..0f22b35 100644
--- a/server/cache-item.tmpl.c
+++ b/server/cache-item.tmpl.c
@@ -93,6 +93,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT *channel_client, 
uint64_t id, size_t siz
 item = spice_new(RedCacheItem, 1);
 
 channel_client->VAR_NAME(available) -= size;
+verify(SPICE_OFFSETOF(RedCacheItem, u.cache_data.lru_link) == 0);
 while (channel_client->VAR_NAME(available) < 0) {
 RedCacheItem *tail = (RedCacheItem 
*)ring_get_tail(_client->VAR_NAME(lru));
 if (!tail) {
diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index c257654..23a8cb8 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -321,10 +321,10 @@ static void cursor_channel_send_item(RedChannelClient 
*rcc, RedPipeItem *pipe_it
 cursor_marshall(rcc, m, SPICE_CONTAINEROF(pipe_item, 
RedCursorPipeItem, base));
 break;
 case RED_PIPE_ITEM_TYPE_INVAL_ONE:
-red_marshall_inval(rcc, m, (RedCacheItem *)pipe_item);
+red_marshall_inval(rcc, m, SPICE_CONTAINEROF(pipe_item, RedCacheItem, 
u.pipe_data));
 break;
 case RED_PIPE_ITEM_TYPE_VERB:
-red_marshall_verb(rcc, (RedVerbItem*)pipe_item);
+red_marshall_verb(rcc, SPICE_CONTAINEROF(pipe_item, RedVerbItem, 
base));
 break;
 case RED_PIPE_ITEM_TYPE_CURSOR_INIT:
 red_reset_cursor_cache(rcc);
diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
index f1dd1bb..65f5a17 100644
--- a/server/dcc-encoders.c
+++ b/server/dcc-encoders.c
@@ -624,7 +624,7 @@ static GlzSharedDictionary *find_glz_dictionary(RedClient 
*client, uint8_t dict_
 
 now = _dictionary_list;
 while ((now = ring_next(_dictionary_list, now))) {
-GlzSharedDictionary *dict = (GlzSharedDictionary *)now;
+GlzSharedDictionary *dict = SPICE_CONTAINEROF(now, 
GlzSharedDictionary, base);
 if ((dict->client == client) && (dict->id == dict_id)) {
 ret = dict;
 break;
diff --git a/server/dcc-send.c b/server/dcc-send.c
index 44b8448..91adbd7 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2381,34 +2381,32 @@ void dcc_send_item(RedChannelClient *rcc, RedPipeItem 
*pipe_item)
 break;
 }
 case RED_PIPE_ITEM_TYPE_INVAL_ONE:
-marshall_inval_palette(rcc, m, (RedCacheItem *)pipe_item);
+marshall_inval_palette(rcc, m, SPICE_CONTAINEROF(pipe_item, 
RedCacheItem, u.pipe_data));
 break;
 case RED_PIPE_ITEM_TYPE_STREAM_CREATE: {
 StreamCreateDestroyItem *item = SPICE_CONTAINEROF(pipe_item, 
StreamCreateDestroyItem, base);
 marshall_stream_start(rcc, m, item->agent);
 break;
 }
-case RED_PIPE_ITEM_TYPE_STREAM_CLIP: {
-RedStreamClipItem* clip_item = (RedStreamClipItem *)pipe_item;
-marshall_stream_clip(rcc, m, clip_item);
+case RED_PIPE_ITEM_TYPE_STREAM_CLIP:
+marshall_stream_clip(rcc, m, SPICE_CONTAINEROF(pipe_item, 
RedStreamClipItem, base));
 break;
-}
 case RED_PIPE_ITEM_TYPE_STREAM_DESTROY: {
 StreamCreateDestroyItem *item = SPICE_CONTAINEROF(pipe_item, 
StreamCreateDestroyItem, base);
 marshall_stream_end(rcc, m, item->agent);
 break;
 }
 case RED_PIPE_ITEM_TYPE_UPGRADE:
-marshall_upgrade(rcc, m, (RedUpgradeItem *)pipe_item);
+marshall_upgrade(rcc, m, SPICE_CONTAINEROF(pipe_item, RedUpgradeItem, 
base));
 break;
 case RED_PIPE_ITEM_TYPE_VERB:
-red_marshall_verb(rcc, (RedVerbItem*)pipe_item);
+red_marshall_verb(rcc, SPICE_CONTAINEROF(pipe_item, RedVerbItem, 
base));
 break;
 case RED_PIPE_ITEM_TYPE_MIGRATE_DATA:
 display_channel_marshall_migrate_data(rcc, m);
 break;
 case RED_PIPE_ITEM_TYPE_IMAGE:
-red_marshall_image(rcc, m, (RedImageItem *)pipe_item);
+red_marshall_image(rcc, m, SPICE_CONTAINEROF(pipe_item, RedImageItem, 
base));
 break;
 case RED_PIPE_ITEM_TYPE_PIXMAP_SYNC:
 display_channel_marshall_pixmap_sync(rcc, m);
diff --git a/server/dcc.c 

[Spice-devel] [PATCH 03/11] Call dcc_send_item directly

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c| 4 ++--
 server/dcc.h | 2 +-
 server/display-channel.c | 7 +--
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 05da07f..8ec22c8 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2367,9 +2367,9 @@ static void reset_send_data(DisplayChannelClient *dcc)
 memset(dcc->send_data.free_list.sync, 0, 
sizeof(dcc->send_data.free_list.sync));
 }
 
-void dcc_send_item(DisplayChannelClient *dcc, RedPipeItem *pipe_item)
+void dcc_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
 {
-RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
+DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
 SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 
 reset_send_data(dcc);
diff --git a/server/dcc.h b/server/dcc.h
index 864a768..a11d25a 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -203,7 +203,7 @@ void   dcc_append_drawable  
 (DisplayCha
 void   dcc_add_drawable_after
(DisplayChannelClient *dcc,
   Drawable 
*drawable,
   
RedPipeItem *pos);
-void   dcc_send_item 
(DisplayChannelClient *dcc,
+void   dcc_send_item 
(RedChannelClient *dcc,
   
RedPipeItem *item);
 intdcc_clear_surface_drawables_from_pipe 
(DisplayChannelClient *dcc,
   int 
surface_id,
diff --git a/server/display-channel.c b/server/display-channel.c
index b9ed285..9f97911 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1925,11 +1925,6 @@ static void on_disconnect(RedChannelClient *rcc)
 display->glz_drawable_count);
 }
 
-static void send_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-dcc_send_item(RCC_TO_DCC(rcc), item);
-}
-
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, 
DisplayChannel, common.base);
@@ -1969,7 +1964,7 @@ DisplayChannel* display_channel_new(SpiceServer *reds, 
RedWorker *worker,
 DisplayChannel *display;
 ChannelCbs cbs = {
 .on_disconnect = on_disconnect,
-.send_item = send_item,
+.send_item = dcc_send_item,
 .handle_migrate_flush_mark = handle_migrate_flush_mark,
 .handle_migrate_data = handle_migrate_data,
 .handle_migrate_data_get_serial = handle_migrate_data_get_serial
-- 
2.7.4

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

[Spice-devel] [PATCH 02/11] Handle reference for RedPipeItem in RedChannel

[Frediano Ziglio]

Remove the need to release the item inside send_item callbacks.
This looks like a partial rollback of previous patch but is
to make clear the intention of the change.
The lifetime of items could extend a bit further but there
are no cases this small lag should cause problems.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c | 1 -
 server/dcc-send.c   | 2 --
 server/inputs-channel.c | 1 -
 server/main-channel.c   | 2 --
 server/red-channel.c| 2 +-
 server/smartcard.c  | 2 --
 server/spicevmc.c   | 2 --
 7 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 104bf51..949cbc4 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -336,7 +336,6 @@ static void cursor_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *pipe_it
 spice_error("invalid pipe item type");
 }
 
-red_pipe_item_unref(pipe_item);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/dcc-send.c b/server/dcc-send.c
index f0f2e16..05da07f 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2455,8 +2455,6 @@ void dcc_send_item(DisplayChannelClient *dcc, RedPipeItem 
*pipe_item)
 spice_warn_if_reached();
 }
 
-red_pipe_item_unref(pipe_item);
-
 // a message is pending
 if (red_channel_client_send_message_pending(rcc)) {
 begin_send_message(rcc);
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 3218a48..4b5813f 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -289,7 +289,6 @@ static void inputs_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_warning("invalid pipe iten %d", base->type);
 break;
 }
-red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/main-channel.c b/server/main-channel.c
index 219212c..70aaff7 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -723,7 +723,6 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_printerr("Init msg for client %p was not sent yet "
"(client is probably during semi-seamless migration). 
Ignoring msg type %d",
rcc->client, base->type);
-red_pipe_item_unref(base);
 return;
 }
 switch (base->type) {
@@ -789,7 +788,6 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 main_channel_marshall_agent_connected(m, rcc, base);
 break;
 };
-red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/red-channel.c b/server/red-channel.c
index 8226bc4..23defc0 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -576,7 +576,7 @@ static void red_channel_client_send_item(RedChannelClient 
*rcc, RedPipeItem *ite
 break;
 default:
 rcc->channel->channel_cbs.send_item(rcc, item);
-return;
+break;
 }
 red_pipe_item_unref(item);
 }
diff --git a/server/smartcard.c b/server/smartcard.c
index e68ccdc..c39aeae 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -498,10 +498,8 @@ static void smartcard_channel_send_item(RedChannelClient 
*rcc, RedPipeItem *item
 break;
 default:
 spice_error("bad pipe item %d", item->type);
-red_pipe_item_unref(item);
 return;
 }
-red_pipe_item_unref(item);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/spicevmc.c b/server/spicevmc.c
index be51d54..1e68909 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -425,10 +425,8 @@ static void 
spicevmc_red_channel_send_item(RedChannelClient *rcc,
 break;
 default:
 spice_error("bad pipe item %d", item->type);
-red_pipe_item_unref(item);
 return;
 }
-red_pipe_item_unref(item);
 red_channel_client_begin_send_message(rcc);
 }
 
-- 
2.7.4

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

Re: [Spice-devel] [PATCH spice-gtk v3 6/7] spice-uri: Check if port is in allowed range

[Pavel Grunt]

On Fri, 2016-05-20 at 14:50 +0200, Christophe Fergeau wrote:
> On Thu, May 19, 2016 at 06:38:08PM +0200, Pavel Grunt wrote:
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c| 8 ++--
> >  tests/test-spice-uri.c | 2 ++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index b483374..6a43461 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -165,8 +165,8 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  uri_port = uriv[1];
> >  
> >  if (uri_port != NULL) {
> > -char *endptr;
> > -guint port = strtoul(uri_port, , 10);
> > +gchar *endptr;
> > +gint64 port = g_ascii_strtoll(uri_port, , 10);
> 
> Not sure this is 100% related to this change? but why not

It helps to detect negative values (strtoul "converts" negative number to
positive/treats it as negative w/o the sign?) - I'll mention the reason for
change in the commit log.
> 
> >  if (*endptr != '\0') {
> >  g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> >  "Invalid uri port: %s", uri_port);
> > @@ -175,6 +175,10 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
> >  goto end;
> >  }
> > +if (port < 0 || port > 65535) {
> > +g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED, "Port out of range");
> > +goto end;
> > +}
> 
> I'd check for <= 0

Ok, I'll change it.

Thanks

Pavel

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

[Spice-devel] [PATCH spice-gtk 1/5] test-session: Fix signed compare

[Pavel Grunt]

The varible is compared with G_N_ELEMENTS which returns unsigned value
---
 tests/session.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/session.c b/tests/session.c
index d065b60..00076f2 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -3,7 +3,7 @@
 static void test_session_uri(void)
 {
 SpiceSession *s;
-gint i;
+guint i;
 
 struct {
 gchar *port;
-- 
2.8.2

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

[Spice-devel] [PATCH spice-gtk 5/5] test-session: Test invalid URIs

[Pavel Grunt]

---
 tests/session.c | 30 --
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/tests/session.c b/tests/session.c
index 00a5a1e..eace40a 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -1,6 +1,31 @@
 #include 
 
-static void test_session_uri(void)
+static void test_session_uri_bad(void)
+{
+SpiceSession *s;
+guint i;
+const gchar *invalid_uris[] = {
+"scheme://host?port", /* invalid scheme */
+"spice://[ipv6-host:42", /* missing closing ']' */
+"spice://host??", /* invalid key in URI */
+"spice://host:5900?unknown=value", /* unknown key */
+"spice://hostname", /* missing port */
+};
+
+s = spice_session_new();
+
+for (i = 0; i < G_N_ELEMENTS(invalid_uris); i++) {
+gchar *uri = NULL;
+g_object_set(s, "uri", invalid_uris[i], NULL);
+g_object_get(s, "uri", , NULL);
+g_assert_null(uri);
+g_free(uri);
+}
+
+g_object_unref(s);
+}
+
+static void test_session_uri_good(void)
 {
 SpiceSession *s;
 guint i;
@@ -123,7 +148,8 @@ int main(int argc, char* argv[])
  * test cases are going to test */
 g_log_set_always_fatal(fatal_mask & G_LOG_LEVEL_MASK);
 
-g_test_add_func("/session/uri", test_session_uri);
+g_test_add_func("/session/bad-uri", test_session_uri_bad);
+g_test_add_func("/session/good-uri", test_session_uri_good);
 
 return g_test_run();
 }
-- 
2.8.2

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

[Spice-devel] [PATCH spice-gtk 0/5] Add more tests for the session's uri

[Pavel Grunt]

Hi,

I plan to merge both uri parsers (one is in spice-session, one in spice-uri).
These patches add more to tests to avoid regressions.

Thanks,

Pavel Grunt (5):
  test-session: Fix signed compare
  test-session: Test alternative way for setting port
  test-session: Do not fail on g_warning
  test-session: Also test hostname, username and password
  test-session: Test invalid URIs

 tests/session.c | 95 +
 1 file changed, 89 insertions(+), 6 deletions(-)

-- 
2.8.2

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

[Spice-devel] [PATCH spice-gtk 3/5] test-session: Do not fail on g_warning

[Pavel Grunt]

Following commit adds tests for password set in uri which will produce
a runtime warning. Reset fatal mask set by g_test_init() to avoid
failing.
---
 tests/session.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tests/session.c b/tests/session.c
index a6276c2..549378b 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -70,8 +70,17 @@ static void test_session_uri(void)
 
 int main(int argc, char* argv[])
 {
+GLogLevelFlags fatal_mask;
+
+fatal_mask = (GLogLevelFlags)g_log_set_always_fatal((GLogLevelFlags) 
G_LOG_FATAL_MASK);
+
 g_test_init(, , NULL);
 
+/* Reset fatal mask set by g_test_init() as we don't want
+ * warnings/criticals to be fatal by default since this is what some of the
+ * test cases are going to test */
+g_log_set_always_fatal(fatal_mask & G_LOG_LEVEL_MASK);
+
 g_test_add_func("/session/uri", test_session_uri);
 
 return g_test_run();
-- 
2.8.2

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

[Spice-devel] [PATCH spice-gtk 2/5] test-session: Test alternative way for setting port

[Pavel Grunt]

---
 tests/session.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/session.c b/tests/session.c
index 00076f2..a6276c2 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -24,6 +24,12 @@ static void test_session_uri(void)
 { NULL, "5930",
   "spice://localhost?port==5930",
   "spice://localhost?tls-port=5930" },
+{ "42", NULL,
+  "spice://localhost:42",
+  "spice://localhost?port=42&" },
+{ "42", "5930",
+  "spice://localhost:42?tls-port=5930",
+  "spice://localhost?port=42=5930" },
 };
 
 /* Set URI and check URI, port and tls_port */
-- 
2.8.2

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

[Spice-devel] [PATCH spice-gtk 4/5] test-session: Also test hostname, username and password

[Pavel Grunt]

---
 tests/session.c | 48 +---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/tests/session.c b/tests/session.c
index 549378b..00a5a1e 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -8,33 +8,63 @@ static void test_session_uri(void)
 struct {
 gchar *port;
 gchar *tls_port;
+gchar *host;
+gchar *username;
+gchar *password;
 gchar *uri_input;
 gchar *uri_output;
 } tests[] = {
 /* Arguments with empty value */
 { "5900", NULL,
+  "localhost",
+  NULL, NULL,
   "spice://localhost?port=5900=",
   "spice://localhost?port=5900&" },
 { "5910", NULL,
-  "spice://localhost?tls-port==5910",
+  "localhost",
+  "user", NULL,
+  "spice://user@localhost?tls-port==5910",
   "spice://localhost?port=5910&" },
 { NULL, "5920",
-  "spice://localhost?tls-port=5920=",
+  "localhost",
+  "user", "password",
+  "spice://user@localhost?tls-port=5920==password",
   "spice://localhost?tls-port=5920" },
 { NULL, "5930",
+  "localhost",
+  NULL, NULL,
   "spice://localhost?port==5930",
   "spice://localhost?tls-port=5930" },
 { "42", NULL,
+  "localhost",
+  NULL, NULL,
   "spice://localhost:42",
   "spice://localhost?port=42&" },
 { "42", "5930",
+  "localhost",
+  NULL, NULL,
   "spice://localhost:42?tls-port=5930",
   "spice://localhost?port=42=5930" },
+{ "42", "5930",
+  "127.0.0.1",
+  NULL, NULL,
+  "spice://127.0.0.1:42?tls-port=5930",
+  "spice://127.0.0.1?port=42=5930" },
+{ "42", "5930",
+  "::1",
+  "user", NULL,
+  "spice://user@[::1]:42?tls-port=5930",
+  "spice://::1?port=42=5930" },
+{ "42", NULL,
+  "abcd::1",
+  NULL, "0",
+  "spice://[abcd::1]?port=42=0",
+  "spice://abcd::1?port=42&" },
 };
 
 /* Set URI and check URI, port and tls_port */
 for (i = 0; i < G_N_ELEMENTS(tests); i++) {
-gchar *uri, *port, *tls_port;
+gchar *uri, *port, *tls_port, *host, *username, *password;
 
 s = spice_session_new();
 g_object_set(s, "uri", tests[i].uri_input, NULL);
@@ -42,13 +72,22 @@ static void test_session_uri(void)
  "uri", ,
  "port", ,
  "tls-port", _port,
+ "host", ,
+ "username", ,
+ "password", ,
   NULL);
 g_assert_cmpstr(tests[i].uri_output, ==, uri);
 g_assert_cmpstr(tests[i].port, ==, port);
 g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
+g_assert_cmpstr(tests[i].host, ==, host);
+g_assert_cmpstr(tests[i].username, ==, username);
+g_assert_cmpstr(tests[i].password, ==, password);
 g_clear_pointer(, g_free);
 g_clear_pointer(, g_free);
 g_clear_pointer(_port, g_free);
+g_clear_pointer(, g_free);
+g_clear_pointer(, g_free);
+g_clear_pointer(, g_free);
 g_object_unref(s);
 }
 
@@ -60,6 +99,9 @@ static void test_session_uri(void)
 g_object_set(s,
  "port", tests[i].port,
  "tls-port", tests[i].tls_port,
+ "host", tests[i].host,
+ "username", tests[i].username,
+ "password", tests[i].password,
   NULL);
 g_object_get(s, "uri", , NULL);
 g_assert_cmpstr(tests[i].uri_output, ==, uri);
-- 
2.8.2

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

Re: [Spice-devel] [PATCH spice-gtk v3 6/7] spice-uri: Check if port is in allowed range

[Christophe Fergeau]

On Thu, May 19, 2016 at 06:38:08PM +0200, Pavel Grunt wrote:
> Related: rhbz#1335239
> ---
>  src/spice-uri.c| 8 ++--
>  tests/test-spice-uri.c | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index b483374..6a43461 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -165,8 +165,8 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  uri_port = uriv[1];
>  
>  if (uri_port != NULL) {
> -char *endptr;
> -guint port = strtoul(uri_port, , 10);
> +gchar *endptr;
> +gint64 port = g_ascii_strtoll(uri_port, , 10);

Not sure this is 100% related to this change? but why not

>  if (*endptr != '\0') {
>  g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>  "Invalid uri port: %s", uri_port);
> @@ -175,6 +175,10 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  g_set_error(error, SPICE_CLIENT_ERROR, 
> SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
>  goto end;
>  }
> +if (port < 0 || port > 65535) {
> +g_set_error(error, SPICE_CLIENT_ERROR, 
> SPICE_CLIENT_ERROR_FAILED, "Port out of range");
> +goto end;
> +}

I'd check for <= 0

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 v3 4/7] spice-uri: Reset SpiceUri before parsing

[Christophe Fergeau]

On Fri, May 20, 2016 at 11:07:58AM +0200, Pavel Grunt wrote:
> > > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > > index 3bdb502..04ea3cb 100644
> > > --- a/src/spice-uri.c
> > > +++ b/src/spice-uri.c
> > > @@ -105,6 +105,13 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > > *_uri, GError **error)
> > >  size_t len;
> > > 
> > >  g_return_val_if_fail(self != NULL, FALSE);
> > > +
> > > +self->scheme = NULL;
> > > +self->hostname = NULL;
> > > +self->port = 0;
> > > +self->user = NULL;
> > > +self->password = NULL;
> > > +
> > 
> > g_clear_pointer to avoid leaks?
> 
> sure, thanks a lot, stupid me /o\

You could introduce a spice_uri_reset() which does that and is reused in
_finalize().

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] Cross-platform mobile (iOS and Android) client

[Sergio L. Pascual]

On Thu, 2016-05-19 at 11:10 +0200, Christophe Fergeau wrote:
> Is this in anyway related to
> https://github.com/iiordanov/remote-desktop-clients ? Or did you
> start
> the Android client from scratch?

There could be some similarities in our glue code, as we took a look at
it to quickly find the entry points we needed to bridge, but for the
most part, it has been written from scratch.

> Is there any improvements that could be made on our side to make your
> life easier with these clients?

My colleague Javier already contributed some patches to implement
features that improve the experience on mobile devices (like the LZ4
compression).

We still apply some minor changes to the spice-gtk package (https://git
hub.com/flexVDI/cerbero/tree/master/recipes/spice-gtk), mostly to fix
some alignment issues on ARMv7.

Probably, it would be nice to apply those changes to master, but it's
no biggie ;-)
 
-- 
Sergio L. Pascual
flexVDI - Chief Technology Officer

Email: sergiolpasc...@flexvdi.com
Skype: sergiolpascual
Phone: +34 876 600 073

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

Re: [Spice-devel] [spice-gtk v1 6/9] file-xfer: a FileTransferOperation per transfer call

[Victor Toso]

On Thu, May 19, 2016 at 01:21:46PM +0200, Victor Toso wrote:
> Each call to spice_main_file_copy_async will now create a
> FileTransferOperation which groups all SpiceFileTransferTasks of the
> copy operation and also the progress_callback passed from Application.
> 
> As pointed in the fix 113093dd00a1cf10f6d3c3589b7589a184cec081, the
> progress_callback should provide information of the whole transfer
> operation; For that reason, there is no need to keep progress_callback
> and progress_callback_data per SpiceFileTransferTask but per
> FileTransferOperation.
> 
> The file_xfer_tasks hash table now has FileTransferOperation instead
> of SpiceFileTransferTask. To improve handling this new operation, I've
> created the helpers:
> 
> * file_transfer_operation_send_progress
> * file_transfer_operation_free_op
> * file_transfer_operation_reset_all
> * file_transfer_operation_find_task_by_id
> * file_transfer_operation_remove_task
> 
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
>  src/channel-main.c | 203 
> -
>  1 file changed, 138 insertions(+), 65 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 28ffe80..efe559c 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -90,8 +90,6 @@ static GList 
> *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
>  GFile **files,
>  GFileCopyFlags flags,
>  GCancellable 
> *cancellable,
> -GFileProgressCallback 
> progress_callback,
> -gpointer 
> progress_callback_data,
>  
> SpiceFileTransferTaskFlushCb flush_callback,
>  gpointer 
> flush_callback_data,
>  GAsyncReadyCallback 
> callback,
> @@ -109,8 +107,6 @@ struct _SpiceFileTransferTaskPrivate
>  GFileInputStream   *file_stream;
>  GFileCopyFlags flags;
>  GCancellable   *cancellable;
> -GFileProgressCallback  progress_callback;
> -gpointer   progress_callback_data;
>  SpiceFileTransferTaskFlushCb   flush_callback;
>  gpointer   flush_callback_data;
>  GAsyncReadyCallbackcallback;
> @@ -153,6 +149,15 @@ typedef struct {
>  SpiceDisplayState   display_state;
>  } SpiceDisplayConfig;
>  
> +typedef struct {
> +GList  *tasks;
> +SpiceMainChannel   *channel;
> +GFileProgressCallback   progress_callback;
> +gpointerprogress_callback_data;
> +goffset total_sent;
> +goffset transfer_size;
> +} FileTransferOperation;
> +
>  struct _SpiceMainChannelPrivate  {
>  enum SpiceMouseMode mouse_mode;
>  enum SpiceMouseMode requested_mouse_mode;
> @@ -254,6 +259,13 @@ static void file_xfer_flushed(SpiceMainChannel *channel, 
> gboolean success);
>  static void spice_main_set_max_clipboard(SpiceMainChannel *self, gint max);
>  static void set_agent_connected(SpiceMainChannel *channel, gboolean 
> connected);
>  
> +static void file_transfer_operation_send_progress(SpiceMainChannel *channel, 
> SpiceFileTransferTask *xfer_task);
> +static void file_transfer_operation_free_op(FileTransferOperation *op);
> +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
> +static SpiceFileTransferTask 
> *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> +  
> guint32 task_id);
> +static void file_transfer_operation_remove_task(SpiceMainChannel *channel, 
> SpiceFileTransferTask *task);
> +
>  /* -- */
>  
>  static const char *agent_msg_types[] = {
> @@ -312,8 +324,7 @@ static void spice_main_channel_init(SpiceMainChannel 
> *channel)
>  
>  c = channel->priv = SPICE_MAIN_CHANNEL_GET_PRIVATE(channel);
>  c->agent_msg_queue = g_queue_new();
> -c->file_xfer_tasks = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> -   NULL, g_object_unref);
> +c->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
>  c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
>  g_object_unref);
>  c->cancellable_volume_info = g_cancellable_new();
> @@ -467,9 +478,6 @@ static void spice_channel_iterate_write(SpiceChannel 
> *channel)
>  static void spice_main_channel_reset_agent(SpiceMainChannel *channel)
>  {
>  

Re: [Spice-devel] Gtk-doc VS Doxygen

[Daniel P. Berrange]

On Fri, May 20, 2016 at 06:44:01AM -0400, Frediano Ziglio wrote:
> Hi,
>   I'd like to have some standard on the internal documentation of 
> spice-server.
> 
> The current situation is quite messy having different styles.
> I think the worst think is not having any documentation, so better messy
> than nothing.
> I really like Doxygen as it's really complete tool however my Gtk-doc 
> knowledge
> is very low and I cannot compare the two.
> I know we use quite a lot of "G" stuff so perhaps people feels more 
> comfortable
> with Gtk-doc.
> I'd like to see the documentation generated and possibly published on web
> so suggestion from the guys who maintain the website are really welcome.

IMHO i'd always use gtk-doc for API documentation for any codebase which is
based on glib, especially if you start to use GObject in any way, as it opens
the door to trivially support gobject introspection in the future as it uses
the same annotations.

From the POV of a developer consuming the API documentation I find the Doxygen
HTML fairly unpleasant to read - the GTK-DOC style/layout feels (subjectively)
more user friendly.

Ultimately though any documentation is better than no documentation and the
real hard work is writing the good quality docs, regardless of format :-)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Gtk-doc VS Doxygen

[Frediano Ziglio]

Hi,
  I'd like to have some standard on the internal documentation of spice-server.

The current situation is quite messy having different styles.
I think the worst think is not having any documentation, so better messy
than nothing.
I really like Doxygen as it's really complete tool however my Gtk-doc knowledge
is very low and I cannot compare the two.
I know we use quite a lot of "G" stuff so perhaps people feels more comfortable
with Gtk-doc.
I'd like to see the documentation generated and possibly published on web
so suggestion from the guys who maintain the website are really welcome.

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

[Spice-devel] [PATCH] Use a marker instead of checking a RedPipeItem presence

[Frediano Ziglio]

This avoids having to retain a pointer just to check item is still in
the queue with ring_item_is_linked(>link).

Signed-off-by: Frediano Ziglio 
---
 server/red-channel.c | 30 ++
 server/red-channel.h |  1 +
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 23defc0..c0a9501 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -49,6 +49,11 @@ typedef struct RedEmptyMsgPipeItem {
 int msg;
 } RedEmptyMsgPipeItem;
 
+typedef struct MarkerPipeItem {
+RedPipeItem base;
+gboolean *item_in_pipe;
+} MarkerPipeItem;
+
 #define PING_TEST_TIMEOUT_MS (MSEC_PER_SEC * 15)
 #define PING_TEST_IDLE_NET_TIMEOUT_MS (MSEC_PER_SEC / 10)
 
@@ -574,6 +579,8 @@ static void red_channel_client_send_item(RedChannelClient 
*rcc, RedPipeItem *ite
 case RED_PIPE_ITEM_TYPE_PING:
 red_channel_client_send_ping(rcc);
 break;
+case RED_PIPE_ITEM_TYPE_MARKER:
+break;
 default:
 rcc->channel->channel_cbs.send_item(rcc, item);
 break;
@@ -2358,13 +2365,21 @@ int 
red_channel_client_wait_outgoing_item(RedChannelClient *rcc,
 }
 }
 
+static void marker_pipe_item_free(MarkerPipeItem *item)
+{
+if (item->item_in_pipe) {
+*item->item_in_pipe = FALSE;
+}
+free(item);
+}
+
 /* TODO: more evil sync stuff. anything with the word wait in it's name. */
 int red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
RedPipeItem *item,
int64_t timeout)
 {
 uint64_t end_time;
-int item_in_pipe;
+gboolean item_in_pipe;
 
 spice_info(NULL);
 
@@ -2374,7 +2389,13 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 end_time = UINT64_MAX;
 }
 
-red_pipe_item_ref(item);
+MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
+
+red_pipe_item_init_full(_item->base, RED_PIPE_ITEM_TYPE_MARKER,
+(GDestroyNotify)marker_pipe_item_free);
+item_in_pipe = TRUE;
+mark_item->item_in_pipe = _in_pipe;
+red_channel_client_pipe_add_after(rcc, _item->base, item);
 
 if (red_channel_client_blocked(rcc)) {
 red_channel_client_receive(rcc);
@@ -2382,7 +2403,7 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 }
 red_channel_client_push(rcc);
 
-while((item_in_pipe = ring_item_is_linked(>link)) &&
+while(item_in_pipe &&
   (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
 usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
 red_channel_client_receive(rcc);
@@ -2390,8 +2411,9 @@ int 
red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
 red_channel_client_push(rcc);
 }
 
-red_pipe_item_unref(item);
 if (item_in_pipe) {
+// still on the queue, make sure won't overwrite the stack variable
+mark_item->item_in_pipe = NULL;
 spice_warning("timeout");
 return FALSE;
 } else {
diff --git a/server/red-channel.h b/server/red-channel.h
index 8e8845e..c4eb436 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -143,6 +143,7 @@ enum {
 RED_PIPE_ITEM_TYPE_MIGRATE,
 RED_PIPE_ITEM_TYPE_EMPTY_MSG,
 RED_PIPE_ITEM_TYPE_PING,
+RED_PIPE_ITEM_TYPE_MARKER,
 
 RED_PIPE_ITEM_TYPE_CHANNEL_BASE=101,
 };
-- 
2.7.4

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

Re: [Spice-devel] [PATCH v2] remove a warning building a test

[Frediano Ziglio]

> 
> On 05/18/2016 07:14 AM, Christophe Fergeau wrote:
> > On Wed, May 18, 2016 at 12:46:03PM +0300, Uri Lublin wrote:
> >>
> >> In man cmsg examples I see they use a pointer:
> >> int *pi = (int*) CMSG_DATA(cmsg);
> >> *fd = *pi;
> > 
> > It seems to work this way, but I'm not sure why this works ;)
> > 
> 
> 
> I read an excelent post from Thiago Macieira (Qt) about this types of
> warnings a long time ago, maybe it will make things clearer for you too...
> 
> http://blog.qt.io/blog/2011/06/10/type-punning-and-strict-aliasing/
> 

Actually there are some mistake in the article.
Situation is sometimes better sometimes worst.

In this

  int i = 42;
  short s = *(short*)

compiler could rewrite to something like

  int i;
  short s = *(short*)
  i = 42;

so s can be anything.

  short s[2] = { 0, 42 };
  int i = *(int *)s;

still here can be anything and the crash is not due to strict aliasing.

I think the union is kind of a missing specification inside the standard
but really should work. Also gcc has a may_alias attribute which should
have been added to the standard (or similar specification). Considering that
C was introduced to help with stuff like kernels it's a big omission.

The inline of operator>> is a bug in the implementation, the standard clearly
state that there are aliasing in that code (as char* and or similar pointers
are used).

The List* -> Node* cast is a programming mistake, the solution is using
a Node** for the tail.

This C code - happily - don't compile anymore:

int i;
char *p1, *p2;
...
*i = i + p1 + p2;

Linux Kernel and other software disable the strict aliasing.

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

[Spice-devel] [PATCH 3/3] Call dcc_send_item directly

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c| 4 ++--
 server/dcc.h | 2 +-
 server/display-channel.c | 7 +--
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 05da07f..8ec22c8 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2367,9 +2367,9 @@ static void reset_send_data(DisplayChannelClient *dcc)
 memset(dcc->send_data.free_list.sync, 0, 
sizeof(dcc->send_data.free_list.sync));
 }
 
-void dcc_send_item(DisplayChannelClient *dcc, RedPipeItem *pipe_item)
+void dcc_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
 {
-RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
+DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
 SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 
 reset_send_data(dcc);
diff --git a/server/dcc.h b/server/dcc.h
index 864a768..a11d25a 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -203,7 +203,7 @@ void   dcc_append_drawable  
 (DisplayCha
 void   dcc_add_drawable_after
(DisplayChannelClient *dcc,
   Drawable 
*drawable,
   
RedPipeItem *pos);
-void   dcc_send_item 
(DisplayChannelClient *dcc,
+void   dcc_send_item 
(RedChannelClient *dcc,
   
RedPipeItem *item);
 intdcc_clear_surface_drawables_from_pipe 
(DisplayChannelClient *dcc,
   int 
surface_id,
diff --git a/server/display-channel.c b/server/display-channel.c
index b9ed285..9f97911 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1925,11 +1925,6 @@ static void on_disconnect(RedChannelClient *rcc)
 display->glz_drawable_count);
 }
 
-static void send_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-dcc_send_item(RCC_TO_DCC(rcc), item);
-}
-
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, 
DisplayChannel, common.base);
@@ -1969,7 +1964,7 @@ DisplayChannel* display_channel_new(SpiceServer *reds, 
RedWorker *worker,
 DisplayChannel *display;
 ChannelCbs cbs = {
 .on_disconnect = on_disconnect,
-.send_item = send_item,
+.send_item = dcc_send_item,
 .handle_migrate_flush_mark = handle_migrate_flush_mark,
 .handle_migrate_data = handle_migrate_data,
 .handle_migrate_data_get_serial = handle_migrate_data_get_serial
-- 
2.7.4

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

[Spice-devel] [PATCH 2/3] Handle reference for RedPipeItem in RedChannel

[Frediano Ziglio]

Remove the need to release the item inside send_item callbacks.
This looks like a partial rollback of previous patch but is
to make clear the intention of the change.
The lifetime of items could extend a bit further but there
are no cases this small lag should cause problems.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c | 1 -
 server/dcc-send.c   | 2 --
 server/inputs-channel.c | 1 -
 server/main-channel.c   | 2 --
 server/red-channel.c| 2 +-
 server/smartcard.c  | 2 --
 server/spicevmc.c   | 2 --
 7 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 104bf51..949cbc4 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -336,7 +336,6 @@ static void cursor_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *pipe_it
 spice_error("invalid pipe item type");
 }
 
-red_pipe_item_unref(pipe_item);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/dcc-send.c b/server/dcc-send.c
index f0f2e16..05da07f 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2455,8 +2455,6 @@ void dcc_send_item(DisplayChannelClient *dcc, RedPipeItem 
*pipe_item)
 spice_warn_if_reached();
 }
 
-red_pipe_item_unref(pipe_item);
-
 // a message is pending
 if (red_channel_client_send_message_pending(rcc)) {
 begin_send_message(rcc);
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 3218a48..4b5813f 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -289,7 +289,6 @@ static void inputs_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_warning("invalid pipe iten %d", base->type);
 break;
 }
-red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/main-channel.c b/server/main-channel.c
index 219212c..70aaff7 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -723,7 +723,6 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_printerr("Init msg for client %p was not sent yet "
"(client is probably during semi-seamless migration). 
Ignoring msg type %d",
rcc->client, base->type);
-red_pipe_item_unref(base);
 return;
 }
 switch (base->type) {
@@ -789,7 +788,6 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 main_channel_marshall_agent_connected(m, rcc, base);
 break;
 };
-red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/red-channel.c b/server/red-channel.c
index 8226bc4..23defc0 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -576,7 +576,7 @@ static void red_channel_client_send_item(RedChannelClient 
*rcc, RedPipeItem *ite
 break;
 default:
 rcc->channel->channel_cbs.send_item(rcc, item);
-return;
+break;
 }
 red_pipe_item_unref(item);
 }
diff --git a/server/smartcard.c b/server/smartcard.c
index e68ccdc..c39aeae 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -498,10 +498,8 @@ static void smartcard_channel_send_item(RedChannelClient 
*rcc, RedPipeItem *item
 break;
 default:
 spice_error("bad pipe item %d", item->type);
-red_pipe_item_unref(item);
 return;
 }
-red_pipe_item_unref(item);
 red_channel_client_begin_send_message(rcc);
 }
 
diff --git a/server/spicevmc.c b/server/spicevmc.c
index be51d54..1e68909 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -425,10 +425,8 @@ static void 
spicevmc_red_channel_send_item(RedChannelClient *rcc,
 break;
 default:
 spice_error("bad pipe item %d", item->type);
-red_pipe_item_unref(item);
 return;
 }
-red_pipe_item_unref(item);
 red_channel_client_begin_send_message(rcc);
 }
 
-- 
2.7.4

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

[Spice-devel] [PATCH 1/3] Remove RedChannel::hold_item callback

[Frediano Ziglio]

This is quite confusing and prone to errors.
Use RedPipeItem reference counting instead.
To compensate for the additional reference due to red_pipe_item_ref
in RedChannel sub class with empty hold_item have to add a
red_pipe_item_unref call in send_item.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c  | 8 
 server/display-channel.c | 8 
 server/inputs-channel.c  | 6 +-
 server/main-channel.c| 8 +---
 server/red-channel.c | 4 ++--
 server/red-channel.h | 2 --
 server/smartcard.c   | 9 ++---
 server/spicevmc.c| 8 +---
 8 files changed, 7 insertions(+), 46 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index fa462c5..104bf51 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -340,13 +340,6 @@ static void cursor_channel_send_item(RedChannelClient 
*rcc, RedPipeItem *pipe_it
 red_channel_client_begin_send_message(rcc);
 }
 
-static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-spice_return_if_fail(item);
-
-red_pipe_item_ref(item);
-}
-
 CursorChannel* cursor_channel_new(RedWorker *worker)
 {
 CursorChannel *cursor_channel;
@@ -354,7 +347,6 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
 ChannelCbs cbs = {
 .on_disconnect =  cursor_channel_client_on_disconnect,
 .send_item = cursor_channel_send_item,
-.hold_item = cursor_channel_hold_pipe_item,
 };
 
 spice_info("create cursor channel");
diff --git a/server/display-channel.c b/server/display-channel.c
index 3f414fd..b9ed285 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1930,13 +1930,6 @@ static void send_item(RedChannelClient *rcc, RedPipeItem 
*item)
 dcc_send_item(RCC_TO_DCC(rcc), item);
 }
 
-static void hold_item(RedChannelClient *rcc, RedPipeItem *item)
-{
-spice_return_if_fail(item);
-
-red_pipe_item_ref(item);
-}
-
 static int handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 DisplayChannel *display_channel = SPICE_CONTAINEROF(rcc->channel, 
DisplayChannel, common.base);
@@ -1977,7 +1970,6 @@ DisplayChannel* display_channel_new(SpiceServer *reds, 
RedWorker *worker,
 ChannelCbs cbs = {
 .on_disconnect = on_disconnect,
 .send_item = send_item,
-.hold_item = hold_item,
 .handle_migrate_flush_mark = handle_migrate_flush_mark,
 .handle_migrate_data = handle_migrate_data,
 .handle_migrate_data_get_serial = handle_migrate_data_get_serial
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index a3c9fb2..3218a48 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -289,6 +289,7 @@ static void inputs_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 spice_warning("invalid pipe iten %d", base->type);
 break;
 }
+red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
@@ -518,10 +519,6 @@ static int inputs_channel_config_socket(RedChannelClient 
*rcc)
 return TRUE;
 }
 
-static void inputs_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-}
-
 static void inputs_connect(RedChannel *channel, RedClient *client,
RedsStream *stream, int migration,
int num_common_caps, uint32_t *common_caps,
@@ -620,7 +617,6 @@ InputsChannel* inputs_channel_new(RedsState *reds)
 channel_cbs.config_socket = inputs_channel_config_socket;
 channel_cbs.on_disconnect = inputs_channel_on_disconnect;
 channel_cbs.send_item = inputs_channel_send_item;
-channel_cbs.hold_item = inputs_channel_hold_pipe_item;
 channel_cbs.alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
 channel_cbs.release_recv_buf = inputs_channel_release_msg_rcv_buf;
 channel_cbs.handle_migrate_data = inputs_channel_handle_migrate_data;
diff --git a/server/main-channel.c b/server/main-channel.c
index d540796..219212c 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -788,9 +788,8 @@ static void main_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *base)
 case RED_PIPE_ITEM_TYPE_MAIN_AGENT_CONNECTED_TOKENS:
 main_channel_marshall_agent_connected(m, rcc, base);
 break;
-default:
-break;
 };
+red_pipe_item_unref(base);
 red_channel_client_begin_send_message(rcc);
 }
 
@@ -1013,10 +1012,6 @@ static int main_channel_config_socket(RedChannelClient 
*rcc)
 return TRUE;
 }
 
-static void main_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem 
*item)
-{
-}
-
 static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 spice_debug(NULL);
@@ -1154,7 +1149,6 @@ MainChannel* main_channel_new(RedsState *reds)
 channel_cbs.config_socket = main_channel_config_socket;
 channel_cbs.on_disconnect = main_channel_client_on_disconnect;
 channel_cbs.send_item = main_channel_send_item;
-   

Re: [Spice-devel] [PATCH spice-gtk v3 6/7] spice-uri: Check if port is in allowed range

[Pavel Grunt]

On Fri, 2016-05-20 at 10:36 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:08PM +0200, Pavel Grunt wrote:
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c| 8 ++--
> >  tests/test-spice-uri.c | 2 ++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index b483374..6a43461 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -165,8 +165,8 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  uri_port = uriv[1];
> >  
> >  if (uri_port != NULL) {
> > -char *endptr;
> > -guint port = strtoul(uri_port, , 10);
> > +gchar *endptr;
> > +gint64 port = g_ascii_strtoll(uri_port, , 10);
> >  if (*endptr != '\0') {
> >  g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> >  "Invalid uri port: %s", uri_port);
> > @@ -175,6 +175,10 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
> >  goto end;
> >  }
> > +if (port < 0 || port > 65535) {
> > +g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED, "Port out of range");
> > +goto end;
> > +}
> >  spice_uri_set_port(self, port);
> >  }
> >  
> > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > index 42c9aad..c32a343 100644
> > --- a/tests/test-spice-uri.c
> > +++ b/tests/test-spice-uri.c
> > @@ -35,6 +35,8 @@ static void test_spice_uri_ipv4(void)
> >  {"http://;, "http", NULL, 3128, NULL, NULL},
> >  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL},
> > /* invalid port */
> >  {"http://127.0.0.1:;, "http", "127.0.0.1", 3128, NULL, NULL},
> > +{"http://127.0.0.1:-80;, "http", "127.0.0.1", 3128, NULL, NULL},
> > +{"http://127.0.0.1:800;, "http", "127.0.0.1", 3128, NULL,
> > NULL},
> >  };
> >  const struct test_case valid_test_cases[] = {
> >  {"http://user:password@host:80;, "http", "host", 80, "user",
> > "password"},
> 
> Looks good. You might want to include test to check the error you have
> included.

Check for the error message ? Code/domain is the same.

I will add it.

Thanks,
Pavel

> 
> Acked-by: Victor Toso 
> 
> > -- 
> > 2.8.2
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 3/7] spice-uri: Add missing include

[Pavel Grunt]

On Fri, 2016-05-20 at 10:14 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:05PM +0200, Pavel Grunt wrote:
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index 8cf870d..3bdb502 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -22,6 +22,7 @@
> > 
> >  #include "spice-client.h"
> >  #include "spice-uri.h"
> > +#include "spice-uri-priv.h"
> 
> The -priv is internal and it always includes the non -priv one.
> I would change the include above spice-uri.h to spice-uri-priv.h

I would include both, but can remove the other one if you prefer.

Pavel

> 
> Acked-by: Victor Toso 
> 
> >  
> >  /**
> >   * SECTION:spice-uri
> > -- 
> > 2.8.2
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 5/7] spice-uri: Do not allow empty port string

[Pavel Grunt]

On Fri, 2016-05-20 at 10:32 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:07PM +0200, Pavel Grunt wrote:
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c| 3 +++
> >  tests/test-spice-uri.c | 1 +
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index 04ea3cb..b483374 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -171,6 +171,9 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> >  "Invalid uri port: %s", uri_port);
> >  goto end;
> > +} else if (endptr == uri_port) {
> > +g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
> > +goto end;
> 
> You might want to use/include the return value of strtoul for zero in this
> case

I would not change the value, parsing failed so SpiceURI should not be consider
valid

Pavel

> 
> Acked-by: Victor Toso 
> 
> >  }
> >  spice_uri_set_port(self, port);
> >  }
> > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > index 1b14dbb..42c9aad 100644
> > --- a/tests/test-spice-uri.c
> > +++ b/tests/test-spice-uri.c
> > @@ -34,6 +34,7 @@ static void test_spice_uri_ipv4(void)
> >  {"http://:80;, "http", NULL, 80, NULL, NULL}, /* missing hostname
> > */
> >  {"http://;, "http", NULL, 3128, NULL, NULL},
> >  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL},
> > /* invalid port */
> > +{"http://127.0.0.1:;, "http", "127.0.0.1", 3128, NULL, NULL},
> >  };
> >  const struct test_case valid_test_cases[] = {
> >  {"http://user:password@host:80;, "http", "host", 80, "user",
> > "password"},
> > -- 
> > 2.8.2
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 4/7] spice-uri: Reset SpiceUri before parsing

[Pavel Grunt]

On Fri, 2016-05-20 at 10:24 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:06PM +0200, Pavel Grunt wrote:
> > Avoid using old values after parsing a new uri.
> 
> A test case with re-using the SpiceURI could be handy
> 
All the tests are reusing SpiceURI

> > 
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c| 7 +++
> >  tests/test-spice-uri.c | 4 ++--
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index 3bdb502..04ea3cb 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -105,6 +105,13 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  size_t len;
> > 
> >  g_return_val_if_fail(self != NULL, FALSE);
> > +
> > +self->scheme = NULL;
> > +self->hostname = NULL;
> > +self->port = 0;
> > +self->user = NULL;
> > +self->password = NULL;
> > +
> 
> g_clear_pointer to avoid leaks?

sure, thanks a lot, stupid me /o\

Pavel
> 
> >  g_return_val_if_fail(_uri != NULL, FALSE);
> >  
> >  uri = dup = g_strdup(_uri);
> > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > index e8cfcc9..1b14dbb 100644
> > --- a/tests/test-spice-uri.c
> > +++ b/tests/test-spice-uri.c
> > @@ -36,10 +36,10 @@ static void test_spice_uri_ipv4(void)
> >  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL},
> > /* invalid port */
> >  };
> >  const struct test_case valid_test_cases[] = {
> > -{"http://127.0.0.1/;, "http", "127.0.0.1", 3128, NULL, NULL},
> > +{"http://user:password@host:80;, "http", "host", 80, "user",
> > "password"},
> > +{"http://127.0.0.1/;, "http", "127.0.0.1", 3128, NULL, NULL}, /*
> > reset user & password */
> >  {"https://127.0.0.1;, "https", "127.0.0.1", 3129, NULL, NULL},
> >  {"127.0.0.1", "http", "127.0.0.1", 3128, NULL, NULL},
> > -{"http://user:password@host:80;, "http", "host", 80, "user",
> > "password"},
> >  };
> >  
> >  guint i;
> > -- 
> > 2.8.2
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 7/7] spice-uri: Add ipv6 support

[Pavel Grunt]

On Fri, 2016-05-20 at 10:54 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:09PM +0200, Pavel Grunt wrote:
> > Just basic support -  http://user:password@[host]:port
> > 
> > Resolves: rhbz#1335239
> > ---
> >  src/spice-uri.c| 25 +
> >  tests/test-spice-uri.c | 42 ++
> >  2 files changed, 63 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index 6a43461..323fbf7 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -150,10 +150,29 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  uri = next;
> >  }
> >  
> > -/* max 2 parts, host:port */
> > -gchar **uriv = g_strsplit(uri, ":", 2);
> > +gchar **uriv;
> >  const gchar *uri_port = NULL;
> >  
> > +if (*uri == '[') { /* ipv6 address */
> > +uriv = g_strsplit(uri + 1, "]", 2);
> > +if (uriv[1] == NULL) {
> > +g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > +"Missing ']' in ipv6 uri");
> > +goto end;
> > +}
> > +if (*uriv[1] == ':') {
> > +uri_port = uriv[1] + 1;
> > +} else if (strlen(uriv[1]) > 0) { /* invalid string after the
> > hostname */
> > +g_set_error(error, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > +"Invalid uri address");
> > +goto end;
> > +}
> > +} else {
> > +/* max 2 parts, host:port */
> > +uriv = g_strsplit(uri, ":", 2);
> > +uri_port = uriv[1];
> > +}
> > +
> 
> This seems to leak (I think Frediano said it before)
> 
This should not leak, uriv is initialized by g_strsplit() and in the
end g_strfreev(uriv) is called.

The leaks come from the patch "spice-uri: Reset SpiceUri before parsing" as you
noticed before

Thanks!
Pavel


> Full valgrind in the end.
> 
> 
> >  if (uriv[0] == NULL || strlen(uriv[0]) == 0) {
> >  g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> >  "Invalid hostname in uri address");
> > @@ -161,8 +180,6 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > *_uri, GError **error)
> >  }
> >  
> >  spice_uri_set_hostname(self, uriv[0]);
> > -if (uriv[0] != NULL)
> > -uri_port = uriv[1];
> >  
> >  if (uri_port != NULL) {
> >  gchar *endptr;
> > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > index c32a343..9cdcb90 100644
> > --- a/tests/test-spice-uri.c
> > +++ b/tests/test-spice-uri.c
> > @@ -66,11 +66,53 @@ static void test_spice_uri_ipv4(void)
> >  g_object_unref(uri);
> >  }
> >  
> > +static void test_spice_uri_ipv6(void)
> > +{
> > +const struct test_case invalid_test_cases[] = {
> > +{"http://[]:80;, "http", NULL, 80, NULL, NULL},
> > +{"http://[::1;, "http", NULL, 3128, NULL, NULL},
> > +{"http://[host]1234;, "http", "host", 3128, NULL, NULL},
> > +{"http://[host]foo/;, "http", "host", 3128, NULL, NULL},
> > +{"http://[::1]:port;, "http", "::1", 3128, NULL, NULL},
> > +{"http://[::127.0.0.1]:;, "http", "::127.0.0.1", 3128, NULL, NULL},
> > +{"http://[::127.0.0.1]:-42;, "http", "::127.0.0.1", 3128, NULL,
> > NULL},
> > +{"[3ffe:2a00:100:7031::1]:4200", "http",
> > "3ffe:2a00:100:7031::1", 3128, NULL, NULL},
> > +};
> > +const struct test_case valid_test_cases[] = {
> > +{"http://user:password@[host]:80/;, "http", "host", 80, "user",
> > "password"},
> > +{"http://user@[1080:0:0:0:8:800:200C:4171]:100;, "http",
> > "1080:0:0:0:8:800:200C:4171", 100,
> > + "user", NULL},
> > +{"https://[1080::8:800:200C:417A];, "https",
> > "1080::8:800:200C:417A", 3129, NULL, NULL},
> > +{"[3ffe:2a00:100:7031::1]", "http", "3ffe:2a00:100:7031::1", 3128,
> > NULL, NULL},
> > +};
> > +
> > +guint i;
> > +
> > +SpiceURI *uri = spice_uri_new();
> > +g_assert_nonnull(uri);
> > +
> > +for (i = 0; i < G_N_ELEMENTS(invalid_test_cases); i++) {
> > +g_assert_false(spice_uri_parse(uri, invalid_test_cases[i].uri,
> > NULL));
> > +}
> > +
> > +for (i = 0; i < G_N_ELEMENTS(valid_test_cases); i++) {
> > +g_assert_true(spice_uri_parse(uri, valid_test_cases[i].uri, NULL));
> > +g_assert_cmpstr(spice_uri_get_scheme(uri), ==,
> > valid_test_cases[i].scheme);
> > +g_assert_cmpstr(spice_uri_get_hostname(uri), ==,
> > valid_test_cases[i].hostname);
> > +g_assert_cmpstr(spice_uri_get_user(uri), ==,
> > valid_test_cases[i].user);
> > +g_assert_cmpstr(spice_uri_get_password(uri), ==,
> > valid_test_cases[i].password);
> > +g_assert_cmpuint(spice_uri_get_port(uri), ==,
> > valid_test_cases[i].port);
> > +}
> > +
> > +g_object_unref(uri);
> > +}
> > +
> >  int main(int argc, char* argv[])
> >  {
> >  

Re: [Spice-devel] [PATCH spice-gtk v3 7/7] spice-uri: Add ipv6 support

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:09PM +0200, Pavel Grunt wrote:
> Just basic support -  http://user:password@[host]:port
> 
> Resolves: rhbz#1335239
> ---
>  src/spice-uri.c| 25 +
>  tests/test-spice-uri.c | 42 ++
>  2 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index 6a43461..323fbf7 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -150,10 +150,29 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  uri = next;
>  }
>  
> -/* max 2 parts, host:port */
> -gchar **uriv = g_strsplit(uri, ":", 2);
> +gchar **uriv;
>  const gchar *uri_port = NULL;
>  
> +if (*uri == '[') { /* ipv6 address */
> +uriv = g_strsplit(uri + 1, "]", 2);
> +if (uriv[1] == NULL) {
> +g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> +"Missing ']' in ipv6 uri");
> +goto end;
> +}
> +if (*uriv[1] == ':') {
> +uri_port = uriv[1] + 1;
> +} else if (strlen(uriv[1]) > 0) { /* invalid string after the 
> hostname */
> +g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> +"Invalid uri address");
> +goto end;
> +}
> +} else {
> +/* max 2 parts, host:port */
> +uriv = g_strsplit(uri, ":", 2);
> +uri_port = uriv[1];
> +}
> +

This seems to leak (I think Frediano said it before)

Full valgrind in the end.


>  if (uriv[0] == NULL || strlen(uriv[0]) == 0) {
>  g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>  "Invalid hostname in uri address");
> @@ -161,8 +180,6 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  }
>  
>  spice_uri_set_hostname(self, uriv[0]);
> -if (uriv[0] != NULL)
> -uri_port = uriv[1];
>  
>  if (uri_port != NULL) {
>  gchar *endptr;
> diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> index c32a343..9cdcb90 100644
> --- a/tests/test-spice-uri.c
> +++ b/tests/test-spice-uri.c
> @@ -66,11 +66,53 @@ static void test_spice_uri_ipv4(void)
>  g_object_unref(uri);
>  }
>  
> +static void test_spice_uri_ipv6(void)
> +{
> +const struct test_case invalid_test_cases[] = {
> +{"http://[]:80;, "http", NULL, 80, NULL, NULL},
> +{"http://[::1;, "http", NULL, 3128, NULL, NULL},
> +{"http://[host]1234;, "http", "host", 3128, NULL, NULL},
> +{"http://[host]foo/;, "http", "host", 3128, NULL, NULL},
> +{"http://[::1]:port;, "http", "::1", 3128, NULL, NULL},
> +{"http://[::127.0.0.1]:;, "http", "::127.0.0.1", 3128, NULL, NULL},
> +{"http://[::127.0.0.1]:-42;, "http", "::127.0.0.1", 3128, NULL, 
> NULL},
> +{"[3ffe:2a00:100:7031::1]:4200", "http", 
> "3ffe:2a00:100:7031::1", 3128, NULL, NULL},
> +};
> +const struct test_case valid_test_cases[] = {
> +{"http://user:password@[host]:80/;, "http", "host", 80, "user", 
> "password"},
> +{"http://user@[1080:0:0:0:8:800:200C:4171]:100;, "http", 
> "1080:0:0:0:8:800:200C:4171", 100,
> + "user", NULL},
> +{"https://[1080::8:800:200C:417A];, "https", 
> "1080::8:800:200C:417A", 3129, NULL, NULL},
> +{"[3ffe:2a00:100:7031::1]", "http", "3ffe:2a00:100:7031::1", 3128, 
> NULL, NULL},
> +};
> +
> +guint i;
> +
> +SpiceURI *uri = spice_uri_new();
> +g_assert_nonnull(uri);
> +
> +for (i = 0; i < G_N_ELEMENTS(invalid_test_cases); i++) {
> +g_assert_false(spice_uri_parse(uri, invalid_test_cases[i].uri, 
> NULL));
> +}
> +
> +for (i = 0; i < G_N_ELEMENTS(valid_test_cases); i++) {
> +g_assert_true(spice_uri_parse(uri, valid_test_cases[i].uri, NULL));
> +g_assert_cmpstr(spice_uri_get_scheme(uri), ==, 
> valid_test_cases[i].scheme);
> +g_assert_cmpstr(spice_uri_get_hostname(uri), ==, 
> valid_test_cases[i].hostname);
> +g_assert_cmpstr(spice_uri_get_user(uri), ==, 
> valid_test_cases[i].user);
> +g_assert_cmpstr(spice_uri_get_password(uri), ==, 
> valid_test_cases[i].password);
> +g_assert_cmpuint(spice_uri_get_port(uri), ==, 
> valid_test_cases[i].port);
> +}
> +
> +g_object_unref(uri);
> +}
> +
>  int main(int argc, char* argv[])
>  {
>  g_test_init(, , NULL);
>  
>  g_test_add_func("/spice_uri/ipv4", test_spice_uri_ipv4);
> +g_test_add_func("/spice_uri/ipv6", test_spice_uri_ipv6);
>
>  return g_test_run();
>  }

Yeah, I would split the tests with good-uri (should work) and bad-uri
(should fail)
/spice-uri/ipv4/good-uri
/spice-uri/ipv6/good-uri
/spice-uri/ipv4/bad-uri
/spice-uri/ipv6/bad-uri

Cheers,
  toso


> -- 
> 2.8.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> 

Re: [Spice-devel] [PATCH spice-gtk v3 6/7] spice-uri: Check if port is in allowed range

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:08PM +0200, Pavel Grunt wrote:
> Related: rhbz#1335239
> ---
>  src/spice-uri.c| 8 ++--
>  tests/test-spice-uri.c | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index b483374..6a43461 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -165,8 +165,8 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  uri_port = uriv[1];
>  
>  if (uri_port != NULL) {
> -char *endptr;
> -guint port = strtoul(uri_port, , 10);
> +gchar *endptr;
> +gint64 port = g_ascii_strtoll(uri_port, , 10);
>  if (*endptr != '\0') {
>  g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>  "Invalid uri port: %s", uri_port);
> @@ -175,6 +175,10 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  g_set_error(error, SPICE_CLIENT_ERROR, 
> SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
>  goto end;
>  }
> +if (port < 0 || port > 65535) {
> +g_set_error(error, SPICE_CLIENT_ERROR, 
> SPICE_CLIENT_ERROR_FAILED, "Port out of range");
> +goto end;
> +}
>  spice_uri_set_port(self, port);
>  }
>  
> diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> index 42c9aad..c32a343 100644
> --- a/tests/test-spice-uri.c
> +++ b/tests/test-spice-uri.c
> @@ -35,6 +35,8 @@ static void test_spice_uri_ipv4(void)
>  {"http://;, "http", NULL, 3128, NULL, NULL},
>  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL}, /* 
> invalid port */
>  {"http://127.0.0.1:;, "http", "127.0.0.1", 3128, NULL, NULL},
> +{"http://127.0.0.1:-80;, "http", "127.0.0.1", 3128, NULL, NULL},
> +{"http://127.0.0.1:800;, "http", "127.0.0.1", 3128, NULL, NULL},
>  };
>  const struct test_case valid_test_cases[] = {
>  {"http://user:password@host:80;, "http", "host", 80, "user", 
> "password"},

Looks good. You might want to include test to check the error you have
included.

Acked-by: Victor Toso 

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

Re: [Spice-devel] [PATCH spice-gtk v3 5/7] spice-uri: Do not allow empty port string

[Victor Toso]

On Fri, May 20, 2016 at 10:32:00AM +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, May 19, 2016 at 06:38:07PM +0200, Pavel Grunt wrote:
> > Related: rhbz#1335239
> > ---
> >  src/spice-uri.c| 3 +++
> >  tests/test-spice-uri.c | 1 +
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > index 04ea3cb..b483374 100644
> > --- a/src/spice-uri.c
> > +++ b/src/spice-uri.c
> > @@ -171,6 +171,9 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> > *_uri, GError **error)
> >  g_set_error(error, SPICE_CLIENT_ERROR, 
> > SPICE_CLIENT_ERROR_FAILED,
> >  "Invalid uri port: %s", uri_port);
> >  goto end;
> > +} else if (endptr == uri_port) {
> > +g_set_error(error, SPICE_CLIENT_ERROR, 
> > SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
> > +goto end;
> 
> You might want to use/include the return value of strtoul for zero in this 
> case
> 
> Acked-by: Victor Toso 
> 
> >  }
> >  spice_uri_set_port(self, port);
> >  }
> > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > index 1b14dbb..42c9aad 100644
> > --- a/tests/test-spice-uri.c
> > +++ b/tests/test-spice-uri.c
> > @@ -34,6 +34,7 @@ static void test_spice_uri_ipv4(void)
> >  {"http://:80;, "http", NULL, 80, NULL, NULL}, /* missing hostname 
> > */
> >  {"http://;, "http", NULL, 3128, NULL, NULL},
> >  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL}, 
> > /* invalid port */
> > +{"http://127.0.0.1:;, "http", "127.0.0.1", 3128, NULL, NULL},
> >  };
> >  const struct test_case valid_test_cases[] = {
> >  {"http://user:password@host:80;, "http", "host", 80, "user", 
> > "password"},
> > -- 
> > 2.8.2

You might want to include a test checking the error that you have set :)

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

Re: [Spice-devel] [PATCH spice-gtk v3 5/7] spice-uri: Do not allow empty port string

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:07PM +0200, Pavel Grunt wrote:
> Related: rhbz#1335239
> ---
>  src/spice-uri.c| 3 +++
>  tests/test-spice-uri.c | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index 04ea3cb..b483374 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -171,6 +171,9 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>  "Invalid uri port: %s", uri_port);
>  goto end;
> +} else if (endptr == uri_port) {
> +g_set_error(error, SPICE_CLIENT_ERROR, 
> SPICE_CLIENT_ERROR_FAILED, "Missing uri port");
> +goto end;

You might want to use/include the return value of strtoul for zero in this case

Acked-by: Victor Toso 

>  }
>  spice_uri_set_port(self, port);
>  }
> diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> index 1b14dbb..42c9aad 100644
> --- a/tests/test-spice-uri.c
> +++ b/tests/test-spice-uri.c
> @@ -34,6 +34,7 @@ static void test_spice_uri_ipv4(void)
>  {"http://:80;, "http", NULL, 80, NULL, NULL}, /* missing hostname */
>  {"http://;, "http", NULL, 3128, NULL, NULL},
>  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL}, /* 
> invalid port */
> +{"http://127.0.0.1:;, "http", "127.0.0.1", 3128, NULL, NULL},
>  };
>  const struct test_case valid_test_cases[] = {
>  {"http://user:password@host:80;, "http", "host", 80, "user", 
> "password"},
> -- 
> 2.8.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 4/7] spice-uri: Reset SpiceUri before parsing

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:06PM +0200, Pavel Grunt wrote:
> Avoid using old values after parsing a new uri.

A test case with re-using the SpiceURI could be handy

>
> Related: rhbz#1335239
> ---
>  src/spice-uri.c| 7 +++
>  tests/test-spice-uri.c | 4 ++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index 3bdb502..04ea3cb 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -105,6 +105,13 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar 
> *_uri, GError **error)
>  size_t len;
>
>  g_return_val_if_fail(self != NULL, FALSE);
> +
> +self->scheme = NULL;
> +self->hostname = NULL;
> +self->port = 0;
> +self->user = NULL;
> +self->password = NULL;
> +

g_clear_pointer to avoid leaks?

>  g_return_val_if_fail(_uri != NULL, FALSE);
>  
>  uri = dup = g_strdup(_uri);
> diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> index e8cfcc9..1b14dbb 100644
> --- a/tests/test-spice-uri.c
> +++ b/tests/test-spice-uri.c
> @@ -36,10 +36,10 @@ static void test_spice_uri_ipv4(void)
>  {"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL}, /* 
> invalid port */
>  };
>  const struct test_case valid_test_cases[] = {
> -{"http://127.0.0.1/;, "http", "127.0.0.1", 3128, NULL, NULL},
> +{"http://user:password@host:80;, "http", "host", 80, "user", 
> "password"},
> +{"http://127.0.0.1/;, "http", "127.0.0.1", 3128, NULL, NULL}, /* 
> reset user & password */
>  {"https://127.0.0.1;, "https", "127.0.0.1", 3129, NULL, NULL},
>  {"127.0.0.1", "http", "127.0.0.1", 3128, NULL, NULL},
> -{"http://user:password@host:80;, "http", "host", 80, "user", 
> "password"},
>  };
>  
>  guint i;
> -- 
> 2.8.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 3/7] spice-uri: Add missing include

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:05PM +0200, Pavel Grunt wrote:
> Related: rhbz#1335239
> ---
>  src/spice-uri.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index 8cf870d..3bdb502 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -22,6 +22,7 @@
>
>  #include "spice-client.h"
>  #include "spice-uri.h"
> +#include "spice-uri-priv.h"

The -priv is internal and it always includes the non -priv one.
I would change the include above spice-uri.h to spice-uri-priv.h

Acked-by: Victor Toso 

>  
>  /**
>   * SECTION:spice-uri
> -- 
> 2.8.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 2/7] spice-uri: Mark parameter as unused

[Victor Toso]

On Thu, May 19, 2016 at 06:38:04PM +0200, Pavel Grunt wrote:
> ---
>  src/spice-uri.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/spice-uri.c b/src/spice-uri.c
> index e1317bd..8cf870d 100644
> --- a/src/spice-uri.c
> +++ b/src/spice-uri.c
> @@ -340,7 +340,7 @@ static void spice_uri_finalize(GObject* obj)
>  G_OBJECT_CLASS (spice_uri_parent_class)->finalize (obj);
>  }
>  
> -static void spice_uri_init (SpiceURI *self)
> +static void spice_uri_init (SpiceURI *self G_GNUC_UNUSED)
>  {
>  }

Acked-by: Victor Toso 

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

Re: [Spice-devel] [PATCH spice-gtk v3 1/7] tests: Add test for SpiceUri

[Victor Toso]

Hi,

On Thu, May 19, 2016 at 06:38:03PM +0200, Pavel Grunt wrote:
> Related: rhbz#1335239
> ---
>  tests/Makefile.am  |  2 ++
>  tests/test-spice-uri.c | 73 
> ++
>  2 files changed, 75 insertions(+)
>  create mode 100644 tests/test-spice-uri.c
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c1d95c1..fb97138 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -4,6 +4,7 @@ noinst_PROGRAMS =
>  TESTS = coroutine\
>   util\
>   session \
> + test-spice-uri  \

The other binaries don't have 'test-' prefix but I actually liked it.
My vote is to change the other binaries to have it too. :)

>   $(NULL)
>
>  if WITH_PHODAV
> @@ -35,6 +36,7 @@ util_SOURCES = util.c
>  coroutine_SOURCES = coroutine.c
>  session_SOURCES = session.c
>  pipe_SOURCES = pipe.c
> +test_spice_uri_SOURCES = test-spice-uri.c
>  usb_acl_helper_SOURCES = usb-acl-helper.c
>  usb_acl_helper_CFLAGS = -DTESTDIR=\"$(abs_builddir)\"
>  mock_acl_helper_SOURCES = mock-acl-helper.c
> diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> new file mode 100644
> index 000..e8cfcc9
> --- /dev/null
> +++ b/tests/test-spice-uri.c
> @@ -0,0 +1,73 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2016 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 
> .
> +*/
> +#include 
> +#include 
> +#include "spice-uri-priv.h"
> +
> +struct test_case {
> +gchar *uri;
> +gchar *scheme;
> +gchar *hostname;
> +guint port;
> +gchar *user;
> +gchar *password;
> +};
> +
> +static void test_spice_uri_ipv4(void)
> +{
> +const struct test_case invalid_test_cases[] = {
> +{"http://:80;, "http", NULL, 80, NULL, NULL}, /* missing hostname */
> +{"http://;, "http", NULL, 3128, NULL, NULL},
> +{"http://127.0.0.1:port;, "http", "127.0.0.1", 3128, NULL, NULL}, /* 
> invalid port */
> +};
> +const struct test_case valid_test_cases[] = {
> +{"http://127.0.0.1/;, "http", "127.0.0.1", 3128, NULL, NULL},
> +{"https://127.0.0.1;, "https", "127.0.0.1", 3129, NULL, NULL},
> +{"127.0.0.1", "http", "127.0.0.1", 3128, NULL, NULL},
> +{"http://user:password@host:80;, "http", "host", 80, "user", 
> "password"},
> +};
> +
> +guint i;
> +
> +SpiceURI *uri = spice_uri_new();
> +g_assert_nonnull(uri);
> +
> +for (i = 0; i < G_N_ELEMENTS(invalid_test_cases); i++) {
> +g_assert_false(spice_uri_parse(uri, invalid_test_cases[i].uri, 
> NULL));
> +}
> +
> +for (i = 0; i < G_N_ELEMENTS(valid_test_cases); i++) {
> +g_assert_true(spice_uri_parse(uri, valid_test_cases[i].uri, NULL));
> +g_assert_cmpstr(spice_uri_get_scheme(uri), ==, 
> valid_test_cases[i].scheme);
> +g_assert_cmpstr(spice_uri_get_hostname(uri), ==, 
> valid_test_cases[i].hostname);
> +g_assert_cmpstr(spice_uri_get_user(uri), ==, 
> valid_test_cases[i].user);
> +g_assert_cmpstr(spice_uri_get_password(uri), ==, 
> valid_test_cases[i].password);
> +g_assert_cmpuint(spice_uri_get_port(uri), ==, 
> valid_test_cases[i].port);
> +}
> +
> +g_object_unref(uri);
> +}
> +
> +int main(int argc, char* argv[])
> +{
> +g_test_init(, , NULL);
> +
> +g_test_add_func("/spice_uri/ipv4", test_spice_uri_ipv4);

For the testpath I would go with "spice-uri" instead of underscore.

You could also split this in two different tests, the ones that should
work and the ones that should not.

IMHO you don't need to send this again, feel free to take/ignore the
suggestions above.

Acked-by: Victor Toso 

> +
> +return g_test_run();
> +}
> -- 
> 2.8.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] Remove unnecessary cursor_pipe_item_ref()

[Pavel Grunt]

Hi,

Am I right that your next step is to remove hold_item() ?

On Thu, 2016-05-19 at 14:05 -0500, Jonathon Jongsma wrote:
> Use the base red_pipe_item_ref() instead of adding an additional static
> wrapper function.

Acked-by: Pavel Grunt 
> ---
>  server/cursor-channel.c | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index c0b2fda..fa462c5 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -340,23 +340,11 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, RedPipeItem *pipe_it
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> -static RedCursorPipeItem *cursor_pipe_item_ref(RedCursorPipeItem *item)
> -{
> -spice_return_val_if_fail(item, NULL);
> -
> -red_pipe_item_ref(item);
> -return item;
> -}
> -
> -
>  static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, RedPipeItem
> *item)
>  {
> -RedCursorPipeItem *cursor_pipe_item;
> -
>  spice_return_if_fail(item);
>  
> -cursor_pipe_item = SPICE_CONTAINEROF(item, RedCursorPipeItem, base);
> -cursor_pipe_item_ref(cursor_pipe_item);
> +red_pipe_item_ref(item);
>  }
>  
>  CursorChannel* cursor_channel_new(RedWorker *worker)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] Simplify DisplayChannel hold_item()

[Pavel Grunt]

On Thu, 2016-05-19 at 14:05 -0500, Jonathon Jongsma wrote:
> Since all pipe items implement refcounting now, just call
> red_pipe_item_ref() and eliminate the switch statement.

Acked-by: Pavel Grunt 
> ---
>  server/display-channel.c | 11 +--
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 1487f10..3f414fd 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1934,16 +1934,7 @@ static void hold_item(RedChannelClient *rcc,
> RedPipeItem *item)
>  {
>  spice_return_if_fail(item);
>  
> -switch (item->type) {
> -case RED_PIPE_ITEM_TYPE_DRAW:
> -case RED_PIPE_ITEM_TYPE_IMAGE:
> -case RED_PIPE_ITEM_TYPE_STREAM_CLIP:
> -case RED_PIPE_ITEM_TYPE_UPGRADE:
> -red_pipe_item_ref(item);
> -break;
> -default:
> -spice_warn_if_reached();
> -}
> +red_pipe_item_ref(item);
>  }
>  
>  static int handle_migrate_flush_mark(RedChannelClient *rcc)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel