Re: [Spice-devel] [spice-server] Avoid to typedef twice SmartCardChannelClient

2016-10-17 Thread Frediano Ziglio 
> 
> SmartCardChannelClient is already defined in smartcard.h which
> is included in the smartcard-channel-client.h header.
> 
> Signed-off-by: Francois Gouget 
> ---
> 
> This patch series is nice and really does help.
> I would add this patch to it.
> 

Is there something missing?

>  server/smartcard-channel-client.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/server/smartcard-channel-client.h
> b/server/smartcard-channel-client.h
> index 4fe1c72..606a95e 100644
> --- a/server/smartcard-channel-client.h
> +++ b/server/smartcard-channel-client.h
> @@ -37,7 +37,6 @@ G_BEGIN_DECLS
>  #define SMARTCARD_CHANNEL_CLIENT_GET_CLASS(obj) \
>  (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_SMARTCARD_CHANNEL_CLIENT,
>  SmartCardChannelClientClass))
>  
> -typedef struct SmartCardChannelClient SmartCardChannelClient;
>  typedef struct SmartCardChannelClientClass SmartCardChannelClientClass;
>  typedef struct SmartCardChannelClientPrivate SmartCardChannelClientPrivate;
>  

Yes, this class it's kind of my 10/10, by our "rules" SmartCardChannelClient
should be defined here but it clash with other headers.

I would ack it and consider a temporary workaround till we decide
a final rule or way to handle these. At least this change is quite minimal.

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

Re: [Spice-devel] protocol: Group the VDAgent clipboard message definitions

2016-10-17 Thread Frediano Ziglio 
> 
> Signed-off-by: Francois Gouget 
> ---
>  spice/vd_agent.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 445b458..c49f596 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -180,6 +180,12 @@ enum {
>  VD_AGENT_CLIPBOARD_IMAGE_JPG,  /* optional */
>  };
>  
> +enum {
> +VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD = 0,
> +VD_AGENT_CLIPBOARD_SELECTION_PRIMARY,
> +VD_AGENT_CLIPBOARD_SELECTION_SECONDARY,
> +};
> +
>  typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
>  #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
>  uint8_t selection;
> @@ -231,12 +237,6 @@ enum {
>  VD_AGENT_END_CAP,
>  };
>  
> -enum {
> -VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD = 0,
> -VD_AGENT_CLIPBOARD_SELECTION_PRIMARY,
> -VD_AGENT_CLIPBOARD_SELECTION_SECONDARY,
> -};
> -
>  typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
>  uint32_t  request;
>  uint32_t caps[0];

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

2016-10-17 Thread Francois Gouget 
On Mon, 17 Oct 2016, Francois Gouget wrote:
[...]
> > Imo this SPICE_NO_DEPRECATED is only meant to be used internally by
> > spice-gtk even if this is not really documented. I would not consider it
> > breakage if this was removed, or changed in incompatible ways.
> > And it turns out it's actually useless as -Wno-deprecated-declarations
> > is used during compilation (which also disables the
> > warnings from GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_REQUIRED :(
> > I'd tend to change that so that -Wno-deprecated-declarations is only
> > used for spicy, and make selective use of
> > G_GNUC_BEGIN_IGNORE_DEPRECATIONS / G_GNUC_END_IGNORE_DEPRECATIONS when
> > needed. Seems to be working with quick and dirty local changes.
> 
> I know you were talking about spice-gtk, but would the spice patch 
> below be what you had in mind?

The dark side of G_GNUC_{BEGIN,END}_IGNORE_DEPRECATIONS is that it's not 
supported on RHEL 6.8:
 * GLib < 2.32 does not provide these macros,
 * and gcc <4.6 does not support the underlying pragmas.


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

Re: [Spice-devel] Very poor video performance using Virt Viewer compared to RDP

2016-10-17 Thread Francois Gouget 
On Mon, 17 Oct 2016, Brad Wilson wrote:

> Ok, so I installed the AC97 audio drivers to fix my audio device in 
> the guest vm.  Using Virt-Viewer with qxl, the video streaming has 
> improved and I am seeing closer to 1.5Mbps network traffic now when 
> watching hd videos but the quality is still suffering a bit.  What I 
> would like to know is if there is a way to allow the spice protocol to 
> use more bandwidth in order to increase video quality and frame rate. 
>  In windows RDP you can set the quality to be super high which gives a 
> much nicer video experience at the cost of about 15Mbps... I am 
> curious if Spice or Virt-Viewer have such a setting? And just to be 
> clear, I can get near native quality using rdp to this same VM. 

There is no such setting. Spice automatically uses all the available 
bandwidth.

That said if for some reason it settled on a bandwidth lower than what 
is truly available, it will only try to increase bandwidth usage again 
after a while and only quite progressively (at least for the GStreamer 
backend, I don't really remember what the mjpeg one does in that case).


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

[Spice-devel] [spice-server] Avoid to typedef twice SmartCardChannelClient

2016-10-17 Thread Francois Gouget 
SmartCardChannelClient is already defined in smartcard.h which
is included in the smartcard-channel-client.h header.

Signed-off-by: Francois Gouget 
---

This patch series is nice and really does help.
I would add this patch to it.

 server/smartcard-channel-client.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/smartcard-channel-client.h 
b/server/smartcard-channel-client.h
index 4fe1c72..606a95e 100644
--- a/server/smartcard-channel-client.h
+++ b/server/smartcard-channel-client.h
@@ -37,7 +37,6 @@ G_BEGIN_DECLS
 #define SMARTCARD_CHANNEL_CLIENT_GET_CLASS(obj) \
 (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_SMARTCARD_CHANNEL_CLIENT, 
SmartCardChannelClientClass))
 
-typedef struct SmartCardChannelClient SmartCardChannelClient;
 typedef struct SmartCardChannelClientClass SmartCardChannelClientClass;
 typedef struct SmartCardChannelClientPrivate SmartCardChannelClientPrivate;
 
-- 
2.9.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] protocol: Group the VDAgent clipboard message definitions

2016-10-17 Thread Francois Gouget 
Signed-off-by: Francois Gouget 
---
 spice/vd_agent.h | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/spice/vd_agent.h b/spice/vd_agent.h
index 445b458..c49f596 100644
--- a/spice/vd_agent.h
+++ b/spice/vd_agent.h
@@ -180,6 +180,12 @@ enum {
 VD_AGENT_CLIPBOARD_IMAGE_JPG,  /* optional */
 };
 
+enum {
+VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD = 0,
+VD_AGENT_CLIPBOARD_SELECTION_PRIMARY,
+VD_AGENT_CLIPBOARD_SELECTION_SECONDARY,
+};
+
 typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
 #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
 uint8_t selection;
@@ -231,12 +237,6 @@ enum {
 VD_AGENT_END_CAP,
 };
 
-enum {
-VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD = 0,
-VD_AGENT_CLIPBOARD_SELECTION_PRIMARY,
-VD_AGENT_CLIPBOARD_SELECTION_SECONDARY,
-};
-
 typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
 uint32_t  request;
 uint32_t caps[0];
-- 
2.9.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server 05/10] Do not typedef DisplayChannel twice

2016-10-17 Thread Frediano Ziglio 
Already defined in dcc.h.

Signed-off-by: Frediano Ziglio 
---
 server/stream.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/server/stream.h b/server/stream.h
index 77223c8..a415e43 100644
--- a/server/stream.h
+++ b/server/stream.h
@@ -42,9 +42,6 @@
 #define RED_STREAM_DEFAULT_LOW_START_BIT_RATE (2.5 * 1024 * 1024) // 2.5Mbps
 #define MAX_FPS 30
 
-/* move back to display_channel once struct private */
-typedef struct DisplayChannel DisplayChannel;
-
 typedef struct Stream Stream;
 
 typedef struct RedStreamActivateReportItem {
-- 
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 09/10] red-worker: Do not include not necessary red-channel-client.h

2016-10-17 Thread Frediano Ziglio 
Definitions are all in red-channel.h

Signed-off-by: Frediano Ziglio 
---
 server/red-worker.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-worker.h b/server/red-worker.h
index dc2ff24..cf61a67 100644
--- a/server/red-worker.h
+++ b/server/red-worker.h
@@ -21,7 +21,7 @@
 #include "red-common.h"
 #include "red-qxl.h"
 #include "red-parse-qxl.h"
-#include "red-channel-client.h"
+#include "red-channel.h"
 
 typedef struct RedWorker RedWorker;
 
-- 
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 07/10] Avoid to typedef twice RedChannel and RedClient

2016-10-17 Thread Frediano Ziglio 
These are already defined in red-channel.h which is included in
red-channel-client.h header.

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 9cc5245..c2c6407 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -49,9 +49,6 @@ G_BEGIN_DECLS
 #define RED_CHANNEL_CLIENT_GET_CLASS(obj) \
 (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_CHANNEL_CLIENT, 
RedChannelClientClass))
 
-typedef struct RedChannel RedChannel;
-typedef struct RedClient RedClient;
-
 typedef struct RedChannelClient RedChannelClient;
 typedef struct RedChannelClientClass RedChannelClientClass;
 typedef struct RedChannelClientPrivate RedChannelClientPrivate;
-- 
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 10/10] RFC: Make code compile using RHEL 6

2016-10-17 Thread Frediano Ziglio 
Quite questionable patch.
Mostly as come typedef are not defined in the place they
naturally should be.

Signed-off-by: Frediano Ziglio 
---
 server/main-channel-client.h | 5 +
 server/main-channel.h| 1 -
 server/red-channel-client.h  | 1 -
 server/stream.h  | 6 ++
 4 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/server/main-channel-client.h b/server/main-channel-client.h
index 360c61e..fc7d9ce 100644
--- a/server/main-channel-client.h
+++ b/server/main-channel-client.h
@@ -21,12 +21,10 @@
 #include 
 
 #include "red-channel-client.h"
+#include "main-channel.h"
 
 G_BEGIN_DECLS
 
-/* FIXME: remove extra MainChannel typedef when possible */
-typedef struct MainChannel MainChannel;
-
 #define TYPE_MAIN_CHANNEL_CLIENT main_channel_client_get_type()
 
 #define MAIN_CHANNEL_CLIENT(obj) \
@@ -40,7 +38,6 @@ typedef struct MainChannel MainChannel;
 #define MAIN_CHANNEL_CLIENT_GET_CLASS(obj) \
 (G_TYPE_INSTANCE_GET_CLASS((obj), TYPE_MAIN_CHANNEL_CLIENT, 
MainChannelClientClass))
 
-typedef struct MainChannelClient MainChannelClient;
 typedef struct MainChannelClientClass MainChannelClientClass;
 typedef struct MainChannelClientPrivate MainChannelClientPrivate;
 
diff --git a/server/main-channel.h b/server/main-channel.h
index e0858d0..2606ff4 100644
--- a/server/main-channel.h
+++ b/server/main-channel.h
@@ -23,7 +23,6 @@
 #include 
 
 #include "red-channel.h"
-#include "main-channel-client.h"
 
 #define MAIN_CHANNEL(channel) ((MainChannel*)(channel))
 
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index c2c6407..cb49454 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -49,7 +49,6 @@ G_BEGIN_DECLS
 #define RED_CHANNEL_CLIENT_GET_CLASS(obj) \
 (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_CHANNEL_CLIENT, 
RedChannelClientClass))
 
-typedef struct RedChannelClient RedChannelClient;
 typedef struct RedChannelClientClass RedChannelClientClass;
 typedef struct RedChannelClientPrivate RedChannelClientPrivate;
 
diff --git a/server/stream.h b/server/stream.h
index a415e43..447eae7 100644
--- a/server/stream.h
+++ b/server/stream.h
@@ -42,8 +42,6 @@
 #define RED_STREAM_DEFAULT_LOW_START_BIT_RATE (2.5 * 1024 * 1024) // 2.5Mbps
 #define MAX_FPS 30
 
-typedef struct Stream Stream;
-
 typedef struct RedStreamActivateReportItem {
 RedPipeItem pipe_item;
 uint32_t stream_id;
@@ -62,7 +60,7 @@ typedef struct StreamStats {
 } StreamStats;
 #endif
 
-typedef struct StreamAgent {
+struct StreamAgent {
 QRegion vis_region; /* the part of the surface area that is currently 
occupied by video
fragments */
 QRegion clip;   /* the current video clipping. It can be different 
from vis_region:
@@ -86,7 +84,7 @@ typedef struct StreamAgent {
 #ifdef STREAM_STATS
 StreamStats stats;
 #endif
-} StreamAgent;
+};
 
 typedef struct RedStreamClipItem {
 RedPipeItem base;
-- 
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 00/10] Some RHEL6 compatibility patches

2016-10-17 Thread Frediano Ziglio 
These patches try to solve (or at least minimize) RHEL6
compatibilty issues.
Beside the last one other patches are small and
sensible and do not break defined rules.

Frediano Ziglio (9):
  Define a compatibility include for GLib
  Include GLib compatibility header where needed
  Avoid to typedef twice CursorChannel
  Avoid to typedef twice DisplayChannelClientPrivate
  Do not typedef DisplayChannel twice
  Include directly used header in dcc.h
  Avoid to typedef twice RedChannel and RedClient
  Include main-channel-client.h where MainChannelClient is used
  red-worker: Do not include not necessary red-channel-client.h
  RFC: Make code compile using RHEL 6

 server/Makefile.am |  1 +
 server/char-device.c   |  1 +
 server/cursor-channel-client.h |  2 +-
 server/dcc-private.h   |  1 -
 server/dcc.c   |  1 +
 server/dcc.h   |  1 +
 server/glib-compat.h   | 47 ++
 server/main-channel-client.h   |  5 +
 server/main-channel.c  |  3 ++-
 server/main-channel.h  |  1 -
 server/red-channel-client.c|  1 +
 server/red-channel-client.h|  4 
 server/red-qxl.c   |  1 +
 server/red-worker.h|  2 +-
 server/reds.c  |  1 +
 server/sound.c |  1 +
 server/stream.c|  1 +
 server/stream.h|  9 ++--
 18 files changed, 63 insertions(+), 20 deletions(-)
 create mode 100644 server/glib-compat.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 08/10] Include main-channel-client.h where MainChannelClient is used

2016-10-17 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/dcc.c  | 1 +
 server/main-channel.c | 3 ++-
 server/red-qxl.c  | 1 +
 server/reds.c | 1 +
 server/sound.c| 1 +
 server/stream.c   | 1 +
 6 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/server/dcc.c b/server/dcc.c
index 3519d2e..0080f10 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -22,6 +22,7 @@
 #include "dcc-private.h"
 #include "display-channel.h"
 #include "red-channel-client-private.h"
+#include "main-channel-client.h"
 #include "spice-server-enums.h"
 
 G_DEFINE_TYPE(DisplayChannelClient, display_channel_client, 
RED_TYPE_CHANNEL_CLIENT)
diff --git a/server/main-channel.c b/server/main-channel.c
index a1b8e31..d8ee159 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -22,9 +22,10 @@
 #include 
 
 #include "red-common.h"
-#include "main-channel.h"
 #include "reds.h"
 #include "red-channel-client.h"
+#include "main-channel.h"
+#include "main-channel-client.h"
 
 int main_channel_is_connected(MainChannel *main_chan)
 {
diff --git a/server/red-qxl.c b/server/red-qxl.c
index e517b41..87d613b 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -36,6 +36,7 @@
 #include "reds.h"
 #include "dispatcher.h"
 #include "red-parse-qxl.h"
+#include "main-channel-client.h"
 
 #include "red-qxl.h"
 
diff --git a/server/reds.c b/server/reds.c
index 79f9c9e..aee27e5 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -73,6 +73,7 @@
 #include "reds-private.h"
 #include "video-encoder.h"
 #include "red-channel-client.h"
+#include "main-channel-client.h"
 
 static void reds_client_monitors_config(RedsState *reds, VDAgentMonitorsConfig 
*monitors_config);
 static gboolean reds_use_client_monitors_config(RedsState *reds);
diff --git a/server/sound.c b/server/sound.c
index 4edf8ed..db23e95 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -41,6 +41,7 @@
 #include "sound.h"
 #include 
 #include "demarshallers.h"
+#include "main-channel-client.h"
 
 #ifndef IOV_MAX
 #define IOV_MAX 1024
diff --git a/server/stream.c b/server/stream.c
index b28c58b..6533111 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -20,6 +20,7 @@
 
 #include "stream.h"
 #include "display-channel.h"
+#include "main-channel-client.h"
 
 #define FPS_TEST_INTERVAL 1
 #define FOREACH_STREAMS(display, item)  \
-- 
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 06/10] Include directly used header in dcc.h

2016-10-17 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/dcc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/server/dcc.h b/server/dcc.h
index 2456f09..7a07981 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -25,6 +25,7 @@
 #include "pixmap-cache.h"
 #include "red-worker.h"
 #include "display-limits.h"
+#include "red-channel-client.h"
 
 G_BEGIN_DECLS
 
-- 
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 03/10] Avoid to typedef twice CursorChannel

2016-10-17 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel-client.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/cursor-channel-client.h b/server/cursor-channel-client.h
index 2336b95..fab6837 100644
--- a/server/cursor-channel-client.h
+++ b/server/cursor-channel-client.h
@@ -24,6 +24,7 @@
 #include "red-common.h"
 #include "red-channel-client.h"
 #include "reds-stream.h"
+#include "cursor-channel.h"
 
 G_BEGIN_DECLS
 
@@ -43,7 +44,6 @@ G_BEGIN_DECLS
 typedef struct CursorChannelClient CursorChannelClient;
 typedef struct CursorChannelClientClass CursorChannelClientClass;
 typedef struct CursorChannelClientPrivate CursorChannelClientPrivate;
-typedef struct CursorChannel CursorChannel;
 
 struct CursorChannelClient
 {
-- 
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 02/10] Include GLib compatibility header where needed

2016-10-17 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/char-device.c| 1 +
 server/red-channel-client.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/server/char-device.c b/server/char-device.c
index 4f01d3c..7775c07 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -25,6 +25,7 @@
 #include "char-device.h"
 #include "red-channel.h"
 #include "reds.h"
+#include "glib-compat.h"
 
 #define CHAR_DEVICE_WRITE_TO_TIMEOUT 100
 #define RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT 3
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 36d9c33..9426b13 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -36,6 +36,7 @@
 #include "red-channel-client.h"
 #include "red-channel-client-private.h"
 #include "red-channel.h"
+#include "glib-compat.h"
 
 static const SpiceDataHeaderOpaque full_header_wrapper;
 static const SpiceDataHeaderOpaque mini_header_wrapper;
-- 
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 04/10] Avoid to typedef twice DisplayChannelClientPrivate

2016-10-17 Thread Frediano Ziglio 
DisplayChannelClientPrivate is already defined in dcc.h which
is included in dcc-private.h header.

Signed-off-by: Frediano Ziglio 
---
 server/dcc-private.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/dcc-private.h b/server/dcc-private.h
index de6ea92..f3aba50 100644
--- a/server/dcc-private.h
+++ b/server/dcc-private.h
@@ -24,7 +24,6 @@
 #include "stream.h"
 #include "red-channel-client.h"
 
-typedef struct DisplayChannelClientPrivate DisplayChannelClientPrivate;
 struct DisplayChannelClientPrivate
 {
 uint32_t id;
-- 
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 01/10] Define a compatibility include for GLib

2016-10-17 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/Makefile.am   |  1 +
 server/glib-compat.h | 47 +++
 2 files changed, 48 insertions(+)
 create mode 100644 server/glib-compat.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 036abcd..dff1ad2 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -162,6 +162,7 @@ libserver_la_SOURCES =  \
dcc-private.h   \
image-encoders.c\
image-encoders.h\
+   glib-compat.h   \
$(spice_built_sources)  \
$(NULL)
 
diff --git a/server/glib-compat.h b/server/glib-compat.h
new file mode 100644
index 000..a20a434
--- /dev/null
+++ b/server/glib-compat.h
@@ -0,0 +1,47 @@
+/* -*- 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 .
+*/
+#ifndef GLIB_COMPAT_H_
+#define GLIB_COMPAT_H_
+
+#include 
+
+#if !GLIB_CHECK_VERSION(2,32,0)
+static inline void
+g_queue_free_full(GQueue *queue, GDestroyNotify free_func)
+{
+   /* quite hack cast but work with standard C call convention */
+   g_queue_foreach(queue, (GFunc) free_func, NULL);
+   g_queue_clear(queue);
+}
+#endif
+
+#if !GLIB_CHECK_VERSION(2,30,0)
+static inline gboolean
+g_queue_remove_boolean(GQueue *queue, gconstpointer data)
+{
+   GList *link = g_queue_find(queue, data);
+   if (!link) {
+   return FALSE;
+   }
+   g_queue_unlink(queue, link);
+   return TRUE;
+}
+#define g_queue_remove g_queue_remove_boolean
+#endif
+
+#endif /* GLIB_COMPAT_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 00/10] Some RHEL6 compatibility patches

2016-10-17 Thread Frediano Ziglio 
These patches try to solve (or at least minimize) RHEL6
compatibilty issues.
Beside the last one other patches are small and
sensible and do not break defined rules.

Frediano Ziglio (9):
  Include GLib compatibility header where needed
  Avoid to typedef twice CursorChannel
  Avoid to typedef twice DisplayChannelClientPrivate
  Do not typedef DisplayChannel twice
  Include directly used header in dcc.h
  Avoid to typedef twice RedChannel and RedClient
  Include main-channel-client.h where MainChannelClient is used
  red-worker: Do not include not necessary red-channel-client.h
  RFC: Make code compile using RHEL 6

test (1):
  Define a compatibility include for GLib

 server/Makefile.am |  1 +
 server/char-device.c   |  1 +
 server/cursor-channel-client.h |  2 +-
 server/dcc-private.h   |  1 -
 server/dcc.c   |  1 +
 server/dcc.h   |  1 +
 server/glib-compat.h   | 47 ++
 server/main-channel-client.h   |  5 +
 server/main-channel.c  |  3 ++-
 server/main-channel.h  |  1 -
 server/red-channel-client.c|  1 +
 server/red-channel-client.h|  4 
 server/red-qxl.c   |  1 +
 server/red-worker.h|  2 +-
 server/reds.c  |  1 +
 server/sound.c |  1 +
 server/stream.c|  1 +
 server/stream.h|  9 ++--
 18 files changed, 63 insertions(+), 20 deletions(-)
 create mode 100644 server/glib-compat.h

-- 
2.7.4

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

Re: [Spice-devel] [spice-gtk v2] clipboard: Fix crash by handling error

2016-10-17 Thread Victor Toso 
Hi,

On Mon, Oct 17, 2016 at 02:38:29PM +0200, Pavel Grunt wrote:
> Hey,
> 
> On Sat, 2016-10-15 at 14:15 +0200, victort...@redhat.com wrote:
> > From: Victor Toso 
> > 
> > As manual states below, text could be NULL for different reasons and
> > we should handle that. I've included a debug message to help
> > identifying possible regressions from wayland's clipboard.
> > 
> > This crash is a regression from 7b0de6217670e0f668aff2949f
> > 
> >  "The text parameter to callback will contain the resulting text if
> >  the request succeeded, or NULL if it failed. This could happen for
> >  various reasons, in particular if the clipboard was empty or if the
> >  contents of the clipboard could not be converted into text form."
> > 
> > Resolves: rhbz#1384676
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  src/spice-gtk-session.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 3ff4e9a..c08483c 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -945,6 +945,11 @@ static void
> > clipboard_received_text_cb(GtkClipboard *clipboard,
> >  if (self == NULL)
> >  return;
> >  
> > +if (text == NULL) {
> > +SPICE_DEBUG ("Failed to retrieve clipboard text");
> an extra space^

Fixed

> > +return;
> > +}
> > +
> >  g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> >  
> >  selection = get_selection_from_clipboard(self->priv,
> > clipboard);
> 
> 
> Acked-by: Pavel Grunt 

Thanks, pushed as
03c016bea939ee4a26e90d80fa1012a993a8ea47
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk v2] clipboard: Fix crash by handling error

2016-10-17 Thread Pavel Grunt 
Hey,

On Sat, 2016-10-15 at 14:15 +0200, victort...@redhat.com wrote:
> From: Victor Toso 
> 
> As manual states below, text could be NULL for different reasons and
> we should handle that. I've included a debug message to help
> identifying possible regressions from wayland's clipboard.
> 
> This crash is a regression from 7b0de6217670e0f668aff2949f
> 
>  "The text parameter to callback will contain the resulting text if
>  the request succeeded, or NULL if it failed. This could happen for
>  various reasons, in particular if the clipboard was empty or if the
>  contents of the clipboard could not be converted into text form."
> 
> Resolves: rhbz#1384676
> 
> Signed-off-by: Victor Toso 
> ---
>  src/spice-gtk-session.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 3ff4e9a..c08483c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -945,6 +945,11 @@ static void
> clipboard_received_text_cb(GtkClipboard *clipboard,
>  if (self == NULL)
>  return;
>  
> +if (text == NULL) {
> +SPICE_DEBUG ("Failed to retrieve clipboard text");
an extra space^
> +return;
> +}
> +
>  g_return_if_fail(SPICE_IS_GTK_SESSION(self));
>  
>  selection = get_selection_from_clipboard(self->priv,
> clipboard);


Acked-by: Pavel Grunt 

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

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

2016-10-17 Thread Francois Gouget 
On Thu, 1 Sep 2016, Christophe Fergeau wrote:

> On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote:
> > 
> > The following patch broke compilation of xf86-video-qxl:
> > 
> > commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
> > Author: Christophe Fergeau 
> > Date:   Thu Mar 5 15:28:22 2015 +0100
> > 
> > Mark unused public API methods/code as deprecated
> > 
> > Acked-by: Jonathon Jongsma 
> > 
> > 
> > The reason is it introduces a #include  in spice-server.h which 
> > is a *public* header! This means any application that needs to include 
> > spice-server.h, like xf86-video-qxl, must now check for GLib even if 
> > they don't use it, like xf86-video-qxl.
> > 
> > This does not make sense and thus the glib.h include must be removed. 
> 
> As danpb pointed out, main reason for that is that we forgot to add
> glib-2.0 as a dependency in the .pc file, if this had been done, this
> change would probably have gone mostly unnoticed.

No. The main reason is that the patch introduced a dependency which 
should not be there. Papering over it with a .pc file changes nothing to 
the fact that third-party projects will now be unable to use the header 
without installing glib first and that there is no good reason for that.



[...]
> Imo this SPICE_NO_DEPRECATED is only meant to be used internally by
> spice-gtk even if this is not really documented. I would not consider it
> breakage if this was removed, or changed in incompatible ways.
> And it turns out it's actually useless as -Wno-deprecated-declarations
> is used during compilation (which also disables the
> warnings from GLIB_VERSION_MIN_REQUIRED/GLIB_VERSION_MAX_REQUIRED :(
> I'd tend to change that so that -Wno-deprecated-declarations is only
> used for spicy, and make selective use of
> G_GNUC_BEGIN_IGNORE_DEPRECATIONS / G_GNUC_END_IGNORE_DEPRECATIONS when
> needed. Seems to be working with quick and dirty local changes.

I know you were talking about spice-gtk, but would the spice patch 
below be what you had in mind?


commit 8b6bb2357ba41426d08c0f322440c19cb8e1b897
Author: Francois Gouget 
Date:   Mon Oct 17 11:48:47 2016 +0200

server: Disable deprecation warnings only where needed

Signed-off-by: Francois Gouget 

diff --git a/server/red-qxl.c b/server/red-qxl.c
index e517b41..51e0dd6 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -971,6 +971,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
 qxl_state->dispatcher = dispatcher_new(RED_WORKER_MESSAGE_COUNT, NULL);
 qxl_state->qxl_worker.major_version = SPICE_INTERFACE_QXL_MAJOR;
 qxl_state->qxl_worker.minor_version = SPICE_INTERFACE_QXL_MINOR;
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 qxl_state->qxl_worker.wakeup = qxl_worker_wakeup;
 qxl_state->qxl_worker.oom = qxl_worker_oom;
 qxl_state->qxl_worker.start = qxl_worker_start;
@@ -987,6 +988,7 @@ void red_qxl_init(RedsState *reds, QXLInstance *qxl)
 qxl_state->qxl_worker.reset_cursor = qxl_worker_reset_cursor;
 qxl_state->qxl_worker.destroy_surface_wait = 
qxl_worker_destroy_surface_wait;
 qxl_state->qxl_worker.loadvm_commands = qxl_worker_loadvm_commands;
+G_GNUC_END_IGNORE_DEPRECATIONS
 
 qxl_state->max_monitors = UINT_MAX;
 qxl->st = qxl_state;
diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index b5baded..5dc173e 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -1241,7 +1241,9 @@ static void replay_handle_create_primary(QXLWorker 
*worker, SpiceReplay *replay)
 spice_printerr(
 "WARNING: %d: original recording event not preceded by a destroy 
primary",
 replay->counter);
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 worker->destroy_primary_surface(worker, 0);
+G_GNUC_END_IGNORE_DEPRECATIONS
 }
 replay->created_primary = TRUE;
 
@@ -1255,7 +1257,9 @@ static void replay_handle_create_primary(QXLWorker 
*worker, SpiceReplay *replay)
 read_binary(replay, "data", , , 0);
 surface.group_id = 0;
 surface.mem = QXLPHYSICAL_FROM_PTR(mem);
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 worker->create_primary_surface(worker, 0, );
+G_GNUC_END_IGNORE_DEPRECATIONS
 }
 
 static void replay_handle_dev_input(QXLWorker *worker, SpiceReplay *replay,
@@ -1264,15 +1268,21 @@ static void replay_handle_dev_input(QXLWorker *worker, 
SpiceReplay *replay,
 switch (message) {
 case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE:
 case RED_WORKER_MESSAGE_CREATE_PRIMARY_SURFACE_ASYNC:
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 replay_handle_create_primary(worker, replay);
+G_GNUC_END_IGNORE_DEPRECATIONS
 break;
 case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE:
 replay->created_primary = FALSE;
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 worker->destroy_primary_surface(worker, 0);
+G_GNUC_END_IGNORE_DEPRECATIONS
 break;

Re: [Spice-devel] [PATCH v2 0/4] Protocol file syntax documentation

2016-10-17 Thread Victor Toso 
Hi,

> > Not sure if I understand you correctly - but you should not modify
> > enums.h by yourself. As the first line of the file says, it should
> > be generated by spice_codegen.py (spice-common repository), eg:
> >
> > ./spice_codegen.py -e spice.proto ../spice-protocol/spice/enums.h

So, that should go in the documentation. I did not know that.

> If I understood correctly what Victor is trying to say is that in
> the channel definition the order of the messages is important as
> messages are associated to numbers more or less in a similar way to
> enum in C. This should go in the TODO under the "Channels" section.

The messages id are given by spice-protocol/spice/enums.h but the
spice-common marshalling code ignores its order as it is generated by
what is defined in spice.proto;

For me, it is really odd that the common code between spice and
spice-gtk dictates spice-protocol definitions. From my initial
perspective, we first define the protocol and then implement
(spice.proto for the serialization code and then the actual handlers in
clint, server and/or agent);

As we have start changing spice.proto to define the spice/enums.h
header, I think we should move spice.proto to spice-protocol code.

>
> OT: looking at file seems that there should be a section for
> protocol specification too.

Indeed :)

Cheers,
  toso

>
> > > 
> > >  --- a/spice/enums.h
> > >  +++ b/spice/enums.h
> > >  @@ -529,6 +529,7 @@ enum {
> > >  SPICE_MSGC_DISPLAY_STREAM_REPORT,
> > >  SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION,
> > >  SPICE_MSGC_DISPLAY_GL_DRAW_DONE,
> > >  +   SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE,
> > > 
> > >  SPICE_MSGC_END_DISPLAY
> > >  };
> > > 
> > > But what I did on spice.proto was:
> > > (the message is before gl-draw-done)
> > > 
> > >  --- a/spice.proto
> > >  +++ b/spice.proto
> > >  @@ -984,6 +984,10 @@ channel DisplayChannel : BaseChannel {
> > >   } preferred_compression;
> > > 
> > >    message {
> > >  +video_codec_type codec_type;
> > >  +} preferred_video_codec_type;
> > >  +
> > >  +message {
> > >   } gl_draw_done;
> > >   };
> > > 
> > > The outcome in common/generated_server_demarshallers.c is
> > > 
> > > (...)
> > >  static parse_msg_func_t funcs2[5] =  {
> > >  parse_msgc_display_init,
> > >  parse_msgc_display_stream_report,
> > >  parse_msgc_display_preferred_compression,
> > >  parse_msgc_display_preferred_video_codec_type
> > >  parse_msgc_display_gl_draw_done,
> > >  };
> > > 
> > > The handlers follow the order of spice.proto, meaning that on
> > > demarshalling it was using parse_msgc_display_gl_draw_done()
> > > function
> > > for SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE message.
> > > 
> > > ;)
> > > 
> > > Cheers,
> > >   toso
> > > 
> > > > 
> > > >  Makefile.am |   2 +-
> > > >  configure.ac|   2 +
> > > >  docs/Makefile.am|  17 ++
> > > >  docs/spice_protocol.txt | 423
> > > > 
> > > >  m4/spice_manual.m4  |  32 
> > > >  5 files changed, 475 insertions(+), 1 deletion(-)
> > > >  create mode 100644 docs/Makefile.am
> > > >  create mode 100644 docs/spice_protocol.txt
> > > >  create mode 100644 m4/spice_manual.m4
> > > > 
> 
> Frediano


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 v3 2/3] Improve MainChannel encapsulation

2016-10-17 Thread Frediano Ziglio 
> 
> From: Jonathon Jongsma 
> 
> Encapsulate MainChannel a bit better in preparation for proting to
> GObject.
> ---
>  server/main-channel-client.c | 24 +++-
>  server/main-channel.c| 15 +++
>  server/main-channel.h|  2 ++
>  3 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 0913028..28d2839 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -439,11 +439,7 @@ void
> main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
>  
>  mcc->priv->mig_wait_connect = FALSE;
>  mcc->priv->mig_connect_ok = success;
> -spice_assert(main_channel->num_clients_mig_wait);
> -spice_assert(!seamless || main_channel->num_clients_mig_wait == 1);
> -if (!--main_channel->num_clients_mig_wait) {
> -reds_on_main_migrate_connected(channel->reds, seamless &&
> success);
> -}
> +main_channel_on_migrate_connected(main_channel, seamless &&
> success);
>  } else {
>  if (success) {
>  spice_printerr("client %p MIGRATE_CANCEL", client);
> @@ -876,7 +872,7 @@ static void main_channel_marshall_notify(RedChannelClient
> *rcc,
>  static void main_channel_fill_migrate_dst_info(MainChannel *main_channel,
> SpiceMigrationDstInfo
> *dst_info)
>  {
> -RedsMigSpice *mig_dst = _channel->mig_target;
> +const RedsMigSpice *mig_dst =
> main_channel_peek_migration_target(main_channel);
>  dst_info->port = mig_dst->port;
>  dst_info->sport = mig_dst->sport;
>  dst_info->host_size = strlen(mig_dst->host) + 1;
> @@ -935,17 +931,19 @@ static void
> main_channel_marshall_migrate_switch(SpiceMarshaller *m, RedChannelC
>  RedChannel *channel = red_channel_client_get_channel(rcc);
>  SpiceMsgMainMigrationSwitchHost migrate;
>  MainChannel *main_ch;
> +const RedsMigSpice *mig_target;
>  
>  spice_printerr("");
>  red_channel_client_init_send_data(rcc,
>  SPICE_MSG_MAIN_MIGRATE_SWITCH_HOST, item);
>  main_ch = SPICE_CONTAINEROF(channel, MainChannel, base);
> -migrate.port = main_ch->mig_target.port;
> -migrate.sport = main_ch->mig_target.sport;
> -migrate.host_size = strlen(main_ch->mig_target.host) + 1;
> -migrate.host_data = (uint8_t *)main_ch->mig_target.host;
> -if (main_ch->mig_target.cert_subject) {
> -migrate.cert_subject_size = strlen(main_ch->mig_target.cert_subject)
> + 1;
> -migrate.cert_subject_data = (uint8_t
> *)main_ch->mig_target.cert_subject;
> +mig_target = main_channel_peek_migration_target(main_ch);
> +migrate.port = mig_target->port;
> +migrate.sport = mig_target->sport;
> +migrate.host_size = strlen(mig_target->host) + 1;
> +migrate.host_data = (uint8_t *)mig_target->host;
> +if (mig_target->cert_subject) {
> +migrate.cert_subject_size = strlen(mig_target->cert_subject) + 1;
> +migrate.cert_subject_data = (uint8_t *)mig_target->cert_subject;
>  } else {
>  migrate.cert_subject_size = 0;
>  migrate.cert_subject_data = NULL;
> diff --git a/server/main-channel.c b/server/main-channel.c
> index a1b8e31..2d2783d 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -419,3 +419,18 @@ int main_channel_migrate_src_complete(MainChannel
> *main_chan, int success)
> }
> return semi_seamless_count;
>  }
> +
> +void main_channel_on_migrate_connected(MainChannel *main_channel, gboolean
> seamless)
> +{
> +g_return_if_fail(main_channel->num_clients_mig_wait);
> +g_warn_if_fail(!seamless || main_channel->num_clients_mig_wait ==
> 1);

I just realized that beside spice_assert replacement this test is different.
Not seamless is the old "seamless && success" condition so this is equivalent
to 

g_warn_if_fail(!(seamless && success) || main_channel->num_clients_mig_wait == 
1);

which is

g_warn_if_fail(!seamless || !success || main_channel->num_clients_mig_wait == 
1);

we should probably pass seamless and success.

Perhaps would be more easier to define a new enum like

enum {
MIG_CONNECT_STANDARD,
MIG_CONNECT_SEAMLESS,
MIG_CONNECT_ERROR
};

that group success and seamless.

Still convinced that changing spice_assert has nothing to do which
this patch rationale.

> +if (!--main_channel->num_clients_mig_wait) {
> +
> reds_on_main_migrate_connected(red_channel_get_server(RED_CHANNEL(main_channel)),
> +   seamless);
> +}
> +}
> +
> +const RedsMigSpice* main_channel_peek_migration_target(MainChannel
> *main_chan)
> +{
> +return _chan->mig_target;
> +}
> diff --git a/server/main-channel.h b/server/main-channel.h
> index e0858d0..529e7be 100644
> --- a/server/main-channel.h
> +++ b/server/main-channel.h
> @@ -78,7 +78,9

Re: [Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd

2016-10-17 Thread Frediano Ziglio 
> 
> I'm not sure that needed as it seems getpeername/getsockname
> accept int fd and return -1 for fd=-1
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/main-channel.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index bf84694..9ff4dcd 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
> *channel, RedClient *client,
>  
>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
>  socklen_t *salen)
>  {
> -return main_chan ?
> getsockname(red_channel_get_first_socket(_chan->base), sa, salen) : -1;
> +int socketfd;
> +
> +if (!main_chan || ((socketfd =
> red_channel_get_first_socket(_chan->base)) < 0)) {
> +return -1;
> +}
> +
> +return getsockname(socketfd, sa, salen);
>  }
>  
>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
>  socklen_t *salen)
>  {
> -return main_chan ?
> getpeername(red_channel_get_first_socket(_chan->base), sa, salen) : -1;
> +int socketfd;
> +
> +if (!main_chan || ((socketfd =
> red_channel_get_first_socket(_chan->base)) < 0)) {
> +return -1;
> +}
> +
> +return getpeername(socketfd, sa, salen);
>  }
>  
>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
>  main_channels?

Mumble... I don't know why but it does not look that good.

These functions assume that there are only one client.
They are used only by spice_server_get_sock_info and spice_server_get_peer_info
which are no more used by recent Qemu so mainly they are dead code.
Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
check for the first client and call getpeername/getsockname by themself?
Or just adding a main_channel_get_socket instead of 2 obsolete main_channel_*
functions?

OT: red_channel_get_first_socket is used only for obsolete or wrong code.

OT: main_channel_close is causing a dandling file descriptor.

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-17 Thread Frediano Ziglio 
> 
> 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(_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(_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.
Beside that is not clear what the change could cause to the
upper layer.
I think the actual logic is supposing that dictionary creation (like
memory allocation) is not failing.

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

Re: [Spice-devel] [PATCH v2 0/4] Protocol file syntax documentation

2016-10-17 Thread Frediano Ziglio 
> 
> Hi Victor,
> 
> On Sat, 2016-10-15 at 15:38 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Thu, Sep 29, 2016 at 12:28:44PM +0100, Frediano Ziglio wrote:
> > > Small update for this patchset:
> > > - fix headers in "Extended protocol documentation";
> > > - added some more documentation on attributes.
> > > 
> > > Frediano Ziglio (4):
> > >   Start adding protocol file documentation
> > >   Start writing some documentation on protocol
> > >   Extended protocol documentation
> > >   More work on attribute protocol documentation
> > 
> > Ah, I thought this was in already, I was going to add some infom.
> > Let's have this, we can do more improvements or move it somewhere
> > else
> > if necessary later on.
> > 
> > Acked-by: Victor Toso 
> > 
> > About what I was going to include is how the order of the messages
> > in
> > spice.proto need to be in the same order of what is declared in the
> > protocol itself otherwise the (de)marshalling could be using the
> > wrong
> > function to (un)serialize.
> > 
> > - full explanation just for reference
> > 
> > I'm playing with a new message to tell (or choose) the preferred
> > video
> > codecs for streams. So I've included a new enum.h like:
> > (note that message is after gl-draw-done here)
> 
> Not sure if I understand you correctly - but you should not modify
> enums.h by yourself. As the first line of the file says, it should be
> generated by spice_codegen.py (spice-common repository), eg:
> 
> ./spice_codegen.py -e spice.proto ../spice-protocol/spice/enums.h
> 
> Pavel

If I understood correctly what Victor is trying to say is that in
the channel definition the order of the messages is important as
messages are associated to numbers more or less in a similar way to
enum in C. This should go in the TODO under the "Channels" section.

OT: looking at file seems that there should be a section for
protocol specification too.

> > 
> >  --- a/spice/enums.h
> >  +++ b/spice/enums.h
> >  @@ -529,6 +529,7 @@ enum {
> >  SPICE_MSGC_DISPLAY_STREAM_REPORT,
> >  SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION,
> >  SPICE_MSGC_DISPLAY_GL_DRAW_DONE,
> >  +   SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE,
> > 
> >  SPICE_MSGC_END_DISPLAY
> >  };
> > 
> > But what I did on spice.proto was:
> > (the message is before gl-draw-done)
> > 
> >  --- a/spice.proto
> >  +++ b/spice.proto
> >  @@ -984,6 +984,10 @@ channel DisplayChannel : BaseChannel {
> >   } preferred_compression;
> > 
> >    message {
> >  +video_codec_type codec_type;
> >  +} preferred_video_codec_type;
> >  +
> >  +message {
> >   } gl_draw_done;
> >   };
> > 
> > The outcome in common/generated_server_demarshallers.c is
> > 
> > (...)
> >  static parse_msg_func_t funcs2[5] =  {
> >  parse_msgc_display_init,
> >  parse_msgc_display_stream_report,
> >  parse_msgc_display_preferred_compression,
> >  parse_msgc_display_preferred_video_codec_type
> >  parse_msgc_display_gl_draw_done,
> >  };
> > 
> > The handlers follow the order of spice.proto, meaning that on
> > demarshalling it was using parse_msgc_display_gl_draw_done()
> > function
> > for SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE message.
> > 
> > ;)
> > 
> > Cheers,
> >   toso
> > 
> > > 
> > >  Makefile.am |   2 +-
> > >  configure.ac|   2 +
> > >  docs/Makefile.am|  17 ++
> > >  docs/spice_protocol.txt | 423
> > > 
> > >  m4/spice_manual.m4  |  32 
> > >  5 files changed, 475 insertions(+), 1 deletion(-)
> > >  create mode 100644 docs/Makefile.am
> > >  create mode 100644 docs/spice_protocol.txt
> > >  create mode 100644 m4/spice_manual.m4
> > > 

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-17 Thread Frediano Ziglio 
> 
> 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

void *image_data;

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

here 

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

> +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, );
> -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 8/8] red-record-qxl: child_output_setup: check fcntl return value

2016-10-17 Thread Frediano Ziglio 
> 
> Also replaced "continue" in while block with an empty
> block (added curly braces).
> 

I think you split this in 7/8.

> Found by coverity.
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/red-record-qxl.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index adc487b..21fc35f 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -845,13 +845,18 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
>  static void child_output_setup(gpointer user_data)
>  {
>  int fd = GPOINTER_TO_INT(user_data);
> +int r;
>  
>  while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
>  }
>  close(fd);
>  
>  // make sure file is not closed calling exec()
> -fcntl(STDOUT_FILENO, F_SETFD, 0);
> +while ((r = fcntl(STDOUT_FILENO, F_SETFD, 0)) < 0 && errno == EINTR) {
> +}
> +if (r < 0) {
> +spice_error("fcntl F_SETFD failed (%d)\n", errno);
> +}
>  }
>  
>  RedRecord *red_record_new(const char *filename)

I tried to understand better this.
First: fcntl(F_SETFD) cannot return EINTR so there's no reason to check
 (unless you check for kernel bugs).
This would change the code to

if (fcntl(STDOUT_FILENO, F_SETFD, 0) < 0) {
spice_error("fcntl F_SETFD failed (%d)\n", errno);
}

Note however that probably this won't do what you are expecting.
This will put the message in the log and then spice-server will continue
and write the recording into a closed pipe so all fprintf internally will
call write which will return EPIPE.

Looking at the called function the file descriptor is the file descriptor
of "f" which is not created with O_CLOEXEC flag so the easier way to remove
this warning is just remove the fcntl line (which is doing nothing).

About O_CLOEXEC would be worth perhaps setting this flag after setting
up file/pipe (before the call to fwrite in red_record_new), something
like

if (fcntl(fileno(f), F_SETFD, O_CLOEXEC) < 0) {
spice_error("fcntl F_SETFD failed (%d)\n", errno);
}

it's a bit racy but not racy would require using G_SPAWN_CLOEXEC_PIPES
in g_spawn_async_with_pipes which is supported since GLib 2.40 or manually
build the pipe with pipe2 and O_CLOEXEC and passing it to 
g_spawn_async_with_pipes
(and opening the record file with "w+e" instead of just "w+").
But I don't think that risking to leak this file descriptor is a big deal...

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

Re: [Spice-devel] [spice-server PATCH 7/8] red-record-qxl: replace continue with empty block

2016-10-17 Thread Frediano Ziglio 
> 
> Spice coding style suggests to use curly braces
> for while blocks.
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/red-record-qxl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 8af2c9c..adc487b 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -846,8 +846,8 @@ static void child_output_setup(gpointer user_data)
>  {
>  int fd = GPOINTER_TO_INT(user_data);
>  
> -while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR)
> -continue;
> +while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
> +}
>  close(fd);
>  
>  // make sure file is not closed calling exec()

There is another occurrence in the same file.
It's weird to see a close bracket just before a statement, perhaps a combination
of the two like

while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
continue;
}

is more "unexpected" ?

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

Re: [Spice-devel] [spice-server PATCH 3/8] input-channel: add a comment to mark fallthrough in switch

2016-10-17 Thread Frediano Ziglio 
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/inputs-channel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 840d5e9..ee81bb4 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -285,6 +285,7 @@ static int inputs_channel_handle_parsed(RedChannelClient
> *rcc, uint32_t size, ui
>  key_down->code == SCROLL_LOCK_SCAN_CODE) {
>  activate_modifiers_watch(inputs_channel, reds);
>  }
> +/* fallthrough */
>  }
>  case SPICE_MSGC_INPUTS_KEY_UP: {
>  SpiceMsgcKeyUp *key_up = message;

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [spice-server PATCH 5/8] mjpeg_encoder_new: allocate memory after sanity check

2016-10-17 Thread Frediano Ziglio 
> This patch prevents a leak in case the function returns early
> 
> Found by coverity.
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/mjpeg-encoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> index 1649516..d95c645 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -1372,10 +1372,11 @@ VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType
> codec_type,
>  bitmap_ref_t bitmap_ref,
>  bitmap_unref_t bitmap_unref)
>  {
> -MJpegEncoder *encoder = spice_new0(MJpegEncoder, 1);
> +MJpegEncoder *encoder;
>  
>  spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG,
>  NULL);
>  
> +encoder = spice_new0(MJpegEncoder, 1);
>  encoder->base.destroy = mjpeg_encoder_destroy;
>  encoder->base.encode_frame = mjpeg_encoder_encode_frame;
>  encoder->base.client_stream_report = mjpeg_encoder_client_stream_report;

Acked-by: Frediano Ziglio 

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

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

2016-10-17 Thread Frediano Ziglio 
> 
> 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(>items))) {
> +if ((ring_item = ring_get_head(>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


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(>items))) {
+if ((ring_item = ring_get_head(_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(>items, ring_item))) {
+if ((ring_item = ring_next(_container->items, ring_item))) {
 now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
 } else {
-now = >base;
+now = _container->base;
 }
 }
 }


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

Re: [Spice-devel] [PATCH spice-server v3 1/3] Add CommonGraphicsChannelPrivate struct

2016-10-17 Thread Pavel Grunt 
On Fri, 2016-10-14 at 10:40 -0400, Frediano Ziglio wrote:
> > 
> > From: Jonathon Jongsma 
> > 
> > Encapsulate private data for CommonGraphicsChannel and prepare for
> > GObject conversion.
> > ---
> >  server/common-graphics-channel.c | 33
> > +++--
> >  server/common-graphics-channel.h | 14 ++
> >  server/cursor-channel-client.c   |  2 +-
> >  server/cursor-channel.c  |  8 +---
> >  server/dcc-send.c|  2 +-
> >  server/dcc.c |  9 +
> >  server/display-channel.c |  6 +++---
> >  server/red-worker.c  | 10 +++---
> >  8 files changed, 59 insertions(+), 25 deletions(-)
> > 
> > diff --git a/server/common-graphics-channel.c
> > b/server/common-graphics-channel.c
> > index bcf7279..b02f3d7 100644
> > --- a/server/common-graphics-channel.c
> > +++ b/server/common-graphics-channel.c
> > @@ -27,6 +27,19 @@
> >  #include "dcc.h"
> >  #include "main-channel-client.h"
> >  
> > +#define CHANNEL_RECEIVE_BUF_SIZE 1024
> > +
> > +struct CommonGraphicsChannelPrivate
> > +{
> > +QXLInstance *qxl;
> > +uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> > +int during_target_migrate; /* TRUE when the client that is
> > associated
> > with the channel
> > +  is during migration. Turned off
> > when the
> > vm is started.
> > +  The flag is used to avoid
> > sending messages
> > that are artifacts
> > +  of the transition from stopped
> > vm to
> > loaded vm (e.g., recreation
> > +  of the primary surface) */
> > +};
> > +
> >  static uint8_t *common_alloc_recv_buf(RedChannelClient *rcc,
> > uint16_t type,
> >  uint32_t size)
> >  {
> >  RedChannel *channel = red_channel_client_get_channel(rcc);
> > @@ -41,7 +54,7 @@ static uint8_t
> > *common_alloc_recv_buf(RedChannelClient
> > *rcc, uint16_t type, uint
> >  spice_critical("unexpected message size %u (max is %d)",
> > size,
> >  CHANNEL_RECEIVE_BUF_SIZE);
> >  return NULL;
> >  }
> > -return common->recv_buf;
> > +return common->priv->recv_buf;
> >  }
> >  
> >  static void common_release_recv_buf(RedChannelClient *rcc,
> > uint16_t type,
> >  uint32_t size,
> > @@ -123,7 +136,23 @@ CommonGraphicsChannel*
> > common_graphics_channel_new(RedsState *server,
> >  spice_return_val_if_fail(channel, NULL);
> >  
> >  common = COMMON_GRAPHICS_CHANNEL(channel);
> > -common->qxl = qxl;
> > +common->priv = g_new0(CommonGraphicsChannelPrivate, 1);
> > +common->priv->qxl = qxl;
> >  return common;
> >  }
> >  
> > +void
> > common_graphics_channel_set_during_target_migrate(CommonGraphicsCh
> > annel
> > *self, gboolean value)
> > +{
> > +self->priv->during_target_migrate = value;
> > +}
> > +
> > +gboolean
> > common_graphics_channel_get_during_target_migrate(CommonGraphicsCh
> > annel
> > *self)
> > +{
> > +return self->priv->during_target_migrate;
> > +}
> > +
> > +QXLInstance*
> > common_graphics_channel_get_qxl(CommonGraphicsChannel *self)
> > +{
> > +return self->priv->qxl;
> > +}
> > +
> > diff --git a/server/common-graphics-channel.h
> > b/server/common-graphics-channel.h
> > index b9473d8..97cd63b 100644
> > --- a/server/common-graphics-channel.h
> > +++ b/server/common-graphics-channel.h
> > @@ -25,21 +25,19 @@ int
> > common_channel_config_socket(RedChannelClient *rcc);
> >  
> >  #define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
> >  
> > -#define CHANNEL_RECEIVE_BUF_SIZE 1024
> > +typedef struct CommonGraphicsChannelPrivate
> > CommonGraphicsChannelPrivate;
> >  typedef struct CommonGraphicsChannel {
> >  RedChannel base; // Must be the first thing
> >  
> > -QXLInstance *qxl;
> > -uint8_t recv_buf[CHANNEL_RECEIVE_BUF_SIZE];
> > -int during_target_migrate; /* TRUE when the client that is
> > associated
> > with the channel
> > -  is during migration. Turned off
> > when the
> > vm is started.
> > -  The flag is used to avoid
> > sending messages
> > that are artifacts
> > -  of the transition from stopped
> > vm to
> > loaded vm (e.g., recreation
> > -  of the primary surface) */
> > +CommonGraphicsChannelPrivate *priv;
> >  } CommonGraphicsChannel;
> >  
> >  #define COMMON_GRAPHICS_CHANNEL(Channel)
> > ((CommonGraphicsChannel*)(Channel))
> >  
> > +void
> > common_graphics_channel_set_during_target_migrate(CommonGraphicsCh
> > annel
> > *self, gboolean value);
> > +gboolean
> > common_graphics_channel_get_during_target_migrate(CommonGraphicsCh
> > annel
> > *self);
> > +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,
> > diff

Re: [Spice-devel] [PATCH v2 0/4] Protocol file syntax documentation

2016-10-17 Thread Pavel Grunt 
Hi Victor,

On Sat, 2016-10-15 at 15:38 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, Sep 29, 2016 at 12:28:44PM +0100, Frediano Ziglio wrote:
> > Small update for this patchset:
> > - fix headers in "Extended protocol documentation";
> > - added some more documentation on attributes.
> > 
> > Frediano Ziglio (4):
> >   Start adding protocol file documentation
> >   Start writing some documentation on protocol
> >   Extended protocol documentation
> >   More work on attribute protocol documentation
> 
> Ah, I thought this was in already, I was going to add some infom.
> Let's have this, we can do more improvements or move it somewhere
> else
> if necessary later on.
> 
> Acked-by: Victor Toso 
> 
> About what I was going to include is how the order of the messages
> in
> spice.proto need to be in the same order of what is declared in the
> protocol itself otherwise the (de)marshalling could be using the
> wrong
> function to (un)serialize.
> 
> - full explanation just for reference
> 
> I'm playing with a new message to tell (or choose) the preferred
> video
> codecs for streams. So I've included a new enum.h like:
> (note that message is after gl-draw-done here)

Not sure if I understand you correctly - but you should not modify
enums.h by yourself. As the first line of the file says, it should be
generated by spice_codegen.py (spice-common repository), eg:

./spice_codegen.py -e spice.proto ../spice-protocol/spice/enums.h

Pavel
> 
>  --- a/spice/enums.h
>  +++ b/spice/enums.h
>  @@ -529,6 +529,7 @@ enum {
>  SPICE_MSGC_DISPLAY_STREAM_REPORT,
>  SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION,
>  SPICE_MSGC_DISPLAY_GL_DRAW_DONE,
>  +   SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE,
> 
>  SPICE_MSGC_END_DISPLAY
>  };
> 
> But what I did on spice.proto was:
> (the message is before gl-draw-done)
> 
>  --- a/spice.proto
>  +++ b/spice.proto
>  @@ -984,6 +984,10 @@ channel DisplayChannel : BaseChannel {
>   } preferred_compression;
> 
>    message {
>  +video_codec_type codec_type;
>  +} preferred_video_codec_type;
>  +
>  +message {
>   } gl_draw_done;
>   };
> 
> The outcome in common/generated_server_demarshallers.c is
> 
> (...)
>  static parse_msg_func_t funcs2[5] =  {
>  parse_msgc_display_init,
>  parse_msgc_display_stream_report,
>  parse_msgc_display_preferred_compression,
>  parse_msgc_display_preferred_video_codec_type
>  parse_msgc_display_gl_draw_done,
>  };
> 
> The handlers follow the order of spice.proto, meaning that on
> demarshalling it was using parse_msgc_display_gl_draw_done()
> function
> for SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE message.
> 
> ;)
> 
> Cheers,
>   toso
> 
> > 
> >  Makefile.am |   2 +-
> >  configure.ac|   2 +
> >  docs/Makefile.am|  17 ++
> >  docs/spice_protocol.txt | 423
> > 
> >  m4/spice_manual.m4  |  32 
> >  5 files changed, 475 insertions(+), 1 deletion(-)
> >  create mode 100644 docs/Makefile.am
> >  create mode 100644 docs/spice_protocol.txt
> >  create mode 100644 m4/spice_manual.m4
> > 
> > -- 
> > 2.7.4
> > 
> > ___
> > 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel