Re: [Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Victor Toso]

Hi,

On Tue, Nov 03, 2015 at 07:18:33AM +0100, Victor Toso wrote:
> On Tue, Nov 03, 2015 at 01:51:41AM +0100, Fabiano Fidêncio wrote:
> > Cast uint64_t to long unsigned on printfs in order to avoid warnings
> > like:
> > usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
> > usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 4 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> > threshold",

Actually... the fix should be using "%llu" i think.

> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> >  ^
> > usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 5 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> > threshold",
> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> >  ^
> > usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 4 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> > threshold",
> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> >  ^
> > usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 5 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> > threshold",
> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> >  ^
> > usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
> > usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 4 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> >  ^
> > usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 
> > 'long unsigned int', but argument 5 has type 'uint64_t {aka long long 
> > unsigned int}' [-Wformat=]
> >  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> >^
> > usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
> >  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> > 
> > A better way to solve the problem would be using %zu (C99 only) instead
> > of doing the cast, but then mingw32 complains about:
> > "warning: unknown conversion type character 'z' in format [-Wformat=]"
> > 
> > Signed-off-by: Fabiano Fidêncio 
> > ---
> >  usbredirhost/usbredirhost.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> > index adf9c5f..bfccfab 100644
> > --- a/usbredirhost/usbredirhost.c
> > +++ b/usbredirhost/usbredirhost.c
> > @@ -1021,12 +1021,14 @@ static int 
> > usbredirhost_can_write_iso_package(struct usbredirhost *host)
> >  if (size >= host->iso_threshold.higher) {
> >  if (!host->iso_threshold.dropping)
> >  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> > threshold",
> > -  size, host->iso_threshold.higher);
> > +  (long unsigned) size,
> > +  (long unsigned) host->iso_threshold.higher);
> >  host->iso_threshold.dropping = true;
> >  } else if (size < host->iso_threshold.lower) {
> >  if (host->iso_threshold.dropping)
> >  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> > threshold",
> > -  size, host->iso_threshold.lower);
> > +  (long unsigned) size,
> > +  (long unsigned) host->iso_threshold.lower);
> >  
> >  host->iso_threshold.dropping = false;
> >  }
> > @@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
> > usbredirhost *host,
> >  host->iso_threshold.lower = reference / 2;
> >  host->iso_threshold.higher = reference * 3;
> >   

Re: [Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Victor Toso]

Hi,

On Tue, Nov 03, 2015 at 01:51:41AM +0100, Fabiano Fidêncio wrote:
> Cast uint64_t to long unsigned on printfs in order to avoid warnings
> like:
> usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
  ^
Actually, shouldn't this be fixed with "%llu" ?

>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> 
> A better way to solve the problem would be using %zu (C99 only) instead
> of doing the cast, but then mingw32 complains about:
> "warning: unknown conversion type character 'z' in format [-Wformat=]"
> 
> Signed-off-by: Fabiano Fidêncio 
> ---
>  usbredirhost/usbredirhost.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> index adf9c5f..bfccfab 100644
> --- a/usbredirhost/usbredirhost.c
> +++ b/usbredirhost/usbredirhost.c
> @@ -1021,12 +1021,14 @@ static int usbredirhost_can_write_iso_package(struct 
> usbredirhost *host)
>  if (size >= host->iso_threshold.higher) {
>  if (!host->iso_threshold.dropping)
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
> -  size, host->iso_threshold.higher);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.higher);
>  host->iso_threshold.dropping = true;
>  } else if (size < host->iso_threshold.lower) {
>  if (host->iso_threshold.dropping)
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
> -  size, host->iso_threshold.lower);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.lower);
>  
>  host->iso_threshold.dropping = false;
>  }
> @@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
> usbredirhost *host,
>  host->iso_threshold.lower = reference / 2;
>  host->iso_threshold.higher = reference * 3;
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> -   host->iso_threshold.higher, host->iso_threshold.lower);
> +   (long unsigned) host->iso_threshold.higher,
> +

Re: [Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Victor Toso]

Hi,

On Tue, Nov 03, 2015 at 01:51:41AM +0100, Fabiano Fidêncio wrote:
> Cast uint64_t to long unsigned on printfs in order to avoid warnings
> like:
> usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
> 
> A better way to solve the problem would be using %zu (C99 only) instead
> of doing the cast, but then mingw32 complains about:
> "warning: unknown conversion type character 'z' in format [-Wformat=]"
> 
> Signed-off-by: Fabiano Fidêncio 
> ---
>  usbredirhost/usbredirhost.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> index adf9c5f..bfccfab 100644
> --- a/usbredirhost/usbredirhost.c
> +++ b/usbredirhost/usbredirhost.c
> @@ -1021,12 +1021,14 @@ static int usbredirhost_can_write_iso_package(struct 
> usbredirhost *host)
>  if (size >= host->iso_threshold.higher) {
>  if (!host->iso_threshold.dropping)
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
> -  size, host->iso_threshold.higher);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.higher);
>  host->iso_threshold.dropping = true;
>  } else if (size < host->iso_threshold.lower) {
>  if (host->iso_threshold.dropping)
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
> -  size, host->iso_threshold.lower);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.lower);
>  
>  host->iso_threshold.dropping = false;
>  }
> @@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
> usbredirhost *host,
>  host->iso_threshold.lower = reference / 2;
>  host->iso_threshold.higher = reference * 3;
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> -   host->iso_threshold.higher, host->iso_threshold.lower);
> +   (long unsigned) host->iso_threshold.higher,
> +   (long unsigned) host->iso_threshold.lower);
>  }
>

Interesting that I did not see any of 

Re: [Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Fabiano Fidêncio]

On Tue, Nov 3, 2015 at 1:51 AM, Fabiano Fidêncio  wrote:
> Cast uint64_t to long unsigned on printfs in order to avoid warnings
> like:
> usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>  ^
> usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
> unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned 
> int}' [-Wformat=]
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
>^
> usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
>  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
>
> A better way to solve the problem would be using %zu (C99 only) instead
> of doing the cast, but then mingw32 complains about:
> "warning: unknown conversion type character 'z' in format [-Wformat=]"
>
> Signed-off-by: Fabiano Fidêncio 
> ---
>  usbredirhost/usbredirhost.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> index adf9c5f..bfccfab 100644
> --- a/usbredirhost/usbredirhost.c
> +++ b/usbredirhost/usbredirhost.c
> @@ -1021,12 +1021,14 @@ static int usbredirhost_can_write_iso_package(struct 
> usbredirhost *host)
>  if (size >= host->iso_threshold.higher) {
>  if (!host->iso_threshold.dropping)
>  DEBUG("START dropping isoc packets %lu buffer > %lu hi 
> threshold",
> -  size, host->iso_threshold.higher);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.higher);
>  host->iso_threshold.dropping = true;
>  } else if (size < host->iso_threshold.lower) {
>  if (host->iso_threshold.dropping)
>  DEBUG("STOP dropping isoc packets %lu buffer < %lu low 
> threshold",
> -  size, host->iso_threshold.lower);
> +  (long unsigned) size,
> +  (long unsigned) host->iso_threshold.lower);
>
>  host->iso_threshold.dropping = false;
>  }
> @@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
> usbredirhost *host,
>  host->iso_threshold.lower = reference / 2;
>  host->iso_threshold.higher = reference * 3;
>  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
> -   host->iso_threshold.higher, host->iso_threshold.lower);
> +   (long unsigned) host->iso_threshold.higher,
> +   (long unsigned) host->iso_threshold.lower);
>  }
>
>  /* Called from both parser read and packet complete cal

[Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Fabiano Fidêncio]

Cast uint64_t to long unsigned on printfs in order to avoid warnings
like:
usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
 ^
usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
 ^
usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
 ^
usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
 ^
usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
 ^
usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
 DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
   ^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
 #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)

A better way to solve the problem would be using %zu (C99 only) instead
of doing the cast, but then mingw32 complains about:
"warning: unknown conversion type character 'z' in format [-Wformat=]"

Signed-off-by: Fabiano Fidêncio 
---
 usbredirhost/usbredirhost.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index adf9c5f..bfccfab 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -1021,12 +1021,14 @@ static int usbredirhost_can_write_iso_package(struct 
usbredirhost *host)
 if (size >= host->iso_threshold.higher) {
 if (!host->iso_threshold.dropping)
 DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
-  size, host->iso_threshold.higher);
+  (long unsigned) size,
+  (long unsigned) host->iso_threshold.higher);
 host->iso_threshold.dropping = true;
 } else if (size < host->iso_threshold.lower) {
 if (host->iso_threshold.dropping)
 DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
-  size, host->iso_threshold.lower);
+  (long unsigned) size,
+  (long unsigned) host->iso_threshold.lower);
 
 host->iso_threshold.dropping = false;
 }
@@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
usbredirhost *host,
 host->iso_threshold.lower = reference / 2;
 host->iso_threshold.higher = reference * 3;
 DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
-   host->iso_threshold.higher, host->iso_threshold.lower);
+   (long unsigned) host->iso_threshold.higher,
+   (long unsigned) host->iso_threshold.lower);
 }
 
 /* Called from both parser read and packet complete callbacks */
-- 
2.5.0

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

[Spice-devel] [PATCH xf86-video-qxl] Xspice: handle parameters with value 0, allows --port 0

[Jeremy White]

Signed-off-by: Jeremy White 
---
 scripts/Xspice | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/Xspice b/scripts/Xspice
index 52b1b16..46c5c1e 100755
--- a/scripts/Xspice
+++ b/scripts/Xspice
@@ -256,7 +256,7 @@ var_args = ['port', 'tls_port', 'disable_ticketing',
 'vdagent_uid', 'vdagent_gid']
 
 for arg in var_args:
-if getattr(args, arg):
+if getattr(args, arg) != None:
 # The Qxl code doesn't respect booleans, so pass them as 0/1
 a = getattr(args, arg)
 if a == True:
-- 
2.1.4

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

Re: [Spice-devel] [PATCH spice ] Add support for clients connecting with the WebSocket protocol.

[Jeremy White]

Hi Pavel,

Thanks for the review.

>>
>> +static void reds_handle_new_link(RedLinkInfo *link);
>>  static void reds_handle_read_header_done(void *opaque)
>>  {
>>  RedLinkInfo *link = (RedLinkInfo *)opaque;
>> @@ -2182,6 +2183,11 @@ static void reds_handle_read_header_done(void *opaque)
>>  header->size = GUINT32_FROM_LE(header->size);
>>  
>>  if (header->magic != SPICE_MAGIC) {
>> +if (reds_stream_is_websocket(link->stream,
>> +(guchar *) header, sizeof(*header))) {
>> +reds_handle_new_link(link);
>> +return;
>> +}
> This looks hacky, it would be nice to detect that we are dealing with 
> websockets
> before reds_handle_read_header_done() is called.

The problem is that we cannot know we're dealing with websockets until
we've done an initial read.  And a websocket connection is going to have
to write at least 16 bytes, so it seemed to me that we may as well read
the 16 bytes in a SpiceLinkHeader.

The alternate I considered was to inject a 3 byte read before
read_header_done and then have that do an async read of
sizeof(SpiceLinkHeader) - 3 if we do not have the 'GET' that signals a
websocket connection.  That didn't seem any better to me; I'm happy to
defer to others if they feel that would be superior (or if there is an
altogether different approach I've missed).

In reviewing this code, there is a flaw I've skipped over.  That is, the
current implementation makes the assumption that the websocket header
will arrive in a single packet; that there is no chance we'll get a few
bytes, and then need another read in order to get the remaining bytes.
I think that, in practice, this will always be the case.  Further,
afaik, we don't really have a mechanism to do a variable length/timed
read in the current code base.  My instinct is to say that limitation is
acceptable; that the complexity of a theoretically correct
implementation would do more harm than accepting this assumption.  But I
bring it up to give others the chance to correct me .

[snip]
>> +websocket_create_reply(rbuf, len, outbuf);
>> +rc = stream->priv->write(stream, outbuf, strlen(outbuf));
>> +if (rc == strlen(outbuf)) {
>> +stream->priv->ws = spice_malloc0(sizeof(*stream->priv->ws));
> 
> It is not freed

Yes, right, will fix it.

>> +int websocket_is_start(gchar *buf, int len)
> 
> It sounds like boolean

Fair enough, I'll fix it.

> 
>> +{
>> +if (len < 4)
>> +return 0;
>> +
>> +if (find_str(buf, "GET", len) == (buf + 3) &&
>> +find_str(buf, "Sec-WebSocket-Protocol: binary", len) &&
>> +find_str(buf, "Sec-WebSocket-Key:", len) &&
>> +buf[len - 2] == '\r' && buf[len - 1] == '\n')
>> +return 1;
>> +
>> +return 0;
>> +}
>> +
>> +int websocket_create_reply(gchar *buf, int len, gchar *outbuf)
> 
> It always returns 0

Yep, should be a void, I'll fix it.

> 
>> +{
>> +gchar *key;
>> +
>> +key = generate_reply_key(buf, len);
> 
> Can NULL be valid value? Should we have a warning for it, or change the return
> value?

NULL is not a valid value; this code path should only be exercised if
we've found the Sec-WebSocket-Key already.

> 
>> +sprintf(outbuf, "HTTP/1.1 101 Switching Protocols\r\n"
>> +"Upgrade: websocket\r\n"
>> +"Connection: Upgrade\r\n"
>> +"Sec-WebSocket-Accept: %s\r\n"
>> +"Sec-WebSocket-Protocol: binary\r\n\r\n", key);
>> +g_free(key);
>> +return 0;
>> +}
>> diff --git a/server/websocket.h b/server/websocket.h
>> new file mode 100644
>> index 000..b35cb5e
>> --- /dev/null
>> +++ b/server/websocket.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + *  Copyright (C) 2015 Jeremy White
>> + *
>> + *  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 
>> > />.
>> + */
>> +
>> +#define WEBSOCKET_MAX_HEADER_SIZE (1 + 9 + 4)
>> +
>> +#define FIN_FLAG0x80
>> +#define TYPE_MASK   0x0F
>> +
>> +#define BINARY_FRAME0x2
>> +#define CLOSE_FRAME 0x8
>> +#define PING_FRAME  0x9
>> +#define PONG_FRAME  0xA
>> +
>> +#define LENGTH_MASK 0x7F
>> +#define LENGTH_16BIT0x7E
>> +#define LENGTH_64BIT0x7F
>> +
>> +#define MASK_FLAG   0x80
>> +
>> +#define WEBSOCKET_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
>> +
>> +

Re: [Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

[Jonathon Jongsma]

On Mon, 2015-11-02 at 11:00 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > fzig...@redhat.com> wrote:
> > > From: Marc-André Lureau 
> > > 
> > > Doing so allows us to remove the extra QXLInstance parameter from
> > > cursor_item_unref() and makes the code a bit cleaner.
> > > 
> > > Also add cursor_item_ref().
> > > 
> > > Signed-off-by: Jonathon Jongsma 
> > > ---
> > >  server/cursor-channel.c | 70
> > >  +++--
> > >  server/cursor-channel.h |  9 ---
> > >  2 files changed, 44 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index eef0121..d145f86 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -25,55 +25,58 @@
> > >  #include "cache_item.tmpl.c"
> > >  #undef CLIENT_CURSOR_CACHE
> > > 
> > > -static inline CursorItem *alloc_cursor_item(void)
> > > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd,
> > > uint32_t
> > > group_id)
> > >  {
> > >  CursorItem *cursor_item;
> > > 
> > > +spice_return_val_if_fail(cmd != NULL, NULL);
> > > +
> > >  cursor_item = g_slice_new0(CursorItem);
> > > +cursor_item->qxl = qxl;
> > >  cursor_item->refs = 1;
> > > +cursor_item->group_id = group_id;
> > > +cursor_item->red_cursor = cmd;
> > > 
> > >  return cursor_item;
> > >  }
> > > 
> > > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t
> > > group_id)
> > > +CursorItem *cursor_item_ref(CursorItem *item)
> > >  {
> > > -CursorItem *cursor_item;
> > > -
> > > -spice_return_val_if_fail(cmd != NULL, NULL);
> > > -cursor_item = alloc_cursor_item();
> > > +spice_return_val_if_fail(item != NULL, NULL);
> > > +spice_return_val_if_fail(item->refs > 0, NULL);
> > > 
> > > -cursor_item->group_id = group_id;
> > > -cursor_item->red_cursor = cmd;
> > > +item->refs++;
> > > 
> > > -return cursor_item;
> > > +return item;
> > >  }
> > > 
> > > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > > +void cursor_item_unref(CursorItem *item)
> > >  {
> > > -if (!--cursor->refs) {
> > > -QXLReleaseInfoExt release_info_ext;
> > > -RedCursorCmd *cursor_cmd;
> > > -
> > > -cursor_cmd = cursor->red_cursor;
> > > -release_info_ext.group_id = cursor->group_id;
> > > -release_info_ext.info = cursor_cmd->release_info;
> > > -qxl->st->qif->release_resource(qxl, release_info_ext);
> > > -red_put_cursor_cmd(cursor_cmd);
> > > -free(cursor_cmd);
> > > -
> > > -g_slice_free(CursorItem, cursor);
> > > -}
> > > +QXLReleaseInfoExt release_info_ext;
> > > +RedCursorCmd *cursor_cmd;
> > > +
> > > +spice_return_if_fail(item != NULL);
> > > +
> > > +if (--item->refs)
> > 
> > Just a nitpick, I would prefer to have a explicit comparison here:
> > if
> > (--items->refs > 0) ...
> > 
> 
> Why not 
> 
>   if (--items->refs != 0) ??
> 
> I mean, at the end the results should be the same if no errors
> managing
> the counters are present. Are you suggesting to change the test to
> avoid
> multiple free (hoping memory is still untouched after the first one)
> ?
> 
> I start suspecting that sending the patch when there are pending
> issue to
> be discussed is not really a good idea.


Hmm, what do you mean by this? What pending issues need to be
discussed?



> 
> > > +return;
> > > +
> > > +cursor_cmd = item->red_cursor;
> > > +release_info_ext.group_id = item->group_id;
> > > +release_info_ext.info = cursor_cmd->release_info;
> > > +item->qxl->st->qif->release_resource(item->qxl,
> > > release_info_ext);
> > > +red_put_cursor_cmd(cursor_cmd);
> > > +free(cursor_cmd);
> > > +
> > > +g_slice_free(CursorItem, item);
> > > +
> > >  }
> > > 
> > >  static void cursor_set_item(CursorChannel *cursor, CursorItem
> > > *item)
> > >  {
> > >  if (cursor->item)
> > > -cursor_item_unref(red_worker_get_qxl(cursor
> > > ->common.worker),
> > > cursor->item);
> > > -
> > > -if (item)
> > > -item->refs++;
> > > +cursor_item_unref(cursor->item);
> > > 
> > > -cursor->item = item;
> > > +cursor->item = item ? cursor_item_ref(item) : NULL;
> > >  }
> > > 
> > >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc,
> > > void *data,
> > >  int num)
> > > @@ -157,7 +160,7 @@ static void
> > > put_cursor_pipe_item(CursorChannelClient
> > > *ccc, CursorPipeItem *pipe_
> > > 
> > >  spice_assert(!pipe_item_is_linked(&pipe_item->base));
> > > 
> > > -cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> > > pipe_item->cursor_item);
> > > +cursor_item_unref(pipe_item->cursor_item);
> > >  free(pipe_item);
> > >  }
> > > 
> > > @@ -404,7 +407,11 @@ void
> > > cursor_channel_process_cmd(CursorChannel *cursor,
> > > RedCursorCmd *cursor_cmd,
> > >  CursorItem *cursor_item;
> > >  int

[Spice-devel] [PATCH v2 03/10] Palette cache: Use correct marshal function

[Jonathon Jongsma]

In order to invalidate a single palette cache item, we were using
spice_marshall_msg_cursor_inval_one(), which is the marshal function
used to send an invalidation message for the Cursor channel's cache.
This didn't cause any problems because SPICE_MSG_CURSOR_INVAL_ONE and
SPICE_MSG_DISPLAY_INVAL_PALETTE have the same message ID and parameters,
but it's better to use the correct marshalling function.
---

As I mentioned in the previous email, the real cause of the warning being fixed
by the previous patch was due to the removal of the PIPE_ITEM_TYPE_INVAL_ONE
case statement from display_channel_send_item(). So leaving this case in the
switch ensures that we don't get the warning. However, we were previously using
cursor-channel marshalling functions to send the display cache messages. It
worked because they happened to have the same message ID, but this patch makes
things more correct.

 server/red_worker.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index e3999ba..7c599b0 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -7626,15 +7626,17 @@ static inline void 
marshall_qxl_drawable(RedChannelClient *rcc,
 red_lossy_marshall_qxl_drawable(display_channel->common.worker, rcc, 
m, dpi);
 }
 
-static inline void red_marshall_inval(RedChannelClient *rcc,
-  SpiceMarshaller *base_marshaller, 
CacheItem *cach_item)
+static inline void red_marshall_inval_palette(RedChannelClient *rcc,
+  SpiceMarshaller *base_marshaller,
+  CacheItem *cache_item)
 {
 SpiceMsgDisplayInvalOne inval_one;
 
-red_channel_client_init_send_data(rcc, cach_item->inval_type, NULL);
-inval_one.id = *(uint64_t *)&cach_item->id;
+red_channel_client_init_send_data(rcc, cache_item->inval_type, NULL);
+inval_one.id = *(uint64_t *)&cache_item->id;
+
+spice_marshall_msg_display_inval_palette(base_marshaller, &inval_one);
 
-spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one);
 }
 
 static void 
display_channel_marshall_migrate_data_surfaces(DisplayChannelClient *dcc,
@@ -8093,7 +8095,7 @@ static void display_channel_send_item(RedChannelClient 
*rcc, PipeItem *pipe_item
 break;
 }
 case PIPE_ITEM_TYPE_INVAL_ONE:
-red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
+red_marshall_inval_palette(rcc, m, (CacheItem *)pipe_item);
 break;
 case PIPE_ITEM_TYPE_STREAM_CREATE: {
 StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, 
create_item);
@@ -9352,7 +9354,6 @@ static void 
display_channel_client_release_item_before_push(DisplayChannelClient
 free(item);
 break;
 }
-case PIPE_ITEM_TYPE_INVAL_ONE:
 case PIPE_ITEM_TYPE_VERB:
 case PIPE_ITEM_TYPE_MIGRATE_DATA:
 case PIPE_ITEM_TYPE_PIXMAP_SYNC:
-- 
2.4.3

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

Re: [Spice-devel] [common 3/5] build-sys: Add the SPICE_WARNING() and SPICE_PRINT_MESSAGES m4 macros

[Francois Gouget]

On Fri, 30 Oct 2015, Christophe Fergeau wrote:

> Hey,
> 
> This looks good, but I was wondering whether this is being copied/pasted
> from some other configure.ac/m4 file, or if this is the initial
> implementation?

It comes from Wine which is also LGPL:
https://source.winehq.org/git/wine.git/blob/HEAD:/COPYING.LIB
https://source.winehq.org/git/wine.git/blob/HEAD:/aclocal.m4


> We'll need to readd the AS_VAR_APPEND fallback which was removed in
> f7ec855af3d , otherwise looks fine

Ok. I'll resubmit.


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

Re: [Spice-devel] [PATCH 03/10] Fix warning due to unexpected pipe item type

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 6:46 PM, Jonathon Jongsma  wrote:
> On Mon, 2015-11-02 at 10:55 -0500, Frediano Ziglio wrote:
>> >
>> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
>> > fzig...@redhat.com> wrote:
>> > > From: Marc-André Lureau 
>> > >
>> > > The specific item type that was not being handled was
>> > > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the
>> > > cursor
>> > > channel, but the analogous item for the display channel is
>> > > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
>> > >
>> > > The exact warning follows:
>> > >
>> > >  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
>> > >  ../../server/dcc-send.c:2442:dcc_send_item: should not be
>> > > reached
>> > >  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
>> > >  ../../server/dcc.c:1595:release_item_before_push: invalid
>> > > item type
>> > >
>> > > Signed-off-by: Jonathon Jongsma 
>> > > ---
>> > >  server/cache_item.tmpl.c |  4 +++-
>> > >  server/red_worker.c  | 15 ---
>> > >  2 files changed, 3 insertions(+), 16 deletions(-)
>> > >
>> > > diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
>> > > index dc314c0..ad2b579 100644
>> > > --- a/server/cache_item.tmpl.c
>> > > +++ b/server/cache_item.tmpl.c
>> > > @@ -21,6 +21,7 @@
>> > >  #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
>> > >  #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
>> > >  #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
>> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
>> > >  #define FUNC_NAME(name) red_cursor_cache_##name
>> > >  #define VAR_NAME(name) cursor_cache_##name
>> > >  #define CHANNEL CursorChannel
>> > > @@ -32,6 +33,7 @@
>> > >  #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
>> > >  #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
>> > >  #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
>> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
>> > >  #define FUNC_NAME(name) red_palette_cache_##name
>> > >  #define VAR_NAME(name) palette_cache_##name
>> > >  #define CHANNEL DisplayChannel
>> > > @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
>> > > *channel_client, CacheItem *item)
>> > >  channel_client->VAR_NAME(items)--;
>> > >  channel_client->VAR_NAME(available) += item->size;
>> > >
>> > > -red_channel_pipe_item_init(&channel->common.base, &item
>> > > ->u.pipe_data,
>> > > PIPE_ITEM_TYPE_INVAL_ONE);
>> > > +red_channel_pipe_item_init(&channel->common.base, &item
>> > > ->u.pipe_data,
>> > > PIPE_ITEM_TYPE);
>> > >  red_channel_client_pipe_add_tail(&channel_client
>> > > ->common.base,
>> > >  &item->u.pipe_data); // for now
>> > >  }
>> > >
>> > > diff --git a/server/red_worker.c b/server/red_worker.c
>> > > index 89cb25c..93a305a 100644
>> > > --- a/server/red_worker.c
>> > > +++ b/server/red_worker.c
>> > > @@ -7805,17 +7805,6 @@ static inline void
>> > > marshall_qxl_drawable(RedChannelClient *rcc,
>> > >  red_lossy_marshall_qxl_drawable(display_channel
>> > > ->common.worker,
>> > >  rcc, m, dpi);
>> > >  }
>> > >
>> > > -static inline void red_marshall_inval(RedChannelClient *rcc,
>> > > -  SpiceMarshaller
>> > > *base_marshaller,
>> > > CacheItem *cach_item)
>> > > -{
>> > > -SpiceMsgDisplayInvalOne inval_one;
>> > > -
>> > > -red_channel_client_init_send_data(rcc, cach_item
>> > > ->inval_type, NULL);
>> > > -inval_one.id = *(uint64_t *)&cach_item->id;
>> > > -
>> > > -spice_marshall_msg_cursor_inval_one(base_marshaller,
>> > > &inval_one);
>> > > -}
>> > > -
>> > >  static void
>> > >  display_channel_marshall_migrate_data_surfaces(DisplayChannelCli
>> > > ent *dcc,
>> > >
>> > >  SpiceMarshaller
>> > > *m,
>> > > int
>> > > lossy)
>> > > @@ -8271,9 +8260,6 @@ static void
>> > > display_channel_send_item(RedChannelClient *rcc, PipeItem
>> > > *pipe_item
>> > >  marshall_qxl_drawable(rcc, m, dpi);
>> > >  break;
>> > >  }
>> > > -case PIPE_ITEM_TYPE_INVAL_ONE:
>> > > -red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
>> > > -break;
>> > >  case PIPE_ITEM_TYPE_STREAM_CREATE: {
>> > >  StreamAgent *agent = SPICE_CONTAINEROF(pipe_item,
>> > > StreamAgent,
>> > >  create_item);
>> > >  red_display_marshall_stream_start(rcc, m, agent);
>> > > @@ -9580,7 +9566,6 @@ static void
>> > > display_channel_client_release_item_before_push(DisplayChannelCli
>> > > ent
>> > >  free(item);
>> > >  break;
>> > >  }
>> > > -case PIPE_ITEM_TYPE_INVAL_ONE:
>> > >  case PIPE_ITEM_TYPE_VERB:
>> > >  case PIPE_ITEM_TYPE_MIGRATE_DATA:
>> > >  case PIPE_ITEM_TYPE_PIXMAP_SYNC:
>> > > --
>> > > 2.4.3
>> > >
>> > > ___
>> > > Spice-devel mailing list
>> > > Spice-devel@lists.freedesktop.org
>> > > htt

Re: [Spice-devel] [PATCH 03/10] Fix warning due to unexpected pipe item type

[Jonathon Jongsma]

On Mon, 2015-11-02 at 10:55 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > fzig...@redhat.com> wrote:
> > > From: Marc-André Lureau 
> > > 
> > > The specific item type that was not being handled was
> > > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the
> > > cursor
> > > channel, but the analogous item for the display channel is
> > > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> > > 
> > > The exact warning follows:
> > > 
> > >  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
> > >  ../../server/dcc-send.c:2442:dcc_send_item: should not be
> > > reached
> > >  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
> > >  ../../server/dcc.c:1595:release_item_before_push: invalid
> > > item type
> > > 
> > > Signed-off-by: Jonathon Jongsma 
> > > ---
> > >  server/cache_item.tmpl.c |  4 +++-
> > >  server/red_worker.c  | 15 ---
> > >  2 files changed, 3 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
> > > index dc314c0..ad2b579 100644
> > > --- a/server/cache_item.tmpl.c
> > > +++ b/server/cache_item.tmpl.c
> > > @@ -21,6 +21,7 @@
> > >  #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
> > >  #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
> > >  #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
> > >  #define FUNC_NAME(name) red_cursor_cache_##name
> > >  #define VAR_NAME(name) cursor_cache_##name
> > >  #define CHANNEL CursorChannel
> > > @@ -32,6 +33,7 @@
> > >  #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
> > >  #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
> > >  #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
> > > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
> > >  #define FUNC_NAME(name) red_palette_cache_##name
> > >  #define VAR_NAME(name) palette_cache_##name
> > >  #define CHANNEL DisplayChannel
> > > @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
> > > *channel_client, CacheItem *item)
> > >  channel_client->VAR_NAME(items)--;
> > >  channel_client->VAR_NAME(available) += item->size;
> > > 
> > > -red_channel_pipe_item_init(&channel->common.base, &item
> > > ->u.pipe_data,
> > > PIPE_ITEM_TYPE_INVAL_ONE);
> > > +red_channel_pipe_item_init(&channel->common.base, &item
> > > ->u.pipe_data,
> > > PIPE_ITEM_TYPE);
> > >  red_channel_client_pipe_add_tail(&channel_client
> > > ->common.base,
> > >  &item->u.pipe_data); // for now
> > >  }
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 89cb25c..93a305a 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -7805,17 +7805,6 @@ static inline void
> > > marshall_qxl_drawable(RedChannelClient *rcc,
> > >  red_lossy_marshall_qxl_drawable(display_channel
> > > ->common.worker,
> > >  rcc, m, dpi);
> > >  }
> > > 
> > > -static inline void red_marshall_inval(RedChannelClient *rcc,
> > > -  SpiceMarshaller
> > > *base_marshaller,
> > > CacheItem *cach_item)
> > > -{
> > > -SpiceMsgDisplayInvalOne inval_one;
> > > -
> > > -red_channel_client_init_send_data(rcc, cach_item
> > > ->inval_type, NULL);
> > > -inval_one.id = *(uint64_t *)&cach_item->id;
> > > -
> > > -spice_marshall_msg_cursor_inval_one(base_marshaller,
> > > &inval_one);
> > > -}
> > > -
> > >  static void
> > >  display_channel_marshall_migrate_data_surfaces(DisplayChannelCli
> > > ent *dcc,
> > >
> > >  SpiceMarshaller
> > > *m,
> > > int
> > > lossy)
> > > @@ -8271,9 +8260,6 @@ static void
> > > display_channel_send_item(RedChannelClient *rcc, PipeItem
> > > *pipe_item
> > >  marshall_qxl_drawable(rcc, m, dpi);
> > >  break;
> > >  }
> > > -case PIPE_ITEM_TYPE_INVAL_ONE:
> > > -red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> > > -break;
> > >  case PIPE_ITEM_TYPE_STREAM_CREATE: {
> > >  StreamAgent *agent = SPICE_CONTAINEROF(pipe_item,
> > > StreamAgent,
> > >  create_item);
> > >  red_display_marshall_stream_start(rcc, m, agent);
> > > @@ -9580,7 +9566,6 @@ static void
> > > display_channel_client_release_item_before_push(DisplayChannelCli
> > > ent
> > >  free(item);
> > >  break;
> > >  }
> > > -case PIPE_ITEM_TYPE_INVAL_ONE:
> > >  case PIPE_ITEM_TYPE_VERB:
> > >  case PIPE_ITEM_TYPE_MIGRATE_DATA:
> > >  case PIPE_ITEM_TYPE_PIXMAP_SYNC:
> > > --
> > > 2.4.3
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > ACK!
> > 
> 
> If I remember this patch is still on 

Re: [Spice-devel] Full featured (qxl compatible) spice web client released

[Daniel P. Berrange]

On Fri, Oct 30, 2015 at 10:05:23PM +0900, Daniel P. Berrange wrote:
> On Fri, Oct 30, 2015 at 01:24:36PM +0100, j...@eyeos.com wrote:
> >  
> > > I note the attribution clause in your license. As it stands now, I
> > > don't think that would cause any trouble (because of the 'however' of 5
> > > (d) of the agpl). But it would probably be useful to get a clear
> > > expression of your intent - is it the case that you are willing to
> > > contribute this only so long as the eyeos logo is prominently displayed
> > > by any one choosing to deploy it?
> > 
> > Our intentions are very simple. We want this to become the defacto html5
> > client for spice. Of course we want to maintain it and develop it
> > openly. Aside from our obvious personal preferences for FOSS the main
> > reason to opensource it is to develop it in community and of course, we
> > are very excited to discuss the next steps with you and anyone
> > interested in web support for spice :) 
> > 
> > And no, we are not willing to have an eyeos logo everywhere this client
> > is used. In fact there is no need for you to display an eyeos logo to
> > use this. 
> > 
> > We choosed AGPL because it protects the spice web client from the ASP
> > loophole. We don't want en eyeos logo or something like this displayed
> > every time this client is used, we just don't want for example a third
> > party company adding support for opus in a private product of its own
> > and not contributing it since this can be used from a service provider
> > (ASP loophole in gpl and similar licenses). 
> > 
> > However, we are open to discussions about the license if you feel this
> > can be a problem for reasonable scenarios.
> 
> Your rationale for using the AGPL as a core license makes sense to me.
> 
> Adding any additional terms to any license though, generally makes me
> concerned, as it is very easy to add seemingly innocuous text that in
> fact has non-obvious legal consequences. So I am a bit conerned about
> the additionl terms wrt attribution & logo display. IANAL though, so
> I will see if I can get any clarity / expert opinion on whether these
> terms are likely to cause any problems for acceptance in distros such
> as Fedora (or Debian) which are quite strict about interpretation of
> licensing.

Ok, so besides the standard AGPLv3 boilerplate text, there are two
paragraphs added to the license header at the top of the various .js
files in the repo

First is

[snip]
The interactive user interfaces in modified source and object code versions
of this program must display Appropriate Legal Notices, as required under
Section 5 of the GNU Affero General Public License version 3.
[/snip]

Since this is just re-stating section 5(d) of the LICENSE file it seems
pretty pointless, but at the same time, mostly harmless.

The second paragraph is the problematic one

[snip]
In accordance with Section 7(b) of the GNU Affero General Public License 
version 3,
these Appropriate Legal Notices must retain the display of the "Powered by
eyeos" logo and retain the original copyright notice. If the display of the 
logo is not reasonably feasible for technical reasons, the Appropriate Legal 
Notices
must display the words "Powered by eyeos" and retain the original copyright 
notice.
[/snip]

This is essentially adding a "badgeware" clause to the license.
Historically opinion has been divided as to whether "badgeware" /
"advertizing" clauses make a license non-free, or are merely an
undesirable attribution approach.

I was pointed at the SugarCRM community license which had an (somewhat
stricter) logo display requirement and there were strong opinions in
Debian that this made it non-free. Although your clause isn't as strict,
I think it is sufficiently similar that people would have the same
opinion about it being non-free.

  https://lists.debian.org/debian-legal/2005/11/msg00114.html

I was also referred to this FOSDEM presentation by a lawyer in the open
source space which puts across the case for all "badgeware" licenses
being considered non-free

  https://archive.fosdem.org/2014/schedule/event/trademark_licenses/

Since you say elsewhere in this thread, that it is not actually your
intention that everyone have to display a logo in their application,
I think it would be beneficial if you were able to re-consider the
terms used in the boilerplate text of the source files. To make the
licensing clear & simple to understand, IMHO, the easiest could be
to use the standard AGPL v3 boilerplate text "as-is" without any
custom additions.

Regards,
Daniel

[1] https://lists.debian.org/debian-legal/2005/11/msg00114.html
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-dev

Re: [Spice-devel] [PATCH 10/10] server: move bitmap related to red_bitmap_utils

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> ---
>  server/Makefile.am|   2 +
>  server/red_bitmap_utils.c |  99 +
>  server/red_bitmap_utils.h |  91 ++
>  server/red_common.h   |  13 
>  server/red_parse_qxl.c|   1 +
>  server/red_worker.c   | 158 
> --
>  server/tree.h |   9 +--
>  7 files changed, 206 insertions(+), 167 deletions(-)
>  create mode 100644 server/red_bitmap_utils.c
>  create mode 100644 server/red_bitmap_utils.h
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index d2a7343..5d28e9e 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -133,6 +133,8 @@ libspice_server_la_SOURCES =\
> pixmap-cache.c  \
> tree.h  \
> tree.c  \
> +   red_bitmap_utils.h  \
> +   red_bitmap_utils.c  \
> utils.h \
> $(NULL)
>
> diff --git a/server/red_bitmap_utils.c b/server/red_bitmap_utils.c
> new file mode 100644
> index 000..d293dae
> --- /dev/null
> +++ b/server/red_bitmap_utils.c
> @@ -0,0 +1,99 @@
> +#include "red_bitmap_utils.h"
> +
> +#define RED_BITMAP_UTILS_RGB16
> +#include "red_bitmap_utils_tmpl.c"
> +#define RED_BITMAP_UTILS_RGB24
> +#include "red_bitmap_utils_tmpl.c"
> +#define RED_BITMAP_UTILS_RGB32
> +#include "red_bitmap_utils_tmpl.c"
> +
> +#define GRADUAL_HIGH_RGB24_TH -0.03
> +#define GRADUAL_HIGH_RGB16_TH 0
> +
> +// setting a more permissive threshold for stream identification in order
> +// not to miss streams that were artificially scaled on the guest (e.g., 
> full screen view
> +// in window media player 12). see red_stream_add_frame
> +#define GRADUAL_MEDIUM_SCORE_TH 0.002
> +
> +// assumes that stride doesn't overflow
> +BitmapGradualType bitmap_get_graduality_level(SpiceBitmap *bitmap)
> +{
> +double score = 0.0;
> +int num_samples = 0;
> +int num_lines;
> +double chunk_score = 0.0;
> +int chunk_num_samples = 0;
> +uint32_t x, i;
> +SpiceChunk *chunk;
> +
> +chunk = bitmap->data->chunk;
> +for (i = 0; i < bitmap->data->num_chunks; i++) {
> +num_lines = chunk[i].len / bitmap->stride;
> +x = bitmap->x;
> +switch (bitmap->format) {
> +case SPICE_BITMAP_FMT_16BIT:
> +compute_lines_gradual_score_rgb16((rgb16_pixel_t 
> *)chunk[i].data, x, num_lines,
> +  &chunk_score, 
> &chunk_num_samples);
> +break;
> +case SPICE_BITMAP_FMT_24BIT:
> +compute_lines_gradual_score_rgb24((rgb24_pixel_t 
> *)chunk[i].data, x, num_lines,
> +  &chunk_score, 
> &chunk_num_samples);
> +break;
> +case SPICE_BITMAP_FMT_32BIT:
> +case SPICE_BITMAP_FMT_RGBA:
> +compute_lines_gradual_score_rgb32((rgb32_pixel_t 
> *)chunk[i].data, x, num_lines,
> +  &chunk_score, 
> &chunk_num_samples);
> +break;
> +default:
> +spice_error("invalid bitmap format (not RGB) %u", 
> bitmap->format);
> +}
> +score += chunk_score;
> +num_samples += chunk_num_samples;
> +}
> +
> +spice_assert(num_samples);
> +score /= num_samples;
> +
> +if (bitmap->format == SPICE_BITMAP_FMT_16BIT) {
> +if (score < GRADUAL_HIGH_RGB16_TH) {
> +return BITMAP_GRADUAL_HIGH;
> +}
> +} else {
> +if (score < GRADUAL_HIGH_RGB24_TH) {
> +return BITMAP_GRADUAL_HIGH;
> +}
> +}
> +
> +if (score < GRADUAL_MEDIUM_SCORE_TH) {
> +return BITMAP_GRADUAL_MEDIUM;
> +} else {
> +return BITMAP_GRADUAL_LOW;
> +}
> +}
> +
> +int bitmap_has_extra_stride(SpiceBitmap *bitmap)
> +{
> +spice_assert(bitmap);
> +if (bitmap_fmt_is_rgb(bitmap->format)) {
> +return ((bitmap->x * bitmap_fmt_get_bytes_per_pixel(bitmap->format)) 
> < bitmap->stride);
> +} else {
> +switch (bitmap->format) {
> +case SPICE_BITMAP_FMT_8BIT:
> +return (bitmap->x < bitmap->stride);
> +case SPICE_BITMAP_FMT_4BIT_BE:
> +case SPICE_BITMAP_FMT_4BIT_LE: {
> +int bytes_width = SPICE_ALIGN(bitmap->x, 2) >> 1;
> +return bytes_width < bitmap->stride;
> +}
> +case SPICE_BITMAP_FMT_1BIT_BE:
> +case SPICE_BITMAP_FMT_1BIT_LE: {
> +int bytes_width = SPICE_ALIGN(bitmap->x, 8) >> 3;
> +return bytes_width < bitmap->stride;
> +}
> +default:
> +spice_error("invalid image type %u", bitmap->format);
> +return 0;
> +}
> +}
> +return 0;
> +}
> diff --git a/server/red_bitmap_utils.h 

Re: [Spice-devel] [PATCH spice ] Add support for clients connecting with the WebSocket protocol.

[Pavel Grunt]

Hi Jeremy,

I really like how it works, that you can use the same port value for html5 and
gtk clients.
Not everything was clear to me, so I put some comments inline.

On Fri, 2015-10-30 at 15:52 -0500, Jeremy White wrote:
> We do this by auto detecting the inbound http(s) 'GET' and probing
> for a well formulated WebSocket binary connection, such as used
> by the spice-html5 client.  If detected, we implement a set of
> cover functions that abstract the read/write/writev functions,
> in a fashion similar to the SASL implemented.
> 
> This includes a limited implementation of the WebSocket protocol,
> sufficient for our purposes.
> 
> Signed-off-by: Jeremy White 
> ---
>  server/Makefile.am   |   2 +
>  server/reds.c|   6 +
>  server/reds_stream.c |  98 
>  server/reds_stream.h |   2 +
>  server/websocket.c   | 432
> +++
>  server/websocket.h   |  62 
>  6 files changed, 602 insertions(+)
>  create mode 100644 server/websocket.c
>  create mode 100644 server/websocket.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 87288cc..e2f5452 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -113,6 +113,8 @@ libspice_server_la_SOURCES =  \
>   reds-private.h  \
>   reds_stream.c   \
>   reds_stream.h   \
> + websocket.c \
> + websocket.h \
>   reds_sw_canvas.c\
>   reds_sw_canvas.h\
>   snd_worker.c\
> diff --git a/server/reds.c b/server/reds.c
> index 1f6774e..701ac9d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2172,6 +2172,7 @@ static void reds_handle_link_error(void *opaque, int
> err)
>  reds_link_free(link);
>  }
>  
> +static void reds_handle_new_link(RedLinkInfo *link);
>  static void reds_handle_read_header_done(void *opaque)
>  {
>  RedLinkInfo *link = (RedLinkInfo *)opaque;
> @@ -2182,6 +2183,11 @@ static void reds_handle_read_header_done(void *opaque)
>  header->size = GUINT32_FROM_LE(header->size);
>  
>  if (header->magic != SPICE_MAGIC) {
> +if (reds_stream_is_websocket(link->stream,
> +(guchar *) header, sizeof(*header))) {
> +reds_handle_new_link(link);
> +return;
> +}
This looks hacky, it would be nice to detect that we are dealing with websockets
before reds_handle_read_header_done() is called.

>  reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
>  reds_link_free(link);
>  return;
> diff --git a/server/reds_stream.c b/server/reds_stream.c
> index 3b47391..9f813cf 100644
> --- a/server/reds_stream.c
> +++ b/server/reds_stream.c
> @@ -30,6 +30,7 @@
>  #include 
>  
>  #include 
> +#include "websocket.h"
>  
>  #include 
>  
> @@ -76,6 +77,17 @@ typedef struct RedsSASL {
>  } RedsSASL;
>  #endif
>  
> +typedef struct {
> +int closed;
> +
> +websocket_frame_t read_frame;
> +guint64 write_remainder;
> +
> +ssize_t (*raw_read)(RedsStream *s, void *buf, size_t nbyte);
> +ssize_t (*raw_write)(RedsStream *s, const void *buf, size_t nbyte);
> +ssize_t (*raw_writev)(RedsStream *s, const struct iovec *iov, int
> iovcnt);
> +} RedsWebSocket;
> +
>  struct RedsStreamPrivate {
>  SSL *ssl;
>  
> @@ -85,6 +97,8 @@ struct RedsStreamPrivate {
>  
>  AsyncRead async_read;
>  
> +RedsWebSocket *ws;
> +
>  /* life time of info:
>   * allocated when creating RedsStream.
>   * deallocated when main_dispatcher handles the
> SPICE_CHANNEL_EVENT_DISCONNECTED
> @@ -1068,3 +1082,87 @@ error:
>  return FALSE;
>  }
>  #endif
> +
> +static ssize_t stream_websocket_read(RedsStream *s, void *buf, size_t size)
> +{
> +int rc;
> +
> +if (s->priv->ws->closed)
> +return 0;
> +
> +rc = websocket_read((void *)s, buf, size, &s->priv->ws->read_frame,
> +(websocket_write_cb_t) s->priv->ws->raw_read,
> +(websocket_write_cb_t) s->priv->ws->raw_write);
> +
> +if (rc == 0)
> +s->priv->ws->closed = 1;
> +
> +return rc;
> +}
> +
> +static ssize_t stream_websocket_write(RedsStream *s, const void *buf, size_t
> size)
> +{
> +if (s->priv->ws->closed) {
> +errno = EPIPE;
> +return -1;
> +}
> +return websocket_write((void *)s, buf, size, &s->priv->ws-
> >write_remainder,
> +(websocket_write_cb_t) s->priv->ws->raw_write);
> +}
> +
> +static ssize_t stream_websocket_writev(RedsStream *s, const struct iovec
> *iov, int iovcnt)
> +{
> +if (s->priv->ws->closed) {
> +errno = EPIPE;
> +return -1;
> +}
> +return websocket_writev((void *)s, (struct iovec *) iov, iovcnt, &s-
> >priv->ws->write_remainder,
> +(websocket_writev_cb_t) s->priv->ws->raw_writev);
> +}
> +
> +/*
> +If we detect that a newly opened strea

Re: [Spice-devel] [PATCH 08/10] server: make more of cursor private

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> ---
>  server/cursor-channel.c | 79 
> +++--
>  server/cursor-channel.h | 51 +++
>  server/red_channel.h|  2 ++
>  server/red_worker.c | 22 +-
>  server/red_worker.h |  1 +
>  5 files changed, 90 insertions(+), 65 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 3ae7756..5995aa5 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -19,6 +19,41 @@
>  #include "common/generated_server_marshallers.h"
>  #include "cursor-channel.h"
>
> +#define CLIENT_CURSOR_CACHE_SIZE 256
> +
> +#define CURSOR_CACHE_HASH_SHIFT 8
> +#define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
> +#define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
> +#define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> +
> +enum {
> +PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
> +PIPE_ITEM_TYPE_CURSOR_INIT,
> +PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> +};
> +
> +typedef struct CursorItem {
> +QXLInstance *qxl;
> +uint32_t group_id;
> +int refs;
> +RedCursorCmd *red_cursor;
> +} CursorItem;
> +
> +G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
> +
> +typedef struct LocalCursor {
> +CursorItem base;
> +SpicePoint16 position;
> +uint32_t data_size;
> +SpiceCursor red_cursor;
> +} LocalCursor;
> +
> +typedef struct CursorPipeItem {
> +PipeItem base;
> +CursorItem *cursor_item;
> +int refs;
> +} CursorPipeItem;
> +
>  struct CursorChannel {
>  CommonChannel common; // Must be the first thing
>
> @@ -34,13 +69,23 @@ struct CursorChannel {
>  #endif
>  };
>
> +struct CursorChannelClient {
> +CommonChannelClient common;
> +
> +CacheItem *cursor_cache[CURSOR_CACHE_HASH_SIZE];
> +Ring cursor_cache_lru;
> +long cursor_cache_available;
> +uint32_t cursor_cache_items;
> +};
> +
> +
>  #define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, 
> common.base)
>
>  #define CLIENT_CURSOR_CACHE
>  #include "cache_item.tmpl.c"
>  #undef CLIENT_CURSOR_CACHE
>
> -CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t 
> group_id)
> +static CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, 
> uint32_t group_id)
>  {
>  CursorItem *cursor_item;
>
> @@ -55,7 +100,7 @@ CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd 
> *cmd, uint32_t group_
>  return cursor_item;
>  }
>
> -CursorItem *cursor_item_ref(CursorItem *item)
> +static CursorItem *cursor_item_ref(CursorItem *item)
>  {
>  spice_return_val_if_fail(item != NULL, NULL);
>  spice_return_val_if_fail(item->refs > 0, NULL);
> @@ -65,7 +110,7 @@ CursorItem *cursor_item_ref(CursorItem *item)
>  return item;
>  }
>
> -void cursor_item_unref(CursorItem *item)
> +static void cursor_item_unref(CursorItem *item)
>  {
>  QXLReleaseInfoExt release_info_ext;
>  RedCursorCmd *cursor_cmd;
> @@ -398,6 +443,17 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
>  return cursor_channel;
>  }
>
> +void cursor_channel_client_migrate(CursorChannelClient* client)
> +{
> +RedChannelClient *rcc;
> +
> +spice_return_if_fail(client);
> +rcc = RED_CHANNEL_CLIENT(client);
> +
> +red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> +red_channel_client_default_migrate(rcc);
> +}
> +
>  CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, 
> RedClient *client, RedsStream *stream,
> int mig_target,
> uint32_t *common_caps, int 
> num_common_caps,
> @@ -496,6 +552,23 @@ void cursor_channel_reset(CursorChannel *cursor)
>  }
>  }
>
> +void cursor_channel_init(CursorChannel *cursor, CursorChannelClient *client)
> +{
> +spice_return_if_fail(cursor);
> +
> +if (red_channel_is_connected(&cursor->common.base)
> +|| COMMON_CHANNEL(cursor)->during_target_migrate) {
> +spice_debug("during_target_migrate: skip init");
> +return;
> +}
> +
> +if (client)
> +red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(client),
> + PIPE_ITEM_TYPE_CURSOR_INIT);
> +else
> +red_channel_pipes_add_type(RED_CHANNEL(cursor), 
> PIPE_ITEM_TYPE_CURSOR_INIT);
> +}
> +
>  void cursor_channel_set_mouse_mode(CursorChannel *cursor, uint32_t mode)
>  {
>  spice_return_if_fail(cursor);
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index d5e5b13..887f847 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -25,55 +25,15 @@
>  #include "cache-item.h"
>  #include "stat.h"
>
> -#define CLIENT_CURSOR_CACHE_SIZE 256
> -
> -#define CURSOR_CACHE_HASH_SHIFT 8
> -#define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
> -#define

Re: [Spice-devel] [PATCH 07/10] server: make cursor channel private

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> ---
>  server/cursor-channel.c | 24 +++-
>  server/cursor-channel.h | 18 ++---
>  server/red_channel.c| 12 ++
>  server/red_channel.h|  6 +++
>  server/red_worker.c | 98 
> ++---
>  server/red_worker.h |  5 +++
>  6 files changed, 92 insertions(+), 71 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 6a1fcea..3ae7756 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -19,6 +19,21 @@
>  #include "common/generated_server_marshallers.h"
>  #include "cursor-channel.h"
>
> +struct CursorChannel {
> +CommonChannel common; // Must be the first thing
> +
> +CursorItem *item;
> +int cursor_visible;
> +SpicePoint16 cursor_position;
> +uint16_t cursor_trail_length;
> +uint16_t cursor_trail_frequency;
> +uint32_t mouse_mode;
> +
> +#ifdef RED_STATISTICS
> +StatNodeRef stat;
> +#endif
> +};
> +
>  #define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, 
> common.base)
>
>  #define CLIENT_CURSOR_CACHE
> @@ -372,7 +387,7 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
>  };
>
>  spice_info("create cursor channel");
> -channel = red_worker_new_channel(worker, sizeof(CursorChannel),
> +channel = red_worker_new_channel(worker, sizeof(CursorChannel), 
> "cursor_channel",
>   SPICE_CHANNEL_CURSOR, 0,
>   &cbs, 
> red_channel_client_handle_message);
>
> @@ -480,3 +495,10 @@ void cursor_channel_reset(CursorChannel *cursor)
>  }
>  }
>  }
> +
> +void cursor_channel_set_mouse_mode(CursorChannel *cursor, uint32_t mode)
> +{
> +spice_return_if_fail(cursor);
> +
> +cursor->mouse_mode = mode;
> +}
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 1639cf9..d5e5b13 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -38,6 +38,8 @@ enum {
>  PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
>  };
>
> +typedef struct CursorChannel CursorChannel;
> +
>  typedef struct CursorItem {
>  QXLInstance *qxl;
>  uint32_t group_id;
> @@ -67,21 +69,6 @@ typedef struct CursorChannelClient {
>  uint32_t cursor_cache_items;
>  } CursorChannelClient;
>
> -typedef struct CursorChannel {
> -CommonChannel common; // Must be the first thing
> -
> -CursorItem *item;
> -int cursor_visible;
> -SpicePoint16 cursor_position;
> -uint16_t cursor_trail_length;
> -uint16_t cursor_trail_frequency;
> -uint32_t mouse_mode;
> -
> -#ifdef RED_STATISTICS
> -StatNodeRef stat;
> -#endif
> -} CursorChannel;
> -
>  G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
>
>  CursorChannel*   cursor_channel_new (RedWorker *worker);
> @@ -89,6 +76,7 @@ void cursor_channel_disconnect  
> (CursorChannel *cursor_channel);
>  void cursor_channel_reset   (CursorChannel *cursor);
>  void cursor_channel_process_cmd (CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
>   uint32_t group_id);
> +void cursor_channel_set_mouse_mode(CursorChannel *cursor, 
> uint32_t mode);
>
>  CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> RedClient *client, RedsStream 
> *stream,
> diff --git a/server/red_channel.c b/server/red_channel.c
> index 34aa9dc..7330ae2 100644
> --- a/server/red_channel.c
> +++ b/server/red_channel.c
> @@ -1151,9 +1151,21 @@ RedChannel *red_channel_create_parser(int size,
>  }
>  channel->incoming_cb.handle_parsed = (handle_parsed_proc)handle_parsed;
>  channel->incoming_cb.parser = parser;
> +
>  return channel;
>  }
>
> +void red_channel_set_stat_node(RedChannel *channel, StatNodeRef stat)
> +{
> +spice_return_if_fail(channel != NULL);
> +spice_return_if_fail(channel->stat == 0);
> +
> +#ifdef RED_STATISTICS
> +channel->stat = stat;
> +channel->out_bytes_counter = stat_add_counter(stat, "out_bytes", TRUE);
> +#endif
> +}
> +
>  void red_channel_register_client_cbs(RedChannel *channel, ClientCbs 
> *client_cbs)
>  {
>  spice_assert(client_cbs->connect || channel->type == SPICE_CHANNEL_MAIN);
> diff --git a/server/red_channel.h b/server/red_channel.h
> index 1f1538e..201a4d2 100644
> --- a/server/red_channel.h
> +++ b/server/red_channel.h
> @@ -32,6 +32,7 @@
>  #include "red_common.h"
>  #include "demarshallers.h"
>  #include "reds_stream.h"
> +#include "stat.h"
>
>  #define MAX_SEND_BUFS 1000
>  #define CLIENT_ACK_WINDOW 20
> @@ -127,6 +128,7 @@ typedef struct OutgoingHandler {
>
>  /* Red Channel interface */
>
> +typedef struct RedsStream RedsStream;
>  typedef struct RedChannel RedChannel;
>  typedef struct RedChannelClient RedChannelClient;
>  typedef struct RedClient

Re: [Spice-devel] [PATCH 06/10] server/red_worker: red_draw_qxl_drawable: protect from NULL dereference in case of buggy driver (or recording)

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Alon Levy 
>
> ---
>  server/red_worker.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 339b353..868de94 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -3838,6 +3838,11 @@ static void red_draw_qxl_drawable(RedWorker *worker, 
> Drawable *drawable)
>
>  image_cache_aging(&worker->image_cache);
>
> +if (!canvas) {
> +spice_warning("ignoring drawable to destroyed surface %d\n", 
> drawable->surface_id);
> +return;
> +}
> +
>  region_add(&surface->draw_dirty_region, &drawable->red_drawable->bbox);
>
>  switch (drawable->red_drawable->type) {
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

ACK from me, but I am not sure if we had an agreement about these one last week.
AFAIR you said it is a situation that never could happen and that we
could drop this patch.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH xf86-video-qxl ] Set the regular spice port only once, and then only if it is not diabled.

[Jeremy White]

On 11/01/2015 09:55 AM, Uri Lublin wrote:
> On 10/30/2015 05:45 PM, Jeremy White wrote:
>> This fixes a bug where Xspice had to listen on two ports, even in an
>> SSL only configuration.
>>
>> Signed-off-by: Jeremy White 
> 
> Ack.

Thanks.  Pushed, with typo s/diabled/disabled/.

Cheers,

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

Re: [Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 5:00 PM, Frediano Ziglio  wrote:
>>
>> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio  wrote:
>> > From: Marc-André Lureau 
>> >
>> > Doing so allows us to remove the extra QXLInstance parameter from
>> > cursor_item_unref() and makes the code a bit cleaner.
>> >
>> > Also add cursor_item_ref().
>> >
>> > Signed-off-by: Jonathon Jongsma 
>> > ---
>> >  server/cursor-channel.c | 70
>> >  +++--
>> >  server/cursor-channel.h |  9 ---
>> >  2 files changed, 44 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
>> > index eef0121..d145f86 100644
>> > --- a/server/cursor-channel.c
>> > +++ b/server/cursor-channel.c
>> > @@ -25,55 +25,58 @@
>> >  #include "cache_item.tmpl.c"
>> >  #undef CLIENT_CURSOR_CACHE
>> >
>> > -static inline CursorItem *alloc_cursor_item(void)
>> > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t
>> > group_id)
>> >  {
>> >  CursorItem *cursor_item;
>> >
>> > +spice_return_val_if_fail(cmd != NULL, NULL);
>> > +
>> >  cursor_item = g_slice_new0(CursorItem);
>> > +cursor_item->qxl = qxl;
>> >  cursor_item->refs = 1;
>> > +cursor_item->group_id = group_id;
>> > +cursor_item->red_cursor = cmd;
>> >
>> >  return cursor_item;
>> >  }
>> >
>> > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
>> > +CursorItem *cursor_item_ref(CursorItem *item)
>> >  {
>> > -CursorItem *cursor_item;
>> > -
>> > -spice_return_val_if_fail(cmd != NULL, NULL);
>> > -cursor_item = alloc_cursor_item();
>> > +spice_return_val_if_fail(item != NULL, NULL);
>> > +spice_return_val_if_fail(item->refs > 0, NULL);
>> >
>> > -cursor_item->group_id = group_id;
>> > -cursor_item->red_cursor = cmd;
>> > +item->refs++;
>> >
>> > -return cursor_item;
>> > +return item;
>> >  }
>> >
>> > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
>> > +void cursor_item_unref(CursorItem *item)
>> >  {
>> > -if (!--cursor->refs) {
>> > -QXLReleaseInfoExt release_info_ext;
>> > -RedCursorCmd *cursor_cmd;
>> > -
>> > -cursor_cmd = cursor->red_cursor;
>> > -release_info_ext.group_id = cursor->group_id;
>> > -release_info_ext.info = cursor_cmd->release_info;
>> > -qxl->st->qif->release_resource(qxl, release_info_ext);
>> > -red_put_cursor_cmd(cursor_cmd);
>> > -free(cursor_cmd);
>> > -
>> > -g_slice_free(CursorItem, cursor);
>> > -}
>> > +QXLReleaseInfoExt release_info_ext;
>> > +RedCursorCmd *cursor_cmd;
>> > +
>> > +spice_return_if_fail(item != NULL);
>> > +
>> > +if (--item->refs)
>>
>> Just a nitpick, I would prefer to have a explicit comparison here: if
>> (--items->refs > 0) ...
>>
>
> Why not
>
>   if (--items->refs != 0) ??
>
> I mean, at the end the results should be the same if no errors managing
> the counters are present.

I just think the check for > 0 is the proper test we want to do,
mainly considering that refs is a int (and not a uint)

> Are you suggesting to change the test to avoid
> multiple free (hoping memory is still untouched after the first one) ?

I really didn't think about this.

>
> I start suspecting that sending the patch when there are pending issue to
> be discussed is not really a good idea.
>
>> > +return;
>> > +
>> > +cursor_cmd = item->red_cursor;
>> > +release_info_ext.group_id = item->group_id;
>> > +release_info_ext.info = cursor_cmd->release_info;
>> > +item->qxl->st->qif->release_resource(item->qxl, release_info_ext);
>> > +red_put_cursor_cmd(cursor_cmd);
>> > +free(cursor_cmd);
>> > +
>> > +g_slice_free(CursorItem, item);
>> > +
>> >  }
>> >
>> >  static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
>> >  {
>> >  if (cursor->item)
>> > -cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
>> > cursor->item);
>> > -
>> > -if (item)
>> > -item->refs++;
>> > +cursor_item_unref(cursor->item);
>> >
>> > -cursor->item = item;
>> > +cursor->item = item ? cursor_item_ref(item) : NULL;
>> >  }
>> >
>> >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data,
>> >  int num)
>> > @@ -157,7 +160,7 @@ static void put_cursor_pipe_item(CursorChannelClient
>> > *ccc, CursorPipeItem *pipe_
>> >
>> >  spice_assert(!pipe_item_is_linked(&pipe_item->base));
>> >
>> > -cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
>> > pipe_item->cursor_item);
>> > +cursor_item_unref(pipe_item->cursor_item);
>> >  free(pipe_item);
>> >  }
>> >
>> > @@ -404,7 +407,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
>> > RedCursorCmd *cursor_cmd,
>> >  CursorItem *cursor_item;
>> >  int cursor_show = FALSE;
>> >
>> > -cursor_item = cursor_item_new(cursor_cmd, group_id);
>> > +spice_return_if_fail(cursor);
>> > +spice_return_if_fail(

Re: [Spice-devel] [PATCH 04/10] Move pipe item enumerations out of red_worker.h

[Frediano Ziglio]

> 
> On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> > From: Marc-André Lureau 
> >
> > Move the cursor-specific pipe item types to cursor-channel.h, and the
> > display-specific types to red_worker.c. Only leave the common
> > definitions in red_worker.h. This prepares for splitting the display
> > channel into a separate file.
> >
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.h |  6 ++
> >  server/red_worker.c | 17 +
> >  server/red_worker.h | 21 +++--
> >  3 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > index f20001c..1639cf9 100644
> > --- a/server/cursor-channel.h
> > +++ b/server/cursor-channel.h
> > @@ -32,6 +32,12 @@
> >  #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
> >  #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> >
> > +enum {
> > +PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
> > +PIPE_ITEM_TYPE_CURSOR_INIT,
> > +PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> > +};
> > +
> >  typedef struct CursorItem {
> >  QXLInstance *qxl;
> >  uint32_t group_id;
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 93a305a..c3b5c36 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -228,6 +228,23 @@ struct SpiceWatch {
> >  void *watch_func_opaque;
> >  };
> >
> > +enum {
> > +PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST,
> > +PIPE_ITEM_TYPE_IMAGE,
> > +PIPE_ITEM_TYPE_STREAM_CREATE,
> > +PIPE_ITEM_TYPE_STREAM_CLIP,
> > +PIPE_ITEM_TYPE_STREAM_DESTROY,
> > +PIPE_ITEM_TYPE_UPGRADE,
> > +PIPE_ITEM_TYPE_MIGRATE_DATA,
> > +PIPE_ITEM_TYPE_PIXMAP_SYNC,
> > +PIPE_ITEM_TYPE_PIXMAP_RESET,
> > +PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> > +PIPE_ITEM_TYPE_CREATE_SURFACE,
> > +PIPE_ITEM_TYPE_DESTROY_SURFACE,
> > +PIPE_ITEM_TYPE_MONITORS_CONFIG,
> > +PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> > +};
> > +
> >  #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS
> >
> >  typedef struct SurfaceCreateItem {
> > diff --git a/server/red_worker.h b/server/red_worker.h
> > index 76502b6..aa97707 100644
> > --- a/server/red_worker.h
> > +++ b/server/red_worker.h
> > @@ -48,25 +48,10 @@ typedef struct CommonChannel {
> >  } CommonChannel;
> >
> >  enum {
> > -PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE,
> > +PIPE_ITEM_TYPE_VERB = PIPE_ITEM_TYPE_CHANNEL_BASE,
> >  PIPE_ITEM_TYPE_INVAL_ONE,
> > -PIPE_ITEM_TYPE_CURSOR,
> > -PIPE_ITEM_TYPE_CURSOR_INIT,
> > -PIPE_ITEM_TYPE_IMAGE,
> > -PIPE_ITEM_TYPE_STREAM_CREATE,
> > -PIPE_ITEM_TYPE_STREAM_CLIP,
> > -PIPE_ITEM_TYPE_STREAM_DESTROY,
> > -PIPE_ITEM_TYPE_UPGRADE,
> > -PIPE_ITEM_TYPE_VERB,
> > -PIPE_ITEM_TYPE_MIGRATE_DATA,
> > -PIPE_ITEM_TYPE_PIXMAP_SYNC,
> > -PIPE_ITEM_TYPE_PIXMAP_RESET,
> > -PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> > -PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> > -PIPE_ITEM_TYPE_CREATE_SURFACE,
> > -PIPE_ITEM_TYPE_DESTROY_SURFACE,
> > -PIPE_ITEM_TYPE_MONITORS_CONFIG,
> > -PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> > +
> > +PIPE_ITEM_TYPE_COMMON_LAST
> >  };
> >
> >  typedef struct VerbItem {
> > --
> > 2.4.3
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ACK!
> 

Merged

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

Re: [Spice-devel] [PATCH 00/10] Backported some patches from refactory branches (2nd Nov)

[Frediano Ziglio]

> > 
> > > 
> > > This patchset supersed last patchset.
> > > 
> > > Changes:
> > > - rebased on upstream master;
> > > - removed merged patches;
> > > - added patches from Jonathon cursor split ones;
> > > - added some patches to the set.
> > > 
> > > I think if we won't receive any comment on Alon Levy's patch
> > > in a week or so I'll remove entirely.
> > > 
> > > The "server: move display_channel_client_create() to dcc_new()"
> > > is the merge of "server: move display_channel_client_new()" and
> > > "s/display_channel_client_new/dcc_new" as discussed.
> > > 
> > > Alon Levy (1):
> > >   server/red_worker: red_draw_qxl_drawable: protect from NULL
> > > dereference in case of buggy driver (or recording)
> > > 
> > > Marc-André Lureau (9):
> > >   server: move display_channel_client_create() to dcc_new()
> > >   Store QXLInstance in CursorItem
> > >   Fix warning due to unexpected pipe item type
> > >   Move pipe item enumerations out of red_worker.h
> > >   Change some asserts to warnings
> > >   server: make cursor channel private
> > >   server: make more of cursor private
> > >   tree: move that to a seperate unit
> > >   server: move bitmap related to red_bitmap_utils
> > > 
> > 
> > Merged "tree: move that to a seperate unit"
> > 
> 
> Merged "server: move display_channel_client_create() to dcc_new()"
> 

Merged "Move pipe item enumerations out of red_worker.h"

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

Re: [Spice-devel] [PATCH 05/10] Change some asserts to warnings

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> Various changes in RedWorker and CursorChannel related to error and
> warning messages.
>
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 28 
>  server/red_worker.c | 22 ++
>  server/red_worker.h |  1 -
>  3 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index d145f86..6a1fcea 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -152,7 +152,8 @@ void cursor_channel_disconnect(CursorChannel 
> *cursor_channel)
>
>  static void put_cursor_pipe_item(CursorChannelClient *ccc, CursorPipeItem 
> *pipe_item)
>  {
> -spice_assert(pipe_item);
> +spice_return_if_fail(pipe_item);
> +spice_return_if_fail(pipe_item->refs > 0);
>
>  if (--pipe_item->refs) {
>  return;
> @@ -239,7 +240,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  PipeItem *pipe_item = &cursor_pipe_item->base;
>  RedCursorCmd *cmd;
>
> -spice_assert(cursor_channel);
> +spice_return_if_fail(cursor_channel);
>
>  cmd = item->red_cursor;
>  switch (cmd->type) {
> @@ -327,7 +328,9 @@ static void cursor_channel_send_item(RedChannelClient 
> *rcc, PipeItem *pipe_item)
>
>  static CursorPipeItem *cursor_pipe_item_ref(CursorPipeItem *item)
>  {
> -spice_assert(item);
> +spice_return_val_if_fail(item, NULL);
> +spice_return_val_if_fail(item->refs > 0, NULL);
> +
>  item->refs++;
>  return item;
>  }
> @@ -336,7 +339,9 @@ static CursorPipeItem 
> *cursor_pipe_item_ref(CursorPipeItem *item)
>  static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem 
> *item)
>  {
>  CursorPipeItem *cursor_pipe_item;
> -spice_assert(item);
> +
> +spice_return_if_fail(item);
> +
>  cursor_pipe_item = SPICE_CONTAINEROF(item, CursorPipeItem, base);
>  cursor_pipe_item_ref(cursor_pipe_item);
>  }
> @@ -383,6 +388,12 @@ CursorChannelClient* 
> cursor_channel_client_new(CursorChannel *cursor, RedClient
> uint32_t *common_caps, int 
> num_common_caps,
> uint32_t *caps, int num_caps)
>  {
> +spice_return_val_if_fail(cursor, NULL);
> +spice_return_val_if_fail(client, NULL);
> +spice_return_val_if_fail(stream, NULL);
> +spice_return_val_if_fail(!num_common_caps || common_caps, NULL);
> +spice_return_val_if_fail(!num_caps || caps, NULL);
> +
>  CursorChannelClient *ccc =
>  (CursorChannelClient*)common_channel_new_client(&cursor->common,
>  
> sizeof(CursorChannelClient),
> @@ -393,11 +404,11 @@ CursorChannelClient* 
> cursor_channel_client_new(CursorChannel *cursor, RedClient
>  num_common_caps,
>  caps,
>  num_caps);
> -if (!ccc) {
> -return NULL;
> -}
> +spice_return_val_if_fail(ccc != NULL, NULL);
> +
>  ring_init(&ccc->cursor_cache_lru);
>  ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
> +
>  return ccc;
>  }
>
> @@ -431,7 +442,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
>  cursor->cursor_trail_frequency = cursor_cmd->u.trail.frequency;
>  break;
>  default:
> -spice_error("invalid cursor command %u", cursor_cmd->type);
> +spice_warning("invalid cursor command %u", cursor_cmd->type);
> +return;
>  }

I would go for spice_warn_if_reached() here.

>
>  if (red_channel_is_connected(&cursor->common.base) &&
> diff --git a/server/red_worker.c b/server/red_worker.c
> index c3b5c36..339b353 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -4169,7 +4169,7 @@ static int red_process_cursor(RedWorker *worker, 
> uint32_t max_pipe_size, int *ri
>  break;
>  }
>  default:
> -spice_error("bad command type");
> +spice_warning("bad command type");
>  }

Same here.

>  n++;
>  }
> @@ -9769,19 +9769,15 @@ static void red_connect_cursor(RedWorker *worker, 
> RedClient *client, RedsStream
>  CursorChannel *channel;
>  CursorChannelClient *ccc;
>
> -if (worker->cursor_channel == NULL) {
> -spice_warning("cursor channel was not created");
> -return;
> -}
> +spice_return_if_fail(worker->cursor_channel != NULL);
> +
>  channel = worker->cursor_channel;
>  spice_info("add cursor channel client");
>  ccc = cursor_channel_client_new(channel, client, stream,
>  migrate,
>  common_caps, num_common_caps,
>  caps, num_caps)

Re: [Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

[Frediano Ziglio]

> 
> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio  wrote:
> > From: Marc-André Lureau 
> >
> > Doing so allows us to remove the extra QXLInstance parameter from
> > cursor_item_unref() and makes the code a bit cleaner.
> >
> > Also add cursor_item_ref().
> >
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.c | 70
> >  +++--
> >  server/cursor-channel.h |  9 ---
> >  2 files changed, 44 insertions(+), 35 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index eef0121..d145f86 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -25,55 +25,58 @@
> >  #include "cache_item.tmpl.c"
> >  #undef CLIENT_CURSOR_CACHE
> >
> > -static inline CursorItem *alloc_cursor_item(void)
> > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t
> > group_id)
> >  {
> >  CursorItem *cursor_item;
> >
> > +spice_return_val_if_fail(cmd != NULL, NULL);
> > +
> >  cursor_item = g_slice_new0(CursorItem);
> > +cursor_item->qxl = qxl;
> >  cursor_item->refs = 1;
> > +cursor_item->group_id = group_id;
> > +cursor_item->red_cursor = cmd;
> >
> >  return cursor_item;
> >  }
> >
> > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> > +CursorItem *cursor_item_ref(CursorItem *item)
> >  {
> > -CursorItem *cursor_item;
> > -
> > -spice_return_val_if_fail(cmd != NULL, NULL);
> > -cursor_item = alloc_cursor_item();
> > +spice_return_val_if_fail(item != NULL, NULL);
> > +spice_return_val_if_fail(item->refs > 0, NULL);
> >
> > -cursor_item->group_id = group_id;
> > -cursor_item->red_cursor = cmd;
> > +item->refs++;
> >
> > -return cursor_item;
> > +return item;
> >  }
> >
> > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > +void cursor_item_unref(CursorItem *item)
> >  {
> > -if (!--cursor->refs) {
> > -QXLReleaseInfoExt release_info_ext;
> > -RedCursorCmd *cursor_cmd;
> > -
> > -cursor_cmd = cursor->red_cursor;
> > -release_info_ext.group_id = cursor->group_id;
> > -release_info_ext.info = cursor_cmd->release_info;
> > -qxl->st->qif->release_resource(qxl, release_info_ext);
> > -red_put_cursor_cmd(cursor_cmd);
> > -free(cursor_cmd);
> > -
> > -g_slice_free(CursorItem, cursor);
> > -}
> > +QXLReleaseInfoExt release_info_ext;
> > +RedCursorCmd *cursor_cmd;
> > +
> > +spice_return_if_fail(item != NULL);
> > +
> > +if (--item->refs)
> 
> Just a nitpick, I would prefer to have a explicit comparison here: if
> (--items->refs > 0) ...
>

Why not 

  if (--items->refs != 0) ??

I mean, at the end the results should be the same if no errors managing
the counters are present. Are you suggesting to change the test to avoid
multiple free (hoping memory is still untouched after the first one) ?

I start suspecting that sending the patch when there are pending issue to
be discussed is not really a good idea.

> > +return;
> > +
> > +cursor_cmd = item->red_cursor;
> > +release_info_ext.group_id = item->group_id;
> > +release_info_ext.info = cursor_cmd->release_info;
> > +item->qxl->st->qif->release_resource(item->qxl, release_info_ext);
> > +red_put_cursor_cmd(cursor_cmd);
> > +free(cursor_cmd);
> > +
> > +g_slice_free(CursorItem, item);
> > +
> >  }
> >
> >  static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
> >  {
> >  if (cursor->item)
> > -cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> > cursor->item);
> > -
> > -if (item)
> > -item->refs++;
> > +cursor_item_unref(cursor->item);
> >
> > -cursor->item = item;
> > +cursor->item = item ? cursor_item_ref(item) : NULL;
> >  }
> >
> >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data,
> >  int num)
> > @@ -157,7 +160,7 @@ static void put_cursor_pipe_item(CursorChannelClient
> > *ccc, CursorPipeItem *pipe_
> >
> >  spice_assert(!pipe_item_is_linked(&pipe_item->base));
> >
> > -cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> > pipe_item->cursor_item);
> > +cursor_item_unref(pipe_item->cursor_item);
> >  free(pipe_item);
> >  }
> >
> > @@ -404,7 +407,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> > RedCursorCmd *cursor_cmd,
> >  CursorItem *cursor_item;
> >  int cursor_show = FALSE;
> >
> > -cursor_item = cursor_item_new(cursor_cmd, group_id);
> > +spice_return_if_fail(cursor);
> > +spice_return_if_fail(cursor_cmd);
> > +
> > +cursor_item =
> > cursor_item_new(red_worker_get_qxl(cursor->common.worker),
> > +  cursor_cmd, group_id);
> >
> >  switch (cursor_cmd->type) {
> >  case QXL_CURSOR_SET:
> > @@ -434,7 +441,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> > RedCursorCmd *cursor_cmd,
> >  red_c

Re: [Spice-devel] [PATCH 04/10] Move pipe item enumerations out of red_worker.h

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> Move the cursor-specific pipe item types to cursor-channel.h, and the
> display-specific types to red_worker.c. Only leave the common
> definitions in red_worker.h. This prepares for splitting the display
> channel into a separate file.
>
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.h |  6 ++
>  server/red_worker.c | 17 +
>  server/red_worker.h | 21 +++--
>  3 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index f20001c..1639cf9 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -32,6 +32,12 @@
>  #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
>  #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
>
> +enum {
> +PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
> +PIPE_ITEM_TYPE_CURSOR_INIT,
> +PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> +};
> +
>  typedef struct CursorItem {
>  QXLInstance *qxl;
>  uint32_t group_id;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 93a305a..c3b5c36 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -228,6 +228,23 @@ struct SpiceWatch {
>  void *watch_func_opaque;
>  };
>
> +enum {
> +PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST,
> +PIPE_ITEM_TYPE_IMAGE,
> +PIPE_ITEM_TYPE_STREAM_CREATE,
> +PIPE_ITEM_TYPE_STREAM_CLIP,
> +PIPE_ITEM_TYPE_STREAM_DESTROY,
> +PIPE_ITEM_TYPE_UPGRADE,
> +PIPE_ITEM_TYPE_MIGRATE_DATA,
> +PIPE_ITEM_TYPE_PIXMAP_SYNC,
> +PIPE_ITEM_TYPE_PIXMAP_RESET,
> +PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> +PIPE_ITEM_TYPE_CREATE_SURFACE,
> +PIPE_ITEM_TYPE_DESTROY_SURFACE,
> +PIPE_ITEM_TYPE_MONITORS_CONFIG,
> +PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> +};
> +
>  #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS
>
>  typedef struct SurfaceCreateItem {
> diff --git a/server/red_worker.h b/server/red_worker.h
> index 76502b6..aa97707 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -48,25 +48,10 @@ typedef struct CommonChannel {
>  } CommonChannel;
>
>  enum {
> -PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE,
> +PIPE_ITEM_TYPE_VERB = PIPE_ITEM_TYPE_CHANNEL_BASE,
>  PIPE_ITEM_TYPE_INVAL_ONE,
> -PIPE_ITEM_TYPE_CURSOR,
> -PIPE_ITEM_TYPE_CURSOR_INIT,
> -PIPE_ITEM_TYPE_IMAGE,
> -PIPE_ITEM_TYPE_STREAM_CREATE,
> -PIPE_ITEM_TYPE_STREAM_CLIP,
> -PIPE_ITEM_TYPE_STREAM_DESTROY,
> -PIPE_ITEM_TYPE_UPGRADE,
> -PIPE_ITEM_TYPE_VERB,
> -PIPE_ITEM_TYPE_MIGRATE_DATA,
> -PIPE_ITEM_TYPE_PIXMAP_SYNC,
> -PIPE_ITEM_TYPE_PIXMAP_RESET,
> -PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> -PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> -PIPE_ITEM_TYPE_CREATE_SURFACE,
> -PIPE_ITEM_TYPE_DESTROY_SURFACE,
> -PIPE_ITEM_TYPE_MONITORS_CONFIG,
> -PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> +
> +PIPE_ITEM_TYPE_COMMON_LAST
>  };
>
>  typedef struct VerbItem {
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

Re: [Spice-devel] [PATCH 03/10] Fix warning due to unexpected pipe item type

[Frediano Ziglio]

> 
> On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio  wrote:
> > From: Marc-André Lureau 
> >
> > The specific item type that was not being handled was
> > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor
> > channel, but the analogous item for the display channel is
> > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> >
> > The exact warning follows:
> >
> >  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
> >  ../../server/dcc-send.c:2442:dcc_send_item: should not be reached
> >  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
> >  ../../server/dcc.c:1595:release_item_before_push: invalid item type
> >
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cache_item.tmpl.c |  4 +++-
> >  server/red_worker.c  | 15 ---
> >  2 files changed, 3 insertions(+), 16 deletions(-)
> >
> > diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
> > index dc314c0..ad2b579 100644
> > --- a/server/cache_item.tmpl.c
> > +++ b/server/cache_item.tmpl.c
> > @@ -21,6 +21,7 @@
> >  #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
> >  #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
> >  #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
> > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
> >  #define FUNC_NAME(name) red_cursor_cache_##name
> >  #define VAR_NAME(name) cursor_cache_##name
> >  #define CHANNEL CursorChannel
> > @@ -32,6 +33,7 @@
> >  #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
> >  #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
> >  #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
> > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
> >  #define FUNC_NAME(name) red_palette_cache_##name
> >  #define VAR_NAME(name) palette_cache_##name
> >  #define CHANNEL DisplayChannel
> > @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT
> > *channel_client, CacheItem *item)
> >  channel_client->VAR_NAME(items)--;
> >  channel_client->VAR_NAME(available) += item->size;
> >
> > -red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data,
> > PIPE_ITEM_TYPE_INVAL_ONE);
> > +red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data,
> > PIPE_ITEM_TYPE);
> >  red_channel_client_pipe_add_tail(&channel_client->common.base,
> >  &item->u.pipe_data); // for now
> >  }
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 89cb25c..93a305a 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -7805,17 +7805,6 @@ static inline void
> > marshall_qxl_drawable(RedChannelClient *rcc,
> >  red_lossy_marshall_qxl_drawable(display_channel->common.worker,
> >  rcc, m, dpi);
> >  }
> >
> > -static inline void red_marshall_inval(RedChannelClient *rcc,
> > -  SpiceMarshaller *base_marshaller,
> > CacheItem *cach_item)
> > -{
> > -SpiceMsgDisplayInvalOne inval_one;
> > -
> > -red_channel_client_init_send_data(rcc, cach_item->inval_type, NULL);
> > -inval_one.id = *(uint64_t *)&cach_item->id;
> > -
> > -spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one);
> > -}
> > -
> >  static void
> >  display_channel_marshall_migrate_data_surfaces(DisplayChannelClient *dcc,
> > SpiceMarshaller
> > *m,
> > int lossy)
> > @@ -8271,9 +8260,6 @@ static void
> > display_channel_send_item(RedChannelClient *rcc, PipeItem *pipe_item
> >  marshall_qxl_drawable(rcc, m, dpi);
> >  break;
> >  }
> > -case PIPE_ITEM_TYPE_INVAL_ONE:
> > -red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> > -break;
> >  case PIPE_ITEM_TYPE_STREAM_CREATE: {
> >  StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent,
> >  create_item);
> >  red_display_marshall_stream_start(rcc, m, agent);
> > @@ -9580,7 +9566,6 @@ static void
> > display_channel_client_release_item_before_push(DisplayChannelClient
> >  free(item);
> >  break;
> >  }
> > -case PIPE_ITEM_TYPE_INVAL_ONE:
> >  case PIPE_ITEM_TYPE_VERB:
> >  case PIPE_ITEM_TYPE_MIGRATE_DATA:
> >  case PIPE_ITEM_TYPE_PIXMAP_SYNC:
> > --
> > 2.4.3
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ACK!
> 

If I remember this patch is still on the list cause form a comment from
Christophe (agreed by Jonathon) this patch should be merged to another.
Jonathon agreed to do some testing.

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

Re: [Spice-devel] [PATCH 03/10] Fix warning due to unexpected pipe item type

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> The specific item type that was not being handled was
> PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor
> channel, but the analogous item for the display channel is
> PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
>
> The exact warning follows:
>
>  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
>  ../../server/dcc-send.c:2442:dcc_send_item: should not be reached
>  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
>  ../../server/dcc.c:1595:release_item_before_push: invalid item type
>
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cache_item.tmpl.c |  4 +++-
>  server/red_worker.c  | 15 ---
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
> index dc314c0..ad2b579 100644
> --- a/server/cache_item.tmpl.c
> +++ b/server/cache_item.tmpl.c
> @@ -21,6 +21,7 @@
>  #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
>  #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
>  #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
> +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
>  #define FUNC_NAME(name) red_cursor_cache_##name
>  #define VAR_NAME(name) cursor_cache_##name
>  #define CHANNEL CursorChannel
> @@ -32,6 +33,7 @@
>  #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
>  #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
>  #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
> +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
>  #define FUNC_NAME(name) red_palette_cache_##name
>  #define VAR_NAME(name) palette_cache_##name
>  #define CHANNEL DisplayChannel
> @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT 
> *channel_client, CacheItem *item)
>  channel_client->VAR_NAME(items)--;
>  channel_client->VAR_NAME(available) += item->size;
>
> -red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, 
> PIPE_ITEM_TYPE_INVAL_ONE);
> +red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, 
> PIPE_ITEM_TYPE);
>  red_channel_client_pipe_add_tail(&channel_client->common.base, 
> &item->u.pipe_data); // for now
>  }
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 89cb25c..93a305a 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -7805,17 +7805,6 @@ static inline void 
> marshall_qxl_drawable(RedChannelClient *rcc,
>  red_lossy_marshall_qxl_drawable(display_channel->common.worker, rcc, 
> m, dpi);
>  }
>
> -static inline void red_marshall_inval(RedChannelClient *rcc,
> -  SpiceMarshaller *base_marshaller, 
> CacheItem *cach_item)
> -{
> -SpiceMsgDisplayInvalOne inval_one;
> -
> -red_channel_client_init_send_data(rcc, cach_item->inval_type, NULL);
> -inval_one.id = *(uint64_t *)&cach_item->id;
> -
> -spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one);
> -}
> -
>  static void 
> display_channel_marshall_migrate_data_surfaces(DisplayChannelClient *dcc,
> SpiceMarshaller 
> *m,
> int lossy)
> @@ -8271,9 +8260,6 @@ static void display_channel_send_item(RedChannelClient 
> *rcc, PipeItem *pipe_item
>  marshall_qxl_drawable(rcc, m, dpi);
>  break;
>  }
> -case PIPE_ITEM_TYPE_INVAL_ONE:
> -red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> -break;
>  case PIPE_ITEM_TYPE_STREAM_CREATE: {
>  StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, 
> create_item);
>  red_display_marshall_stream_start(rcc, m, agent);
> @@ -9580,7 +9566,6 @@ static void 
> display_channel_client_release_item_before_push(DisplayChannelClient
>  free(item);
>  break;
>  }
> -case PIPE_ITEM_TYPE_INVAL_ONE:
>  case PIPE_ITEM_TYPE_VERB:
>  case PIPE_ITEM_TYPE_MIGRATE_DATA:
>  case PIPE_ITEM_TYPE_PIXMAP_SYNC:
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

Re: [Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio  wrote:
> From: Marc-André Lureau 
>
> Doing so allows us to remove the extra QXLInstance parameter from
> cursor_item_unref() and makes the code a bit cleaner.
>
> Also add cursor_item_ref().
>
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 70 
> +++--
>  server/cursor-channel.h |  9 ---
>  2 files changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index eef0121..d145f86 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -25,55 +25,58 @@
>  #include "cache_item.tmpl.c"
>  #undef CLIENT_CURSOR_CACHE
>
> -static inline CursorItem *alloc_cursor_item(void)
> +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t 
> group_id)
>  {
>  CursorItem *cursor_item;
>
> +spice_return_val_if_fail(cmd != NULL, NULL);
> +
>  cursor_item = g_slice_new0(CursorItem);
> +cursor_item->qxl = qxl;
>  cursor_item->refs = 1;
> +cursor_item->group_id = group_id;
> +cursor_item->red_cursor = cmd;
>
>  return cursor_item;
>  }
>
> -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> +CursorItem *cursor_item_ref(CursorItem *item)
>  {
> -CursorItem *cursor_item;
> -
> -spice_return_val_if_fail(cmd != NULL, NULL);
> -cursor_item = alloc_cursor_item();
> +spice_return_val_if_fail(item != NULL, NULL);
> +spice_return_val_if_fail(item->refs > 0, NULL);
>
> -cursor_item->group_id = group_id;
> -cursor_item->red_cursor = cmd;
> +item->refs++;
>
> -return cursor_item;
> +return item;
>  }
>
> -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> +void cursor_item_unref(CursorItem *item)
>  {
> -if (!--cursor->refs) {
> -QXLReleaseInfoExt release_info_ext;
> -RedCursorCmd *cursor_cmd;
> -
> -cursor_cmd = cursor->red_cursor;
> -release_info_ext.group_id = cursor->group_id;
> -release_info_ext.info = cursor_cmd->release_info;
> -qxl->st->qif->release_resource(qxl, release_info_ext);
> -red_put_cursor_cmd(cursor_cmd);
> -free(cursor_cmd);
> -
> -g_slice_free(CursorItem, cursor);
> -}
> +QXLReleaseInfoExt release_info_ext;
> +RedCursorCmd *cursor_cmd;
> +
> +spice_return_if_fail(item != NULL);
> +
> +if (--item->refs)

Just a nitpick, I would prefer to have a explicit comparison here: if
(--items->refs > 0) ...

> +return;
> +
> +cursor_cmd = item->red_cursor;
> +release_info_ext.group_id = item->group_id;
> +release_info_ext.info = cursor_cmd->release_info;
> +item->qxl->st->qif->release_resource(item->qxl, release_info_ext);
> +red_put_cursor_cmd(cursor_cmd);
> +free(cursor_cmd);
> +
> +g_slice_free(CursorItem, item);
> +
>  }
>
>  static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
>  {
>  if (cursor->item)
> -cursor_item_unref(red_worker_get_qxl(cursor->common.worker), 
> cursor->item);
> -
> -if (item)
> -item->refs++;
> +cursor_item_unref(cursor->item);
>
> -cursor->item = item;
> +cursor->item = item ? cursor_item_ref(item) : NULL;
>  }
>
>  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int 
> num)
> @@ -157,7 +160,7 @@ static void put_cursor_pipe_item(CursorChannelClient 
> *ccc, CursorPipeItem *pipe_
>
>  spice_assert(!pipe_item_is_linked(&pipe_item->base));
>
> -cursor_item_unref(red_worker_get_qxl(ccc->common.worker), 
> pipe_item->cursor_item);
> +cursor_item_unref(pipe_item->cursor_item);
>  free(pipe_item);
>  }
>
> @@ -404,7 +407,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
>  CursorItem *cursor_item;
>  int cursor_show = FALSE;
>
> -cursor_item = cursor_item_new(cursor_cmd, group_id);
> +spice_return_if_fail(cursor);
> +spice_return_if_fail(cursor_cmd);
> +
> +cursor_item = cursor_item_new(red_worker_get_qxl(cursor->common.worker),
> +  cursor_cmd, group_id);
>
>  switch (cursor_cmd->type) {
>  case QXL_CURSOR_SET:
> @@ -434,7 +441,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
>  red_channel_pipes_new_add(&cursor->common.base,
>new_cursor_pipe_item, cursor_item);
>  }
> -cursor_item_unref(red_worker_get_qxl(cursor->common.worker), 
> cursor_item);
> +
> +cursor_item_unref(cursor_item);
>  }
>
>  void cursor_channel_reset(CursorChannel *cursor)
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 293cfc1..f20001c 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -33,6 +33,7 @@
>  #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
>
>  typedef struct CursorItem {
> +QXLInstance *qxl;
>  uint32_t group_id;
>  in

Re: [Spice-devel] [PATCH 09/14] server: make cursor channel private

[Frediano Ziglio]

> 
> On Mon, Oct 26, 2015 at 06:48:38AM -0400, Frediano Ziglio wrote:
> > 
> > > 
> > > On Fri, Oct 23, 2015 at 10:29:04AM -0400, Frediano Ziglio wrote:
> > > > It would be much better to define a public structure like
> > > > 
> > > > struct CursorChannel {
> > > >CommonChannel common;
> > > > };
> > > > 
> > > > and a private one (in cursor-channel.c) like
> > > > 
> > > > struct CursorChannel {
> > > >CommonChannel common;
> > > >... any field needed ...
> > > > }
> > > > 
> > > > and define the macro as
> > > > 
> > > > #define RED_CHANNEL(Channel) (&(Channel)->common.base)
> > > 
> > > This kind of macros does not work as soon as you introduce another level
> > > of inheritance:
> > > 
> > > struct SpecializedCursorChannel {
> > > CursorChannel parent;
> > > /* more stuff */
> > > };
> > > 
> > > You cannot use your RED_CHANNEL macro to get a RedChannel from a
> > > SpecializedCursorChannel instance.
> > > 
> > > Christophe
> > > 
> > 
> > Yes, this does not prevent the macro to convert an integer array to a
> > RedChannel
> > without warnings. How was implemented before prevented it.
> 
> I understand what benefits your suggested implementation had, but my
> point is that it will not handle some perfectly valid use cases.
> As Jonathon was pointing out, this can be seen as a step on the way
> toward a more object-oriented approach.
> 
> At this point, the gist of the patch is using the macro rather than
> direct foo->base access, so how it's implemented is not that important
> (ie we can go with your suggestion if that works everywhere). It's
> likely to be replaced with a glorified cast (cast + runtime type check)
> in the future though.
> 
> Christophe
> 

Hi,
  just to make it clear. You and Jonathon agreed this patch and with macros
as is just temporary and as it's the usual way the "class" are implemented in
C. However the patch is still in the patchset for two reasons:
- there are other patches this patch depends on. It's not easy to move
  the patch not not straighforward;
- nobody acked it!

If somebody ack it I could spend some time and rebase/push it.

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

Re: [Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 2:50 PM, Fabiano Fidêncio  wrote:
> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio  wrote:
>>
>>>
>>> From: Marc-André Lureau 
>>>
>>> Move function from server/red_worker.c to new server/display-channel.c.
>>> ---
>>>  server/Makefile.am   |  1 +
>>>  server/display-channel.c | 38 +++
>>>  server/display-channel.h | 50 ---
>>>  server/red_worker.c  | 68
>>>  +++-
>>>  4 files changed, 89 insertions(+), 68 deletions(-)
>>>  create mode 100644 server/display-channel.c
>>>
>>> diff --git a/server/Makefile.am b/server/Makefile.am
>>> index 87288cc..428417b 100644
>>> --- a/server/Makefile.am
>>> +++ b/server/Makefile.am
>>> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =  \
>>>   red_parse_qxl.h \
>>>   red_worker.c\
>>>   red_worker.h\
>>> + display-channel.c   \
>>>   display-channel.h   \
>>>   cursor-channel.c\
>>>   cursor-channel.h\
>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>> new file mode 100644
>>> index 000..00be665
>>> --- /dev/null
>>> +++ b/server/display-channel.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> +   Copyright (C) 2009-2015 Red Hat, Inc.
>>> +
>>> +   This library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   This library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with this library; if not, see
>>> .
>>> +*/
>>> +#include "display-channel.h"
>>> +
>>> +DisplayChannelClient *dcc_new(DisplayChannel *display,
>>> +  RedClient *client, RedsStream *stream,
>>> +  int mig_target,
>>> +  uint32_t *common_caps, int num_common_caps,
>>> +  uint32_t *caps, int num_caps)
>>> +{
>>> +DisplayChannelClient *dcc;
>>> +
>>> +dcc = (DisplayChannelClient*)common_channel_new_client(
>>> +(CommonChannel *)display, sizeof(DisplayChannelClient),
>>> +client, stream, mig_target, TRUE,
>>> +common_caps, num_common_caps,
>>> +caps, num_caps);
>>> +spice_return_val_if_fail(dcc, NULL);
>>> +
>>> +ring_init(&dcc->palette_cache_lru);
>>> +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>>> +
>>> +return dcc;
>>> +}
>>> diff --git a/server/display-channel.h b/server/display-channel.h
>>> index e1ddc11..5d2eee5 100644
>>> --- a/server/display-channel.h
>>> +++ b/server/display-channel.h
>>> @@ -15,14 +15,47 @@
>>> You should have received a copy of the GNU Lesser General Public
>>> License along with this library; if not, see
>>> .
>>>  */
>>> -#ifndef RED_WORKER_CLIENT_H_
>>> -# define RED_WORKER_CLIENT_H_
>>> +#ifndef DISPLAY_CHANNEL_H_
>>> +# define DISPLAY_CHANNEL_H_
>>> +
>>> +#include 
>>>
>>>  #include "red_worker.h"
>>> +#include "reds_stream.h"
>>>  #include "cache-item.h"
>>>  #include "pixmap-cache.h"
>>> +#ifdef USE_OPENGL
>>> +#include "common/ogl_ctx.h"
>>> +#include "reds_gl_canvas.h"
>>> +#endif /* USE_OPENGL */
>>> +#include "reds_sw_canvas.h"
>>> +#include "glz_encoder_dictionary.h"
>>> +#include "glz_encoder.h"
>>> +#include "stat.h"
>>> +#include "reds.h"
>>> +#include "mjpeg_encoder.h"
>>> +#include "red_memslots.h"
>>> +#include "red_parse_qxl.h"
>>> +#include "red_record_qxl.h"
>>> +#include "jpeg_encoder.h"
>>> +#ifdef USE_LZ4
>>> +#include "lz4_encoder.h"
>>> +#endif
>>> +#include "demarshallers.h"
>>> +#include "zlib_encoder.h"
>>> +#include "red_channel.h"
>>> +#include "red_dispatcher.h"
>>> +#include "dispatcher.h"
>>> +#include "main_channel.h"
>>> +#include "migration_protocol.h"
>>> +#include "main_dispatcher.h"
>>> +#include "spice_server_utils.h"
>>> +#include "spice_bitmap_utils.h"
>>> +#include "spice_image_cache.h"
>>>  #include "utils.h"
>>>
>>> +typedef struct DisplayChannel DisplayChannel;
>>> +
>>>  typedef struct Drawable Drawable;
>>>
>>>  #define PALETTE_CACHE_HASH_SHIFT 8
>>> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>>>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>>>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>>>
>>> +#define CLIENT_PALETTE_CACHE_SIZE 128
>>> +
>>>  /* Each drawa

Re: [Spice-devel] [PATCH 00/10] Backported some patches from refactory branches (2nd Nov)

[Frediano Ziglio]

> 
> > 
> > This patchset supersed last patchset.
> > 
> > Changes:
> > - rebased on upstream master;
> > - removed merged patches;
> > - added patches from Jonathon cursor split ones;
> > - added some patches to the set.
> > 
> > I think if we won't receive any comment on Alon Levy's patch
> > in a week or so I'll remove entirely.
> > 
> > The "server: move display_channel_client_create() to dcc_new()"
> > is the merge of "server: move display_channel_client_new()" and
> > "s/display_channel_client_new/dcc_new" as discussed.
> > 
> > Alon Levy (1):
> >   server/red_worker: red_draw_qxl_drawable: protect from NULL
> > dereference in case of buggy driver (or recording)
> > 
> > Marc-André Lureau (9):
> >   server: move display_channel_client_create() to dcc_new()
> >   Store QXLInstance in CursorItem
> >   Fix warning due to unexpected pipe item type
> >   Move pipe item enumerations out of red_worker.h
> >   Change some asserts to warnings
> >   server: make cursor channel private
> >   server: make more of cursor private
> >   tree: move that to a seperate unit
> >   server: move bitmap related to red_bitmap_utils
> > 
> 
> Merged "tree: move that to a seperate unit"
> 

Merged "server: move display_channel_client_create() to dcc_new()"

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

Re: [Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 3:14 PM, Fabiano Fidêncio  wrote:
> On Mon, Nov 2, 2015 at 2:50 PM, Fabiano Fidêncio  wrote:
>> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio  wrote:
>>>

 From: Marc-André Lureau 

 Move function from server/red_worker.c to new server/display-channel.c.
 ---
  server/Makefile.am   |  1 +
  server/display-channel.c | 38 +++
  server/display-channel.h | 50 ---
  server/red_worker.c  | 68
  +++-
  4 files changed, 89 insertions(+), 68 deletions(-)
  create mode 100644 server/display-channel.c

 diff --git a/server/Makefile.am b/server/Makefile.am
 index 87288cc..428417b 100644
 --- a/server/Makefile.am
 +++ b/server/Makefile.am
 @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =  \
   red_parse_qxl.h \
   red_worker.c\
   red_worker.h\
 + display-channel.c   \
   display-channel.h   \
   cursor-channel.c\
   cursor-channel.h\
 diff --git a/server/display-channel.c b/server/display-channel.c
 new file mode 100644
 index 000..00be665
 --- /dev/null
 +++ b/server/display-channel.c
 @@ -0,0 +1,38 @@
 +/*
 +   Copyright (C) 2009-2015 Red Hat, Inc.
 +
 +   This library is free software; you can redistribute it and/or
 +   modify it under the terms of the GNU Lesser General Public
 +   License as published by the Free Software Foundation; either
 +   version 2.1 of the License, or (at your option) any later version.
 +
 +   This library is distributed in the hope that it will be useful,
 +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 +   Lesser General Public License for more details.
 +
 +   You should have received a copy of the GNU Lesser General Public
 +   License along with this library; if not, see
 .
 +*/
 +#include "display-channel.h"
 +
 +DisplayChannelClient *dcc_new(DisplayChannel *display,
 +  RedClient *client, RedsStream *stream,
 +  int mig_target,
 +  uint32_t *common_caps, int num_common_caps,
 +  uint32_t *caps, int num_caps)
 +{
 +DisplayChannelClient *dcc;
 +
 +dcc = (DisplayChannelClient*)common_channel_new_client(
 +(CommonChannel *)display, sizeof(DisplayChannelClient),
 +client, stream, mig_target, TRUE,
 +common_caps, num_common_caps,
 +caps, num_caps);
 +spice_return_val_if_fail(dcc, NULL);
 +
 +ring_init(&dcc->palette_cache_lru);
 +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
 +
 +return dcc;
 +}
 diff --git a/server/display-channel.h b/server/display-channel.h
 index e1ddc11..5d2eee5 100644
 --- a/server/display-channel.h
 +++ b/server/display-channel.h
 @@ -15,14 +15,47 @@
 You should have received a copy of the GNU Lesser General Public
 License along with this library; if not, see
 .
  */
 -#ifndef RED_WORKER_CLIENT_H_
 -# define RED_WORKER_CLIENT_H_
 +#ifndef DISPLAY_CHANNEL_H_
 +# define DISPLAY_CHANNEL_H_
 +
 +#include 

  #include "red_worker.h"
 +#include "reds_stream.h"
  #include "cache-item.h"
  #include "pixmap-cache.h"
 +#ifdef USE_OPENGL
 +#include "common/ogl_ctx.h"
 +#include "reds_gl_canvas.h"
 +#endif /* USE_OPENGL */
 +#include "reds_sw_canvas.h"
 +#include "glz_encoder_dictionary.h"
 +#include "glz_encoder.h"
 +#include "stat.h"
 +#include "reds.h"
 +#include "mjpeg_encoder.h"
 +#include "red_memslots.h"
 +#include "red_parse_qxl.h"
 +#include "red_record_qxl.h"
 +#include "jpeg_encoder.h"
 +#ifdef USE_LZ4
 +#include "lz4_encoder.h"
 +#endif
 +#include "demarshallers.h"
 +#include "zlib_encoder.h"
 +#include "red_channel.h"
 +#include "red_dispatcher.h"
 +#include "dispatcher.h"
 +#include "main_channel.h"
 +#include "migration_protocol.h"
 +#include "main_dispatcher.h"
 +#include "spice_server_utils.h"
 +#include "spice_bitmap_utils.h"
 +#include "spice_image_cache.h"
  #include "utils.h"

 +typedef struct DisplayChannel DisplayChannel;
 +
  typedef struct Drawable Drawable;

  #define PALETTE_CACHE_HASH_SHIFT 8
 @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
  #define PALETTE_CA

Re: [Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Frediano Ziglio]

> 
> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio  wrote:
> >
> >>
> >> From: Marc-André Lureau 
> >>
> >> Move function from server/red_worker.c to new server/display-channel.c.
> >> ---
> >>  server/Makefile.am   |  1 +
> >>  server/display-channel.c | 38 +++
> >>  server/display-channel.h | 50 ---
> >>  server/red_worker.c  | 68
> >>  +++-
> >>  4 files changed, 89 insertions(+), 68 deletions(-)
> >>  create mode 100644 server/display-channel.c
> >>
> >> diff --git a/server/Makefile.am b/server/Makefile.am
> >> index 87288cc..428417b 100644
> >> --- a/server/Makefile.am
> >> +++ b/server/Makefile.am
> >> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =  \
> >>   red_parse_qxl.h \
> >>   red_worker.c\
> >>   red_worker.h\
> >> + display-channel.c   \
> >>   display-channel.h   \
> >>   cursor-channel.c\
> >>   cursor-channel.h\
> >> diff --git a/server/display-channel.c b/server/display-channel.c
> >> new file mode 100644
> >> index 000..00be665
> >> --- /dev/null
> >> +++ b/server/display-channel.c
> >> @@ -0,0 +1,38 @@
> >> +/*
> >> +   Copyright (C) 2009-2015 Red Hat, Inc.
> >> +
> >> +   This library is free software; you can redistribute it and/or
> >> +   modify it under the terms of the GNU Lesser General Public
> >> +   License as published by the Free Software Foundation; either
> >> +   version 2.1 of the License, or (at your option) any later version.
> >> +
> >> +   This library is distributed in the hope that it will be useful,
> >> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> +   Lesser General Public License for more details.
> >> +
> >> +   You should have received a copy of the GNU Lesser General Public
> >> +   License along with this library; if not, see
> >> .
> >> +*/
> >> +#include "display-channel.h"
> >> +
> >> +DisplayChannelClient *dcc_new(DisplayChannel *display,
> >> +  RedClient *client, RedsStream *stream,
> >> +  int mig_target,
> >> +  uint32_t *common_caps, int num_common_caps,
> >> +  uint32_t *caps, int num_caps)
> >> +{
> >> +DisplayChannelClient *dcc;
> >> +
> >> +dcc = (DisplayChannelClient*)common_channel_new_client(
> >> +(CommonChannel *)display, sizeof(DisplayChannelClient),
> >> +client, stream, mig_target, TRUE,
> >> +common_caps, num_common_caps,
> >> +caps, num_caps);
> >> +spice_return_val_if_fail(dcc, NULL);
> >> +
> >> +ring_init(&dcc->palette_cache_lru);
> >> +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> >> +
> >> +return dcc;
> >> +}
> >> diff --git a/server/display-channel.h b/server/display-channel.h
> >> index e1ddc11..5d2eee5 100644
> >> --- a/server/display-channel.h
> >> +++ b/server/display-channel.h
> >> @@ -15,14 +15,47 @@
> >> You should have received a copy of the GNU Lesser General Public
> >> License along with this library; if not, see
> >> .
> >>  */
> >> -#ifndef RED_WORKER_CLIENT_H_
> >> -# define RED_WORKER_CLIENT_H_
> >> +#ifndef DISPLAY_CHANNEL_H_
> >> +# define DISPLAY_CHANNEL_H_
> >> +
> >> +#include 
> >>
> >>  #include "red_worker.h"
> >> +#include "reds_stream.h"
> >>  #include "cache-item.h"
> >>  #include "pixmap-cache.h"
> >> +#ifdef USE_OPENGL
> >> +#include "common/ogl_ctx.h"
> >> +#include "reds_gl_canvas.h"
> >> +#endif /* USE_OPENGL */
> >> +#include "reds_sw_canvas.h"
> >> +#include "glz_encoder_dictionary.h"
> >> +#include "glz_encoder.h"
> >> +#include "stat.h"
> >> +#include "reds.h"
> >> +#include "mjpeg_encoder.h"
> >> +#include "red_memslots.h"
> >> +#include "red_parse_qxl.h"
> >> +#include "red_record_qxl.h"
> >> +#include "jpeg_encoder.h"
> >> +#ifdef USE_LZ4
> >> +#include "lz4_encoder.h"
> >> +#endif
> >> +#include "demarshallers.h"
> >> +#include "zlib_encoder.h"
> >> +#include "red_channel.h"
> >> +#include "red_dispatcher.h"
> >> +#include "dispatcher.h"
> >> +#include "main_channel.h"
> >> +#include "migration_protocol.h"
> >> +#include "main_dispatcher.h"
> >> +#include "spice_server_utils.h"
> >> +#include "spice_bitmap_utils.h"
> >> +#include "spice_image_cache.h"
> >>  #include "utils.h"
> >>
> >> +typedef struct DisplayChannel DisplayChannel;
> >> +
> >>  typedef struct Drawable Drawable;
> >>
> >>  #define PALETTE_CACHE_HASH_SHIFT 8
> >> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
> >>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
> >>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MA

Re: [Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Fabiano Fidêncio]

On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio  wrote:
>
>>
>> From: Marc-André Lureau 
>>
>> Move function from server/red_worker.c to new server/display-channel.c.
>> ---
>>  server/Makefile.am   |  1 +
>>  server/display-channel.c | 38 +++
>>  server/display-channel.h | 50 ---
>>  server/red_worker.c  | 68
>>  +++-
>>  4 files changed, 89 insertions(+), 68 deletions(-)
>>  create mode 100644 server/display-channel.c
>>
>> diff --git a/server/Makefile.am b/server/Makefile.am
>> index 87288cc..428417b 100644
>> --- a/server/Makefile.am
>> +++ b/server/Makefile.am
>> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =  \
>>   red_parse_qxl.h \
>>   red_worker.c\
>>   red_worker.h\
>> + display-channel.c   \
>>   display-channel.h   \
>>   cursor-channel.c\
>>   cursor-channel.h\
>> diff --git a/server/display-channel.c b/server/display-channel.c
>> new file mode 100644
>> index 000..00be665
>> --- /dev/null
>> +++ b/server/display-channel.c
>> @@ -0,0 +1,38 @@
>> +/*
>> +   Copyright (C) 2009-2015 Red Hat, Inc.
>> +
>> +   This library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   This library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with this library; if not, see
>> .
>> +*/
>> +#include "display-channel.h"
>> +
>> +DisplayChannelClient *dcc_new(DisplayChannel *display,
>> +  RedClient *client, RedsStream *stream,
>> +  int mig_target,
>> +  uint32_t *common_caps, int num_common_caps,
>> +  uint32_t *caps, int num_caps)
>> +{
>> +DisplayChannelClient *dcc;
>> +
>> +dcc = (DisplayChannelClient*)common_channel_new_client(
>> +(CommonChannel *)display, sizeof(DisplayChannelClient),
>> +client, stream, mig_target, TRUE,
>> +common_caps, num_common_caps,
>> +caps, num_caps);
>> +spice_return_val_if_fail(dcc, NULL);
>> +
>> +ring_init(&dcc->palette_cache_lru);
>> +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>> +
>> +return dcc;
>> +}
>> diff --git a/server/display-channel.h b/server/display-channel.h
>> index e1ddc11..5d2eee5 100644
>> --- a/server/display-channel.h
>> +++ b/server/display-channel.h
>> @@ -15,14 +15,47 @@
>> You should have received a copy of the GNU Lesser General Public
>> License along with this library; if not, see
>> .
>>  */
>> -#ifndef RED_WORKER_CLIENT_H_
>> -# define RED_WORKER_CLIENT_H_
>> +#ifndef DISPLAY_CHANNEL_H_
>> +# define DISPLAY_CHANNEL_H_
>> +
>> +#include 
>>
>>  #include "red_worker.h"
>> +#include "reds_stream.h"
>>  #include "cache-item.h"
>>  #include "pixmap-cache.h"
>> +#ifdef USE_OPENGL
>> +#include "common/ogl_ctx.h"
>> +#include "reds_gl_canvas.h"
>> +#endif /* USE_OPENGL */
>> +#include "reds_sw_canvas.h"
>> +#include "glz_encoder_dictionary.h"
>> +#include "glz_encoder.h"
>> +#include "stat.h"
>> +#include "reds.h"
>> +#include "mjpeg_encoder.h"
>> +#include "red_memslots.h"
>> +#include "red_parse_qxl.h"
>> +#include "red_record_qxl.h"
>> +#include "jpeg_encoder.h"
>> +#ifdef USE_LZ4
>> +#include "lz4_encoder.h"
>> +#endif
>> +#include "demarshallers.h"
>> +#include "zlib_encoder.h"
>> +#include "red_channel.h"
>> +#include "red_dispatcher.h"
>> +#include "dispatcher.h"
>> +#include "main_channel.h"
>> +#include "migration_protocol.h"
>> +#include "main_dispatcher.h"
>> +#include "spice_server_utils.h"
>> +#include "spice_bitmap_utils.h"
>> +#include "spice_image_cache.h"
>>  #include "utils.h"
>>
>> +typedef struct DisplayChannel DisplayChannel;
>> +
>>  typedef struct Drawable Drawable;
>>
>>  #define PALETTE_CACHE_HASH_SHIFT 8
>> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>>
>> +#define CLIENT_PALETTE_CACHE_SIZE 128
>> +
>>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>>
>> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>>  uint64_t streams_max_bi

Re: [Spice-devel] [PATCH] hw/usb/dev-audio.c: make USB audio card sound perfect

[Gerd Hoffmann]

  Hi,

> > Higher chance is with video playback.  lip sync issues might show up,
> > although you probably still have to watch carefully to actually notice.
> 
> Regarding the video playback workload: video playback is not a
> low-latency audio use case.  That's why you won't notice any difference
> if the USB audio device has a larger buffer size.

Well, yes, it isn't low-latency indeed, but probably still sensitive to
the buffer size.  The buffer we are talking about here is a pure
host-side thing, the guest doesn't know it exists and how big it is.

> This can be implemented with large audio buffers,

Yes, but for that the player needs to know how big the buffer is so the
audio clock math is correct ...

cheers,
  Gerd


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

Re: [Spice-devel] [PATCH] hw/usb/dev-audio.c: make USB audio card sound perfect

[Stefan Hajnoczi]

On Fri, Oct 30, 2015 at 11:59:29AM +0100, Gerd Hoffmann wrote:
> > > What bothers me is that you have no qualms about making latency on
> > > everyone's system worse.
> > 
> > How do you know it makes sound on other people's systems worse? If you have
> > actually done any testing, I would like to see the results. 
> 
> It's real.  With that change we *do* actually trade latency for better
> sound quality.
> 
> You probably wouldn't notice with pure music playback.
> 
> Higher chance is with video playback.  lip sync issues might show up,
> although you probably still have to watch carefully to actually notice.

Regarding the video playback workload: video playback is not a
low-latency audio use case.  That's why you won't notice any difference
if the USB audio device has a larger buffer size.

Lip sync is about keeping the video frame and audio track timestamps
within reasonable timing bounds.  Video player application periodically
checks the drift between the audio sample clock and the video clock.
This can be implemented with large audio buffers, there is no need to
sync every couple of milliseconds because the clock drift isn't that
extreme.

For examples of actual low-latency audio use cases, see:
http://manual.ardour.org/synchronization/latency-and-latency-compensation/

Typically the application needs to process audio samples and play them
back within ~10 milliseconds.  An example is playing a software
synthesizer.  If the synthesizer application uses 40 millisecond
buffers, then the piano will feel laggy because the user only hears the
note after 40 milliseconds.  Therefore, the low-latency audio use cases
need small buffers to respond quickly enough.

I downloaded Ubuntu Studio but haven't had time to try running it in a
VM for testing yet:
https://ubuntustudio.org/download/


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

Re: [Spice-devel] [PATCH 00/10] Backported some patches from refactory branches (2nd Nov)

[Frediano Ziglio]

> 
> This patchset supersed last patchset.
> 
> Changes:
> - rebased on upstream master;
> - removed merged patches;
> - added patches from Jonathon cursor split ones;
> - added some patches to the set.
> 
> I think if we won't receive any comment on Alon Levy's patch
> in a week or so I'll remove entirely.
> 
> The "server: move display_channel_client_create() to dcc_new()"
> is the merge of "server: move display_channel_client_new()" and
> "s/display_channel_client_new/dcc_new" as discussed.
> 
> Alon Levy (1):
>   server/red_worker: red_draw_qxl_drawable: protect from NULL
> dereference in case of buggy driver (or recording)
> 
> Marc-André Lureau (9):
>   server: move display_channel_client_create() to dcc_new()
>   Store QXLInstance in CursorItem
>   Fix warning due to unexpected pipe item type
>   Move pipe item enumerations out of red_worker.h
>   Change some asserts to warnings
>   server: make cursor channel private
>   server: make more of cursor private
>   tree: move that to a seperate unit
>   server: move bitmap related to red_bitmap_utils
> 

Merged "tree: move that to a seperate unit"

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

Re: [Spice-devel] [PATCH 09/10] tree: move that to a seperate unit

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> ---
>  server/Makefile.am  |   2 +
>  server/red_worker.c | 266
>  +++-
>  server/tree.c   | 182 +++
>  server/tree.h   | 111 ++
>  4 files changed, 306 insertions(+), 255 deletions(-)
>  create mode 100644 server/tree.c
>  create mode 100644 server/tree.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 428417b..d2a7343 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -131,6 +131,8 @@ libspice_server_la_SOURCES =  \
>   spice_image_cache.c \
>   pixmap-cache.h  \
>   pixmap-cache.c  \
> + tree.h  \
> + tree.c  \
>   utils.h \
>   $(NULL)
>  
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 601805e..0049cca 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -63,6 +63,8 @@
>  #include "red_worker.h"
>  #include "spice_timer_queue.h"
>  #include "cursor-channel.h"
> +#include "tree.h"
> +#include "utils.h"
>  
>  //#define COMPRESS_STAT
>  //#define DUMP_BITMAP
> @@ -405,52 +407,6 @@ struct DisplayChannel {
>  #endif
>  };
>  
> -enum {
> -TREE_ITEM_TYPE_DRAWABLE,
> -TREE_ITEM_TYPE_CONTAINER,
> -TREE_ITEM_TYPE_SHADOW,
> -};
> -
> -typedef struct TreeItem {
> -RingItem siblings_link;
> -uint32_t type;
> -struct Container *container;
> -QRegion rgn;
> -} TreeItem;
> -
> -#define IS_DRAW_ITEM(item) ((item)->type == TREE_ITEM_TYPE_DRAWABLE)
> -
> -typedef struct Shadow {
> -TreeItem base;
> -QRegion on_hold;
> -struct DrawItem* owner;
> -} Shadow;
> -
> -typedef struct Container {
> -TreeItem base;
> -Ring items;
> -} Container;
> -
> -typedef struct DrawItem {
> -TreeItem base;
> -uint8_t effect;
> -uint8_t container_root;
> -Shadow *shadow;
> -} DrawItem;
> -
> -typedef enum {
> -BITMAP_GRADUAL_INVALID,
> -BITMAP_GRADUAL_NOT_AVAIL,
> -BITMAP_GRADUAL_LOW,
> -BITMAP_GRADUAL_MEDIUM,
> -BITMAP_GRADUAL_HIGH,
> -} BitmapGradualType;
> -
> -typedef struct DependItem {
> -Drawable *drawable;
> -RingItem ring_item;
> -} DependItem;
> -
>  typedef struct DrawablePipeItem {
>  RingItem base;  /* link for a list of pipe items held by Drawable */
>  PipeItem dpi_pipe_item; /* link for the client's pipe itself */
> @@ -459,35 +415,6 @@ typedef struct DrawablePipeItem {
>  uint8_t refs;
>  } DrawablePipeItem;
>  
> -struct Drawable {
> -uint8_t refs;
> -RingItem surface_list_link;
> -RingItem list_link;
> -DrawItem tree_item;
> -Ring pipes;
> -PipeItem *pipe_item_rest;
> -uint32_t size_pipe_item_rest;
> -RedDrawable *red_drawable;
> -
> -Ring glz_ring;
> -
> -red_time_t creation_time;
> -int frames_count;
> -int gradual_frames_count;
> -int last_gradual_frame;
> -Stream *stream;
> -Stream *sized_stream;
> -int streamable;
> -BitmapGradualType copy_bitmap_graduality;
> -uint32_t group_id;
> -DependItem depend_items[3];
> -
> -int surface_id;
> -int surfaces_dest[3];
> -
> -uint32_t process_commands_generation;
> -};
> -
>  typedef struct _Drawable _Drawable;
>  struct _Drawable {
>  union {
> @@ -908,91 +835,6 @@ static inline int validate_surface(RedWorker *worker,
> uint32_t surface_id)
>  return 1;
>  }
>  
> -static const char *draw_type_to_str(uint8_t type)
> -{
> -switch (type) {
> -case QXL_DRAW_FILL:
> -return "QXL_DRAW_FILL";
> -case QXL_DRAW_OPAQUE:
> -return "QXL_DRAW_OPAQUE";
> -case QXL_DRAW_COPY:
> -return "QXL_DRAW_COPY";
> -case QXL_DRAW_TRANSPARENT:
> -return "QXL_DRAW_TRANSPARENT";
> -case QXL_DRAW_ALPHA_BLEND:
> -return "QXL_DRAW_ALPHA_BLEND";
> -case QXL_COPY_BITS:
> -return "QXL_COPY_BITS";
> -case QXL_DRAW_BLEND:
> -return "QXL_DRAW_BLEND";
> -case QXL_DRAW_BLACKNESS:
> -return "QXL_DRAW_BLACKNESS";
> -case QXL_DRAW_WHITENESS:
> -return "QXL_DRAW_WHITENESS";
> -case QXL_DRAW_INVERS:
> -return "QXL_DRAW_INVERS";
> -case QXL_DRAW_ROP3:
> -return "QXL_DRAW_ROP3";
> -case QXL_DRAW_COMPOSITE:
> -return "QXL_DRAW_COMPOSITE";
> -case QXL_DRAW_STROKE:
> -return "QXL_DRAW_STROKE";
> -case QXL_DRAW_TEXT:
> -return "QXL_DRAW_TEXT";
> -default:
> -return "?";
> -}
> -}
> -
> -static void show_red_drawable(RedWorker *worker, RedDrawable *drawable,
> const char *prefix)
> -{
> -if (prefix) {
> -printf("%s: ", prefix);
> -}
> -
> -printf("%s effect %d bbox(%d %d %d %d)",
> -   draw_type_to_str(drawable->type),
> -   drawable->effect,
> -   drawable->bbox.top,
> -   drawable->bbox.left,
> -

Re: [Spice-devel] spice refactoring: workflow suggestion

[Victor Toso]

Hi,

On Mon, Nov 02, 2015 at 06:34:39AM -0500, Frediano Ziglio wrote:
>
> >
> > Hi,
> >
> > As in a mailing list it is a bit hard to track of the status of each
> > PATCH (acked/reviewed/pushed) and we still have tons of batch of
> > patches to apply/test/review I would like to suggest the following:
> >
> > 1-) When a batch is pushed, send an email in-reply-to 00/00 mail saying
> > that patches were pushed.
> > -- This really helps when one is behind following the changes
> >
>
> This can be done.
> On the same subject I thing the spreadsheet should be publicly available
> (read only). It mainly contains list of open source patches, progress,
> some report.
>

Great. I don't think sharing the spreadsheet is a problem.

> > 2-) Push series as a whole when acked instead of pushing each patch
> > when acked. In case a few patches of a series are taking time but a
> > few have been acked, maybe pushing those acked but let us know with
> > an email
> > -- Reviewing a patch to later on see that it was pushed already means
> > time lost. I believe an email is quicker then matching which patch awas
> > pushed or not.
> >
>
> These series are quite different from normal ones as the patches inside
> are quite different one from another, they don't share the same aim.
> It never happened that all patches was accepted at once.
> I agree a reply saying "pushed" could work.
>

Indeed. Having a way to track which patch was pushed or not seems good
enough to me.

(PS: I spent some time taking a look in our patchwork [0] and I think it
is just a matter of using the right tools [1] to make this interface all
we need to track the patches... but I'm not sure yet.

[0] https://patchwork.freedesktop.org/project/Spice/list/
[1] such as git-pw
http://patchwork-freedesktop.readthedocs.org/en/latest/manual.html#git-pw

> > 3-) Might sound silly but as this involves lots of patchs in a short
> > period, it might be interesting to have acked-by, tested-by, reviewed-by
> > ? At least for the refactoring...
> >
>
> I add the Acked-by. Nobody seems to review but not ack to but if somebody
> want to distinguish the two cases is free to tell review instead of ack.
> Do you think we should add a reviewed-by line for each people that commented
> the patch?

Not really, only for those who reply with reviewed-by I guess.

My point is just adding informationg to better track changes. Many
changes require more information to check who was involved.

This could be a bit annoying/disnecessary but I thought it would be
worth mentioning it here.

> I don't know if is worth to add the tested-by. I do some small test before
> committing but is really dirty and fast. I mean, style/move patches you can
> just test that is mainly compiling, working but fix for instance would
> require reproduction and I don't think this is done strictly for all fix
> (we must take also in consideration there are really few fixes compared
> to other changes).
> Just to not scare people on testing we run quite often some automated test
> and I/we ran different tests for leaks and memory errors but more
> periodically than for single patches.
>

Yeah, tested-by only seems interesting to have if one does some extent
tests with each patches. As it is automated it is probably not
necessary.

> > Any thoughts?
> > Regards,
> >   Victor Toso
>
> I really agree something is wrong in the process.
>
> I have the sensation that the acknowledge process is a bit weird and
> different than usual. One reason is that author is not changing the
> patches that are changed by somebody else. This makes the editor a
> co-author removing him the theoretical possibility to ack the patch.

Not sure how one can handle this in a better way.
  - toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-common] build-sys: Add missing # to comment

[Christophe Fergeau]

On Fri, Oct 30, 2015 at 12:01:20PM -0500, Jonathon Jongsma wrote:
> ACK

Thanks, pushed.

Christophe


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

Re: [Spice-devel] spice refactoring: workflow suggestion

[Frediano Ziglio]

> 
> Hi,
> 
> As in a mailing list it is a bit hard to track of the status of each
> PATCH (acked/reviewed/pushed) and we still have tons of batch of
> patches to apply/test/review I would like to suggest the following:
> 
> 1-) When a batch is pushed, send an email in-reply-to 00/00 mail saying
> that patches were pushed.
> -- This really helps when one is behind following the changes
> 

This can be done.
On the same subject I thing the spreadsheet should be publicly available
(read only). It mainly contains list of open source patches, progress,
some report.

> 2-) Push series as a whole when acked instead of pushing each patch
> when acked. In case a few patches of a series are taking time but a
> few have been acked, maybe pushing those acked but let us know with
> an email
> -- Reviewing a patch to later on see that it was pushed already means
> time lost. I believe an email is quicker then matching which patch awas
> pushed or not.
> 

These series are quite different from normal ones as the patches inside
are quite different one from another, they don't share the same aim.
It never happened that all patches was accepted at once.
I agree a reply saying "pushed" could work.

> 3-) Might sound silly but as this involves lots of patchs in a short
> period, it might be interesting to have acked-by, tested-by, reviewed-by
> ? At least for the refactoring...
> 

I add the Acked-by. Nobody seems to review but not ack to but if somebody
want to distinguish the two cases is free to tell review instead of ack.
Do you think we should add a reviewed-by line for each people that commented
the patch?
I don't know if is worth to add the tested-by. I do some small test before
committing but is really dirty and fast. I mean, style/move patches you can
just test that is mainly compiling, working but fix for instance would
require reproduction and I don't think this is done strictly for all fix
(we must take also in consideration there are really few fixes compared
to other changes).
Just to not scare people on testing we run quite often some automated test
and I/we ran different tests for leaks and memory errors but more
periodically than for single patches.

> Any thoughts?
> Regards,
>   Victor Toso

I really agree something is wrong in the process.

I have the sensation that the acknowledge process is a bit weird and
different than usual. One reason is that author is not changing the
patches that are changed by somebody else. This makes the editor a
co-author removing him the theoretical possibility to ack the patch.

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

Re: [Spice-devel] [PATCH 3.1/12] Store QXLInstance in CursorItem

[Frediano Ziglio]

> 
> On Fri, 2015-10-30 at 03:24 -0400, Frediano Ziglio wrote:
> > > 
> > > From: Marc-André Lureau 
> > > 
> > > Doing so allows us to remove the extra QXLInstance parameter from
> > > cursor_item_unref() and makes the code a bit cleaner.
> > > 
> > > Also add cursor_item_ref().
> > > 
> > > Signed-off-by: Jonathon Jongsma 
> > > ---
> > >  server/cursor-channel.c | 70
> > >  +++--
> > >  server/cursor-channel.h |  9 ---
> > >  2 files changed, 44 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index 6cc2b93..2d7afc6 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -25,55 +25,58 @@
> > >  #include "cache_item.tmpl.c"
> > >  #undef CLIENT_CURSOR_CACHE
> > >  
> > > -static inline CursorItem *alloc_cursor_item(void)
> > > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd,
> > > uint32_t
> > > group_id)
> > >  {
> > >  CursorItem *cursor_item;
> > >  
> > > +spice_return_val_if_fail(cmd != NULL, NULL);
> > > +
> > >  cursor_item = g_slice_new0(CursorItem);
> > > +cursor_item->qxl = qxl;
> > >  cursor_item->refs = 1;
> > > +cursor_item->group_id = group_id;
> > > +cursor_item->red_cursor = cmd;
> > >  
> > >  return cursor_item;
> > >  }
> > >  
> > > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> > > +CursorItem *cursor_item_ref(CursorItem *item)
> > >  {
> > > -CursorItem *cursor_item;
> > > -
> > > -spice_return_val_if_fail(cmd != NULL, NULL);
> > > -cursor_item = alloc_cursor_item();
> > > +spice_return_val_if_fail(item != NULL, NULL);
> > > +spice_return_val_if_fail(item->refs > 0, NULL);
> > 
> > This catch a memory corruption, should abort the program,
> > I suggest spice_error, spice_critical or spice_assert.
> 
> It can suggest memory corruption, yes. But I'm not sure that it should
> be fatal. This is exactly the same behavior as g_object_ref(), so I
> don't see any good reason to make this warning fatal when the exact
> same problem with a gobject will not be fatal.
> 

Although glib is now "server aware" you have to consider was born as a
client library where some problem are considered in a different way.
Also GObject pointers are a bit more complicated (for instance they support
weak pointers) that could lead to some temporary strange state.
The counters in our code should be handled in a single thread (from worker),
start from 1, incremented for ref, decremented and object freed on zero.
If everything follows these rules counters are always >0.
If they became <= 0 it means to me:
- some unrelated memory corruption (counters written from unexpected code,
  physical memory errors, invalid dma transfers or OS bugs). Not the type
  of errors you can handle in a safe way;
- counter was incremented till overflow. You are spending lot of memory
  with a lot of objects (there are at least 2^31 pointers to this object!)
  something is really wrong in your program; DoS is likely to happen;
- counter was decremented more then expected. You have pointer to now freed
  data. Dandling pointers are bound to heap memory corruptions which is
  a security problem.

In a server context I don't see the point to keep the program going on these
conditions beside opening the doors to hackers.

> > 
> > >  
> > > -cursor_item->group_id = group_id;
> > > -cursor_item->red_cursor = cmd;
> > > +item->refs++;
> > >  
> > > -return cursor_item;
> > > +return item;
> > >  }
> > >  
> > > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > > +void cursor_item_unref(CursorItem *item)
> > >  {
> > > -if (!--cursor->refs) {
> > > -QXLReleaseInfoExt release_info_ext;
> > > -RedCursorCmd *cursor_cmd;
> > > -
> > > -cursor_cmd = cursor->red_cursor;
> > > -release_info_ext.group_id = cursor->group_id;
> > > -release_info_ext.info = cursor_cmd->release_info;
> > > -qxl->st->qif->release_resource(qxl, release_info_ext);
> > > -red_put_cursor_cmd(cursor_cmd);
> > > -free(cursor_cmd);
> > > -
> > > -g_slice_free(CursorItem, cursor);
> > > -}
> > > +QXLReleaseInfoExt release_info_ext;
> > > +RedCursorCmd *cursor_cmd;
> > > +
> > > +spice_return_if_fail(item != NULL);
> > > +
> > > +if (--item->refs)
> > > +return;
> > > +
> > > +cursor_cmd = item->red_cursor;
> > > +release_info_ext.group_id = item->group_id;
> > > +release_info_ext.info = cursor_cmd->release_info;
> > > +item->qxl->st->qif->release_resource(item->qxl,
> > > release_info_ext);
> > > +red_put_cursor_cmd(cursor_cmd);
> > > +free(cursor_cmd);
> > > +
> > > +g_slice_free(CursorItem, item);
> > > +
> > >  }
> > >  
> > >  static void cursor_set_item(CursorChannel *cursor, CursorItem
> > > *item)
> > >  {
> > >  if (cursor->item)
> > > -cursor_item_unref(red_worke

[Spice-devel] spice refactoring: workflow suggestion

[Victor Toso]

Hi,

As in a mailing list it is a bit hard to track of the status of each
PATCH (acked/reviewed/pushed) and we still have tons of batch of
patches to apply/test/review I would like to suggest the following:

1-) When a batch is pushed, send an email in-reply-to 00/00 mail saying
that patches were pushed.
-- This really helps when one is behind following the changes

2-) Push series as a whole when acked instead of pushing each patch
when acked. In case a few patches of a series are taking time but a
few have been acked, maybe pushing those acked but let us know with
an email
-- Reviewing a patch to later on see that it was pushed already means
time lost. I believe an email is quicker then matching which patch awas
pushed or not.

3-) Might sound silly but as this involves lots of patchs in a short
period, it might be interesting to have acked-by, tested-by, reviewed-by
? At least for the refactoring...

Any thoughts?
Regards,
  Victor Toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Move function from server/red_worker.c to new server/display-channel.c.
> ---
>  server/Makefile.am   |  1 +
>  server/display-channel.c | 38 +++
>  server/display-channel.h | 50 ---
>  server/red_worker.c  | 68
>  +++-
>  4 files changed, 89 insertions(+), 68 deletions(-)
>  create mode 100644 server/display-channel.c
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 87288cc..428417b 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =  \
>   red_parse_qxl.h \
>   red_worker.c\
>   red_worker.h\
> + display-channel.c   \
>   display-channel.h   \
>   cursor-channel.c\
>   cursor-channel.h\
> diff --git a/server/display-channel.c b/server/display-channel.c
> new file mode 100644
> index 000..00be665
> --- /dev/null
> +++ b/server/display-channel.c
> @@ -0,0 +1,38 @@
> +/*
> +   Copyright (C) 2009-2015 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see
> .
> +*/
> +#include "display-channel.h"
> +
> +DisplayChannelClient *dcc_new(DisplayChannel *display,
> +  RedClient *client, RedsStream *stream,
> +  int mig_target,
> +  uint32_t *common_caps, int num_common_caps,
> +  uint32_t *caps, int num_caps)
> +{
> +DisplayChannelClient *dcc;
> +
> +dcc = (DisplayChannelClient*)common_channel_new_client(
> +(CommonChannel *)display, sizeof(DisplayChannelClient),
> +client, stream, mig_target, TRUE,
> +common_caps, num_common_caps,
> +caps, num_caps);
> +spice_return_val_if_fail(dcc, NULL);
> +
> +ring_init(&dcc->palette_cache_lru);
> +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> +
> +return dcc;
> +}
> diff --git a/server/display-channel.h b/server/display-channel.h
> index e1ddc11..5d2eee5 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -15,14 +15,47 @@
> You should have received a copy of the GNU Lesser General Public
> License along with this library; if not, see
> .
>  */
> -#ifndef RED_WORKER_CLIENT_H_
> -# define RED_WORKER_CLIENT_H_
> +#ifndef DISPLAY_CHANNEL_H_
> +# define DISPLAY_CHANNEL_H_
> +
> +#include 
>  
>  #include "red_worker.h"
> +#include "reds_stream.h"
>  #include "cache-item.h"
>  #include "pixmap-cache.h"
> +#ifdef USE_OPENGL
> +#include "common/ogl_ctx.h"
> +#include "reds_gl_canvas.h"
> +#endif /* USE_OPENGL */
> +#include "reds_sw_canvas.h"
> +#include "glz_encoder_dictionary.h"
> +#include "glz_encoder.h"
> +#include "stat.h"
> +#include "reds.h"
> +#include "mjpeg_encoder.h"
> +#include "red_memslots.h"
> +#include "red_parse_qxl.h"
> +#include "red_record_qxl.h"
> +#include "jpeg_encoder.h"
> +#ifdef USE_LZ4
> +#include "lz4_encoder.h"
> +#endif
> +#include "demarshallers.h"
> +#include "zlib_encoder.h"
> +#include "red_channel.h"
> +#include "red_dispatcher.h"
> +#include "dispatcher.h"
> +#include "main_channel.h"
> +#include "migration_protocol.h"
> +#include "main_dispatcher.h"
> +#include "spice_server_utils.h"
> +#include "spice_bitmap_utils.h"
> +#include "spice_image_cache.h"
>  #include "utils.h"
>  
> +typedef struct DisplayChannel DisplayChannel;
> +
>  typedef struct Drawable Drawable;
>  
>  #define PALETTE_CACHE_HASH_SHIFT 8
> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>  
> +#define CLIENT_PALETTE_CACHE_SIZE 128
> +
>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>  
> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>  uint64_t streams_max_bit_rate;
>  };
>  
> -#endif /* RED_WORKER_CLIENT_H_ */
> +DisplayChannelClient*  dcc_new
> (DisplayChannel *display,
> +
> RedClient
> *client,
> +
> RedsStream
> *stream,
> +  

[Spice-devel] [PATCH 08/10] server: make more of cursor private

[Frediano Ziglio]

From: Marc-André Lureau 

---
 server/cursor-channel.c | 79 +++--
 server/cursor-channel.h | 51 +++
 server/red_channel.h|  2 ++
 server/red_worker.c | 22 +-
 server/red_worker.h |  1 +
 5 files changed, 90 insertions(+), 65 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 3ae7756..5995aa5 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -19,6 +19,41 @@
 #include "common/generated_server_marshallers.h"
 #include "cursor-channel.h"
 
+#define CLIENT_CURSOR_CACHE_SIZE 256
+
+#define CURSOR_CACHE_HASH_SHIFT 8
+#define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
+#define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
+#define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
+
+enum {
+PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
+PIPE_ITEM_TYPE_CURSOR_INIT,
+PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
+};
+
+typedef struct CursorItem {
+QXLInstance *qxl;
+uint32_t group_id;
+int refs;
+RedCursorCmd *red_cursor;
+} CursorItem;
+
+G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
+
+typedef struct LocalCursor {
+CursorItem base;
+SpicePoint16 position;
+uint32_t data_size;
+SpiceCursor red_cursor;
+} LocalCursor;
+
+typedef struct CursorPipeItem {
+PipeItem base;
+CursorItem *cursor_item;
+int refs;
+} CursorPipeItem;
+
 struct CursorChannel {
 CommonChannel common; // Must be the first thing
 
@@ -34,13 +69,23 @@ struct CursorChannel {
 #endif
 };
 
+struct CursorChannelClient {
+CommonChannelClient common;
+
+CacheItem *cursor_cache[CURSOR_CACHE_HASH_SIZE];
+Ring cursor_cache_lru;
+long cursor_cache_available;
+uint32_t cursor_cache_items;
+};
+
+
 #define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, 
common.base)
 
 #define CLIENT_CURSOR_CACHE
 #include "cache_item.tmpl.c"
 #undef CLIENT_CURSOR_CACHE
 
-CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t 
group_id)
+static CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, 
uint32_t group_id)
 {
 CursorItem *cursor_item;
 
@@ -55,7 +100,7 @@ CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd 
*cmd, uint32_t group_
 return cursor_item;
 }
 
-CursorItem *cursor_item_ref(CursorItem *item)
+static CursorItem *cursor_item_ref(CursorItem *item)
 {
 spice_return_val_if_fail(item != NULL, NULL);
 spice_return_val_if_fail(item->refs > 0, NULL);
@@ -65,7 +110,7 @@ CursorItem *cursor_item_ref(CursorItem *item)
 return item;
 }
 
-void cursor_item_unref(CursorItem *item)
+static void cursor_item_unref(CursorItem *item)
 {
 QXLReleaseInfoExt release_info_ext;
 RedCursorCmd *cursor_cmd;
@@ -398,6 +443,17 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
 return cursor_channel;
 }
 
+void cursor_channel_client_migrate(CursorChannelClient* client)
+{
+RedChannelClient *rcc;
+
+spice_return_if_fail(client);
+rcc = RED_CHANNEL_CLIENT(client);
+
+red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
+red_channel_client_default_migrate(rcc);
+}
+
 CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, 
RedClient *client, RedsStream *stream,
int mig_target,
uint32_t *common_caps, int 
num_common_caps,
@@ -496,6 +552,23 @@ void cursor_channel_reset(CursorChannel *cursor)
 }
 }
 
+void cursor_channel_init(CursorChannel *cursor, CursorChannelClient *client)
+{
+spice_return_if_fail(cursor);
+
+if (red_channel_is_connected(&cursor->common.base)
+|| COMMON_CHANNEL(cursor)->during_target_migrate) {
+spice_debug("during_target_migrate: skip init");
+return;
+}
+
+if (client)
+red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(client),
+ PIPE_ITEM_TYPE_CURSOR_INIT);
+else
+red_channel_pipes_add_type(RED_CHANNEL(cursor), 
PIPE_ITEM_TYPE_CURSOR_INIT);
+}
+
 void cursor_channel_set_mouse_mode(CursorChannel *cursor, uint32_t mode)
 {
 spice_return_if_fail(cursor);
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index d5e5b13..887f847 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -25,55 +25,15 @@
 #include "cache-item.h"
 #include "stat.h"
 
-#define CLIENT_CURSOR_CACHE_SIZE 256
-
-#define CURSOR_CACHE_HASH_SHIFT 8
-#define CURSOR_CACHE_HASH_SIZE (1 << CURSOR_CACHE_HASH_SHIFT)
-#define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
-#define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
-
-enum {
-PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
-PIPE_ITEM_TYPE_CURSOR_INIT,
-PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
-};
-
 typedef struct CursorChannel CursorChannel;
+typedef struct CursorChannelClient CursorChann

[Spice-devel] [PATCH 07/10] server: make cursor channel private

[Frediano Ziglio]

From: Marc-André Lureau 

---
 server/cursor-channel.c | 24 +++-
 server/cursor-channel.h | 18 ++---
 server/red_channel.c| 12 ++
 server/red_channel.h|  6 +++
 server/red_worker.c | 98 ++---
 server/red_worker.h |  5 +++
 6 files changed, 92 insertions(+), 71 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 6a1fcea..3ae7756 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -19,6 +19,21 @@
 #include "common/generated_server_marshallers.h"
 #include "cursor-channel.h"
 
+struct CursorChannel {
+CommonChannel common; // Must be the first thing
+
+CursorItem *item;
+int cursor_visible;
+SpicePoint16 cursor_position;
+uint16_t cursor_trail_length;
+uint16_t cursor_trail_frequency;
+uint32_t mouse_mode;
+
+#ifdef RED_STATISTICS
+StatNodeRef stat;
+#endif
+};
+
 #define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, 
common.base)
 
 #define CLIENT_CURSOR_CACHE
@@ -372,7 +387,7 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
 };
 
 spice_info("create cursor channel");
-channel = red_worker_new_channel(worker, sizeof(CursorChannel),
+channel = red_worker_new_channel(worker, sizeof(CursorChannel), 
"cursor_channel",
  SPICE_CHANNEL_CURSOR, 0,
  &cbs, red_channel_client_handle_message);
 
@@ -480,3 +495,10 @@ void cursor_channel_reset(CursorChannel *cursor)
 }
 }
 }
+
+void cursor_channel_set_mouse_mode(CursorChannel *cursor, uint32_t mode)
+{
+spice_return_if_fail(cursor);
+
+cursor->mouse_mode = mode;
+}
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index 1639cf9..d5e5b13 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -38,6 +38,8 @@ enum {
 PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
 };
 
+typedef struct CursorChannel CursorChannel;
+
 typedef struct CursorItem {
 QXLInstance *qxl;
 uint32_t group_id;
@@ -67,21 +69,6 @@ typedef struct CursorChannelClient {
 uint32_t cursor_cache_items;
 } CursorChannelClient;
 
-typedef struct CursorChannel {
-CommonChannel common; // Must be the first thing
-
-CursorItem *item;
-int cursor_visible;
-SpicePoint16 cursor_position;
-uint16_t cursor_trail_length;
-uint16_t cursor_trail_frequency;
-uint32_t mouse_mode;
-
-#ifdef RED_STATISTICS
-StatNodeRef stat;
-#endif
-} CursorChannel;
-
 G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
 
 CursorChannel*   cursor_channel_new (RedWorker *worker);
@@ -89,6 +76,7 @@ void cursor_channel_disconnect  
(CursorChannel *cursor_channel);
 void cursor_channel_reset   (CursorChannel *cursor);
 void cursor_channel_process_cmd (CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
  uint32_t group_id);
+void cursor_channel_set_mouse_mode(CursorChannel *cursor, 
uint32_t mode);
 
 CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
RedClient *client, RedsStream 
*stream,
diff --git a/server/red_channel.c b/server/red_channel.c
index 34aa9dc..7330ae2 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1151,9 +1151,21 @@ RedChannel *red_channel_create_parser(int size,
 }
 channel->incoming_cb.handle_parsed = (handle_parsed_proc)handle_parsed;
 channel->incoming_cb.parser = parser;
+
 return channel;
 }
 
+void red_channel_set_stat_node(RedChannel *channel, StatNodeRef stat)
+{
+spice_return_if_fail(channel != NULL);
+spice_return_if_fail(channel->stat == 0);
+
+#ifdef RED_STATISTICS
+channel->stat = stat;
+channel->out_bytes_counter = stat_add_counter(stat, "out_bytes", TRUE);
+#endif
+}
+
 void red_channel_register_client_cbs(RedChannel *channel, ClientCbs 
*client_cbs)
 {
 spice_assert(client_cbs->connect || channel->type == SPICE_CHANNEL_MAIN);
diff --git a/server/red_channel.h b/server/red_channel.h
index 1f1538e..201a4d2 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -32,6 +32,7 @@
 #include "red_common.h"
 #include "demarshallers.h"
 #include "reds_stream.h"
+#include "stat.h"
 
 #define MAX_SEND_BUFS 1000
 #define CLIENT_ACK_WINDOW 20
@@ -127,6 +128,7 @@ typedef struct OutgoingHandler {
 
 /* Red Channel interface */
 
+typedef struct RedsStream RedsStream;
 typedef struct RedChannel RedChannel;
 typedef struct RedChannelClient RedChannelClient;
 typedef struct RedClient RedClient;
@@ -335,10 +337,13 @@ struct RedChannel {
 // TODO: when different channel_clients are in different threads from 
Channel -> need to protect!
 pthread_t thread_id;
 #ifdef RED_STATISTICS
+StatNodeRef stat;
 uint64_t *out_bytes_counter;
 #endif
 };
 
+#define RED_CHANNEL(Channel) ((RedChannel *)(Channel))
+
 /*
  *

[Spice-devel] [PATCH 00/10] Backported some patches from refactory branches (2nd Nov)

[Frediano Ziglio]

This patchset supersed last patchset.

Changes:
- rebased on upstream master;
- removed merged patches;
- added patches from Jonathon cursor split ones;
- added some patches to the set.

I think if we won't receive any comment on Alon Levy's patch
in a week or so I'll remove entirely.

The "server: move display_channel_client_create() to dcc_new()"
is the merge of "server: move display_channel_client_new()" and
"s/display_channel_client_new/dcc_new" as discussed.

Alon Levy (1):
  server/red_worker: red_draw_qxl_drawable: protect from NULL
dereference in case of buggy driver (or recording)

Marc-André Lureau (9):
  server: move display_channel_client_create() to dcc_new()
  Store QXLInstance in CursorItem
  Fix warning due to unexpected pipe item type
  Move pipe item enumerations out of red_worker.h
  Change some asserts to warnings
  server: make cursor channel private
  server: make more of cursor private
  tree: move that to a seperate unit
  server: move bitmap related to red_bitmap_utils

 server/Makefile.am|   5 +
 server/cache_item.tmpl.c  |   4 +-
 server/cursor-channel.c   | 195 +++---
 server/cursor-channel.h   |  60 +
 server/display-channel.c  |  38 +++
 server/display-channel.h  |  50 +++-
 server/red_bitmap_utils.c |  99 +++
 server/red_bitmap_utils.h |  91 +++
 server/red_channel.c  |  12 +
 server/red_channel.h  |   8 +
 server/red_common.h   |  13 -
 server/red_parse_qxl.c|   1 +
 server/red_worker.c   | 661 --
 server/red_worker.h   |  28 +-
 server/tree.c | 182 +
 server/tree.h | 104 
 16 files changed, 863 insertions(+), 688 deletions(-)
 create mode 100644 server/display-channel.c
 create mode 100644 server/red_bitmap_utils.c
 create mode 100644 server/red_bitmap_utils.h
 create mode 100644 server/tree.c
 create mode 100644 server/tree.h

-- 
2.4.3

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

[Spice-devel] [PATCH 05/10] Change some asserts to warnings

[Frediano Ziglio]

From: Marc-André Lureau 

Various changes in RedWorker and CursorChannel related to error and
warning messages.

Signed-off-by: Jonathon Jongsma 
---
 server/cursor-channel.c | 28 
 server/red_worker.c | 22 ++
 server/red_worker.h |  1 -
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index d145f86..6a1fcea 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -152,7 +152,8 @@ void cursor_channel_disconnect(CursorChannel 
*cursor_channel)
 
 static void put_cursor_pipe_item(CursorChannelClient *ccc, CursorPipeItem 
*pipe_item)
 {
-spice_assert(pipe_item);
+spice_return_if_fail(pipe_item);
+spice_return_if_fail(pipe_item->refs > 0);
 
 if (--pipe_item->refs) {
 return;
@@ -239,7 +240,7 @@ static void cursor_marshall(RedChannelClient *rcc,
 PipeItem *pipe_item = &cursor_pipe_item->base;
 RedCursorCmd *cmd;
 
-spice_assert(cursor_channel);
+spice_return_if_fail(cursor_channel);
 
 cmd = item->red_cursor;
 switch (cmd->type) {
@@ -327,7 +328,9 @@ static void cursor_channel_send_item(RedChannelClient *rcc, 
PipeItem *pipe_item)
 
 static CursorPipeItem *cursor_pipe_item_ref(CursorPipeItem *item)
 {
-spice_assert(item);
+spice_return_val_if_fail(item, NULL);
+spice_return_val_if_fail(item->refs > 0, NULL);
+
 item->refs++;
 return item;
 }
@@ -336,7 +339,9 @@ static CursorPipeItem *cursor_pipe_item_ref(CursorPipeItem 
*item)
 static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem 
*item)
 {
 CursorPipeItem *cursor_pipe_item;
-spice_assert(item);
+
+spice_return_if_fail(item);
+
 cursor_pipe_item = SPICE_CONTAINEROF(item, CursorPipeItem, base);
 cursor_pipe_item_ref(cursor_pipe_item);
 }
@@ -383,6 +388,12 @@ CursorChannelClient* 
cursor_channel_client_new(CursorChannel *cursor, RedClient
uint32_t *common_caps, int 
num_common_caps,
uint32_t *caps, int num_caps)
 {
+spice_return_val_if_fail(cursor, NULL);
+spice_return_val_if_fail(client, NULL);
+spice_return_val_if_fail(stream, NULL);
+spice_return_val_if_fail(!num_common_caps || common_caps, NULL);
+spice_return_val_if_fail(!num_caps || caps, NULL);
+
 CursorChannelClient *ccc =
 (CursorChannelClient*)common_channel_new_client(&cursor->common,
 
sizeof(CursorChannelClient),
@@ -393,11 +404,11 @@ CursorChannelClient* 
cursor_channel_client_new(CursorChannel *cursor, RedClient
 num_common_caps,
 caps,
 num_caps);
-if (!ccc) {
-return NULL;
-}
+spice_return_val_if_fail(ccc != NULL, NULL);
+
 ring_init(&ccc->cursor_cache_lru);
 ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
+
 return ccc;
 }
 
@@ -431,7 +442,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
 cursor->cursor_trail_frequency = cursor_cmd->u.trail.frequency;
 break;
 default:
-spice_error("invalid cursor command %u", cursor_cmd->type);
+spice_warning("invalid cursor command %u", cursor_cmd->type);
+return;
 }
 
 if (red_channel_is_connected(&cursor->common.base) &&
diff --git a/server/red_worker.c b/server/red_worker.c
index c3b5c36..339b353 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -4169,7 +4169,7 @@ static int red_process_cursor(RedWorker *worker, uint32_t 
max_pipe_size, int *ri
 break;
 }
 default:
-spice_error("bad command type");
+spice_warning("bad command type");
 }
 n++;
 }
@@ -9769,19 +9769,15 @@ static void red_connect_cursor(RedWorker *worker, 
RedClient *client, RedsStream
 CursorChannel *channel;
 CursorChannelClient *ccc;
 
-if (worker->cursor_channel == NULL) {
-spice_warning("cursor channel was not created");
-return;
-}
+spice_return_if_fail(worker->cursor_channel != NULL);
+
 channel = worker->cursor_channel;
 spice_info("add cursor channel client");
 ccc = cursor_channel_client_new(channel, client, stream,
 migrate,
 common_caps, num_common_caps,
 caps, num_caps);
-if (!ccc) {
-return;
-}
+spice_return_if_fail(ccc != NULL);
 #ifdef RED_STATISTICS
 channel->stat = stat_add_node(worker->stat, "cursor_channel", TRUE);
 channel->common.base.out_bytes_counter = stat_add_counter(channel->stat, 
"out_bytes", TRUE);
@@ -10485,10 +10481,12 @@ void handle_dev_cursor_channel_create(void *opaque, 
void *payloa

[Spice-devel] [PATCH 03/10] Fix warning due to unexpected pipe item type

[Frediano Ziglio]

From: Marc-André Lureau 

The specific item type that was not being handled was
PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor
channel, but the analogous item for the display channel is
PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.

The exact warning follows:

 (/usr/bin/qemu-kvm:24458): Spice-Warning **:
 ../../server/dcc-send.c:2442:dcc_send_item: should not be reached
 (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
 ../../server/dcc.c:1595:release_item_before_push: invalid item type

Signed-off-by: Jonathon Jongsma 
---
 server/cache_item.tmpl.c |  4 +++-
 server/red_worker.c  | 15 ---
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c
index dc314c0..ad2b579 100644
--- a/server/cache_item.tmpl.c
+++ b/server/cache_item.tmpl.c
@@ -21,6 +21,7 @@
 #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY
 #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE
 #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE
+#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE
 #define FUNC_NAME(name) red_cursor_cache_##name
 #define VAR_NAME(name) cursor_cache_##name
 #define CHANNEL CursorChannel
@@ -32,6 +33,7 @@
 #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY
 #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE
 #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE
+#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE
 #define FUNC_NAME(name) red_palette_cache_##name
 #define VAR_NAME(name) palette_cache_##name
 #define CHANNEL DisplayChannel
@@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT *channel_client, 
CacheItem *item)
 channel_client->VAR_NAME(items)--;
 channel_client->VAR_NAME(available) += item->size;
 
-red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, 
PIPE_ITEM_TYPE_INVAL_ONE);
+red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, 
PIPE_ITEM_TYPE);
 red_channel_client_pipe_add_tail(&channel_client->common.base, 
&item->u.pipe_data); // for now
 }
 
diff --git a/server/red_worker.c b/server/red_worker.c
index 89cb25c..93a305a 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -7805,17 +7805,6 @@ static inline void 
marshall_qxl_drawable(RedChannelClient *rcc,
 red_lossy_marshall_qxl_drawable(display_channel->common.worker, rcc, 
m, dpi);
 }
 
-static inline void red_marshall_inval(RedChannelClient *rcc,
-  SpiceMarshaller *base_marshaller, 
CacheItem *cach_item)
-{
-SpiceMsgDisplayInvalOne inval_one;
-
-red_channel_client_init_send_data(rcc, cach_item->inval_type, NULL);
-inval_one.id = *(uint64_t *)&cach_item->id;
-
-spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one);
-}
-
 static void 
display_channel_marshall_migrate_data_surfaces(DisplayChannelClient *dcc,
SpiceMarshaller *m,
int lossy)
@@ -8271,9 +8260,6 @@ static void display_channel_send_item(RedChannelClient 
*rcc, PipeItem *pipe_item
 marshall_qxl_drawable(rcc, m, dpi);
 break;
 }
-case PIPE_ITEM_TYPE_INVAL_ONE:
-red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
-break;
 case PIPE_ITEM_TYPE_STREAM_CREATE: {
 StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, 
create_item);
 red_display_marshall_stream_start(rcc, m, agent);
@@ -9580,7 +9566,6 @@ static void 
display_channel_client_release_item_before_push(DisplayChannelClient
 free(item);
 break;
 }
-case PIPE_ITEM_TYPE_INVAL_ONE:
 case PIPE_ITEM_TYPE_VERB:
 case PIPE_ITEM_TYPE_MIGRATE_DATA:
 case PIPE_ITEM_TYPE_PIXMAP_SYNC:
-- 
2.4.3

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

[Spice-devel] [PATCH 10/10] server: move bitmap related to red_bitmap_utils

[Frediano Ziglio]

From: Marc-André Lureau 

---
 server/Makefile.am|   2 +
 server/red_bitmap_utils.c |  99 +
 server/red_bitmap_utils.h |  91 ++
 server/red_common.h   |  13 
 server/red_parse_qxl.c|   1 +
 server/red_worker.c   | 158 --
 server/tree.h |   9 +--
 7 files changed, 206 insertions(+), 167 deletions(-)
 create mode 100644 server/red_bitmap_utils.c
 create mode 100644 server/red_bitmap_utils.h

diff --git a/server/Makefile.am b/server/Makefile.am
index d2a7343..5d28e9e 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -133,6 +133,8 @@ libspice_server_la_SOURCES =\
pixmap-cache.c  \
tree.h  \
tree.c  \
+   red_bitmap_utils.h  \
+   red_bitmap_utils.c  \
utils.h \
$(NULL)
 
diff --git a/server/red_bitmap_utils.c b/server/red_bitmap_utils.c
new file mode 100644
index 000..d293dae
--- /dev/null
+++ b/server/red_bitmap_utils.c
@@ -0,0 +1,99 @@
+#include "red_bitmap_utils.h"
+
+#define RED_BITMAP_UTILS_RGB16
+#include "red_bitmap_utils_tmpl.c"
+#define RED_BITMAP_UTILS_RGB24
+#include "red_bitmap_utils_tmpl.c"
+#define RED_BITMAP_UTILS_RGB32
+#include "red_bitmap_utils_tmpl.c"
+
+#define GRADUAL_HIGH_RGB24_TH -0.03
+#define GRADUAL_HIGH_RGB16_TH 0
+
+// setting a more permissive threshold for stream identification in order
+// not to miss streams that were artificially scaled on the guest (e.g., full 
screen view
+// in window media player 12). see red_stream_add_frame
+#define GRADUAL_MEDIUM_SCORE_TH 0.002
+
+// assumes that stride doesn't overflow
+BitmapGradualType bitmap_get_graduality_level(SpiceBitmap *bitmap)
+{
+double score = 0.0;
+int num_samples = 0;
+int num_lines;
+double chunk_score = 0.0;
+int chunk_num_samples = 0;
+uint32_t x, i;
+SpiceChunk *chunk;
+
+chunk = bitmap->data->chunk;
+for (i = 0; i < bitmap->data->num_chunks; i++) {
+num_lines = chunk[i].len / bitmap->stride;
+x = bitmap->x;
+switch (bitmap->format) {
+case SPICE_BITMAP_FMT_16BIT:
+compute_lines_gradual_score_rgb16((rgb16_pixel_t *)chunk[i].data, 
x, num_lines,
+  &chunk_score, 
&chunk_num_samples);
+break;
+case SPICE_BITMAP_FMT_24BIT:
+compute_lines_gradual_score_rgb24((rgb24_pixel_t *)chunk[i].data, 
x, num_lines,
+  &chunk_score, 
&chunk_num_samples);
+break;
+case SPICE_BITMAP_FMT_32BIT:
+case SPICE_BITMAP_FMT_RGBA:
+compute_lines_gradual_score_rgb32((rgb32_pixel_t *)chunk[i].data, 
x, num_lines,
+  &chunk_score, 
&chunk_num_samples);
+break;
+default:
+spice_error("invalid bitmap format (not RGB) %u", bitmap->format);
+}
+score += chunk_score;
+num_samples += chunk_num_samples;
+}
+
+spice_assert(num_samples);
+score /= num_samples;
+
+if (bitmap->format == SPICE_BITMAP_FMT_16BIT) {
+if (score < GRADUAL_HIGH_RGB16_TH) {
+return BITMAP_GRADUAL_HIGH;
+}
+} else {
+if (score < GRADUAL_HIGH_RGB24_TH) {
+return BITMAP_GRADUAL_HIGH;
+}
+}
+
+if (score < GRADUAL_MEDIUM_SCORE_TH) {
+return BITMAP_GRADUAL_MEDIUM;
+} else {
+return BITMAP_GRADUAL_LOW;
+}
+}
+
+int bitmap_has_extra_stride(SpiceBitmap *bitmap)
+{
+spice_assert(bitmap);
+if (bitmap_fmt_is_rgb(bitmap->format)) {
+return ((bitmap->x * bitmap_fmt_get_bytes_per_pixel(bitmap->format)) < 
bitmap->stride);
+} else {
+switch (bitmap->format) {
+case SPICE_BITMAP_FMT_8BIT:
+return (bitmap->x < bitmap->stride);
+case SPICE_BITMAP_FMT_4BIT_BE:
+case SPICE_BITMAP_FMT_4BIT_LE: {
+int bytes_width = SPICE_ALIGN(bitmap->x, 2) >> 1;
+return bytes_width < bitmap->stride;
+}
+case SPICE_BITMAP_FMT_1BIT_BE:
+case SPICE_BITMAP_FMT_1BIT_LE: {
+int bytes_width = SPICE_ALIGN(bitmap->x, 8) >> 3;
+return bytes_width < bitmap->stride;
+}
+default:
+spice_error("invalid image type %u", bitmap->format);
+return 0;
+}
+}
+return 0;
+}
diff --git a/server/red_bitmap_utils.h b/server/red_bitmap_utils.h
new file mode 100644
index 000..38cb88a
--- /dev/null
+++ b/server/red_bitmap_utils.h
@@ -0,0 +1,91 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2009-2015 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the ter

[Spice-devel] [PATCH 06/10] server/red_worker: red_draw_qxl_drawable: protect from NULL dereference in case of buggy driver (or recording)

[Frediano Ziglio]

From: Alon Levy 

---
 server/red_worker.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/server/red_worker.c b/server/red_worker.c
index 339b353..868de94 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3838,6 +3838,11 @@ static void red_draw_qxl_drawable(RedWorker *worker, 
Drawable *drawable)
 
 image_cache_aging(&worker->image_cache);
 
+if (!canvas) {
+spice_warning("ignoring drawable to destroyed surface %d\n", 
drawable->surface_id);
+return;
+}
+
 region_add(&surface->draw_dirty_region, &drawable->red_drawable->bbox);
 
 switch (drawable->red_drawable->type) {
-- 
2.4.3

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

[Spice-devel] [PATCH 01/10] server: move display_channel_client_create() to dcc_new()

[Frediano Ziglio]

From: Marc-André Lureau 

Move function from server/red_worker.c to new server/display-channel.c.
---
 server/Makefile.am   |  1 +
 server/display-channel.c | 38 +++
 server/display-channel.h | 50 ---
 server/red_worker.c  | 68 +++-
 4 files changed, 89 insertions(+), 68 deletions(-)
 create mode 100644 server/display-channel.c

diff --git a/server/Makefile.am b/server/Makefile.am
index 87288cc..428417b 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -105,6 +105,7 @@ libspice_server_la_SOURCES =\
red_parse_qxl.h \
red_worker.c\
red_worker.h\
+   display-channel.c   \
display-channel.h   \
cursor-channel.c\
cursor-channel.h\
diff --git a/server/display-channel.c b/server/display-channel.c
new file mode 100644
index 000..00be665
--- /dev/null
+++ b/server/display-channel.c
@@ -0,0 +1,38 @@
+/*
+   Copyright (C) 2009-2015 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see .
+*/
+#include "display-channel.h"
+
+DisplayChannelClient *dcc_new(DisplayChannel *display,
+  RedClient *client, RedsStream *stream,
+  int mig_target,
+  uint32_t *common_caps, int num_common_caps,
+  uint32_t *caps, int num_caps)
+{
+DisplayChannelClient *dcc;
+
+dcc = (DisplayChannelClient*)common_channel_new_client(
+(CommonChannel *)display, sizeof(DisplayChannelClient),
+client, stream, mig_target, TRUE,
+common_caps, num_common_caps,
+caps, num_caps);
+spice_return_val_if_fail(dcc, NULL);
+
+ring_init(&dcc->palette_cache_lru);
+dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
+
+return dcc;
+}
diff --git a/server/display-channel.h b/server/display-channel.h
index e1ddc11..5d2eee5 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -15,14 +15,47 @@
You should have received a copy of the GNU Lesser General Public
License along with this library; if not, see .
 */
-#ifndef RED_WORKER_CLIENT_H_
-# define RED_WORKER_CLIENT_H_
+#ifndef DISPLAY_CHANNEL_H_
+# define DISPLAY_CHANNEL_H_
+
+#include 
 
 #include "red_worker.h"
+#include "reds_stream.h"
 #include "cache-item.h"
 #include "pixmap-cache.h"
+#ifdef USE_OPENGL
+#include "common/ogl_ctx.h"
+#include "reds_gl_canvas.h"
+#endif /* USE_OPENGL */
+#include "reds_sw_canvas.h"
+#include "glz_encoder_dictionary.h"
+#include "glz_encoder.h"
+#include "stat.h"
+#include "reds.h"
+#include "mjpeg_encoder.h"
+#include "red_memslots.h"
+#include "red_parse_qxl.h"
+#include "red_record_qxl.h"
+#include "jpeg_encoder.h"
+#ifdef USE_LZ4
+#include "lz4_encoder.h"
+#endif
+#include "demarshallers.h"
+#include "zlib_encoder.h"
+#include "red_channel.h"
+#include "red_dispatcher.h"
+#include "dispatcher.h"
+#include "main_channel.h"
+#include "migration_protocol.h"
+#include "main_dispatcher.h"
+#include "spice_server_utils.h"
+#include "spice_bitmap_utils.h"
+#include "spice_image_cache.h"
 #include "utils.h"
 
+typedef struct DisplayChannel DisplayChannel;
+
 typedef struct Drawable Drawable;
 
 #define PALETTE_CACHE_HASH_SHIFT 8
@@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
 #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
 #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
 
+#define CLIENT_PALETTE_CACHE_SIZE 128
+
 /* Each drawable can refer to at most 3 images: src, brush and mask */
 #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
 
@@ -193,4 +228,13 @@ struct DisplayChannelClient {
 uint64_t streams_max_bit_rate;
 };
 
-#endif /* RED_WORKER_CLIENT_H_ */
+DisplayChannelClient*  dcc_new   
(DisplayChannel *display,
+  
RedClient *client,
+  
RedsStream *stream,
+  int 
mig_target,
+  

[Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

[Frediano Ziglio]

From: Marc-André Lureau 

Doing so allows us to remove the extra QXLInstance parameter from
cursor_item_unref() and makes the code a bit cleaner.

Also add cursor_item_ref().

Signed-off-by: Jonathon Jongsma 
---
 server/cursor-channel.c | 70 +++--
 server/cursor-channel.h |  9 ---
 2 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index eef0121..d145f86 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -25,55 +25,58 @@
 #include "cache_item.tmpl.c"
 #undef CLIENT_CURSOR_CACHE
 
-static inline CursorItem *alloc_cursor_item(void)
+CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t 
group_id)
 {
 CursorItem *cursor_item;
 
+spice_return_val_if_fail(cmd != NULL, NULL);
+
 cursor_item = g_slice_new0(CursorItem);
+cursor_item->qxl = qxl;
 cursor_item->refs = 1;
+cursor_item->group_id = group_id;
+cursor_item->red_cursor = cmd;
 
 return cursor_item;
 }
 
-CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
+CursorItem *cursor_item_ref(CursorItem *item)
 {
-CursorItem *cursor_item;
-
-spice_return_val_if_fail(cmd != NULL, NULL);
-cursor_item = alloc_cursor_item();
+spice_return_val_if_fail(item != NULL, NULL);
+spice_return_val_if_fail(item->refs > 0, NULL);
 
-cursor_item->group_id = group_id;
-cursor_item->red_cursor = cmd;
+item->refs++;
 
-return cursor_item;
+return item;
 }
 
-void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
+void cursor_item_unref(CursorItem *item)
 {
-if (!--cursor->refs) {
-QXLReleaseInfoExt release_info_ext;
-RedCursorCmd *cursor_cmd;
-
-cursor_cmd = cursor->red_cursor;
-release_info_ext.group_id = cursor->group_id;
-release_info_ext.info = cursor_cmd->release_info;
-qxl->st->qif->release_resource(qxl, release_info_ext);
-red_put_cursor_cmd(cursor_cmd);
-free(cursor_cmd);
-
-g_slice_free(CursorItem, cursor);
-}
+QXLReleaseInfoExt release_info_ext;
+RedCursorCmd *cursor_cmd;
+
+spice_return_if_fail(item != NULL);
+
+if (--item->refs)
+return;
+
+cursor_cmd = item->red_cursor;
+release_info_ext.group_id = item->group_id;
+release_info_ext.info = cursor_cmd->release_info;
+item->qxl->st->qif->release_resource(item->qxl, release_info_ext);
+red_put_cursor_cmd(cursor_cmd);
+free(cursor_cmd);
+
+g_slice_free(CursorItem, item);
+
 }
 
 static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
 {
 if (cursor->item)
-cursor_item_unref(red_worker_get_qxl(cursor->common.worker), 
cursor->item);
-
-if (item)
-item->refs++;
+cursor_item_unref(cursor->item);
 
-cursor->item = item;
+cursor->item = item ? cursor_item_ref(item) : NULL;
 }
 
 static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int 
num)
@@ -157,7 +160,7 @@ static void put_cursor_pipe_item(CursorChannelClient *ccc, 
CursorPipeItem *pipe_
 
 spice_assert(!pipe_item_is_linked(&pipe_item->base));
 
-cursor_item_unref(red_worker_get_qxl(ccc->common.worker), 
pipe_item->cursor_item);
+cursor_item_unref(pipe_item->cursor_item);
 free(pipe_item);
 }
 
@@ -404,7 +407,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
 CursorItem *cursor_item;
 int cursor_show = FALSE;
 
-cursor_item = cursor_item_new(cursor_cmd, group_id);
+spice_return_if_fail(cursor);
+spice_return_if_fail(cursor_cmd);
+
+cursor_item = cursor_item_new(red_worker_get_qxl(cursor->common.worker),
+  cursor_cmd, group_id);
 
 switch (cursor_cmd->type) {
 case QXL_CURSOR_SET:
@@ -434,7 +441,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
 red_channel_pipes_new_add(&cursor->common.base,
   new_cursor_pipe_item, cursor_item);
 }
-cursor_item_unref(red_worker_get_qxl(cursor->common.worker), cursor_item);
+
+cursor_item_unref(cursor_item);
 }
 
 void cursor_channel_reset(CursorChannel *cursor)
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index 293cfc1..f20001c 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -33,6 +33,7 @@
 #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
 
 typedef struct CursorItem {
+QXLInstance *qxl;
 uint32_t group_id;
 int refs;
 RedCursorCmd *red_cursor;
@@ -83,14 +84,14 @@ void cursor_channel_reset   
(CursorChannel *cursor);
 void cursor_channel_process_cmd (CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
  uint32_t group_id);
 
-CursorItem*  cursor_item_new(RedCursorCmd *cmd, uint32_t 
group_id);
-void cursor_item_

[Spice-devel] [PATCH 04/10] Move pipe item enumerations out of red_worker.h

[Frediano Ziglio]

From: Marc-André Lureau 

Move the cursor-specific pipe item types to cursor-channel.h, and the
display-specific types to red_worker.c. Only leave the common
definitions in red_worker.h. This prepares for splitting the display
channel into a separate file.

Signed-off-by: Jonathon Jongsma 
---
 server/cursor-channel.h |  6 ++
 server/red_worker.c | 17 +
 server/red_worker.h | 21 +++--
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index f20001c..1639cf9 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -32,6 +32,12 @@
 #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
 #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
 
+enum {
+PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
+PIPE_ITEM_TYPE_CURSOR_INIT,
+PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
+};
+
 typedef struct CursorItem {
 QXLInstance *qxl;
 uint32_t group_id;
diff --git a/server/red_worker.c b/server/red_worker.c
index 93a305a..c3b5c36 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -228,6 +228,23 @@ struct SpiceWatch {
 void *watch_func_opaque;
 };
 
+enum {
+PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST,
+PIPE_ITEM_TYPE_IMAGE,
+PIPE_ITEM_TYPE_STREAM_CREATE,
+PIPE_ITEM_TYPE_STREAM_CLIP,
+PIPE_ITEM_TYPE_STREAM_DESTROY,
+PIPE_ITEM_TYPE_UPGRADE,
+PIPE_ITEM_TYPE_MIGRATE_DATA,
+PIPE_ITEM_TYPE_PIXMAP_SYNC,
+PIPE_ITEM_TYPE_PIXMAP_RESET,
+PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
+PIPE_ITEM_TYPE_CREATE_SURFACE,
+PIPE_ITEM_TYPE_DESTROY_SURFACE,
+PIPE_ITEM_TYPE_MONITORS_CONFIG,
+PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
+};
+
 #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS
 
 typedef struct SurfaceCreateItem {
diff --git a/server/red_worker.h b/server/red_worker.h
index 76502b6..aa97707 100644
--- a/server/red_worker.h
+++ b/server/red_worker.h
@@ -48,25 +48,10 @@ typedef struct CommonChannel {
 } CommonChannel;
 
 enum {
-PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE,
+PIPE_ITEM_TYPE_VERB = PIPE_ITEM_TYPE_CHANNEL_BASE,
 PIPE_ITEM_TYPE_INVAL_ONE,
-PIPE_ITEM_TYPE_CURSOR,
-PIPE_ITEM_TYPE_CURSOR_INIT,
-PIPE_ITEM_TYPE_IMAGE,
-PIPE_ITEM_TYPE_STREAM_CREATE,
-PIPE_ITEM_TYPE_STREAM_CLIP,
-PIPE_ITEM_TYPE_STREAM_DESTROY,
-PIPE_ITEM_TYPE_UPGRADE,
-PIPE_ITEM_TYPE_VERB,
-PIPE_ITEM_TYPE_MIGRATE_DATA,
-PIPE_ITEM_TYPE_PIXMAP_SYNC,
-PIPE_ITEM_TYPE_PIXMAP_RESET,
-PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
-PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
-PIPE_ITEM_TYPE_CREATE_SURFACE,
-PIPE_ITEM_TYPE_DESTROY_SURFACE,
-PIPE_ITEM_TYPE_MONITORS_CONFIG,
-PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
+
+PIPE_ITEM_TYPE_COMMON_LAST
 };
 
 typedef struct VerbItem {
-- 
2.4.3

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

[Spice-devel] [PATCH 09/10] tree: move that to a seperate unit

[Frediano Ziglio]

From: Marc-André Lureau 

---
 server/Makefile.am  |   2 +
 server/red_worker.c | 266 +++-
 server/tree.c   | 182 +++
 server/tree.h   | 111 ++
 4 files changed, 306 insertions(+), 255 deletions(-)
 create mode 100644 server/tree.c
 create mode 100644 server/tree.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 428417b..d2a7343 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -131,6 +131,8 @@ libspice_server_la_SOURCES =\
spice_image_cache.c \
pixmap-cache.h  \
pixmap-cache.c  \
+   tree.h  \
+   tree.c  \
utils.h \
$(NULL)
 
diff --git a/server/red_worker.c b/server/red_worker.c
index 601805e..0049cca 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -63,6 +63,8 @@
 #include "red_worker.h"
 #include "spice_timer_queue.h"
 #include "cursor-channel.h"
+#include "tree.h"
+#include "utils.h"
 
 //#define COMPRESS_STAT
 //#define DUMP_BITMAP
@@ -405,52 +407,6 @@ struct DisplayChannel {
 #endif
 };
 
-enum {
-TREE_ITEM_TYPE_DRAWABLE,
-TREE_ITEM_TYPE_CONTAINER,
-TREE_ITEM_TYPE_SHADOW,
-};
-
-typedef struct TreeItem {
-RingItem siblings_link;
-uint32_t type;
-struct Container *container;
-QRegion rgn;
-} TreeItem;
-
-#define IS_DRAW_ITEM(item) ((item)->type == TREE_ITEM_TYPE_DRAWABLE)
-
-typedef struct Shadow {
-TreeItem base;
-QRegion on_hold;
-struct DrawItem* owner;
-} Shadow;
-
-typedef struct Container {
-TreeItem base;
-Ring items;
-} Container;
-
-typedef struct DrawItem {
-TreeItem base;
-uint8_t effect;
-uint8_t container_root;
-Shadow *shadow;
-} DrawItem;
-
-typedef enum {
-BITMAP_GRADUAL_INVALID,
-BITMAP_GRADUAL_NOT_AVAIL,
-BITMAP_GRADUAL_LOW,
-BITMAP_GRADUAL_MEDIUM,
-BITMAP_GRADUAL_HIGH,
-} BitmapGradualType;
-
-typedef struct DependItem {
-Drawable *drawable;
-RingItem ring_item;
-} DependItem;
-
 typedef struct DrawablePipeItem {
 RingItem base;  /* link for a list of pipe items held by Drawable */
 PipeItem dpi_pipe_item; /* link for the client's pipe itself */
@@ -459,35 +415,6 @@ typedef struct DrawablePipeItem {
 uint8_t refs;
 } DrawablePipeItem;
 
-struct Drawable {
-uint8_t refs;
-RingItem surface_list_link;
-RingItem list_link;
-DrawItem tree_item;
-Ring pipes;
-PipeItem *pipe_item_rest;
-uint32_t size_pipe_item_rest;
-RedDrawable *red_drawable;
-
-Ring glz_ring;
-
-red_time_t creation_time;
-int frames_count;
-int gradual_frames_count;
-int last_gradual_frame;
-Stream *stream;
-Stream *sized_stream;
-int streamable;
-BitmapGradualType copy_bitmap_graduality;
-uint32_t group_id;
-DependItem depend_items[3];
-
-int surface_id;
-int surfaces_dest[3];
-
-uint32_t process_commands_generation;
-};
-
 typedef struct _Drawable _Drawable;
 struct _Drawable {
 union {
@@ -908,91 +835,6 @@ static inline int validate_surface(RedWorker *worker, 
uint32_t surface_id)
 return 1;
 }
 
-static const char *draw_type_to_str(uint8_t type)
-{
-switch (type) {
-case QXL_DRAW_FILL:
-return "QXL_DRAW_FILL";
-case QXL_DRAW_OPAQUE:
-return "QXL_DRAW_OPAQUE";
-case QXL_DRAW_COPY:
-return "QXL_DRAW_COPY";
-case QXL_DRAW_TRANSPARENT:
-return "QXL_DRAW_TRANSPARENT";
-case QXL_DRAW_ALPHA_BLEND:
-return "QXL_DRAW_ALPHA_BLEND";
-case QXL_COPY_BITS:
-return "QXL_COPY_BITS";
-case QXL_DRAW_BLEND:
-return "QXL_DRAW_BLEND";
-case QXL_DRAW_BLACKNESS:
-return "QXL_DRAW_BLACKNESS";
-case QXL_DRAW_WHITENESS:
-return "QXL_DRAW_WHITENESS";
-case QXL_DRAW_INVERS:
-return "QXL_DRAW_INVERS";
-case QXL_DRAW_ROP3:
-return "QXL_DRAW_ROP3";
-case QXL_DRAW_COMPOSITE:
-return "QXL_DRAW_COMPOSITE";
-case QXL_DRAW_STROKE:
-return "QXL_DRAW_STROKE";
-case QXL_DRAW_TEXT:
-return "QXL_DRAW_TEXT";
-default:
-return "?";
-}
-}
-
-static void show_red_drawable(RedWorker *worker, RedDrawable *drawable, const 
char *prefix)
-{
-if (prefix) {
-printf("%s: ", prefix);
-}
-
-printf("%s effect %d bbox(%d %d %d %d)",
-   draw_type_to_str(drawable->type),
-   drawable->effect,
-   drawable->bbox.top,
-   drawable->bbox.left,
-   drawable->bbox.bottom,
-   drawable->bbox.right);
-
-switch (drawable->type) {
-case QXL_DRAW_FILL:
-case QXL_DRAW_OPAQUE:
-case QXL_DRAW_COPY:
-case QXL_DRAW_TRANSPARENT:
-case QXL_DRAW_ALPHA_BLEND:
-case QXL_COPY_BITS:
-case QXL_DRAW_BLEND:
-case QXL_DRAW_BLACKNESS:
-case QXL_DRAW_WHITE

[Spice-devel] [ANNOUNCE] usbredir 0.7.1 release

[Victor Toso]

Hi all,

If no one objects, I'll be doing the 0.7.1 release shortly

* Changelogs for the release (acked by Hans)

usbredir-0.7.1   29 Oct 2015

-usbredirfilter
 -force check to device which had all interfaces skipped. This fix a bug
  which allow a KVM device to be redirect when it should not
-usbredirparser:
 -allow missing capabilities from source host when loading a USB
  redirection stream during a qemu migration
-usbredirhost:
 -new callback to drop isoc packets when application's pending writes buffer
  size is too big; The threshold calculation aims at 10fps as worst case to
  give at least 150ms of continuous data to application.

* Contribuitors to this release

Christophe Fergeau (3):
  Fix 'Inalid' typo in error messages
  protocol: Fix 'concatonated' typo in doc text
  Initialize usb_redir_hello_header to 0 in usbredirparser_init

Hans de Goede (2):
  usbredirhost: Add 2798:0001 to the do-not-reset blacklist
  usbredirserver/usbredirtestclient: Fix build on FreeBSD

Marc-André Lureau (2):
  build-sys: learn to autogen out-of-tree
  build-sys: add git.mk to generate .gitignore

Victor Toso (2):
  usbredirhost: new callback to drop isoc packets
  Prepare for 0.7.1 release

Dr. David Alan Gilbert (1):
  Allow missing capabilities from source host

Fabiano Fidêncio (1):
  usbredirtestclient: Fix a memory leak

Uri Lublin (1):
  usbredirfilter_check: force check a device if all its interfaces are 
skipped

Cheers,
  Victor Toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel