Re: [Spice-devel] [PATCH spice 2/2] Do not enable statistic by default

2016-10-26 Thread Pavel Grunt 
On Wed, 2016-10-26 at 10:56 -0400, Frediano Ziglio wrote:
> > 
> > ---
> >  server/Makefile.am | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index 2a6010b..0514a2c 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -3,7 +3,6 @@ SUBDIRS = . tests
> >  
> >  AM_CPPFLAGS =  \
> >     -DSPICE_SERVER_INTERNAL \
> > -   -DRED_STATISTICS\
> >     $(COMMON_CFLAGS)\
> >     $(GLIB2_CFLAGS) \
> >     $(GOBJECT2_CFLAGS)  \
> 
> I think the rationale here is nobody uses these.

yes. I don't think statistics are interesting for everybody. It is
more a feature for developers, lets not force them.

> Actually I think these statistics could be extended with stuff
> like image compression (size ratio at least, perhaps cpu could
> be expensive).
Why not, it can be useful for the performance measure Jonathon talked
about.
> 
We have many defines to extend debugging:
 * DEBUG
 * DEBUG_DISPATCHER
 * DEBUG_ENCODE
 * COMPRESS_DEBUG
 * STREAM_STAT
 * COMPRESS_STAT
 * RED_WORKER_STAT
 * RED_STATISTIC
and more are in spice-common

imho we should consider merging them, I believe it would make them
easier to use

Pavel

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

Re: [Spice-devel] QXL-WDDM-DOD v0.4-1 released

2016-10-26 Thread Dmitry Fleytman 
Hi Christophe,

> On 26 Oct 2016, at 19:17 PM, Christophe Fergeau  wrote:
> 
> Hey Sameeh,
> 
> On Wed, Oct 26, 2016 at 06:37:12PM +0300, Sameeh Jubran wrote:
>> And as Frediano mentioned this could be an issue with the mouse as there
>> was set of patches that weren't accepted which solves a performance
>> issue caused by the mouse mode when running vd-agent.
> 
> I did not check which patches exactly you have in mind, but I'd expect
> there were review comments/questions on these patches to be addressed
> before moving forward with them? If they are causing such bad
> performance issues, we should revisit them ;)

Sameeh meant patches that disable forwarding of
mouse events to device when VDAgent is running.

Patches were written in assumption that running
VDAgent always means system is in client mouse mode.
And, as turned out, this assumption was wrong so
patches need rework.

This task is in our list and we’re investigating what can be done.

According to earlier discussions on the list the best way is to indicate
mouse mode to driver via device interface.


Dmitry.

> 
> Christophe

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

Re: [Spice-devel] [PATCH spice 1/2] Allow to compile without RED_STATISTICS

2016-10-26 Thread Pavel Grunt 
On Wed, 2016-10-26 at 10:54 -0400, Frediano Ziglio wrote:
> Looks like we are working on similar stuff!


> 
> > 
> > ---
> >  server/dcc-send.c| 2 +-
> >  server/main-channel-client.c | 2 +-
> >  server/red-channel.c | 6 ++
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > index ef67f97..54844cf 100644
> > --- a/server/dcc-send.c
> > +++ b/server/dcc-send.c
> > @@ -185,7 +185,7 @@ static void
> > red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> >    SpiceImage
> > *image,
> >    SpiceImage
> > *io_image,
> >    int is_lossy)
> >  {
> > -DisplayChannel *display_channel =
> > +DisplayChannel *display_channel G_GNUC_UNUSED =
> >  DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> >  DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
> >  
> > diff --git a/server/main-channel-client.c b/server/main-channel-
> > client.c
> > index 836c50e..0c8a3a4 100644
> > --- a/server/main-channel-client.c
> > +++ b/server/main-channel-client.c
> > @@ -161,11 +161,11 @@ static void
> > main_channel_client_set_property(GObject
> > *object,
> >  }
> >  }
> >  
> > -static void ping_timer_cb(void *opaque);
> >  static void main_channel_client_constructed(GObject *object)
> >  {
> >  G_OBJECT_CLASS(main_channel_client_parent_class)-
> > >constructed(object);
> >  #ifdef RED_STATISTICS
> > +static void ping_timer_cb(void *opaque);
> >  MainChannelClient *self = MAIN_CHANNEL_CLIENT(object);
> >  RedsState *reds =
> >  red_channel_get_server(red_channel_client_get_channel(RED
> > _CHANNEL_CLIENT(object)));
> 
> I think this does not compile or cause a warning+error on rhel6.
ok, i will fix it
> 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 3b14fbf..12bf941 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -104,10 +104,8 @@ struct RedChannelPrivate
> >  // from Channel -> need to protect!
> >  pthread_t thread_id;
> >  RedsState *reds;
> > -#ifdef RED_STATISTICS
> >  StatNodeRef stat;
> >  uint64_t *out_bytes_counter;
> > -#endif
> >  };
> >  
> >  enum {
> 
> This is weird, you should not need this.
they have to be declared since they are used:
out_bytes_counter is set in red_channel_init
red_channel_set_stat_node checks  'stat'

of course these can be avoided by putting ifdefs where needed, but I
though the intention was to avoid doing that

> 
> > @@ -210,7 +208,7 @@ static void
> > red_channel_client_default_peer_on_error(RedChannelClient *rcc)
> >  static void red_channel_on_output(void *opaque, int n)
> >  {
> >  RedChannelClient *rcc = opaque;
> > -RedChannel *self = red_channel_client_get_channel(rcc);
> > +RedChannel *self G_GNUC_UNUSED =
> > red_channel_client_get_channel(rcc);
> >  
> >  red_channel_client_on_output(opaque, n);
> >  
> 
> I would also use the initialization.

What do you mean ?

> 
> > @@ -336,7 +334,7 @@ red_channel_init(RedChannel *self)
> >  
> >  red_channel_set_common_cap(self,
> > SPICE_COMMON_CAP_MINI_HEADER);
> >  self->priv->thread_id = pthread_self();
> > -self->priv->out_bytes_counter = 0;
> > +self->priv->out_bytes_counter = NULL;
> >  
> >  // TODO: send incoming_cb as parameters instead of
> > duplicating?
> >  self->priv->incoming_cb.on_error =
> 
> Actually this could even be removed as structure is zeroed at
> initialization but surely NULL is better then 0.
ok

Pavel

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

[Spice-devel] [PATCH 4/4] Convert RedClient to GObject

2016-10-26 Thread Jonathon Jongsma 
Signed-off-by: Jonathon Jongsma 
---
 server/main-dispatcher.c|  12 ++---
 server/red-channel-client.c |   1 -
 server/red-client.c | 117 
 server/red-client.h |  29 +++
 server/reds.c   |   2 +-
 5 files changed, 123 insertions(+), 38 deletions(-)

diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index fe29b2d..09ac1cc 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -208,7 +208,7 @@ static void main_dispatcher_handle_migrate_complete(void 
*opaque,
 MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete = payload;
 
 reds_on_client_seamless_migrate_complete(self->priv->reds, 
mig_complete->client);
-red_client_unref(mig_complete->client);
+g_object_unref(mig_complete->client);
 }
 
 static void main_dispatcher_handle_mm_time_latency(void *opaque,
@@ -217,7 +217,7 @@ static void main_dispatcher_handle_mm_time_latency(void 
*opaque,
 MainDispatcher *self = opaque;
 MainDispatcherMmTimeLatencyMessage *msg = payload;
 reds_set_client_mm_time_latency(self->priv->reds, msg->client, 
msg->latency);
-red_client_unref(msg->client);
+g_object_unref(msg->client);
 }
 
 static void main_dispatcher_handle_client_disconnect(void *opaque,
@@ -228,7 +228,7 @@ static void main_dispatcher_handle_client_disconnect(void 
*opaque,
 
 spice_debug("client=%p", msg->client);
 reds_client_disconnect(self->priv->reds, msg->client);
-red_client_unref(msg->client);
+g_object_unref(msg->client);
 }
 
 void main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self,
@@ -241,7 +241,7 @@ void 
main_dispatcher_seamless_migrate_dst_complete(MainDispatcher *self,
 return;
 }
 
-msg.client = red_client_ref(client);
+msg.client = g_object_ref(client);
 dispatcher_send_message(DISPATCHER(self), 
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
 &msg);
 }
@@ -255,7 +255,7 @@ void main_dispatcher_set_mm_time_latency(MainDispatcher 
*self, RedClient *client
 return;
 }
 
-msg.client = red_client_ref(client);
+msg.client = g_object_ref(client);
 msg.latency = latency;
 dispatcher_send_message(DISPATCHER(self), 
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
 &msg);
@@ -267,7 +267,7 @@ void main_dispatcher_client_disconnect(MainDispatcher 
*self, RedClient *client)
 
 if (!red_client_is_disconnecting(client)) {
 spice_debug("client %p", client);
-msg.client = red_client_ref(client);
+msg.client = g_object_ref(client);
 dispatcher_send_message(DISPATCHER(self), 
MAIN_DISPATCHER_CLIENT_DISCONNECT,
 &msg);
 } else {
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 86d2305..048286b 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -36,7 +36,6 @@
 #include "red-channel-client.h"
 #include "red-channel-client-private.h"
 #include "red-client.h"
-#include "glib-compat.h"
 
 static const SpiceDataHeaderOpaque full_header_wrapper;
 static const SpiceDataHeaderOpaque mini_header_wrapper;
diff --git a/server/red-client.c b/server/red-client.c
index 3984672..91f4c17 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -27,6 +27,7 @@
 GLIST_FOREACH((_client ? (_client)->channels : NULL), _iter, 
RedChannelClient, _data)
 
 struct RedClient {
+GObject parent;
 RedsState *reds;
 GList *channels;
 MainChannelClient *mcc;
@@ -47,36 +48,109 @@ struct RedClient {
 int refs;
 };
 
-RedClient *red_client_ref(RedClient *client)
+struct RedClientClass
 {
-spice_assert(client);
-g_atomic_int_inc(&client->refs);
-return client;
+GObjectClass parent_class;
+};
+
+G_DEFINE_TYPE(RedClient, red_client, G_TYPE_OBJECT)
+
+enum {
+PROP0,
+PROP_SPICE_SERVER,
+PROP_MIGRATED
+};
+
+static void
+red_client_get_property (GObject*object,
+ guint   property_id,
+ GValue *value,
+ GParamSpec *pspec)
+{
+RedClient *self = RED_CLIENT(object);
+
+switch (property_id)
+{
+case PROP_SPICE_SERVER:
+g_value_set_pointer(value, self->reds);
+break;
+case PROP_MIGRATED:
+g_value_set_boolean(value, self->during_target_migrate);
+break;
+default:
+G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
+}
 }
 
-RedClient *red_client_unref(RedClient *client)
+static void
+red_client_set_property (GObject  *object,
+ guint property_id,
+ const GValue *value,
+ GParamSpec   *pspec)
 {
-if (g_atomic_int_dec_and_test(&client->refs)) {
-spice_debug("release client=%p", client);
-pthread_mutex_destroy(&client->lock);
-free(client);
-

[Spice-devel] [PATCH 2/4] Convert RedsState::clients to GList

2016-10-26 Thread Jonathon Jongsma 
Switch from a Ring to a GList so that we can hide the internals of
RedClient in a future commit.
---
 server/red-channel.h  |  1 -
 server/reds-private.h |  3 +--
 server/reds.c | 54 ---
 3 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/server/red-channel.h b/server/red-channel.h
index 1b46c01..528babc 100644
--- a/server/red-channel.h
+++ b/server/red-channel.h
@@ -328,7 +328,6 @@ const RedChannelCapabilities* 
red_channel_get_local_capabilities(RedChannel *sel
 
 struct RedClient {
 RedsState *reds;
-RingItem link;
 GList *channels;
 MainChannelClient *mcc;
 pthread_mutex_t lock; // different channels can be in different threads
diff --git a/server/reds-private.h b/server/reds-private.h
index cc13ba7..ce78945 100644
--- a/server/reds-private.h
+++ b/server/reds-private.h
@@ -94,8 +94,7 @@ struct RedsState {
 SpiceWatch *secure_listen_watch;
 RedCharDeviceVDIPort *agent_dev;
 int pending_mouse_event;
-Ring clients;
-int num_clients;
+GList *clients;
 MainChannel *main_channel;
 InputsChannel *inputs_channel;
 
diff --git a/server/reds.c b/server/reds.c
index de0d356..90437c0 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -595,13 +595,12 @@ void reds_client_disconnect(RedsState *reds, RedClient 
*client)
 red_char_device_client_remove(RED_CHAR_DEVICE(reds->agent_dev), 
client);
 }
 
-ring_remove(&client->link);
-reds->num_clients--;
+reds->clients = g_list_remove(reds->clients, client);
 red_client_destroy(client);
 
// TODO: we need to handle agent properly for all clients (e.g., cut 
and paste, how? Maybe throw away messages
// if we are in the middle of one from another client)
-if (reds->num_clients == 0) {
+if (g_list_length(reds->clients) == 0) {
 /* Let the agent know the client is disconnected */
 if (reds->agent_dev->priv->agent_attached) {
 RedCharDeviceWriteBuffer *char_dev_buf;
@@ -644,11 +643,12 @@ void reds_client_disconnect(RedsState *reds, RedClient 
*client)
 // reds_client_disconnect
 static void reds_disconnect(RedsState *reds)
 {
-RingItem *link, *next;
+GListIter iter;
+RedClient *client;
 
 spice_info(NULL);
-RING_FOREACH_SAFE(link, next, &reds->clients) {
-reds_client_disconnect(reds, SPICE_CONTAINEROF(link, RedClient, link));
+GLIST_FOREACH(reds->clients, iter, RedClient, client) {
+reds_client_disconnect(reds, client);
 }
 reds_mig_cleanup(reds);
 }
@@ -973,7 +973,7 @@ void reds_handle_agent_mouse_event(RedsState *reds, const 
VDAgentMouseState *mou
 
 static int reds_get_n_clients(RedsState *reds)
 {
-return reds ? reds->num_clients : 0;
+return reds ? g_list_length(reds->clients) : 0;
 }
 
 SPICE_GNUC_VISIBLE int spice_server_get_num_clients(SpiceServer *reds)
@@ -1003,7 +1003,7 @@ static void reds_fill_channels(RedsState *reds, 
SpiceMsgChannels *channels_info)
 
 GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
 uint32_t type, id;
-if (reds->num_clients > 1 &&
+if (g_list_length(reds->clients) > 1 &&
 !channel_supports_multiple_clients(channel)) {
 continue;
 }
@@ -1245,7 +1245,7 @@ void reds_on_main_channel_migrate(RedsState *reds, 
MainChannelClient *mcc)
 RedCharDeviceVDIPort *agent_dev = reds->agent_dev;
 uint32_t read_data_len;
 
-spice_assert(reds->num_clients == 1);
+spice_assert(g_list_length(reds->clients) == 1);
 
 if (agent_dev->priv->read_state != VDI_PORT_READ_STATE_READ_DATA) {
 return;
@@ -1718,12 +1718,10 @@ static void 
reds_mig_target_client_disconnect_all(RedsState *reds)
 
 static int reds_find_client(RedsState *reds, RedClient *client)
 {
-RingItem *item;
-
-RING_FOREACH(item, &reds->clients) {
-RedClient *list_client;
+GListIter iter;
+RedClient *list_client;
 
-list_client = SPICE_CONTAINEROF(item, RedClient, link);
+GLIST_FOREACH(reds->clients, iter, RedClient, list_client) {
 if (list_client == client) {
 return TRUE;
 }
@@ -1734,13 +1732,14 @@ static int reds_find_client(RedsState *reds, RedClient 
*client)
 /* should be used only when there is one client */
 static RedClient *reds_get_client(RedsState *reds)
 {
-spice_assert(reds->num_clients <= 1);
+gint n = g_list_length(reds->clients);
+spice_assert(n <= 1);
 
-if (reds->num_clients == 0) {
+if (n == 0) {
 return NULL;
 }
 
-return SPICE_CONTAINEROF(ring_get_head(&reds->clients), RedClient, link);
+return reds->clients->data;
 }
 
 // TODO: now that main is a separate channel this should
@@ -1787,8 +1786,7 @@ static void reds_handle_main_link(RedsState *reds, 
RedLinkInfo *link)
 reds_link_free(link);
 caps = (uint32_t *)((uint8_t *)link_mess + link_mess->caps_offset);
 client = red_client_new(reds, mig_target);
-ring_add(&reds->cli

[Spice-devel] [PATCH 0/4] Convert RedClient to GObject

2016-10-26 Thread Jonathon Jongsma 
This is a small patch series to convert the last major object to GObject.
Originally, these were all a single large patch, but have been split to make
reviewing a bit simpler.

Jonathon Jongsma (4):
  Re-arrange channel client creation to avoid exposing client lock
  Convert RedsState::clients to GList
  Move RedClient to a separate file
  Convert RedClient to GObject

 server/Makefile.am   |   2 +
 server/char-device.c |   6 +-
 server/common-graphics-channel.c |   2 +-
 server/dcc.c |   1 +
 server/dummy-channel-client.c|  34 +---
 server/inputs-channel.c  |   1 +
 server/main-channel-client.c |   1 +
 server/main-channel.c|   1 +
 server/main-dispatcher.c |  16 +-
 server/red-channel-client.c  |  35 +---
 server/red-channel.c | 218 ++-
 server/red-channel.h |  63 +--
 server/red-client.c  | 362 +++
 server/red-client.h  |  76 
 server/reds-private.h|   3 +-
 server/reds.c|  61 ---
 server/sound.c   |   1 +
 server/stream.c  |   1 +
 18 files changed, 513 insertions(+), 371 deletions(-)
 create mode 100644 server/red-client.c
 create mode 100644 server/red-client.h

-- 
2.7.4

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

[Spice-devel] [PATCH 3/4] Move RedClient to a separate file

2016-10-26 Thread Jonathon Jongsma 
Also move the RedClient struct out of the header to avoid accessing the
internals from other files.
---
 server/Makefile.am   |   2 +
 server/char-device.c |   6 +-
 server/common-graphics-channel.c |   2 +-
 server/dcc.c |   1 +
 server/dummy-channel-client.c|   1 +
 server/inputs-channel.c  |   1 +
 server/main-channel-client.c |   1 +
 server/main-channel.c|   1 +
 server/main-dispatcher.c |   4 +-
 server/red-channel-client.c  |   2 +-
 server/red-channel.c | 240 ++--
 server/red-channel.h |  62 +
 server/red-client.c  | 287 +++
 server/red-client.h  |  65 +
 server/reds.c|   5 +-
 server/sound.c   |   1 +
 server/stream.c  |   1 +
 17 files changed, 387 insertions(+), 295 deletions(-)
 create mode 100644 server/red-client.c
 create mode 100644 server/red-client.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 7aada48..a74d5ee 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -102,6 +102,8 @@ libserver_la_SOURCES =  \
red-channel-client.c\
red-channel-client.h\
red-channel-client-private.h\
+   red-client.c\
+   red-client.h\
dummy-channel.c \
dummy-channel.h \
dummy-channel-client.c  \
diff --git a/server/char-device.c b/server/char-device.c
index 7775c07..318bf3c 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -23,7 +23,7 @@
 #include 
 #include 
 #include "char-device.h"
-#include "red-channel.h"
+#include "red-client.h"
 #include "reds.h"
 #include "glib-compat.h"
 
@@ -711,8 +711,10 @@ static RedCharDeviceClient 
*red_char_device_client_new(RedClient *client,
 dev_client->max_send_queue_size = max_send_queue_size;
 dev_client->do_flow_control = do_flow_control;
 if (do_flow_control) {
+RedsState *reds = red_client_get_server(client);
+
 dev_client->wait_for_tokens_timer =
-reds_core_timer_add(client->reds, 
device_client_wait_for_tokens_timeout,
+reds_core_timer_add(reds, device_client_wait_for_tokens_timeout,
 dev_client);
 if (!dev_client->wait_for_tokens_timer) {
 spice_error("failed to create wait for tokens timer");
diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index e3a3ded..08c2c78 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -25,7 +25,7 @@
 
 #include "common-graphics-channel.h"
 #include "dcc.h"
-#include "main-channel-client.h"
+#include "red-client.h"
 
 #define CHANNEL_RECEIVE_BUF_SIZE 1024
 
diff --git a/server/dcc.c b/server/dcc.c
index d430d43..86507c0 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -23,6 +23,7 @@
 #include "display-channel.h"
 #include "display-channel-private.h"
 #include "red-channel-client-private.h"
+#include "red-client.h"
 #include "main-channel-client.h"
 #include "spice-server-enums.h"
 
diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
index c937c87..0d318a7 100644
--- a/server/dummy-channel-client.c
+++ b/server/dummy-channel-client.c
@@ -20,6 +20,7 @@
 
 #include "dummy-channel-client.h"
 #include "red-channel.h"
+#include "red-client.h"
 
 static void dummy_channel_client_initable_interface_init(GInitableIface 
*iface);
 
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index aadcf4b..ae8d393 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -40,6 +40,7 @@
 #include "reds-stream.h"
 #include "red-channel.h"
 #include "red-channel-client.h"
+#include "red-client.h"
 #include "inputs-channel-client.h"
 #include "main-channel-client.h"
 #include "inputs-channel.h"
diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index daa3dfc..b2f4d56 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -26,6 +26,7 @@
 #include "main-channel-client.h"
 #include "main-channel.h"
 #include "red-channel-client.h"
+#include "red-client.h"
 #include "reds.h"
 
 #define NET_TEST_WARMUP_BYTES 0
diff --git a/server/main-channel.c b/server/main-channel.c
index cf5ee6a..b900b62 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -24,6 +24,7 @@
 #include "red-common.h"
 #include "reds.h"
 #include "red-channel-client.h"
+#include "red-client.h"
 #include "main-channel.h"
 #include "main-channel-client.h"
 
diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index bc0de24..fe29b2d 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -24,7 +24,7 @@
 #include "red-common.h"
 #include "dispatcher.h"
 #includ

[Spice-devel] [PATCH 1/4] Re-arrange channel client creation to avoid exposing client lock

2016-10-26 Thread Jonathon Jongsma 
**FIXME: introduces crash when session vdagent connects

Instead of requiring the channel client to lock the client's lock,
re-arrange things so that all of the locking can be internal to
RedClient methods. So instead of having a pre-create validate function,
we simply move the check to red_client_add_channel() and return an error
if a channel already exists. This encapsulates the client implementation
better and also reduces code duplication in RedChannelClient and
DummyChannelClient.
---


**NOTE**: The "FIXME" comment above was added at some point during the branch
development while bisecting a crash. I don't remember any additional details
other than what is stated in the commit log above, and so I don't know exactly
what scenario triggered the crash. I can no longer reproduce this bug, but
decided to leave the comment in for now in the hopes that it might prompt
others to do a bit more extensive testing to see if you can reproduce the
issue.


 server/dummy-channel-client.c | 33 -
 server/red-channel-client.c   | 32 +++-
 server/red-channel.c  | 26 --
 server/red-channel.h  |  2 +-
 4 files changed, 32 insertions(+), 61 deletions(-)

diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
index b7fee6f..c937c87 100644
--- a/server/dummy-channel-client.c
+++ b/server/dummy-channel-client.c
@@ -35,48 +35,23 @@ struct DummyChannelClientPrivate
 gboolean connected;
 };
 
-static int dummy_channel_client_pre_create_validate(RedChannel *channel, 
RedClient  *client)
-{
-uint32_t type, id;
-g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-if (red_client_get_channel(client, type, id)) {
-spice_printerr("Error client %p: duplicate channel type %d id %d",
-   client, type, id);
-return FALSE;
-}
-return TRUE;
-}
-
 static gboolean dummy_channel_client_initable_init(GInitable *initable,
GCancellable *cancellable,
GError **error)
 {
 GError *local_error = NULL;
-DummyChannelClient *self = DUMMY_CHANNEL_CLIENT(initable);
-RedChannelClient *rcc = RED_CHANNEL_CLIENT(self);
+RedChannelClient *rcc = RED_CHANNEL_CLIENT(initable);
 RedClient *client = red_channel_client_get_client(rcc);
 RedChannel *channel = red_channel_client_get_channel(rcc);
-uint32_t type, id;
-
-g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-pthread_mutex_lock(&client->lock);
-if (!dummy_channel_client_pre_create_validate(channel,
-  client)) {
-g_set_error(&local_error,
-SPICE_SERVER_ERROR,
-SPICE_SERVER_ERROR_FAILED,
-"Client %p: duplicate channel type %d id %d",
-client, type, id);
-goto cleanup;
-}
 
 rcc->incoming.header.data = rcc->incoming.header_buf;
 
+if (!red_client_add_channel(client, rcc, &local_error))
+goto cleanup;
+
 red_channel_add_client(channel, rcc);
-red_client_add_channel(client, rcc);
 
 cleanup:
-pthread_mutex_unlock(&client->lock);
 if (local_error) {
 g_warning("Failed to create channel client: %s", local_error->message);
 g_propagate_error(error, local_error);
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index c562e8e..195ee3e 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -851,18 +851,6 @@ static const SpiceDataHeaderOpaque mini_header_wrapper = 
{NULL, sizeof(SpiceMini
   
mini_header_get_msg_type,
   
mini_header_get_msg_size};
 
-static int red_channel_client_pre_create_validate(RedChannel *channel, 
RedClient  *client)
-{
-uint32_t type, id;
-g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-if (red_client_get_channel(client, type, id)) {
-spice_printerr("Error client %p: duplicate channel type %d id %d",
-   client, type, id);
-return FALSE;
-}
-return TRUE;
-}
-
 static gboolean red_channel_client_initable_init(GInitable *initable,
  GCancellable *cancellable,
  GError **error)
@@ -870,20 +858,9 @@ static gboolean red_channel_client_initable_init(GInitable 
*initable,
 GError *local_error = NULL;
 SpiceCoreInterfaceInternal *core;
 RedChannelClient *self = RED_CHANNEL_CLIENT(initable);
-pthread_mutex_lock(&self->priv->client->lock);
-if (!red_channel_client_pre_create_validate(self->priv->channel, 
self->priv->client)) {
-uint32_t id, type;
-g_object_get(self->priv->channel,
- "channel-typ

Re: [Spice-devel] QXL-WDDM-DOD v0.4-1 released

2016-10-26 Thread Christophe Fergeau 
Hey Sameeh,

On Wed, Oct 26, 2016 at 06:37:12PM +0300, Sameeh Jubran wrote:
> And as Frediano mentioned this could be an issue with the mouse as there
> was set of patches that weren't accepted which solves a performance
> issue caused by the mouse mode when running vd-agent.

I did not check which patches exactly you have in mind, but I'd expect
there were review comments/questions on these patches to be addressed
before moving forward with them? If they are causing such bad
performance issues, we should revisit them ;)

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 qxl-wddm-dod] Indicate all Qxl devices as VGA

2016-10-26 Thread Dmitry Fleytman 

> On 26 Oct 2016, at 12:30 PM, Christophe Fergeau  wrote:
> 
> Hey,
> 
> On Wed, Oct 19, 2016 at 07:01:45PM +0300, Sameeh Jubran wrote:
>> Currently the qxl device with id 0 is being set as an internal display
>> while all the rest are set as VGA connector. This commit solves this
>> issue that was discussed in detail here:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1314600
> 
> For what it's worth, a description of the issue this was causing (at
> least a short summary) would be nice to have in the commit log in
> addition to the bugzilla link (someone might be looking at that code in
> a plane, the link might go away for some reason, …)

Hi Christophe,

Sure! Next time we will pay attention to this.
Thanks for pointing out.

Dmitry

> 
> Christophe

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

Re: [Spice-devel] QXL-WDDM-DOD v0.4-1 released

2016-10-26 Thread Sameeh Jubran 
Hi Fabio,

My team and I are the ones that are currently working on it.
First of all thanks for reporting the issue. In order to further
investigate the performance
issues can you please provide me with the qxl device revision that you are
using.
Moreover, please try to use the up-to-date win-vdagent which can be found
in this repository:
https://cgit.freedesktop.org/spice/win32/vd_agent
I am not sure where the most recent binaries for win-vdagent can be found.
(if
anyone knows please reply with the link).

And as Frediano mentioned this could be an issue with the mouse as there
was set of patches
that weren't accepted which solves a performance issue caused by the mouse
mode when running
vd-agent.

On Wed, Oct 26, 2016 at 6:31 PM, Frediano Ziglio  wrote:

> >
> > Il 25/10/2016 16:16, Dmitry Fleytman ha scritto:
> > > Hi All,
> > >
> > > We are glad to notify you that the first official version of
> QXL-WDDM-DOD
> > > driver with Windows 10 support has been released.
> > >
> > > This version was built from tag v0.4-1
> > > @ https://gitlab.com/spice/qxl-wddm-dod/
> > >
> > > Precompiled and signed by Red Hat binaries are available at:
> > > https://www.spice-space.org/download/windows/qxl-wddm-dod/
> qxl-wddm-dod-0.4-1/qxlwddm-0.4-1.zip
> > >
> > > Source tarball is available at:
> > > https://www.spice-space.org/download/windows/qxl-wddm-dod/
> qxl-wddm-dod-0.4-1/qxlwddm-0.4-1-sources.zip
> > >
> > > Best regards,
> > > Dmitry
> > Thanks to any people worked/working on it.
> > I tried it on windows 10 pro 64 bit when before I had qxl-dod taken from
> > flexvdi-guest-tools-2.2.9.exe (source: https://github.com/flexVDI/
> qxl-dod)
> > The new official driver is working but performance is bad, with any
> > video local or web, fullscreen or not, also on very low resolution
> > (1024x768), mouse move lag is very high and near unusable, performance
> > bad is visible also on basic use case like websites scrolling.
> > Before install new driver I removed the unofficial flex one from windows
> > device manager, checked that new is used and also rebooted windows.
> > I also tried different clients with different spice-gtk/virt-viewer
> > version on both windows and linux.
> > Same result, the thing seems related to the new driver.
> > I suppose that flex driver have some different that make the performance
> > better or there is a bug/unexpected case in new driver, but I not
> > checked the source code and I don't have experience in windows drivers
> > coding.
> > If you need more informations/tests tell me and I'll post them.
> >
> > Thanks for any reply and sorry for my bad english.
> >
> >
>
> Ciao Fabio,
>
> I think what you are experiencing is not real performance but
> a different way the mouse is handled.
> I was looking at differences in our driver and FlexVDI one and this
> is one of them.
> We (at least some people related to RedHat and some other from
> FlexVDI) are willing to merge the two efforts.
> Fabio, do you have some Windows 8 machine you can test?
> Would be really helpful.
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin *
*Software Engineer @ Daynix .*
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] QXL-WDDM-DOD v0.4-1 released

2016-10-26 Thread Frediano Ziglio 
> 
> Il 25/10/2016 16:16, Dmitry Fleytman ha scritto:
> > Hi All,
> >
> > We are glad to notify you that the first official version of QXL-WDDM-DOD
> > driver with Windows 10 support has been released.
> >
> > This version was built from tag v0.4-1
> > @ https://gitlab.com/spice/qxl-wddm-dod/
> >
> > Precompiled and signed by Red Hat binaries are available at:
> > https://www.spice-space.org/download/windows/qxl-wddm-dod/qxl-wddm-dod-0.4-1/qxlwddm-0.4-1.zip
> >
> > Source tarball is available at:
> > https://www.spice-space.org/download/windows/qxl-wddm-dod/qxl-wddm-dod-0.4-1/qxlwddm-0.4-1-sources.zip
> >
> > Best regards,
> > Dmitry
> Thanks to any people worked/working on it.
> I tried it on windows 10 pro 64 bit when before I had qxl-dod taken from
> flexvdi-guest-tools-2.2.9.exe (source: https://github.com/flexVDI/qxl-dod)
> The new official driver is working but performance is bad, with any
> video local or web, fullscreen or not, also on very low resolution
> (1024x768), mouse move lag is very high and near unusable, performance
> bad is visible also on basic use case like websites scrolling.
> Before install new driver I removed the unofficial flex one from windows
> device manager, checked that new is used and also rebooted windows.
> I also tried different clients with different spice-gtk/virt-viewer
> version on both windows and linux.
> Same result, the thing seems related to the new driver.
> I suppose that flex driver have some different that make the performance
> better or there is a bug/unexpected case in new driver, but I not
> checked the source code and I don't have experience in windows drivers
> coding.
> If you need more informations/tests tell me and I'll post them.
> 
> Thanks for any reply and sorry for my bad english.
> 
> 

Ciao Fabio,

I think what you are experiencing is not real performance but
a different way the mouse is handled.
I was looking at differences in our driver and FlexVDI one and this
is one of them.
We (at least some people related to RedHat and some other from
FlexVDI) are willing to merge the two efforts.
Fabio, do you have some Windows 8 machine you can test?
Would be really helpful.

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

Re: [Spice-devel] [spice-server PATCH 6/8] red_get_image_data_flat: allocate mem after sanity check

2016-10-26 Thread Uri Lublin 

On 10/17/2016 01:11 PM, Frediano Ziglio wrote:


This patch prevents possible memory leak.

Found by coverity.

Signed-off-by: Uri Lublin 
---
 server/red-parse-qxl.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index d75e27e..4dcf4ee 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -371,13 +371,16 @@ static SpiceChunks
*red_get_image_data_flat(RedMemSlotInfo *slots, int group_id,
 {
 SpiceChunks *data;
 int error;
+unsigned long bitmap_virt;
+


This assumes LP64 architectures, why not using a


Hi Frediano,

I think no assumption is introduced by this patch.

memslot_get_virt returns unsigned long so that's the type
I used for bitmap_virt.

I do not think using void* makes it "safer".

Possibly we can change memslot_get_virt to remove the assumption.

Thanks,
Uri.



void *image_data;


+bitmap_virt = memslot_get_virt(slots, addr, size, group_id, &error);


here

image_data = (void*)memslot_get_virt(slots, addr, size, group_id, &error);


+if (error) {
+return 0;
+}

 data = spice_chunks_new(1);
 data->data_size  = size;
-data->chunk[0].data  = (void*)memslot_get_virt(slots, addr, size,
group_id, &error);
-if (error) {
-return 0;
-}
+data->chunk[0].data  = (void*)bitmap_virt;


here

data->chunk[0].data  = image_data;


 data->chunk[0].len   = size;
 return data;
 }


This is portable to LLP64 (I know, Linux is always LP64 but it does not
cost much).

Frediano



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

Re: [Spice-devel] [spice-server PATCH 2/8] image_encoders: check shared_dict before accessing it

2016-10-26 Thread Uri Lublin 

On 10/17/2016 01:29 PM, Frediano Ziglio wrote:


In both image_encoders_restore_glz_dictionary() and
image_encoders_get_glz_dictionary() shared-dict may
be NULL if size is too large, and the server gets
size from the network.

Both functions end up calling glz_enc_dictionary_create()
that calls glz_dictionary_window_create() where size is
checked.

Found by coverity.

Signed-off-by: Uri Lublin 
---
 server/image-encoders.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/server/image-encoders.c b/server/image-encoders.c
index 39aca6c..9dfabd6 100644
--- a/server/image-encoders.c
+++ b/server/image-encoders.c
@@ -746,12 +746,13 @@ gboolean
image_encoders_get_glz_dictionary(ImageEncoders *enc,
 shared_dict->refs++;
 } else {
 shared_dict = create_glz_dictionary(enc, client, id, window_size);
+spice_return_val_if_fail(shared_dict != NULL, FALSE);
 glz_dictionary_list = g_list_prepend(glz_dictionary_list,
 shared_dict);
 }

 pthread_mutex_unlock(&glz_dictionary_list_lock);
 enc->glz_dict = shared_dict;
-return shared_dict != NULL;
+return TRUE;
 }

 static GlzSharedDictionary *restore_glz_dictionary(ImageEncoders *enc,
@@ -782,12 +783,13 @@ gboolean
image_encoders_restore_glz_dictionary(ImageEncoders *enc,
 shared_dict->refs++;
 } else {
 shared_dict = restore_glz_dictionary(enc, client, id, restore_data);
+spice_return_val_if_fail(shared_dict != NULL, FALSE);
 glz_dictionary_list = g_list_prepend(glz_dictionary_list,
 shared_dict);
 }

 pthread_mutex_unlock(&glz_dictionary_list_lock);
 enc->glz_dict = shared_dict;
-return shared_dict != NULL;
+return TRUE;
 }

 gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id)


Note that you are creating dead locks.


Yes, your are right.
I'll send a replacement patch that fixes that.


Beside that is not clear what the change could cause to the
upper layer.


Upper layer should already handle a case where these functions
return NULL.


I think the actual logic is supposing that dictionary creation (like
memory allocation) is not failing.


Since it depends on a value that comes from the client that
assumption should not be made. Alternatively we
can specifically check the number early

Thanks,
Uri.

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

Re: [Spice-devel] QXL-WDDM-DOD v0.4-1 released

2016-10-26 Thread Fabio Fantoni 
Il 25/10/2016 16:16, Dmitry Fleytman ha scritto:
> Hi All,
>
> We are glad to notify you that the first official version of QXL-WDDM-DOD
> driver with Windows 10 support has been released.
>
> This version was built from tag v0.4-1
> @ https://gitlab.com/spice/qxl-wddm-dod/
>
> Precompiled and signed by Red Hat binaries are available at:
> https://www.spice-space.org/download/windows/qxl-wddm-dod/qxl-wddm-dod-0.4-1/qxlwddm-0.4-1.zip
>
> Source tarball is available at:
> https://www.spice-space.org/download/windows/qxl-wddm-dod/qxl-wddm-dod-0.4-1/qxlwddm-0.4-1-sources.zip
>
> Best regards,
> Dmitry
Thanks to any people worked/working on it.
I tried it on windows 10 pro 64 bit when before I had qxl-dod taken from
flexvdi-guest-tools-2.2.9.exe (source: https://github.com/flexVDI/qxl-dod)
The new official driver is working but performance is bad, with any
video local or web, fullscreen or not, also on very low resolution
(1024x768), mouse move lag is very high and near unusable, performance
bad is visible also on basic use case like websites scrolling.
Before install new driver I removed the unofficial flex one from windows
device manager, checked that new is used and also rebooted windows.
I also tried different clients with different spice-gtk/virt-viewer
version on both windows and linux.
Same result, the thing seems related to the new driver.
I suppose that flex driver have some different that make the performance
better or there is a bug/unexpected case in new driver, but I not
checked the source code and I don't have experience in windows drivers
coding.
If you need more informations/tests tell me and I'll post them.

Thanks for any reply and sorry for my bad english.



smime.p7s
Description: Firma crittografica S/MIME
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server PATCH 1/8] current_remove: rename internal variable 'container'

2016-10-26 Thread Uri Lublin 

On 10/17/2016 12:45 PM, Frediano Ziglio wrote:


It shadows the outer one.

Signed-off-by: Uri Lublin 
---

Possibly more meaningful names than container and container2
should be used

---
 server/display-channel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index b9366b5..49650e1 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -359,16 +359,16 @@ static void current_remove(DisplayChannel *display,
TreeItem *item)
 drawable_remove_from_pipes(drawable);
 current_remove_drawable(display, drawable);
 } else {
-Container *container = CONTAINER(now);
+Container *container2 = CONTAINER(now);

 spice_assert(now->type == TREE_ITEM_TYPE_CONTAINER);

-if ((ring_item = ring_get_head(&container->items))) {
+if ((ring_item = ring_get_head(&container2->items))) {
 now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
 continue;
 }
 ring_item = now->siblings_link.prev;
-container_free(container);
+container_free(container2);
 }
 if (now == item) {
 return;


Why not something like


Hi Frediano,

Your names are better.
Also possible container_of_now instead of now_container.

Thanks,
Uri.




diff --git a/server/display-channel.c b/server/display-channel.c
index 69edd35..0c6c1ef 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -350,7 +350,7 @@ static void current_remove(DisplayChannel *display, 
TreeItem *item)

 /* depth-first tree traversal, TODO: do a to tree_foreach()? */
 for (;;) {
-Container *container = now->container;
+Container *now_container = now->container;
 RingItem *ring_item;

 if (now->type == TREE_ITEM_TYPE_DRAWABLE) {
@@ -359,25 +359,25 @@ static void current_remove(DisplayChannel *display, 
TreeItem *item)
 drawable_remove_from_pipes(drawable);
 current_remove_drawable(display, drawable);
 } else {
-Container *container = CONTAINER(now);
+Container *now_as_container = CONTAINER(now);

 spice_assert(now->type == TREE_ITEM_TYPE_CONTAINER);

-if ((ring_item = ring_get_head(&container->items))) {
+if ((ring_item = ring_get_head(&now_as_container->items))) {
 now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
 continue;
 }
 ring_item = now->siblings_link.prev;
-container_free(container);
+container_free(now_as_container);
 }
 if (now == item) {
 return;
 }

-if ((ring_item = ring_next(&container->items, ring_item))) {
+if ((ring_item = ring_next(&now_container->items, ring_item))) {
 now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
 } else {
-now = &container->base;
+now = &now_container->base;
 }
 }
 }


Frediano



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

[Spice-devel] [PATCH spice-server] Make reds_stat utility work with both 32 and 64 bit architectures.

2016-10-26 Thread Frediano Ziglio 
Due to alignment problems the structure of statistics file is
different between 32 and 64 bit. This as on 32 bit uint64_t is
aligned to 4 bytes instead of 8 so sizeof(SpiceStat) can be either
20 (32 bit) or 24 (64 bit).
This cause reds_stat utility to be bit dependent.
Detect the correct SpiceStat size and use that information.

Signed-off-by: Frediano Ziglio 
---
 tools/reds_stat.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/reds_stat.c b/tools/reds_stat.c
index 3e966d4..69260bf 100644
--- a/tools/reds_stat.c
+++ b/tools/reds_stat.c
@@ -25,18 +25,22 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 
 #define TAB_LEN 4
 #define VALUE_TABS 7
 #define INVALID_STAT_REF (~(uint32_t)0)
 
-static SpiceStat *reds_stat = (SpiceStat *)MAP_FAILED;
+verify(sizeof(SpiceStat) == 20 || sizeof(SpiceStat) == 24);
+
+static SpiceStatNode *reds_nodes = NULL;
 static uint64_t *values = NULL;
 
 static void print_stat_tree(int32_t node_index, int depth)
 {
-SpiceStatNode *node = &reds_stat->nodes[node_index];
+SpiceStatNode *node = &reds_nodes[node_index];
 
 if ((node->flags & SPICE_STAT_NODE_MASK_SHOW) == 
SPICE_STAT_NODE_MASK_SHOW) {
 printf("%*s%s", depth * TAB_LEN, "", node->name);
@@ -66,6 +70,9 @@ int main(int argc, char **argv)
 int shm_name_len;
 int ret = -1;
 int fd;
+struct stat st;
+unsigned header_size = sizeof(SpiceStat);
+SpiceStat *reds_stat = (SpiceStat *)MAP_FAILED;
 
 if (argc != 2 || !(kvm_pid = atoi(argv[1]))) {
 printf("usage: reds_stat [qemu_pid] (e.g. `pgrep qemu`)\n");
@@ -84,6 +91,12 @@ int main(int argc, char **argv)
 }
 shm_size = sizeof(SpiceStat);
 reds_stat = (SpiceStat *)mmap(NULL, shm_size, PROT_READ, MAP_SHARED, fd, 
0);
+if (fstat(fd, &st) == 0) {
+unsigned size = st.st_size % sizeof(SpiceStatNode);
+if (size == 20 || size == 24) {
+header_size = size;
+}
+}
 if (reds_stat == (SpiceStat *)MAP_FAILED) {
 perror("mmap");
 goto error;
@@ -104,12 +117,13 @@ int main(int argc, char **argv)
 if (num_of_nodes != reds_stat->num_of_nodes) {
 num_of_nodes = reds_stat->num_of_nodes;
 shm_old_size = shm_size;
-shm_size = sizeof(SpiceStat) + num_of_nodes * 
sizeof(SpiceStatNode);
+shm_size = header_size + num_of_nodes * sizeof(SpiceStatNode);
 reds_stat = mremap(reds_stat, shm_old_size, shm_size, 
MREMAP_MAYMOVE);
 if (reds_stat == (SpiceStat *)MAP_FAILED) {
 perror("mremap");
 goto error;
 }
+reds_nodes = (SpiceStatNode *)((char *) reds_stat + header_size);
 values = (uint64_t *)realloc(values, num_of_nodes * 
sizeof(uint64_t));
 if (values == NULL) {
 perror("realloc");
-- 
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 2/2] Do not enable statistic by default

2016-10-26 Thread Frediano Ziglio 
> 
> ---
>  server/Makefile.am | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 2a6010b..0514a2c 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -3,7 +3,6 @@ SUBDIRS = . tests
>  
>  AM_CPPFLAGS =\
>   -DSPICE_SERVER_INTERNAL \
> - -DRED_STATISTICS\
>   $(COMMON_CFLAGS)\
>   $(GLIB2_CFLAGS) \
>   $(GOBJECT2_CFLAGS)  \

I think the rationale here is nobody uses these.
Actually I think these statistics could be extended with stuff
like image compression (size ratio at least, perhaps cpu could
be expensive).

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

Re: [Spice-devel] [PATCH spice 1/2] Allow to compile without RED_STATISTICS

2016-10-26 Thread Frediano Ziglio 
Looks like we are working on similar stuff!

> 
> ---
>  server/dcc-send.c| 2 +-
>  server/main-channel-client.c | 2 +-
>  server/red-channel.c | 6 ++
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index ef67f97..54844cf 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -185,7 +185,7 @@ static void
> red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
>SpiceImage *image,
>SpiceImage *io_image,
>int is_lossy)
>  {
> -DisplayChannel *display_channel =
> +DisplayChannel *display_channel G_GNUC_UNUSED =
>  DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
>  DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
>  
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 836c50e..0c8a3a4 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -161,11 +161,11 @@ static void main_channel_client_set_property(GObject
> *object,
>  }
>  }
>  
> -static void ping_timer_cb(void *opaque);
>  static void main_channel_client_constructed(GObject *object)
>  {
>  G_OBJECT_CLASS(main_channel_client_parent_class)->constructed(object);
>  #ifdef RED_STATISTICS
> +static void ping_timer_cb(void *opaque);
>  MainChannelClient *self = MAIN_CHANNEL_CLIENT(object);
>  RedsState *reds =
>  
> red_channel_get_server(red_channel_client_get_channel(RED_CHANNEL_CLIENT(object)));

I think this does not compile or cause a warning+error on rhel6.

> diff --git a/server/red-channel.c b/server/red-channel.c
> index 3b14fbf..12bf941 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -104,10 +104,8 @@ struct RedChannelPrivate
>  // from Channel -> need to protect!
>  pthread_t thread_id;
>  RedsState *reds;
> -#ifdef RED_STATISTICS
>  StatNodeRef stat;
>  uint64_t *out_bytes_counter;
> -#endif
>  };
>  
>  enum {

This is weird, you should not need this.

> @@ -210,7 +208,7 @@ static void
> red_channel_client_default_peer_on_error(RedChannelClient *rcc)
>  static void red_channel_on_output(void *opaque, int n)
>  {
>  RedChannelClient *rcc = opaque;
> -RedChannel *self = red_channel_client_get_channel(rcc);
> +RedChannel *self G_GNUC_UNUSED = red_channel_client_get_channel(rcc);
>  
>  red_channel_client_on_output(opaque, n);
>  

I would also use the initialization.

> @@ -336,7 +334,7 @@ red_channel_init(RedChannel *self)
>  
>  red_channel_set_common_cap(self, SPICE_COMMON_CAP_MINI_HEADER);
>  self->priv->thread_id = pthread_self();
> -self->priv->out_bytes_counter = 0;
> +self->priv->out_bytes_counter = NULL;
>  
>  // TODO: send incoming_cb as parameters instead of duplicating?
>  self->priv->incoming_cb.on_error =

Actually this could even be removed as structure is zeroed at
initialization but surely NULL is better then 0.

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

[Spice-devel] [PATCH spice 2/2] Do not enable statistic by default

2016-10-26 Thread Pavel Grunt 
---
 server/Makefile.am | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/Makefile.am b/server/Makefile.am
index 2a6010b..0514a2c 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -3,7 +3,6 @@ SUBDIRS = . tests
 
 AM_CPPFLAGS =  \
-DSPICE_SERVER_INTERNAL \
-   -DRED_STATISTICS\
$(COMMON_CFLAGS)\
$(GLIB2_CFLAGS) \
$(GOBJECT2_CFLAGS)  \
-- 
2.10.1

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

[Spice-devel] [PATCH spice 1/2] Allow to compile without RED_STATISTICS

2016-10-26 Thread Pavel Grunt 
---
 server/dcc-send.c| 2 +-
 server/main-channel-client.c | 2 +-
 server/red-channel.c | 6 ++
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index ef67f97..54844cf 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -185,7 +185,7 @@ static void 
red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
   SpiceImage *image, 
SpiceImage *io_image,
   int is_lossy)
 {
-DisplayChannel *display_channel =
+DisplayChannel *display_channel G_GNUC_UNUSED =
 DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
 DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
 
diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index 836c50e..0c8a3a4 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -161,11 +161,11 @@ static void main_channel_client_set_property(GObject 
*object,
 }
 }
 
-static void ping_timer_cb(void *opaque);
 static void main_channel_client_constructed(GObject *object)
 {
 G_OBJECT_CLASS(main_channel_client_parent_class)->constructed(object);
 #ifdef RED_STATISTICS
+static void ping_timer_cb(void *opaque);
 MainChannelClient *self = MAIN_CHANNEL_CLIENT(object);
 RedsState *reds =
 
red_channel_get_server(red_channel_client_get_channel(RED_CHANNEL_CLIENT(object)));
diff --git a/server/red-channel.c b/server/red-channel.c
index 3b14fbf..12bf941 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -104,10 +104,8 @@ struct RedChannelPrivate
 // from Channel -> need to protect!
 pthread_t thread_id;
 RedsState *reds;
-#ifdef RED_STATISTICS
 StatNodeRef stat;
 uint64_t *out_bytes_counter;
-#endif
 };
 
 enum {
@@ -210,7 +208,7 @@ static void 
red_channel_client_default_peer_on_error(RedChannelClient *rcc)
 static void red_channel_on_output(void *opaque, int n)
 {
 RedChannelClient *rcc = opaque;
-RedChannel *self = red_channel_client_get_channel(rcc);
+RedChannel *self G_GNUC_UNUSED = red_channel_client_get_channel(rcc);
 
 red_channel_client_on_output(opaque, n);
 
@@ -336,7 +334,7 @@ red_channel_init(RedChannel *self)
 
 red_channel_set_common_cap(self, SPICE_COMMON_CAP_MINI_HEADER);
 self->priv->thread_id = pthread_self();
-self->priv->out_bytes_counter = 0;
+self->priv->out_bytes_counter = NULL;
 
 // TODO: send incoming_cb as parameters instead of duplicating?
 self->priv->incoming_cb.on_error =
-- 
2.10.1

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

Re: [Spice-devel] github mirror

2016-10-26 Thread Victor Toso 
Hi,

On Sat, Jun 11, 2016 at 10:18:51AM +0800, Kevin Won wrote:
> Yes, please tell me how, and what I can do to meet the needs. Thank you.

Thanks Kevin. I can setup gitlab to push new changes to github, so we
can have it mirrored. Could you please add me to SPICE?

https://github.com/victortoso

Cheers,
  toso

>  原始邮件 
> From:"Victor Toso";
> Date:2016年5月11日(星期三) 晚上10:09
> To:"Marc-Andr� Lureau";
> Cc:"spice-devel";"hsm";
> Subject:Re: [Spice-devel] github mirror
> 
> Hey!
> 
> On Tue, Nov 24, 2015 at 01:57:11PM +0100, Marc-Andr� Lureau wrote:
> > Hi,
> >
> > Recently, I was given admin acces to SPICE github mirror. I have done
> > a bit of cleanup and update, and started adding travis integration. I
> > have a few travis patches for libcacard and spice that I am going to
> > send.
> >
> > Currently, the mirroring is done by using a simple clone script
> > manually (https://github.com/SPICE/github-scripts/blob/master/clone.sh).
> > In the future it would be nice to have a hook in fdo git server to
> > push new changes, I am not sure yet how to get there, any help
> > appreciated.
> >
> > If you want to be admin, ask Shumen or me, and we should be able to
> > give you that.
> >
> > cheers
> 
> I created a mirror on gitlab as well [0]. It was quite easy to mirror
> freedesktop from it.
> 
> [0] https://gitlab.com/u/spice
> 
> If the updates on github are done manually, I could set up gitlab's user
> to do it. Gitlab would work as a bridge for github. Gitlab has some sort
> of cron job to pull/push content every hour.
> 
> Let me know if you are interested.
> 
>   toso
> >
> > --
> > Marc-Andr� Lureau
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [RFC PATCH spice-server] replay: Use setjmp/longjmp for error handling

2016-10-26 Thread Frediano Ziglio 
To avoid checking for error in all the path use setjmp/longjmp.
This allow to jump directly to the error handling code in
spice_replay_next_cmd.

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

I'm not a great fun of setjmp/longjmp but I have to say
this way simplify different checks.

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 78de48b..daaca0d 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -23,13 +23,16 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+
 #include "reds.h"
 #include "red-qxl.h"
 #include "red-common.h"
 #include "memslot.h"
 #include "red-parse-qxl.h"
 #include "red-replay-qxl.h"
-#include 
 
 #define QXLPHYSICAL_FROM_PTR(ptr) ((QXLPHYSICAL)(intptr_t)(ptr))
 #define QXLPHYSICAL_TO_PTR(phy) ((void*)(intptr_t)(phy))
@@ -42,6 +45,7 @@ typedef enum {
 struct SpiceReplay {
 FILE *fd;
 gboolean error;
+jmp_buf jmp_error;
 int counter;
 bool created_primary;
 
@@ -57,18 +61,24 @@ struct SpiceReplay {
 pthread_cond_t cond;
 };
 
+static void SPICE_GNUC_NORETURN replay_got_error(SpiceReplay *replay)
+{
+replay->error = TRUE;
+longjmp(replay->jmp_error, 1);
+}
+
 static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
 {
 if (replay->error || feof(replay->fd) ||
 fread(buf, 1, size, replay->fd) != size) {
-replay->error = TRUE;
+replay_got_error(replay);
 return 0;
 }
 return size;
 }
 
 __attribute__((format(scanf, 2, 3)))
-static replay_t replay_fscanf_check(SpiceReplay *replay, const char *fmt, ...)
+static void replay_fscanf_check(SpiceReplay *replay, const char *fmt, ...)
 {
 va_list ap;
 int ret;
@@ -76,19 +86,17 @@ static replay_t replay_fscanf_check(SpiceReplay *replay, 
const char *fmt, ...)
 replay->end_pos = -1;
 
 if (replay->error) {
-return REPLAY_ERROR;
+replay_got_error(replay);
 }
 if (feof(replay->fd)) {
-replay->error = TRUE;
-return REPLAY_ERROR;
+replay_got_error(replay);
 }
 va_start(ap, fmt);
 ret = vfscanf(replay->fd, fmt, ap);
 va_end(ap);
 if (ret == EOF || replay->end_pos < 0) {
-replay->error = TRUE;
+replay_got_error(replay);
 }
-return replay->error ? REPLAY_ERROR : REPLAY_OK;
 }
 #define replay_fscanf(r, fmt, ...) \
 replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, &r->end_pos)
@@ -217,8 +225,8 @@ static void hexdump(uint8_t *hex, uint8_t bytes)
 }
 #endif
 
-static replay_t read_binary(SpiceReplay *replay, const char *prefix, size_t 
*size, uint8_t
-**buf, size_t base_size)
+static void read_binary(SpiceReplay *replay, const char *prefix, size_t *size,
+uint8_t **buf, size_t base_size)
 {
 char template[1024];
 int with_zlib = -1;
@@ -228,9 +236,6 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 
 snprintf(template, sizeof(template), "binary %%d %s %%ld:%%n", prefix);
 replay_fscanf_check(replay, template, &with_zlib, size, &replay->end_pos);
-if (replay->error) {
-return REPLAY_ERROR;
-}
 
 if (*buf == NULL) {
 *buf = replay_malloc(replay, *size + base_size);
@@ -246,13 +251,8 @@ static replay_t read_binary(SpiceReplay *replay, const 
char *prefix, size_t *siz
 int ret;
 
 replay_fscanf(replay, "%u:", &zlib_size);
-if (replay->error) {
-return REPLAY_ERROR;
-}
 zlib_buffer = replay_malloc(replay, zlib_size);
-if (replay_fread(replay, zlib_buffer, zlib_size) != zlib_size) {
-return REPLAY_ERROR;
-}
+replay_fread(replay, zlib_buffer, zlib_size);
 strm.zalloc = Z_NULL;
 strm.zfree = Z_NULL;
 strm.opaque = Z_NULL;
@@ -272,7 +272,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
  * it seems it may kill the red_worker thread (so a chunk is
  * left hanging and the rest of the message is never written).
  * Let it pass */
-return REPLAY_ERROR;
+replay_got_error(replay);
 }
 if (ret != Z_OK) {
 spice_warn_if_reached();
@@ -284,7 +284,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 replay_fread(replay, *buf + base_size, *size);
 }
 #endif
-return replay_fscanf(replay, "\n");
+replay_fscanf(replay, "\n");
 }
 
 static ssize_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
@@ -296,25 +296,19 @@ static ssize_t red_replay_data_chunks(SpiceReplay 
*replay, const char *prefix,
 QXLDataChunk *cur, *next;
 
 replay_fscanf(replay, "data_chunks %u %zu\n", &count_chunks, &data_size);
-if (replay->error) {
-

Re: [Spice-devel] [client] gtk: Remove an obsolete comment

2016-10-26 Thread Christophe Fergeau 
On Tue, Oct 18, 2016 at 07:06:54PM +0200, Francois Gouget wrote:
> On Tue, 18 Oct 2016, Pavel Grunt wrote:
> 
> > On Tue, 2016-10-18 at 05:30 -0400, Frediano Ziglio wrote:
> > > > 
> > > > The GTK+ compatibility code has been gathered in a single file
> > > > long ago.
> > > > 
> > > 
> > similar thing is in vncdisplaykeymap.c
> 
> It's a bit different. Those comments still have the corresponding 
> compatibility defines. They can probably be removed but I'll let 
> specialists verify that it won't introduce compatibility issues.

Yeah, I think they can be dropped.

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 qxl-wddm-dod] Indicate all Qxl devices as VGA

2016-10-26 Thread Christophe Fergeau 
Hey,

On Wed, Oct 19, 2016 at 07:01:45PM +0300, Sameeh Jubran wrote:
> Currently the qxl device with id 0 is being set as an internal display
> while all the rest are set as VGA connector. This commit solves this
> issue that was discussed in detail here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1314600

For what it's worth, a description of the issue this was causing (at
least a short summary) would be nice to have in the commit log in
addition to the bugzilla link (someone might be looking at that code in
a plane, the link might go away for some reason, ...)

Christophe


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

[Spice-devel] [PATCH spice-server] Remove g_smartcard_channel global

2016-10-26 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/reds.c  | 2 +-
 server/reds.h  | 1 +
 server/smartcard.c | 7 ++-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 45d23d4..19ca568 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -464,7 +464,7 @@ void reds_unregister_channel(RedsState *reds, RedChannel 
*channel)
 }
 }
 
-static RedChannel *reds_find_channel(RedsState *reds, uint32_t type, uint32_t 
id)
+RedChannel *reds_find_channel(RedsState *reds, uint32_t type, uint32_t id)
 {
 GListIter it;
 RedChannel *channel;
diff --git a/server/reds.h b/server/reds.h
index cd62fc1..28e3444 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -47,6 +47,7 @@ void reds_enable_mm_time(RedsState *reds);
 uint32_t reds_get_mm_time(void);
 void reds_register_channel(RedsState *reds, RedChannel *channel);
 void reds_unregister_channel(RedsState *reds, RedChannel *channel);
+RedChannel *reds_find_channel(RedsState *reds, uint32_t type, uint32_t id);
 int reds_get_mouse_mode(RedsState *reds); // used by inputs_channel
 gboolean reds_config_get_agent_mouse(const RedsState *reds); // used by 
inputs_channel
 int reds_has_vdagent(RedsState *reds); // used by inputs channel
diff --git a/server/smartcard.c b/server/smartcard.c
index 655eaf7..53919c0 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -602,14 +602,11 @@ red_smartcard_channel_class_init(RedSmartcardChannelClass 
*klass)
 
 }
 
-/* FIXME: remove global */
-RedSmartcardChannel *g_smartcard_channel;
-
 static void smartcard_init(RedsState *reds)
 {
-spice_assert(!g_smartcard_channel);
+spice_assert(!reds_find_channel(reds, SPICE_CHANNEL_SMARTCARD, 0));
 
-g_smartcard_channel = red_smartcard_channel_new(reds);
+red_smartcard_channel_new(reds);
 }
 
 
-- 
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-server 02/12] Be consistent with opaque type

2016-10-26 Thread Frediano Ziglio 
> 
> On Wed, Oct 26, 2016 at 02:43:22AM -0400, Frediano Ziglio wrote:
> > I think that my reasoning is more focuses on the "get refcounting for free"
> > not being
> > a great design so willing to change and I prefer a compile error in the
> > future
> > instead on the "base type".
> 
> Well, this makes the intent of the code harder to get imo, this falls
> apart as soon as an intermediate class is added, ... so I really don't
> think explicit mentions of the 'base' field in the code is something we
> should encourage. If wrapped in a macro, why not, but this still leaves
> the problem that this only work with one level of inheritance.
> 
> Christophe
> 

I prefer having to change the source instead of spending hours
digging into core files. If base is removed no warnings are issues
and memory corruptions happens.

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

[Spice-devel] [PATCH spice-server] Fix core-interface type regression

2016-10-26 Thread Frediano Ziglio 
This was introduced with 96e94c6f32d4345ea25ef7a474fbd92de03b38ad
(Convert RedChannel hierarchy to GObject).
The type for "core-interface" property should be
SpiceCoreInterfaceInternal, not SpiceCoreInterface.

Signed-off-by: Frediano Ziglio 
---
 server/dummy-channel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/dummy-channel.c b/server/dummy-channel.c
index c43221b..61a8d95 100644
--- a/server/dummy-channel.c
+++ b/server/dummy-channel.c
@@ -50,7 +50,8 @@ static void dummy_watch_update_mask(SpiceWatch *watch, int 
event_mask)
 {
 }
 
-static SpiceWatch *dummy_watch_add(int fd, int event_mask, SpiceWatchFunc 
func, void *opaque)
+static SpiceWatch *dummy_watch_add(const SpiceCoreInterfaceInternal *iface,
+   int fd, int event_mask, SpiceWatchFunc 
func, void *opaque)
 {
 return NULL; // apparently allowed?
 }
@@ -60,7 +61,7 @@ static void dummy_watch_remove(SpiceWatch *watch)
 }
 
 // TODO: actually, since I also use channel_client_dummym, no need for core. 
Can be NULL
-static const SpiceCoreInterface dummy_core = {
+static const SpiceCoreInterfaceInternal dummy_core = {
 .watch_update_mask = dummy_watch_update_mask,
 .watch_add = dummy_watch_add,
 .watch_remove = dummy_watch_remove,
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v2] Remove red_pipe_add_verb family function

2016-10-26 Thread Frediano Ziglio 
These functions were implementing the same stuff as empty
messages functions provided by RedChannel so reuse them.

The implementation seems a bit different but the result
is the same. Specifically:
- RedEmptyMsgPipeItem::msg is int while RedVerbItem::verb was
  uint16_t however this data goes into the message type which
  is uint16_t (a 16 bit on the network protocol);
- red_channel_client_send_empty_msg calls
  red_channel_client_begin_send_message while red_marshall_verb
  not. However red_marshall_verb calls are followed by either
  cursor_channel_send_item or dcc_send_item which always
  call red_channel_client_begin_send_message.
  Note that in dcc_send_item when an empty message is sent
  red_channel_client_send_message_pending always returns
  true;
- when a PipeItem is created red_channel_client_pipe_add_empty_msg
  calls red_channel_client_push while red_pipe_add_verb does not.
  This actually make very few difference as this kind of item are
  never removed from the queue and a push is forced in every case
  running the event handler for the stream watch (see
  prepare_pipe_add and red_channel_client_event).

Signed-off-by: Frediano Ziglio 
---
 server/common-graphics-channel.h | 34 +-
 server/cursor-channel.c  |  5 +
 server/dcc-send.c|  3 ---
 server/dcc.c |  2 +-
 server/display-channel.c |  2 +-
 server/red-worker.c  |  4 ++--
 6 files changed, 6 insertions(+), 44 deletions(-)

Changes since v1:
- make commit message clearer.

diff --git a/server/common-graphics-channel.h b/server/common-graphics-channel.h
index a8c3f3d..4d88148 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -65,43 +65,11 @@ gboolean 
common_graphics_channel_get_during_target_migrate(CommonGraphicsChannel
 QXLInstance* common_graphics_channel_get_qxl(CommonGraphicsChannel *self);
 
 enum {
-RED_PIPE_ITEM_TYPE_VERB = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
-RED_PIPE_ITEM_TYPE_INVAL_ONE,
+RED_PIPE_ITEM_TYPE_INVAL_ONE = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
 
 RED_PIPE_ITEM_TYPE_COMMON_LAST
 };
 
-typedef struct RedVerbItem {
-RedPipeItem base;
-uint16_t verb;
-} RedVerbItem;
-
-static inline void red_marshall_verb(RedChannelClient *rcc, RedVerbItem *item)
-{
-red_channel_client_init_send_data(rcc, item->verb, NULL);
-}
-
-static inline void red_pipe_add_verb(RedChannelClient* rcc, uint16_t verb)
-{
-RedVerbItem *item = spice_new(RedVerbItem, 1);
-
-red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_VERB);
-item->verb = verb;
-red_channel_client_pipe_add(rcc, &item->base);
-}
-
-static inline void red_pipe_add_verb_proxy(RedChannelClient *rcc, gpointer 
data)
-{
-uint16_t verb = GPOINTER_TO_UINT(data);
-red_pipe_add_verb(rcc, verb);
-}
-
-
-static inline void red_pipes_add_verb(RedChannel *channel, uint16_t verb)
-{
-red_channel_apply_clients_data(channel, red_pipe_add_verb_proxy, 
GUINT_TO_POINTER(verb));
-}
-
 G_END_DECLS
 
 #endif /* _COMMON_GRAPHICS_CHANNEL_H */
diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index dbe3dfc..65c14a2 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -286,9 +286,6 @@ static void cursor_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *pipe_it
 case RED_PIPE_ITEM_TYPE_INVAL_ONE:
 red_marshall_inval(rcc, m, SPICE_CONTAINEROF(pipe_item, RedCacheItem, 
u.pipe_data));
 break;
-case RED_PIPE_ITEM_TYPE_VERB:
-red_marshall_verb(rcc, SPICE_UPCAST(RedVerbItem, pipe_item));
-break;
 case RED_PIPE_ITEM_TYPE_CURSOR_INIT:
 cursor_channel_client_reset_cursor_cache(rcc);
 red_marshall_cursor_init(ccc, m, pipe_item);
@@ -376,7 +373,7 @@ void cursor_channel_reset(CursorChannel *cursor)
 if (red_channel_is_connected(channel)) {
 red_channel_pipes_add_type(channel, 
RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
 if 
(!common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_CHANNEL(cursor)))
 {
-red_pipes_add_verb(channel, SPICE_MSG_CURSOR_RESET);
+red_channel_pipes_add_empty_msg(channel, SPICE_MSG_CURSOR_RESET);
 }
 if (!red_channel_wait_all_sent(channel,
COMMON_CLIENT_TIMEOUT)) {
diff --git a/server/dcc-send.c b/server/dcc-send.c
index ef67f97..01722c5 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -2399,9 +2399,6 @@ void dcc_send_item(RedChannelClient *rcc, RedPipeItem 
*pipe_item)
 case RED_PIPE_ITEM_TYPE_UPGRADE:
 marshall_upgrade(rcc, m, SPICE_UPCAST(RedUpgradeItem, pipe_item));
 break;
-case RED_PIPE_ITEM_TYPE_VERB:
-red_marshall_verb(rcc, SPICE_UPCAST(RedVerbItem, pipe_item));
-break;
 case RED_PIPE_ITEM_TYPE_MIGRATE_DATA:
 display_channel_marshall_migrate_data(rcc, m);
 break;
diff --git a/server/dcc.c b/server/dcc.c
index d430d43..79fc7fd 100644
--- a/s

Re: [Spice-devel] [PATCH spice-server] Remove duplicate include

2016-10-26 Thread Christophe Fergeau 

Acked-by: Christophe Fergeau 

On Wed, Oct 26, 2016 at 08:52:26AM +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/main-channel-client.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index daa3dfc..836c50e 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -19,8 +19,6 @@
>  #endif
>  
>  #include 
> -
> -#include "main-channel-client.h"
>  #include 
>  
>  #include "main-channel-client.h"
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 02/12] Be consistent with opaque type

2016-10-26 Thread Christophe Fergeau 
On Wed, Oct 26, 2016 at 02:43:22AM -0400, Frediano Ziglio wrote:
> I think that my reasoning is more focuses on the "get refcounting for free" 
> not being
> a great design so willing to change and I prefer a compile error in the future
> instead on the "base type".

Well, this makes the intent of the code harder to get imo, this falls
apart as soon as an intermediate class is added, ... so I really don't
think explicit mentions of the 'base' field in the code is something we
should encourage. If wrapped in a macro, why not, but this still leaves
the problem that this only work with one level of inheritance.

Christophe

> 
> Frediano
> 
> > 
> > 
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/reds.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 79f9c9e..a71029f 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -745,7 +745,8 @@ static void reds_agent_remove(RedsState *reds)
> > >  
> > >  static void vdi_port_read_buf_release(uint8_t *data, void *opaque)
> > >  {
> > > -red_pipe_item_unref((RedPipeItem *)opaque);
> > > +RedVDIReadBuf *read_buf = (RedVDIReadBuf *)opaque;
> > > +red_pipe_item_unref(&read_buf->base);
> > >  }
> > >  
> > >  /*
> > > --
> > > 2.7.4
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 


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

[Spice-devel] [PATCH spice-server] Remove duplicate include

2016-10-26 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/main-channel-client.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index daa3dfc..836c50e 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -19,8 +19,6 @@
 #endif
 
 #include 
-
-#include "main-channel-client.h"
 #include 
 
 #include "main-channel-client.h"
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server] Fix handle-acks regression

2016-10-26 Thread Frediano Ziglio 
This was introduced with 96e94c6f32d4345ea25ef7a474fbd92de03b38ad
(Convert RedChannel hierarchy to GObject).
The handle-acks settings was TRUE for CursorChannel and DisplayChannel.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c  | 1 +
 server/display-channel.c | 1 +
 2 files changed, 2 insertions(+)

I discover this rebasing some other patches that actually
were attempting to set this flag to FALSE.
But I think that the change should not be done as a
regression in another patch.

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index dbe3dfc..df3561a 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -314,6 +314,7 @@ CursorChannel* cursor_channel_new(RedsState *server, 
QXLInstance *qxl,
 "channel-type", SPICE_CHANNEL_CURSOR,
 "migration-flags", 0,
 "qxl", qxl,
+"handle-acks", TRUE,
 NULL);
 }
 
diff --git a/server/display-channel.c b/server/display-channel.c
index bcf26bb..63f9986 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1993,6 +1993,7 @@ DisplayChannel* display_channel_new(RedsState *reds,
"qxl", qxl,
"n-surfaces", n_surfaces,
"video-codecs", video_codecs,
+   "handle-acks", TRUE,
NULL);
 if (display) {
 display_channel_set_stream_video(display, stream_video);
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v2] Move capability initialisation into channel creation

2016-10-26 Thread Frediano Ziglio 
No reason why RedWorker should know the capabilities of
DisplayChannel.

Signed-off-by: Frediano Ziglio 
---
 server/display-channel.c | 6 +-
 server/red-worker.c  | 3 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

Changes since v1:
- rebased on master.

Was already acked but the rebase changed a bit too much to keep
the ack.

diff --git a/server/display-channel.c b/server/display-channel.c
index bcf26bb..5521641 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2025,6 +2025,7 @@ static void
 display_channel_constructed(GObject *object)
 {
 DisplayChannel *self = DISPLAY_CHANNEL(object);
+RedChannel *channel = RED_CHANNEL(self);
 
 G_OBJECT_CLASS(display_channel_parent_class)->constructed(object);
 
@@ -2037,7 +2038,6 @@ display_channel_constructed(GObject *object)
 stat_init(&self->priv->__exclude_stat, "__exclude", 
CLOCK_THREAD_CPUTIME_ID);
 #ifdef RED_STATISTICS
 RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
-RedChannel *channel = RED_CHANNEL(self);
 self->priv->cache_hits_counter =
 stat_add_counter(reds, red_channel_get_stat_node(channel),
  "cache_hits", TRUE);
@@ -2051,6 +2051,10 @@ display_channel_constructed(GObject *object)
 image_cache_init(&self->priv->image_cache);
 self->priv->stream_video = SPICE_STREAM_VIDEO_OFF;
 display_channel_init_streams(self);
+
+red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
+red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);
+red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
 }
 
 void display_channel_process_surface_cmd(DisplayChannel *display,
diff --git a/server/red-worker.c b/server/red-worker.c
index f7f0726..ec8ebce 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -1379,9 +1379,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
 red_channel_set_stat_node(channel, stat_add_node(reds, worker->stat, 
"display_channel", TRUE));
 red_channel_register_client_cbs(channel, client_display_cbs, dispatcher);
 g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
-red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
-red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);
-red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
 reds_register_channel(reds, channel);
 
 return worker;
-- 
2.7.4

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