Re: [Spice-devel] [PATCH v2] fixup! Change Drawable->pipes from Ring to GList

[Jonathon Jongsma]

On Fri, 2016-09-16 at 12:57 -0400, Frediano Ziglio wrote:
> > 
> > 
> > Fix potential infinite loop
> > ---
> >  server/stream.c | 11 ---
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/stream.c b/server/stream.c
> > index 934b236..49b5910 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -331,11 +331,10 @@ void detach_stream(DisplayChannel *display,
> > Stream
> > *stream)
> >  static void before_reattach_stream(DisplayChannel *display,
> > Stream *stream, Drawable
> > *new_frame)
> >  {
> > -RedDrawablePipeItem *dpi;
> >  DisplayChannelClient *dcc;
> >  int index;
> >  StreamAgent *agent;
> > -GList *dpi_item;
> > +GList *dpi_link, *dpi_next;
> >  GList *link, *link_next;
> >  
> >  spice_return_if_fail(stream->current);
> > @@ -350,10 +349,9 @@ static void
> > before_reattach_stream(DisplayChannel
> > *display,
> >  }
> >  
> >  index = display_channel_get_stream_id(display, stream);
> > -dpi_item = stream->current->pipes;
> > -while (dpi_item) {
> > -GList *dpi_next = dpi_item->next;
> > -dpi = dpi_item->data;
> > +for (dpi_link = stream->current->pipes; dpi_link; dpi_link =
> > dpi_next) {
> > +RedDrawablePipeItem *dpi = dpi_link->data;
> > +dpi_next = dpi_link->next;
> >  dcc = dpi->dcc;
> >  agent = dcc_get_stream_agent(dcc, index);
> >  
> > @@ -373,7 +371,6 @@ static void
> > before_reattach_stream(DisplayChannel
> > *display,
> >  ++agent->drops;
> >  }
> >  }
> > -dpi_item = dpi_next;
> >  }
> >  
> >  
> 
> Acked-by: Frediano Ziglio 
> 
> After all these fixup and changes I got this
> https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=jj2
> (just if you want to check)
> 
> I think we should be in a good state with the acks too.
> 
> Frediano

Here's what I had: https://cgit.freedesktop.org/~jjongsma/spice-server/
log/?h=refactory-review

I had already squashed a couple of your fixup patches. It seems that
there is one difference between our branches. A change that Pavel
suggested:


$ git diff fziglio/jj2
diff --git a/server/dcc.c b/server/dcc.c
index 486d31b..f0a1ef8 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -21,7 +21,6 @@
 
 #include "dcc-private.h"
 #include "display-channel.h"
-#include "dcc.h"
 #include "red-channel-client-private.h"
 
 #define DISPLAY_CLIENT_SHORT_TIMEOUT 150ULL //nano
diff --git a/server/display-channel.c b/server/display-channel.c
index c12020f..cd9c937 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2065,15 +2065,15 @@ gboolean
display_channel_validate_surface(DisplayChannel *display, uint32_t surf
 {
 if SPICE_UNLIKELY(surface_id >= display->priv->n_surfaces) {
 spice_warning("invalid surface_id %u", surface_id);
-return 0;
+return FALSE;
 }
 if (!display->priv->surfaces[surface_id].context.canvas) {
 spice_warning("canvas address is %p for %d (and is NULL)\n",
&(display->priv-
>surfaces[surface_id].context.canvas), surface_id);
 spice_warning("failed on %d", surface_id);
-return 0;
+return FALSE;
 }
-return 1;
+return TRUE;
 }
 
 void display_channel_update_monitors_config(DisplayChannel *display,

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

Re: [Spice-devel] [PATCH v2] fixup! Change Drawable->pipes from Ring to GList

[Frediano Ziglio]

> 
> Fix potential infinite loop
> ---
>  server/stream.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/server/stream.c b/server/stream.c
> index 934b236..49b5910 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -331,11 +331,10 @@ void detach_stream(DisplayChannel *display, Stream
> *stream)
>  static void before_reattach_stream(DisplayChannel *display,
> Stream *stream, Drawable *new_frame)
>  {
> -RedDrawablePipeItem *dpi;
>  DisplayChannelClient *dcc;
>  int index;
>  StreamAgent *agent;
> -GList *dpi_item;
> +GList *dpi_link, *dpi_next;
>  GList *link, *link_next;
>  
>  spice_return_if_fail(stream->current);
> @@ -350,10 +349,9 @@ static void before_reattach_stream(DisplayChannel
> *display,
>  }
>  
>  index = display_channel_get_stream_id(display, stream);
> -dpi_item = stream->current->pipes;
> -while (dpi_item) {
> -GList *dpi_next = dpi_item->next;
> -dpi = dpi_item->data;
> +for (dpi_link = stream->current->pipes; dpi_link; dpi_link = dpi_next) {
> +RedDrawablePipeItem *dpi = dpi_link->data;
> +dpi_next = dpi_link->next;
>  dcc = dpi->dcc;
>  agent = dcc_get_stream_agent(dcc, index);
>  
> @@ -373,7 +371,6 @@ static void before_reattach_stream(DisplayChannel
> *display,
>  ++agent->drops;
>  }
>  }
> -dpi_item = dpi_next;
>  }
>  
>  

Acked-by: Frediano Ziglio 

After all these fixup and changes I got this
https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=jj2
(just if you want to check)

I think we should be in a good state with the acks too.

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

[Spice-devel] [PATCH v2] fixup! Change Drawable->pipes from Ring to GList

[Jonathon Jongsma]

Fix potential infinite loop
---
 server/stream.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/server/stream.c b/server/stream.c
index 934b236..49b5910 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -331,11 +331,10 @@ void detach_stream(DisplayChannel *display, Stream 
*stream)
 static void before_reattach_stream(DisplayChannel *display,
Stream *stream, Drawable *new_frame)
 {
-RedDrawablePipeItem *dpi;
 DisplayChannelClient *dcc;
 int index;
 StreamAgent *agent;
-GList *dpi_item;
+GList *dpi_link, *dpi_next;
 GList *link, *link_next;
 
 spice_return_if_fail(stream->current);
@@ -350,10 +349,9 @@ static void before_reattach_stream(DisplayChannel *display,
 }
 
 index = display_channel_get_stream_id(display, stream);
-dpi_item = stream->current->pipes;
-while (dpi_item) {
-GList *dpi_next = dpi_item->next;
-dpi = dpi_item->data;
+for (dpi_link = stream->current->pipes; dpi_link; dpi_link = dpi_next) {
+RedDrawablePipeItem *dpi = dpi_link->data;
+dpi_next = dpi_link->next;
 dcc = dpi->dcc;
 agent = dcc_get_stream_agent(dcc, index);
 
@@ -373,7 +371,6 @@ static void before_reattach_stream(DisplayChannel *display,
 ++agent->drops;
 }
 }
-dpi_item = dpi_next;
 }
 
 
-- 
2.7.4

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

Re: [Spice-devel] [PATCH] fixup! Change Drawable->pipes from Ring to GList

[Jonathon Jongsma]

On Fri, 2016-09-16 at 12:25 -0400, Frediano Ziglio wrote:
> > 
> > 
> > Fix potential infinite loop
> > ---
> > You can still introduce a foreach macro if you'd like, but this
> > should fix
> > the
> > infinite loop for now.
> > 
> 
> Macro could be introduced back later if we decide to.
> 
> > 
> >  server/stream.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/stream.c b/server/stream.c
> > index 934b236..be3e437 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -331,7 +331,6 @@ void detach_stream(DisplayChannel *display,
> > Stream
> > *stream)
> >  static void before_reattach_stream(DisplayChannel *display,
> > Stream *stream, Drawable
> > *new_frame)
> >  {
> > -RedDrawablePipeItem *dpi;
> >  DisplayChannelClient *dcc;
> >  int index;
> >  StreamAgent *agent;
> > @@ -352,8 +351,8 @@ static void
> > before_reattach_stream(DisplayChannel
> > *display,
> >  index = display_channel_get_stream_id(display, stream);
> >  dpi_item = stream->current->pipes;
> >  while (dpi_item) {
> > -GList *dpi_next = dpi_item->next;
> > -dpi = dpi_item->data;
> > +RedDrawablePipeItem *dpi = dpi_item->data;
> > +dpi_item = dpi_item->next;
> >  dcc = dpi->dcc;
> >  agent = dcc_get_stream_agent(dcc, index);
> >  
> 
> Wouldn't a
> 
>    for (; dpi_item; dpi_item = dpi_next) 
> 
> be enough (moving dpi_next declaration outside) ?
> 
> Having dpi_item as the next item could be confusing as it's used
> for the current item in other contexts.

> On the same naming dpi_item is not a dpi (RedDrawablePipeItem) which
> could be confusing too. A dpi_link perhaps would be better or reuse
> directly the link/link_next variable.

Sure, that could work as well. I just used this approach because it was
a smaller change. I did have the same concerns you mentioned and
dismissed them at the time. But I'll make the change.



> 
> > 
> > @@ -373,7 +372,6 @@ static void
> > before_reattach_stream(DisplayChannel
> > *display,
> >  ++agent->drops;
> >  }
> >  }
> > -dpi_item = dpi_next;
> >  }
> >  
> 
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] fixup! Change Drawable->pipes from Ring to GList

[Frediano Ziglio]

> 
> Fix potential infinite loop
> ---
> You can still introduce a foreach macro if you'd like, but this should fix
> the
> infinite loop for now.
> 

Macro could be introduced back later if we decide to.

>  server/stream.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/server/stream.c b/server/stream.c
> index 934b236..be3e437 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -331,7 +331,6 @@ void detach_stream(DisplayChannel *display, Stream
> *stream)
>  static void before_reattach_stream(DisplayChannel *display,
> Stream *stream, Drawable *new_frame)
>  {
> -RedDrawablePipeItem *dpi;
>  DisplayChannelClient *dcc;
>  int index;
>  StreamAgent *agent;
> @@ -352,8 +351,8 @@ static void before_reattach_stream(DisplayChannel
> *display,
>  index = display_channel_get_stream_id(display, stream);
>  dpi_item = stream->current->pipes;
>  while (dpi_item) {
> -GList *dpi_next = dpi_item->next;
> -dpi = dpi_item->data;
> +RedDrawablePipeItem *dpi = dpi_item->data;
> +dpi_item = dpi_item->next;
>  dcc = dpi->dcc;
>  agent = dcc_get_stream_agent(dcc, index);
>  

Wouldn't a

   for (; dpi_item; dpi_item = dpi_next) 

be enough (moving dpi_next declaration outside) ?

Having dpi_item as the next item could be confusing as it's used
for the current item in other contexts.

On the same naming dpi_item is not a dpi (RedDrawablePipeItem) which
could be confusing too. A dpi_link perhaps would be better or reuse
directly the link/link_next variable.

> @@ -373,7 +372,6 @@ static void before_reattach_stream(DisplayChannel
> *display,
>  ++agent->drops;
>  }
>  }
> -dpi_item = dpi_next;
>  }
>  

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

[Spice-devel] Necessary code ??

[Frediano Ziglio]

Hi,
  I came to this code in dcc.c (dcc_clear_surface_drawables_from_pipe function):

/*
 * in case that the pipe didn't contain any item that is dependent on the 
surface, but
 * there is one during sending. Use a shorter timeout, since it is just one 
item
 */
return red_channel_client_wait_outgoing_item(rcc, 
DISPLAY_CLIENT_SHORT_TIMEOUT);

This function is called when there are no items in the pipe related to the 
surface
specified (as the comment says).
However I really don't understand why the call is needed.
When an item is extracted from the queue send_item is called then the message is
marshalled and sent to the client. Now all structure should be freed and the 
message
would be send in any case to the client so what's the purpose of waiting?
I understood that all this wait is for making sure nobody is using some 
structure.
Is this related to urgent data (actually free list sent by the DisplayChannel) ?
This urgent handling is quite confusing, wouldn't be enough to insert the 
current
item back into the queue and then all urgent data in front too instead of using
all these fields?

Is somebody confident with these part of the code?

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

[Spice-devel] [PATCH] fixup! Change Drawable->pipes from Ring to GList

[Jonathon Jongsma]

Fix potential infinite loop
---
You can still introduce a foreach macro if you'd like, but this should fix the
infinite loop for now.

 server/stream.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/server/stream.c b/server/stream.c
index 934b236..be3e437 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -331,7 +331,6 @@ void detach_stream(DisplayChannel *display, Stream *stream)
 static void before_reattach_stream(DisplayChannel *display,
Stream *stream, Drawable *new_frame)
 {
-RedDrawablePipeItem *dpi;
 DisplayChannelClient *dcc;
 int index;
 StreamAgent *agent;
@@ -352,8 +351,8 @@ static void before_reattach_stream(DisplayChannel *display,
 index = display_channel_get_stream_id(display, stream);
 dpi_item = stream->current->pipes;
 while (dpi_item) {
-GList *dpi_next = dpi_item->next;
-dpi = dpi_item->data;
+RedDrawablePipeItem *dpi = dpi_item->data;
+dpi_item = dpi_item->next;
 dcc = dpi->dcc;
 agent = dcc_get_stream_agent(dcc, index);
 
@@ -373,7 +372,6 @@ static void before_reattach_stream(DisplayChannel *display,
 ++agent->drops;
 }
 }
-dpi_item = dpi_next;
 }
 
 
-- 
2.7.4

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

Re: [Spice-devel] [PATCH] fixup! RedChannelClient: store pipe items in a GQueue

[Jonathon Jongsma]

On Fri, 2016-09-16 at 10:05 +0100, Frediano Ziglio wrote:
> Minor changes:
> - use same name for dcc_add_surface_area_image argument in header
>   and source;
> - avoid using 2 variable in a for loop, is not much readable and
>   confusing. This also was fixing a regression quite hard to spot
>   so make sure code is less easy to break in the future;
> - remove check in red_drawable_pipe_item_free. Now
> RedDrawablePipeItem
>   is not forced to know in which container is it and the check should
>   be up to container. Also this was potentially O(n) expensive with
>   the GQueue implementation.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dcc.c | 31 +++
>  server/dcc.h |  2 +-
>  2 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index 184a944..cca3ce5 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -68,7 +68,7 @@ int dcc_drawable_is_in_pipe(DisplayChannelClient
> *dcc, Drawable *drawable)
>  int dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc,
> int surface_id,
>    int wait_if_used)
>  {
> -GList *l, *item_pos = NULL;
> +GList *l;
>  int x;
>  RedChannelClient *rcc;
>  
> @@ -77,13 +77,13 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
> surface
> no other drawable depends on them */
>  
>  rcc = RED_CHANNEL_CLIENT(dcc);
> -for (l = rcc->priv->pipe.head; l != NULL; item_pos = NULL) {
> +for (l = rcc->priv->pipe.head; l != NULL; ) {
>  Drawable *drawable;
>  RedDrawablePipeItem *dpi = NULL;
>  int depend_found = FALSE;
>  RedPipeItem *item = l->data;
> +GList *item_pos = l;
>  
> -item_pos = l;
>  l = l->next;
>  if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
>  dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem,
> dpi_pipe_item);
> @@ -108,11 +108,11 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
> surface
>  
>  if (depend_found) {
>  spice_debug("surface %d dependent item found %p, %p",
> surface_id, drawable, item);
> -if (wait_if_used) {
> -break;
> -} else {
> +if (!wait_if_used) {
>  return TRUE;
>  }
> +return red_channel_client_wait_pipe_item_sent(rcc,
> item_pos,
> +  COMMON_CLI
> ENT_TIMEOUT);
>  }
>  }
>  
> @@ -120,18 +120,11 @@ int
> dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
> surface
>  return TRUE;
>  }
>  
> -if (item_pos) {
> -return
> red_channel_client_wait_pipe_item_sent(RED_CHANNEL_CLIENT(dcc),
> item_pos,
> -  COMMON_CLIENT_
> TIMEOUT);
> -} else {
> -/*
> - * in case that the pipe didn't contain any item that is
> dependent on the surface, but
> - * there is one during sending. Use a shorter timeout, since
> it is just one item
> - */
> -return
> red_channel_client_wait_outgoing_item(RED_CHANNEL_CLIENT(dcc),
> - DISPLAY_CLIENT_
> SHORT_TIMEOUT);
> -}
> -return TRUE;
> +/*
> + * in case that the pipe didn't contain any item that is
> dependent on the surface, but
> + * there is one during sending. Use a shorter timeout, since it
> is just one item
> + */
> +return red_channel_client_wait_outgoing_item(rcc,
> DISPLAY_CLIENT_SHORT_TIMEOUT);
>  }
>  
>  void dcc_create_surface(DisplayChannelClient *dcc, int surface_id)
> @@ -284,8 +277,6 @@ static void
> red_drawable_pipe_item_free(RedPipeItem *item)
>   dpi_pipe_item);
>  spice_assert(item->refcount == 0);
>  
> -spice_warn_if_fail(!red_channel_client_pipe_item_is_linked(RED_C
> HANNEL_CLIENT(dpi->dcc),
> -   
> >dpi_pipe_item));
>  if (ring_item_is_linked(>base)) {
>  ring_remove(>base);
>  }
> diff --git a/server/dcc.h b/server/dcc.h
> index 7f93186..a2478b9 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -129,7 +129,7 @@
> void   dcc_push_surface_image
> (DisplayCha
>  RedImageItem
> * dcc_add_surface_area_image(DisplayChann
> elClient *dcc,
>  
>   int surface_id,
>  
>   SpiceRect *area,
> -
>   GList *pos,
> +
>   GList *pipe_item_pos,
>  
>   int can_lossy);
>  

Re: [Spice-devel] [PATCH v2 8/9] Change RedCharDevice::write_queue to GQueue

[Jonathon Jongsma]

On Thu, 2016-09-15 at 14:51 -0500, Jonathon Jongsma wrote:
> On Thu, 2016-09-15 at 16:55 +0200, Pavel Grunt wrote:
> > 
> > Hey,
> > 
> > On Wed, 2016-09-14 at 11:53 -0500, Jonathon Jongsma wrote:
> > > 
> > > 
> > > Change a couple more Rings to GQueue
> > > ---
> > > Changes in v2:
> > >  - use GQueue, not GList
> > > 
> > >  server/char-device.c | 79 +-
> > > -
> > > -
> > > 
> 
> 
> > 
> > > 
> > > @@ -1119,8 +1104,8 @@ red_char_device_finalize(GObject *object)
> > >  reds_core_timer_remove(self->priv->reds, self->priv-
> > > > 
> > > > 
> > > > write_to_dev_timer);
> > >  self->priv->write_to_dev_timer = NULL;
> > >  }
> > > -write_buffers_queue_free(>priv->write_queue);
> > > -write_buffers_queue_free(>priv->write_bufs_pool);
> > > +write_buffers_queue_free(self->priv->write_queue);
> > > +write_buffers_queue_free(self->priv->write_bufs_pool);
> > 
> > Did you consider using g_queue_free_full() ?
> > 
> > Thanks,
> > Pavel
> > 
> 
> 
> I did consider it, but that would free the GQueue structure as well
> as
> all of the elements in the queue. I don't want to do that here. We
> want
> a valid GQueue, but it should be empty.
> 

I wrote a follow-up to this yesterday but it seems I forgot to send
it? 

Anyway, I misspoke here. I was thinking of a different code location
where I was considerin g_queue_free_full. I could indeed have used it
here. But you've probably noticed that my updated patch switched to
storing the GQueue in the struct by value rather by pointer. So it no
longer needs to be freed independantly.

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

[Spice-devel] [RFC PATCH 2/2] Introduce some macro to simplify iteration on GList

[Frediano Ziglio]

Noting that coding by hand these loop introduced some regression
I'm trying to introduce back from macros.
Before trying something harder to make possible to bind the type of
the content I'm trying some simple macro as were before.
I added the type to avoid some blindly void* casts.
Also the GListIter is introduced to avoid the possibility to exchange
easily some parameters.

Signed-off-by: Frediano Ziglio 
---
 server/red-common.h   | 18 ++
 server/reds-private.h |  3 +++
 server/reds.c | 66 +--
 3 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/server/red-common.h b/server/red-common.h
index 7ab7e15..190fd9c 100644
--- a/server/red-common.h
+++ b/server/red-common.h
@@ -62,4 +62,22 @@ extern const SpiceCoreInterfaceInternal event_loop_core;
 
 typedef struct RedsState RedsState;
 
+typedef struct GListIter {
+GList *link;
+GList *next;
+} GListIter;
+
+#define GLIST_FOREACH_GENERIC(_list, _iter, _type, _data, _dir) \
+for (_iter.link = _list; \
+(_data = (_type *) (_iter.link ? _iter.link->data : NULL), \
+ _iter.next = (_iter.link ? _iter.link->_dir : NULL), \
+ _iter.link) != NULL; \
+ _iter.link = _iter.next)
+
+#define GLIST_FOREACH(_list, _iter, _type, _data) \
+GLIST_FOREACH_GENERIC(_list, _iter, _type, _data, next)
+
+#define GLIST_FOREACH_REVERSED(_list, _iter, _type, _data) \
+GLIST_FOREACH_GENERIC(_list, _iter, _type, _data, prev)
+
 #endif
diff --git a/server/reds-private.h b/server/reds-private.h
index 29645bf..a044a57 100644
--- a/server/reds-private.h
+++ b/server/reds-private.h
@@ -156,4 +156,7 @@ struct RedsState {
 MainDispatcher *main_dispatcher;
 };
 
+#define FOREACH_QXL_INSTANCE(_reds, _iter, _qxl) \
+GLIST_FOREACH(_reds->qxl_instances, _iter, QXLInstance, _qxl)
+
 #endif
diff --git a/server/reds.c b/server/reds.c
index 8e9dc7b..1321797 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -688,15 +688,15 @@ int reds_get_mouse_mode(RedsState *reds)
 
 static void reds_set_mouse_mode(RedsState *reds, uint32_t mode)
 {
-GList *l;
+GListIter it;
+QXLInstance *qxl;
 
 if (reds->mouse_mode == mode) {
 return;
 }
 reds->mouse_mode = mode;
 
-for (l = reds->qxl_instances; l != NULL; l = l->next) {
-QXLInstance *qxl = (QXLInstance *)l->data;
+FOREACH_QXL_INSTANCE(reds, it, qxl) {
 red_qxl_set_mouse_mode(qxl, mode);
 }
 
@@ -1717,14 +1717,11 @@ static void reds_mig_target_client_free(RedsState 
*reds, RedsMigTargetClient *mi
 
 static void reds_mig_target_client_disconnect_all(RedsState *reds)
 {
-GList *l, *next;
+GListIter it;
+RedsMigTargetClient *mig_client;
 
-l = reds->mig_target_clients;
-while (l) {
-next = l->next;
-RedsMigTargetClient *mig_client = l->data;
+GLIST_FOREACH(reds->mig_target_clients, it, RedsMigTargetClient, 
mig_client) {
 reds_client_disconnect(reds, mig_client->client);
-l = next;
 }
 }
 
@@ -4315,13 +4312,14 @@ void reds_update_client_mouse_allowed(RedsState *reds)
 int allow_now = FALSE;
 int x_res = 0;
 int y_res = 0;
-GList *l;
 int num_active_workers = g_list_length(reds->qxl_instances);
 
 if (num_active_workers > 0) {
+GListIter it;
+QXLInstance *qxl;
+
 allow_now = TRUE;
-for (l = reds->qxl_instances; l != NULL && allow_now; l = l->next) {
-QXLInstance *qxl = l->data;
+FOREACH_QXL_INSTANCE(reds, it, qxl) {
 if (red_qxl_get_primary_active(qxl)) {
 allow_now = red_qxl_get_allow_client_mouse(qxl, _res, 
_res);
 break;
@@ -4342,15 +4340,14 @@ void reds_update_client_mouse_allowed(RedsState *reds)
 
 static gboolean reds_use_client_monitors_config(RedsState *reds)
 {
-GList *l;
+GListIter it;
+QXLInstance *qxl;
 
 if (reds->qxl_instances == NULL) {
 return FALSE;
 }
 
-for (l = reds->qxl_instances; l != NULL ; l = l->next) {
-QXLInstance *qxl = l->data;
-
+FOREACH_QXL_INSTANCE(reds, it, qxl) {
 if (!red_qxl_use_client_monitors_config(qxl))
 return FALSE;
 }
@@ -4359,10 +4356,10 @@ static gboolean 
reds_use_client_monitors_config(RedsState *reds)
 
 static void reds_client_monitors_config(RedsState *reds, VDAgentMonitorsConfig 
*monitors_config)
 {
-GList *l;
+GListIter it;
+QXLInstance *qxl;
 
-for (l = reds->qxl_instances; l != NULL; l = l->next) {
-QXLInstance *qxl = l->data;
+FOREACH_QXL_INSTANCE(reds, it, qxl) {
 if (!red_qxl_client_monitors_config(qxl, monitors_config)) {
 /* this is a normal condition, some qemu devices might not 
implement it */
 spice_debug("QXLInterface::client_monitors_config failed\n");
@@ -4385,10 +4382,10 @@ static int calc_compression_level(RedsState *reds)
 void reds_on_ic_change(RedsState *reds)
 {
 int 

Re: [Spice-devel] [PATCH 2/2] Optimise client pipe passing pipe position instead of data

[Frediano Ziglio]

> 
> On Wed, 2016-09-14 at 06:30 -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > I haven't looked very carefully at the changes, but a few comments
> > > about readability:
> > > 
> > > On Mon, Sep 12, 2016 at 04:32:03PM +0100, Frediano Ziglio wrote:
> > > > 
> > > > This avoid to have the search the data scanning all the
> > > > queue changing the order of these operations from O(n) to O(1).
> > > > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  server/dcc-send.c   |  9 -
> > > >  server/dcc.c| 20 +---
> > > >  server/dcc.h|  2 +-
> > > >  server/red-channel-client.c | 32 +
> > > > ---
> > > >  server/red-channel-client.h |  4 +++-
> > > >  5 files changed, 42 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/server/dcc.c b/server/dcc.c
> > > > index 0d672df..ece37d9 100644
> > > > --- a/server/dcc.c
> > > > +++ b/server/dcc.c
> > > > @@ -78,15 +77,14 @@ int
> > > > dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc,
> > > > int
> > > > surface
> > > > no other drawable depends on them */
> > > >  
> > > >  rcc = RED_CHANNEL_CLIENT(dcc);
> > > > -l = rcc->priv->pipe.head;
> > > > -while (l != NULL) {
> > > > -GList *cur = l;
> > > > +for (l = rcc->priv->pipe.head; l != NULL; item_pos = NULL) {
> > > 
> > > Please no :) the while() version might be better. The  for()
> > > version
> > > with the loop index being updated within the loop, but only after
> > > doing
> > > a bit of work with the previous value of the loop index is really
> > > unexpected.
> > > 
> > 
> > What can I say: I give up!
> 
> Give up? Why?
> 
> > 
> > This patch is not related to GQueue nor to optimization.
> > This fixes a regression introduced in the first patch.
> 
> Which patch was the regression introduced in? Was that patch merged
> already?
> 

Was introduced by one GList (now fixed, was not integrated).
Specifically was dcc_clear_surface_drawables_from_pipe.
In the initial code (before GList/GQueue) the item and iterator was represented
by the same variable item so being a single variable by definition was
coherent to itself. When GList was introduced there were 2 variable,
l and item_pos. Unfortunately after the loop item was used by not
being coherent with l at that time a regression was introduced.

> > I spotted this regression the first time (some months ago) this
> > patch was posted on the mailing list but no matter saying that some
> > path touched some sensible code that required more attention, no
> > matter all my consideration about complexity no one pick up again
> > these patches and noted the issue.
> 
> I'm afraid that I don't remember a discussion from some months ago. If
> there was a regression introduced by a patch that has already been
> merged, then you should feel free to ping for review if something was
> not getting reviewed. I wasn't aware of any regressions.
> 
> On the other hand, if the regression was introduced in one of the
> patches that are still under review, then I don't quite understand your
> frustration. Isn't finding and fixing regressions part of what a review
> is for?
> 

Honestly I really don't understand why this was not spotted just by me.
These "exercises" are usually used in interviews from junior level and
didn't expect to take so much time and effort so I was really upset of
the few attention taken for these patches even when some changes are
touching sensible piece of software but probably my way to see the code
make quite easy for me to spot these issue that I found other people
not finding them a bit of a joke.
Maybe also some other frustration not related specifically to this.

> 
> > 
> > Now I could think different causes:
> > - we are too tired of these patches and we don't pay much attention;
> > - we look more at style than behavior;
> > - we are too busy to spend too much time on the review.
> > Any of the reason do not seem really a good reason to me.
> > Any other reason I don't see?
> > 
> 
> Do you have suggestions for improving things?
> 
> Jonathon
> 

I still think that more attention should be done to critical piece
of software.
On the code part I'm trying to introduce back some macro to avoid
manually mistake iterating lists.
On the long run I had different changes and branches to integrate
mostly born extending some changes introduced in different patches.
Just an example of "extension" now that RedPipeItem has no link in
it... it's not more a PipeItem and all classes should probably be
renamed, not counting the changes that can be made on multiple
clients (a subject we should probably discuss about).

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

[Spice-devel] [RFC PATCH 1/2] RedChannel: Add FOREACH_CHANNEL and use it to iterate

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel.c | 50 +++---
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 474fe68..166221e 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -31,6 +31,13 @@
 #include "main-dispatcher.h"
 #include "utils.h"
 
+#define FOREACH_CHANNEL(client, _link, _next, _data)   \
+for (_link = (client ? (client)->channels : NULL); \
+ (_data = (_link ? _link->data : NULL), \
+ _next = (_link ? _link->next : NULL), \
+ _link) != NULL; \
+ _link = _next)
+
 /*
  * Lifetime of RedChannel, RedChannelClient and RedClient:
  * RedChannel is created and destroyed by the calls to
@@ -483,14 +490,13 @@ void red_channel_apply_clients_data(RedChannel *channel, 
channel_client_callback
 
 int red_channel_all_blocked(RedChannel *channel)
 {
-GList *link;
+GList *link, *next;
 RedChannelClient *rcc;
 
 if (!channel || !channel->clients) {
 return FALSE;
 }
-for (link = channel->clients; link != NULL; link = link->next) {
-rcc = link->data;
+FOREACH_CLIENT(channel, link, next, rcc) {
 if (!red_channel_client_is_blocked(rcc)) {
 return FALSE;
 }
@@ -577,14 +583,16 @@ RedClient *red_client_unref(RedClient *client)
 
 void red_client_set_migration_seamless(RedClient *client) // dest
 {
-GList *link;
+GList *link, *next;
+RedChannelClient *rcc;
+
 spice_assert(client->during_target_migrate);
 pthread_mutex_lock(>lock);
 client->seamless_migrate = TRUE;
 /* update channel clients that got connected before the migration
  * type was set. red_client_add_channel will handle newer channel clients 
*/
-for (link = client->channels; link != NULL; link = link->next) {
-if (red_channel_client_set_migration_seamless(link->data))
+FOREACH_CHANNEL(client, link, next, rcc) {
+if (red_channel_client_set_migration_seamless(rcc))
 client->num_migrated_channels++;
 }
 pthread_mutex_unlock(>lock);
@@ -603,15 +611,11 @@ void red_client_migrate(RedClient *client)
   " this might be a BUG",
   client->thread_id, pthread_self());
 }
-link = client->channels;
-while (link) {
-next = link->next;
-rcc = link->data;
+FOREACH_CHANNEL(client, link, next, rcc) {
 channel = red_channel_client_get_channel(rcc);
 if (red_channel_client_is_connected(rcc)) {
 channel->client_cbs.migrate(rcc);
 }
-link = next;
 }
 }
 
@@ -628,13 +632,10 @@ void red_client_destroy(RedClient *client)
   client->thread_id,
   pthread_self());
 }
-link = client->channels;
-while (link) {
+FOREACH_CHANNEL(client, link, next, rcc) {
 RedChannel *channel;
-next = link->next;
 // some channels may be in other threads, so disconnection
 // is not synchronous.
-rcc = link->data;
 channel = red_channel_client_get_channel(rcc);
 red_channel_client_set_destroying(rcc);
 // some channels may be in other threads. However we currently
@@ -646,7 +647,6 @@ void red_client_destroy(RedClient *client)
 spice_assert(red_channel_client_pipe_is_empty(rcc));
 spice_assert(red_channel_client_no_item_being_sent(rcc));
 red_channel_client_destroy(rcc);
-link = next;
 }
 red_client_unref(client);
 }
@@ -654,13 +654,12 @@ void red_client_destroy(RedClient *client)
 /* client->lock should be locked */
 RedChannelClient *red_client_get_channel(RedClient *client, int type, int id)
 {
-GList *link;
+GList *link, *next;
 RedChannelClient *rcc;
 RedChannelClient *ret = NULL;
 
-for (link = client->channels; link != NULL; link = link->next) {
+FOREACH_CHANNEL(client, link, next, rcc) {
 RedChannel *channel;
-rcc = link->data;
 channel = red_channel_client_get_channel(rcc);
 if (channel->type == type && channel->id == id) {
 ret = rcc;
@@ -692,6 +691,7 @@ void red_client_set_main(RedClient *client, 
MainChannelClient *mcc) {
 void red_client_semi_seamless_migrate_complete(RedClient *client)
 {
 GList *link, *next;
+RedChannelClient *rcc;
 
 pthread_mutex_lock(>lock);
 if (!client->during_target_migrate || client->seamless_migrate) {
@@ -700,11 +700,8 @@ void red_client_semi_seamless_migrate_complete(RedClient 
*client)
 return;
 }
 client->during_target_migrate = FALSE;
-link = client->channels;
-while (link) {
-next = link->next;
-red_channel_client_semi_seamless_migration_complete(link->data);
-link = next;
+FOREACH_CHANNEL(client, link, next, rcc) {
+red_channel_client_semi_seamless_migration_complete(rcc);
 }
 

[Spice-devel] [RFC PATCH 0/2] Reintroduce some macro to iterate containers

[Frediano Ziglio]

With the introduction of GList/GQueue these macro were removed
at the expense of some minor regressions.
The first patch was started time ago and resemble more to old
macro while the second try to use some more safe way.

Frediano Ziglio (2):
  RedChannel: Add FOREACH_CHANNEL and use it to iterate
  Introduce some macro to simplify iteration on GList

 server/red-channel.c  | 50 ++
 server/red-common.h   | 18 ++
 server/reds-private.h |  3 +++
 server/reds.c | 66 +--
 4 files changed, 76 insertions(+), 61 deletions(-)

-- 
2.7.4

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

Re: [Spice-devel] [vdagent-linux v2 4/5] build-sys: remove prefix from filenames

[Victor Toso]

Hi,

On Fri, Sep 16, 2016 at 10:38:56AM -0400, Frediano Ziglio wrote:
> > 
> > ---
> >  .gitignore |  6 +
> >  Makefile.am| 28
> >  +++---
> >  src/vdagent/{vdagent-audio.c => audio.c}   |  2 +-
> >  src/vdagent/{vdagent-audio.h => audio.h}   |  4 ++--
> >  src/vdagent/{vdagent-file-xfers.c => file-xfers.c} |  4 ++--
> >  src/vdagent/{vdagent-file-xfers.h => file-xfers.h} |  0
> >  src/vdagent/vdagent.c  |  6 ++---
> >  src/vdagent/{vdagent-x11-priv.h => x11-priv.h} |  0
> >  src/vdagent/{vdagent-x11-randr.c => x11-randr.c}   |  4 ++--
> >  src/vdagent/{vdagent-x11.c => x11.c}   |  4 ++--
> >  src/vdagent/{vdagent-x11.h => x11.h}   |  4 ++--
> >  src/vdagentd/{vdagentd-uinput.c => uinput.c}   |  4 ++--
> >  src/vdagentd/{vdagentd-uinput.h => uinput.h}   |  0
> >  src/vdagentd/vdagentd.c|  6 ++---
> >  .../{vdagent-virtio-port.c => virtio-port.c}   |  2 +-
> >  .../{vdagent-virtio-port.h => virtio-port.h}   |  4 ++--
> >  src/vdagentd/{vdagentd-xorg-conf.c => xorg-conf.c} |  2 +-
> >  src/vdagentd/{vdagentd-xorg-conf.h => xorg-conf.h} |  0
> >  18 files changed, 43 insertions(+), 37 deletions(-)
> >  rename src/vdagent/{vdagent-audio.c => audio.c} (99%)
> >  rename src/vdagent/{vdagent-audio.h => audio.h} (91%)
> >  rename src/vdagent/{vdagent-file-xfers.c => file-xfers.c} (99%)
> >  rename src/vdagent/{vdagent-file-xfers.h => file-xfers.h} (100%)
> >  rename src/vdagent/{vdagent-x11-priv.h => x11-priv.h} (100%)
> >  rename src/vdagent/{vdagent-x11-randr.c => x11-randr.c} (99%)
> >  rename src/vdagent/{vdagent-x11.c => x11.c} (99%)
> >  rename src/vdagent/{vdagent-x11.h => x11.h} (95%)
> >  rename src/vdagentd/{vdagentd-uinput.c => uinput.c} (99%)
> >  rename src/vdagentd/{vdagentd-uinput.h => uinput.h} (100%)
> >  rename src/vdagentd/{vdagent-virtio-port.c => virtio-port.c} (99%)
> >  rename src/vdagentd/{vdagent-virtio-port.h => virtio-port.h} (97%)
> >  rename src/vdagentd/{vdagentd-xorg-conf.c => xorg-conf.c} (99%)
> >  rename src/vdagentd/{vdagentd-xorg-conf.h => xorg-conf.h} (100%)
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 4c039ee..ae47a90 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -8,6 +8,12 @@ src/stamp-h1
> >  src/*.o
> >  src/.deps
> >  src/.dirstamp
> > +src/vdagent/*.o
> > +src/vdagent/.deps
> > +src/vdagent/.dirstamp
> > +src/vdagentd/*.o
> > +src/vdagentd/.deps
> > +src/vdagentd/.dirstamp
> >  config.log
> >  config.status
> >  aclocal.m4
> > diff --git a/Makefile.am b/Makefile.am
> > index 861d806..7755f09 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -29,14 +29,14 @@ src_spice_vdagent_LDADD =   \
> >  
> >  src_spice_vdagent_SOURCES =\
> > $(common_sources)   \
> > -   src/vdagent/vdagent-audio.c \
> > -   src/vdagent/vdagent-audio.h \
> > -   src/vdagent/vdagent-file-xfers.c\
> > -   src/vdagent/vdagent-file-xfers.h\
> > -   src/vdagent/vdagent-x11-priv.h  \
> > -   src/vdagent/vdagent-x11-randr.c \
> > -   src/vdagent/vdagent-x11.c   \
> > -   src/vdagent/vdagent-x11.h   \
> > +   src/vdagent/audio.c \
> > +   src/vdagent/audio.h \
> > +   src/vdagent/file-xfers.c\
> > +   src/vdagent/file-xfers.h\
> > +   src/vdagent/x11-priv.h  \
> > +   src/vdagent/x11-randr.c \
> > +   src/vdagent/x11.c   \
> > +   src/vdagent/x11.h   \
> > src/vdagent/vdagent.c   \
> > $(NULL)
> >  
> > @@ -63,12 +63,12 @@ src_spice_vdagentd_SOURCES =\
> > $(common_sources)   \
> > src/vdagentd/vdagentd.c \
> > src/vdagentd/session-info.h \
> > -   src/vdagentd/vdagentd-uinput.c  \
> > -   src/vdagentd/vdagentd-uinput.h  \
> > -   src/vdagentd/vdagentd-xorg-conf.c   \
> > -   src/vdagentd/vdagentd-xorg-conf.h   \
> > -   src/vdagentd/vdagent-virtio-port.c  \
> > -   src/vdagentd/vdagent-virtio-port.h  \
> > +   src/vdagentd/uinput.c   \
> > +   src/vdagentd/uinput.h   \
> > +   src/vdagentd/xorg-conf.c\
> > +   src/vdagentd/xorg-conf.h\
> > +   src/vdagentd/virtio-port.c  \
> > +   src/vdagentd/virtio-port.h  \
> > $(NULL)
> >  
> >  if HAVE_CONSOLE_KIT
> > diff --git a/src/vdagent/vdagent-audio.c b/src/vdagent/audio.c
> > similarity index 99%
> > rename from src/vdagent/vdagent-audio.c
> > rename to src/vdagent/audio.c
> > index 6b11cd8..e823569 100644
> > --- a/src/vdagent/vdagent-audio.c
> > +++ b/src/vdagent/audio.c
> > @@ -26,7 +26,7 @@
> >  #include 

Re: [Spice-devel] [vdagent-linux v2 4/5] build-sys: remove prefix from filenames

[Frediano Ziglio]

> 
> ---
>  .gitignore |  6 +
>  Makefile.am| 28
>  +++---
>  src/vdagent/{vdagent-audio.c => audio.c}   |  2 +-
>  src/vdagent/{vdagent-audio.h => audio.h}   |  4 ++--
>  src/vdagent/{vdagent-file-xfers.c => file-xfers.c} |  4 ++--
>  src/vdagent/{vdagent-file-xfers.h => file-xfers.h} |  0
>  src/vdagent/vdagent.c  |  6 ++---
>  src/vdagent/{vdagent-x11-priv.h => x11-priv.h} |  0
>  src/vdagent/{vdagent-x11-randr.c => x11-randr.c}   |  4 ++--
>  src/vdagent/{vdagent-x11.c => x11.c}   |  4 ++--
>  src/vdagent/{vdagent-x11.h => x11.h}   |  4 ++--
>  src/vdagentd/{vdagentd-uinput.c => uinput.c}   |  4 ++--
>  src/vdagentd/{vdagentd-uinput.h => uinput.h}   |  0
>  src/vdagentd/vdagentd.c|  6 ++---
>  .../{vdagent-virtio-port.c => virtio-port.c}   |  2 +-
>  .../{vdagent-virtio-port.h => virtio-port.h}   |  4 ++--
>  src/vdagentd/{vdagentd-xorg-conf.c => xorg-conf.c} |  2 +-
>  src/vdagentd/{vdagentd-xorg-conf.h => xorg-conf.h} |  0
>  18 files changed, 43 insertions(+), 37 deletions(-)
>  rename src/vdagent/{vdagent-audio.c => audio.c} (99%)
>  rename src/vdagent/{vdagent-audio.h => audio.h} (91%)
>  rename src/vdagent/{vdagent-file-xfers.c => file-xfers.c} (99%)
>  rename src/vdagent/{vdagent-file-xfers.h => file-xfers.h} (100%)
>  rename src/vdagent/{vdagent-x11-priv.h => x11-priv.h} (100%)
>  rename src/vdagent/{vdagent-x11-randr.c => x11-randr.c} (99%)
>  rename src/vdagent/{vdagent-x11.c => x11.c} (99%)
>  rename src/vdagent/{vdagent-x11.h => x11.h} (95%)
>  rename src/vdagentd/{vdagentd-uinput.c => uinput.c} (99%)
>  rename src/vdagentd/{vdagentd-uinput.h => uinput.h} (100%)
>  rename src/vdagentd/{vdagent-virtio-port.c => virtio-port.c} (99%)
>  rename src/vdagentd/{vdagent-virtio-port.h => virtio-port.h} (97%)
>  rename src/vdagentd/{vdagentd-xorg-conf.c => xorg-conf.c} (99%)
>  rename src/vdagentd/{vdagentd-xorg-conf.h => xorg-conf.h} (100%)
> 
> diff --git a/.gitignore b/.gitignore
> index 4c039ee..ae47a90 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -8,6 +8,12 @@ src/stamp-h1
>  src/*.o
>  src/.deps
>  src/.dirstamp
> +src/vdagent/*.o
> +src/vdagent/.deps
> +src/vdagent/.dirstamp
> +src/vdagentd/*.o
> +src/vdagentd/.deps
> +src/vdagentd/.dirstamp
>  config.log
>  config.status
>  aclocal.m4
> diff --git a/Makefile.am b/Makefile.am
> index 861d806..7755f09 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -29,14 +29,14 @@ src_spice_vdagent_LDADD = \
>  
>  src_spice_vdagent_SOURCES =  \
>   $(common_sources)   \
> - src/vdagent/vdagent-audio.c \
> - src/vdagent/vdagent-audio.h \
> - src/vdagent/vdagent-file-xfers.c\
> - src/vdagent/vdagent-file-xfers.h\
> - src/vdagent/vdagent-x11-priv.h  \
> - src/vdagent/vdagent-x11-randr.c \
> - src/vdagent/vdagent-x11.c   \
> - src/vdagent/vdagent-x11.h   \
> + src/vdagent/audio.c \
> + src/vdagent/audio.h \
> + src/vdagent/file-xfers.c\
> + src/vdagent/file-xfers.h\
> + src/vdagent/x11-priv.h  \
> + src/vdagent/x11-randr.c \
> + src/vdagent/x11.c   \
> + src/vdagent/x11.h   \
>   src/vdagent/vdagent.c   \
>   $(NULL)
>  
> @@ -63,12 +63,12 @@ src_spice_vdagentd_SOURCES =  \
>   $(common_sources)   \
>   src/vdagentd/vdagentd.c \
>   src/vdagentd/session-info.h \
> - src/vdagentd/vdagentd-uinput.c  \
> - src/vdagentd/vdagentd-uinput.h  \
> - src/vdagentd/vdagentd-xorg-conf.c   \
> - src/vdagentd/vdagentd-xorg-conf.h   \
> - src/vdagentd/vdagent-virtio-port.c  \
> - src/vdagentd/vdagent-virtio-port.h  \
> + src/vdagentd/uinput.c   \
> + src/vdagentd/uinput.h   \
> + src/vdagentd/xorg-conf.c\
> + src/vdagentd/xorg-conf.h\
> + src/vdagentd/virtio-port.c  \
> + src/vdagentd/virtio-port.h  \
>   $(NULL)
>  
>  if HAVE_CONSOLE_KIT
> diff --git a/src/vdagent/vdagent-audio.c b/src/vdagent/audio.c
> similarity index 99%
> rename from src/vdagent/vdagent-audio.c
> rename to src/vdagent/audio.c
> index 6b11cd8..e823569 100644
> --- a/src/vdagent/vdagent-audio.c
> +++ b/src/vdagent/audio.c
> @@ -26,7 +26,7 @@
>  #include 
>  #include 
>  #include 
> -#include "vdagent-audio.h"
> +#include "audio.h"
>  
>  #define ALSA_MUTE   0
>  #define ALSA_UNMUTE 1
> diff --git a/src/vdagent/vdagent-audio.h b/src/vdagent/audio.h
> similarity index 91%
> rename 

Re: [Spice-devel] [PATCH 2/2] fixup! Add DisplayChannelPrivate struct

[Jonathon Jongsma]

On Fri, 2016-09-16 at 10:39 +0100, Frediano Ziglio wrote:
> Remove added FIXME introducing an helper function.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 12 
>  server/display-channel.h |  2 ++
>  server/red-worker.c  | 16 ++--
>  3 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index d193925..6682d77 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2099,3 +2099,15 @@ void
> display_channel_reset_image_cache(DisplayChannel *self)
>  {
>  image_cache_reset(>priv->image_cache);
>  }
> +
> +void display_channel_debug_oom(DisplayChannel *display, const char
> *msg)
> +{
> +RedChannel *channel = RED_CHANNEL(display);
> +
> +spice_debug("%s #draw=%u, #glz_draw=%u current %u pipes %u",
> +msg,
> +display->priv->drawable_count,
> +display->priv-
> >encoder_shared_data.glz_drawable_count,
> +display->priv->current_size,
> +red_channel_sum_pipes_size(channel));
> +}
> diff --git a/server/display-channel.h b/server/display-channel.h
> index ce5d419..ab344fa 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -306,6 +306,8 @@ void
> set_monitors_config_to_primary(DisplayChannel *display);
>  gboolean display_channel_validate_surface(DisplayChannel *display,
> uint32_t surface_id);
>  void display_channel_reset_image_cache(DisplayChannel *self);
>  
> +void display_channel_debug_oom(DisplayChannel *display, const char
> *msg);
> +
>  static inline int is_equal_path(SpicePath *path1, SpicePath *path2)
>  {
>  SpicePathSeg *seg1, *seg2;
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 92ab59c..2cde7fd 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -773,13 +773,7 @@ static void handle_dev_oom(void *opaque, void
> *payload)
>  
>  spice_return_if_fail(worker->running);
>  // streams? but without streams also leak
> -#if FIXME
> -spice_debug("OOM1 #draw=%u, #glz_draw=%u current %u pipes %u",
> -display->drawable_count,
> -display->encoder_shared_data.glz_drawable_count,
> -display->current_size,
> -red_channel_sum_pipes_size(display_red_channel));
> -#endif
> +display_channel_debug_oom(display, "OOM1");
>  while (red_process_display(worker, _is_empty)) {
>  red_channel_push(display_red_channel);
>  }
> @@ -787,13 +781,7 @@ static void handle_dev_oom(void *opaque, void
> *payload)
>  display_channel_free_some(worker->display_channel);
>  red_qxl_flush_resources(worker->qxl);
>  }
> -#if FIXME
> -spice_debug("OOM2 #draw=%u, #glz_draw=%u current %u pipes %u",
> -display->drawable_count,
> -display->encoder_shared_data.glz_drawable_count,
> -display->current_size,
> -red_channel_sum_pipes_size(display_red_channel));
> -#endif
> +display_channel_debug_oom(display, "OOM2");
>  red_qxl_clear_pending(worker->qxl->st,
> RED_DISPATCHER_PENDING_OOM);
>  }
>  


Thanks, I missed those FIXMEs.


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

Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel

[Frediano Ziglio]

> > 
> > Add a couple new functions to the header so that they can be called by
> > other objects rather than poking into the internals of the struct.
> > ---
> >  server/dcc-send.c| 16 +--
> >  server/display-channel.c | 71
> >  
> >  server/display-channel.h | 37 +
> >  server/red-worker.c  | 44 ++
> >  server/stream.c  | 18 ++--
> >  5 files changed, 109 insertions(+), 77 deletions(-)
> > 
> 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 108e69b..56bb029 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -203,6 +203,13 @@ void display_channel_surface_unref(DisplayChannel
> > *display, uint32_t surface_id)
> >  spice_warn_if_fail(ring_is_empty(>depend_on_me));
> >  }
> >  
> > +/* TODO: perhaps rename to "ready" or "realized" ? */
> > +bool display_channel_surface_has_canvas(DisplayChannel *display,
> > +uint32_t surface_id)
> > +{
> > +return display->surfaces[surface_id].context.canvas != NULL;
> > +}
> > +
> 
> I honestly prefer bool and true/false but we decided to use gboolean and not
> bool.
> This function is mainly doing the same think of validate_surface without
> the logging stuff.
> Perhaps adding a flag "warnings" to validate_surface is enough?
> 
> 
> ...
> 
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 7b71480..022cd93 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -208,10 +208,17 @@ struct DisplayChannel {
> >  ImageEncoderSharedData encoder_shared_data;
> >  };
> >  
> > -static inline int get_stream_id(DisplayChannel *display, Stream *stream)
> > -{
> > -return (int)(stream - display->streams_buf);
> > -}
> > +
> > +#define FOREACH_DCC(channel, _link, _next, _data)   \
> > +for (_link = (channel ? RED_CHANNEL(channel)->clients : NULL), \
> > + _next = (_link ? _link->next : NULL), \
> > + _data = (_link ? _link->data : NULL); \
> > + _link; \
> > + _link = _next, \
> > + _next = (_link ? _link->next : NULL), \
> > + _data = (_link ? _link->data : NULL))
> > +
> > +int display_channel_get_stream_id(DisplayChannel *display, Stream
> > *stream);
> >  
> >  typedef struct RedSurfaceDestroyItem {
> >  RedPipeItem pipe_item;
> > @@ -258,6 +265,8 @@ void
> > display_channel_compress_stats_print  (const Disp
> >  void   display_channel_compress_stats_reset
> >  (DisplayChannel *display);
> >  void   display_channel_surface_unref
> >  (DisplayChannel *display,
> >
> > uint32_t
> >
> > surface_id);
> > +bool   display_channel_surface_has_canvas
> > (DisplayChannel *display,
> > +
> 
> Although this is consistent with the rest it does not follow our style.
> 
> > uint32_t
> > surface_id);
> >  void   display_channel_current_flush
> >  (DisplayChannel *display,
> >int
> >
> > surface_id);
> >  intdisplay_channel_wait_for_migrate_data
> >  (DisplayChannel *display);
> ...
> 

Actually.
Would not be easier to have a 

RedSurface *display_channel_get_surface(DisplayChannel *display, uint32_t 
surface_id);

which check surface_id and canvas and return the valid surface or NULL?

This at the same time replace validation of surface ids and accessing surfaces
array inside DisplayChannel.

Personally I don't like that much the display_channel_surface_has_canvas as
it gives implementation detail to the user which could ignore the presence
of a "canvas" in the surface.

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

Re: [Spice-devel] [PATCH] fixup! Improve encapsulation of DisplayChannel

[Pavel Grunt]

On Fri, 2016-09-16 at 13:26 +0100, Frediano Ziglio wrote:
> Style fix
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/display-channel.c | 4 ++--
>  server/display-channel.h | 3 +--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 56bb029..4e643dd 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -204,8 +204,8 @@ void
> display_channel_surface_unref(DisplayChannel *display, uint32_t
> surface_id)
>  }
>  
>  /* TODO: perhaps rename to "ready" or "realized" ? */
> -bool display_channel_surface_has_canvas(DisplayChannel *display,
> -uint32_t surface_id)
> +gboolean display_channel_surface_has_canvas(DisplayChannel
> *display,
> +uint32_t surface_id)
>  {
>  return display->surfaces[surface_id].context.canvas != NULL;
>  }
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 022cd93..ba62950 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -265,8 +265,6 @@
> void   display_channel_compress_stats_print 
>  (const Disp
>  void   display_channel_compress_stats_reset
>   (DisplayChannel *display);
>  void   display_channel_surface_unref   
>   (DisplayChannel *display,
> 
>    uint32_t surface_id);
> -bool   display_channel_surface_has_canvas  
>   (DisplayChannel *display,
> -   
>    uint32_t surface_id);
>  void   display_channel_current_flush   
>   (DisplayChannel *display,
> 
>    int surface_id);
>  intdisplay_channel_wait_for_migrate_data   
>   (DisplayChannel *display);
> @@ -295,6 +293,7 @@ void
> display_channel_update_monitors_config(DisplayChannel *display,
> QXLMonitors
>  void set_monitors_config_to_primary(DisplayChannel *display);
>  
>  gboolean display_channel_validate_surface(DisplayChannel *display,
> uint32_t surface_id);
> +gboolean display_channel_surface_has_canvas(DisplayChannel
> *display, uint32_t surface_id);
>  void display_channel_reset_image_cache(DisplayChannel *self);
>  
>  static inline int is_equal_path(SpicePath *path1, SpicePath *path2)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] fixup! Improve encapsulation of DisplayChannel

[Frediano Ziglio]

Style fix

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

diff --git a/server/display-channel.c b/server/display-channel.c
index 56bb029..4e643dd 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -204,8 +204,8 @@ void display_channel_surface_unref(DisplayChannel *display, 
uint32_t surface_id)
 }
 
 /* TODO: perhaps rename to "ready" or "realized" ? */
-bool display_channel_surface_has_canvas(DisplayChannel *display,
-uint32_t surface_id)
+gboolean display_channel_surface_has_canvas(DisplayChannel *display,
+uint32_t surface_id)
 {
 return display->surfaces[surface_id].context.canvas != NULL;
 }
diff --git a/server/display-channel.h b/server/display-channel.h
index 022cd93..ba62950 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -265,8 +265,6 @@ void   
display_channel_compress_stats_print  (const Disp
 void   display_channel_compress_stats_reset  
(DisplayChannel *display);
 void   display_channel_surface_unref 
(DisplayChannel *display,
   uint32_t 
surface_id);
-bool   display_channel_surface_has_canvas
(DisplayChannel *display,
-  uint32_t 
surface_id);
 void   display_channel_current_flush 
(DisplayChannel *display,
   int 
surface_id);
 intdisplay_channel_wait_for_migrate_data 
(DisplayChannel *display);
@@ -295,6 +293,7 @@ void display_channel_update_monitors_config(DisplayChannel 
*display, QXLMonitors
 void set_monitors_config_to_primary(DisplayChannel *display);
 
 gboolean display_channel_validate_surface(DisplayChannel *display, uint32_t 
surface_id);
+gboolean display_channel_surface_has_canvas(DisplayChannel *display, uint32_t 
surface_id);
 void display_channel_reset_image_cache(DisplayChannel *self);
 
 static inline int is_equal_path(SpicePath *path1, SpicePath *path2)
-- 
2.7.4

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

[Spice-devel] [PATCH v3 6/8] replay: Propagate error correctly in replay_fread

[Frediano Ziglio]

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 6914c17..6950f98 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -57,16 +57,14 @@ struct SpiceReplay {
 pthread_cond_t cond;
 };
 
-static int replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
+static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf, size_t size)
 {
-if (replay->error) {
-return 0;
-}
-if (feof(replay->fd)) {
+if (replay->error || feof(replay->fd) ||
+fread(buf, 1, size, replay->fd) != size) {
 replay->error = TRUE;
 return 0;
 }
-return fread(buf, size, 1, replay->fd);
+return size;
 }
 
 __attribute__((format(scanf, 2, 3)))
-- 
2.7.4

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

[Spice-devel] [PATCH v3 4/8] replay: Move error check

[Frediano Ziglio]

Do the check after replay_fscanf to make sure everything
is fine before calling red_replay_compat_drawable or
red_replay_native_drawable.

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index a5b9553..5fcb243 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -1063,10 +1063,10 @@ static QXLCompatDrawable 
*red_replay_compat_drawable(SpiceReplay *replay, uint32
 
 static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay, uint32_t flags)
 {
+replay_fscanf(replay, "drawable\n");
 if (replay->error) {
 return 0;
 }
-replay_fscanf(replay, "drawable\n");
 if (flags & QXL_COMMAND_FLAG_COMPAT) {
 return QXLPHYSICAL_FROM_PTR(red_replay_compat_drawable(replay, flags));
 } else {
-- 
2.7.4

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

[Spice-devel] [PATCH v3 8/8] replay: use unsigned in formatting

[Frediano Ziglio]

Avoid negative syntax. Also could prevent some memory problem is number
get too big.

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 1442686..69669dc 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -209,7 +209,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 {
 char template[1024];
 int with_zlib = -1;
-int zlib_size;
+unsigned int zlib_size;
 uint8_t *zlib_buffer;
 z_stream strm;
 
@@ -233,7 +233,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 int ret;
 GList *elem;
 
-replay_fscanf(replay, "%d:", _size);
+replay_fscanf(replay, "%u:", _size);
 if (replay->error) {
 return REPLAY_ERROR;
 }
@@ -281,11 +281,11 @@ static ssize_t red_replay_data_chunks(SpiceReplay 
*replay, const char *prefix,
   uint8_t **mem, size_t base_size)
 {
 size_t data_size;
-int count_chunks;
+unsigned int count_chunks;
 size_t next_data_size;
 QXLDataChunk *cur, *next;
 
-replay_fscanf(replay, "data_chunks %d %zu\n", _chunks, _size);
+replay_fscanf(replay, "data_chunks %u %zu\n", _chunks, _size);
 if (replay->error) {
 return -1;
 }
@@ -376,9 +376,9 @@ static void red_replay_path_free(SpiceReplay *replay, 
QXLPHYSICAL p)
 static QXLClipRects *red_replay_clip_rects(SpiceReplay *replay)
 {
 QXLClipRects *qxl = NULL;
-int num_rects;
+unsigned int num_rects;
 
-replay_fscanf(replay, "num_rects %d\n", _rects);
+replay_fscanf(replay, "num_rects %u\n", _rects);
 if (replay->error) {
 return NULL;
 }
@@ -443,9 +443,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 replay_fscanf(replay, "has_palette %d\n", _palette);
 if (has_palette) {
 QXLPalette *qp;
-int i, num_ents;
+unsigned int i, num_ents;
 
-replay_fscanf(replay, "qp.num_ents %d\n", _ents);
+replay_fscanf(replay, "qp.num_ents %u\n", _ents);
 if (replay->error) {
 return NULL;
 }
-- 
2.7.4

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

[Spice-devel] [PATCH v3 7/8] replay: Use proper formatting for scanf family

[Frediano Ziglio]

Currently on Linux PRIu64 and SCNu64 are the same but just to make
sure in the future use the correct macros.

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 6950f98..1442686 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -423,7 +423,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 
 qxl = (QXLImage*)replay_malloc0(replay, sizeof(QXLImage));
 elem = replay->allocated;
-replay_fscanf(replay, "descriptor.id %"PRIu64"\n", >descriptor.id);
+replay_fscanf(replay, "descriptor.id %"SCNu64"\n", >descriptor.id);
 replay_fscanf(replay, "descriptor.type %d\n", ); qxl->descriptor.type 
= temp;
 replay_fscanf(replay, "descriptor.flags %d\n", ); 
qxl->descriptor.flags = temp;
 replay_fscanf(replay, "descriptor.width %d\n", >descriptor.width);
@@ -452,7 +452,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 qp = replay_malloc(replay, sizeof(QXLPalette) + num_ents * 
sizeof(qp->ents[0]));
 qp->num_ents = num_ents;
 qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
-replay_fscanf(replay, "unique %"PRIu64"\n", >unique);
+replay_fscanf(replay, "unique %"SCNu64"\n", >unique);
 for (i = 0; i < num_ents; i++) {
 replay_fscanf(replay, "ents %d\n", >ents[i]);
 }
@@ -1296,7 +1296,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt* 
spice_replay_next_cmd(SpiceReplay *replay,
 int counter;
 
 while (what != 0) {
-replay_fscanf(replay, "event %d %d %d %"PRIu64"\n", ,
+replay_fscanf(replay, "event %d %d %d %"SCNu64"\n", ,
 , , );
 if (replay->error) {
 goto error;
@@ -1308,7 +1308,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt* 
spice_replay_next_cmd(SpiceReplay *replay,
 cmd = g_new(QXLCommandExt, 1);
 cmd->cmd.type = type;
 cmd->group_id = 0;
-spice_debug("command %"PRIu64", %d\r", timestamp, cmd->cmd.type);
+spice_debug("command %"SCNu64", %d\r", timestamp, cmd->cmd.type);
 switch (cmd->cmd.type) {
 case QXL_CMD_DRAW:
 cmd->flags = 0;
-- 
2.7.4

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

[Spice-devel] [PATCH v3 0/8] Improve error handling in red-replay-qxl.c

[Frediano Ziglio]

Detect error in files and handle more gracefully.
This version:
- fix some formatting issue (Pavel);
- merged some patches;
- improved an error check (Pavel);
- added two patches to improve replay.

Frediano Ziglio (8):
  replay: Record allocations in a GList to handle errors
  replay: Handle errors reading strings from record file
  replay: Detect errors from red_replay_data_chunks
  replay: Move error check
  replay: Update pointer in allocated list
  replay: Propagate error correctly in replay_fread
  replay: Use proper formatting for scanf family
  replay: use unsigned in formatting

 server/red-replay-qxl.c | 239 +---
 1 file changed, 184 insertions(+), 55 deletions(-)

-- 
2.7.4

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

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

[Frediano Ziglio]

Avoid to free invalid pointer.

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 5fcb243..6914c17 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -413,6 +413,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 int temp;
 int has_palette;
 int has_image;
+GList *elem;
 
 replay_fscanf(replay, "image %d\n", _image);
 if (replay->error) {
@@ -423,6 +424,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 }
 
 qxl = (QXLImage*)replay_malloc0(replay, sizeof(QXLImage));
+elem = replay->allocated;
 replay_fscanf(replay, "descriptor.id %"PRIu64"\n", >descriptor.id);
 replay_fscanf(replay, "descriptor.type %d\n", ); qxl->descriptor.type 
= temp;
 replay_fscanf(replay, "descriptor.flags %d\n", ); 
qxl->descriptor.flags = temp;
@@ -485,8 +487,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 if (replay->error) {
 return NULL;
 }
-qxl = realloc(qxl, sizeof(QXLImageDescriptor) + sizeof(QXLQUICData) +
-  qxl->quic.data_size);
+qxl = spice_realloc(qxl, sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+qxl->quic.data_size);
+elem->data = qxl;
 size = red_replay_data_chunks(replay, "quic.data", 
(uint8_t**)>quic.data, 0);
 spice_assert(size == qxl->quic.data_size);
 break;
-- 
2.7.4

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

[Spice-devel] [PATCH v3 2/8] replay: Handle errors reading strings from record file

[Frediano Ziglio]

To check fscanf read all needed information a dummy "%n" is appended
to any string and the value stored there is tested. This as scanf family
could return a valid value but not entirely process the string so
adding a "%n" and checking this was processed make sure all expected
string is found.
The code to check for a specific string is now a bit more complicated
as replay_fscanf use a macro which append a constant string.
The "error" field is used to mark any error happened, so in most cases
there is no explicit check beside when this could cause a problem
(for instance results of replay_fscanf used which would result in
uninitialised variable usage).

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 7eddaf4..968a605 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -49,6 +49,7 @@ struct SpiceReplay {
 GArray *id_map_inv; // replay id -> record id
 GArray *id_free; // free list
 int nsurfaces;
+int end_pos;
 
 GList *allocated;
 
@@ -69,11 +70,13 @@ static int replay_fread(SpiceReplay *replay, uint8_t *buf, 
size_t size)
 }
 
 __attribute__((format(scanf, 2, 3)))
-static replay_t replay_fscanf(SpiceReplay *replay, const char *fmt, ...)
+static replay_t replay_fscanf_check(SpiceReplay *replay, const char *fmt, ...)
 {
 va_list ap;
 int ret;
 
+replay->end_pos = -1;
+
 if (replay->error) {
 return REPLAY_ERROR;
 }
@@ -84,11 +87,13 @@ static replay_t replay_fscanf(SpiceReplay *replay, const 
char *fmt, ...)
 va_start(ap, fmt);
 ret = vfscanf(replay->fd, fmt, ap);
 va_end(ap);
-if (ret == EOF) {
+if (ret == EOF || replay->end_pos < 0) {
 replay->error = TRUE;
 }
 return replay->error ? REPLAY_ERROR : REPLAY_OK;
 }
+#define replay_fscanf(r, fmt, ...) \
+replay_fscanf_check(r, fmt "%n", ## __VA_ARGS__, >end_pos)
 
 static inline void *replay_malloc(SpiceReplay *replay, size_t size)
 {
@@ -210,9 +215,11 @@ static replay_t read_binary(SpiceReplay *replay, const 
char *prefix, size_t *siz
 uint8_t *zlib_buffer;
 z_stream strm;
 
-snprintf(template, sizeof(template), "binary %%d %s %%ld:", prefix);
-if (replay_fscanf(replay, template, _zlib, size) == REPLAY_ERROR)
+snprintf(template, sizeof(template), "binary %%d %s %%ld:%%n", prefix);
+replay_fscanf_check(replay, template, _zlib, size, >end_pos);
+if (replay->error) {
 return REPLAY_ERROR;
+}
 
 if (*buf == NULL) {
 *buf = replay_malloc(replay, *size + base_size);
@@ -224,15 +231,19 @@ static replay_t read_binary(SpiceReplay *replay, const 
char *prefix, size_t *siz
 hexdump(*buf + base_size, *size);
 }
 #else
-spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
 if (with_zlib) {
 int ret;
 GList *elem;
 
 replay_fscanf(replay, "%d:", _size);
+if (replay->error) {
+return REPLAY_ERROR;
+}
 zlib_buffer = replay_malloc(replay, zlib_size);
 elem = replay->allocated;
-replay_fread(replay, zlib_buffer, zlib_size);
+if (replay_fread(replay, zlib_buffer, zlib_size) != zlib_size) {
+return REPLAY_ERROR;
+}
 strm.zalloc = Z_NULL;
 strm.zfree = Z_NULL;
 strm.opaque = Z_NULL;
@@ -265,8 +276,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 replay_fread(replay, *buf + base_size, *size);
 }
 #endif
-replay_fscanf(replay, "\n");
-return REPLAY_OK;
+return replay_fscanf(replay, "\n");
 }
 
 static size_t red_replay_data_chunks(SpiceReplay *replay, const char *prefix,
@@ -337,8 +347,9 @@ static void red_replay_rect_ptr(SpiceReplay *replay, const 
char *prefix, QXLRect
 {
 char template[1024];
 
-snprintf(template, sizeof(template), "rect %s %%d %%d %%d %%d\n", prefix);
-replay_fscanf(replay, template, >top, >left, >bottom, 
>right);
+snprintf(template, sizeof(template), "rect %s %%d %%d %%d %%d\n%%n", 
prefix);
+replay_fscanf_check(replay, template, >top, >left, >bottom, 
>right,
+>end_pos);
 }
 
 static QXLPath *red_replay_path(SpiceReplay *replay)
@@ -364,6 +375,9 @@ static QXLClipRects *red_replay_clip_rects(SpiceReplay 
*replay)
 int num_rects;
 
 replay_fscanf(replay, "num_rects %d\n", _rects);
+if (replay->error) {
+return NULL;
+}
 red_replay_data_chunks(replay, "clip_rects", (uint8_t**), 
sizeof(QXLClipRects));
 qxl->num_rects = num_rects;
 return qxl;
@@ -392,6 +406,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 int has_image;
 
 replay_fscanf(replay, "image %d\n", _image);
+if (replay->error) {
+return NULL;
+}
 if (!has_image) {
 return NULL;
 }
@@ 

[Spice-devel] [PATCH v3 1/8] replay: Record allocations in a GList to handle errors

[Frediano Ziglio]

Allocations are kept into a GList to be able to free in case some
errors happened.

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

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 7ce8f47..7eddaf4 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -50,6 +50,8 @@ struct SpiceReplay {
 GArray *id_free; // free list
 int nsurfaces;
 
+GList *allocated;
+
 pthread_mutex_t mutex;
 pthread_cond_t cond;
 };
@@ -88,6 +90,20 @@ static replay_t replay_fscanf(SpiceReplay *replay, const 
char *fmt, ...)
 return replay->error ? REPLAY_ERROR : REPLAY_OK;
 }
 
+static inline void *replay_malloc(SpiceReplay *replay, size_t size)
+{
+void *mem = spice_malloc(size);
+replay->allocated = g_list_prepend(replay->allocated, mem);
+return mem;
+}
+
+static inline void *replay_malloc0(SpiceReplay *replay, size_t size)
+{
+void *mem = replay_malloc(replay, size);
+memset(mem, 0, size);
+return mem;
+}
+
 static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
 {
 uint32_t newid = 0;
@@ -199,7 +215,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 return REPLAY_ERROR;
 
 if (*buf == NULL) {
-*buf = spice_malloc(*size + base_size);
+*buf = replay_malloc(replay, *size + base_size);
 }
 #if 0
 {
@@ -211,9 +227,11 @@ static replay_t read_binary(SpiceReplay *replay, const 
char *prefix, size_t *siz
 spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
 if (with_zlib) {
 int ret;
+GList *elem;
 
 replay_fscanf(replay, "%d:", _size);
-zlib_buffer = spice_malloc(zlib_size);
+zlib_buffer = replay_malloc(replay, zlib_size);
+elem = replay->allocated;
 replay_fread(replay, zlib_buffer, zlib_size);
 strm.zalloc = Z_NULL;
 strm.zfree = Z_NULL;
@@ -241,6 +259,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
 }
 }
 (void)inflateEnd();
+replay->allocated = g_list_remove_link(replay->allocated, elem);
 free(zlib_buffer); // TODO - avoid repeat alloc/dealloc by keeping last
 } else {
 replay_fread(replay, *buf + base_size, *size);
@@ -377,7 +396,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 return NULL;
 }
 
-qxl = spice_new0(QXLImage, 1);
+qxl = (QXLImage*)replay_malloc0(replay, sizeof(QXLImage));
 replay_fscanf(replay, "descriptor.id %"PRIu64"\n", >descriptor.id);
 replay_fscanf(replay, "descriptor.type %d\n", ); qxl->descriptor.type 
= temp;
 replay_fscanf(replay, "descriptor.flags %d\n", ); 
qxl->descriptor.flags = temp;
@@ -398,7 +417,7 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
 int i, num_ents;
 
 replay_fscanf(replay, "qp.num_ents %d\n", _ents);
-qp = spice_malloc(sizeof(QXLPalette) + num_ents * 
sizeof(qp->ents[0]));
+qp = replay_malloc(replay, sizeof(QXLPalette) + num_ents * 
sizeof(qp->ents[0]));
 qp->num_ents = num_ents;
 qxl->bitmap.palette = QXLPHYSICAL_FROM_PTR(qp);
 replay_fscanf(replay, "unique %"PRIu64"\n", >unique);
@@ -788,7 +807,7 @@ static void red_replay_composite_free(SpiceReplay *replay, 
QXLComposite *qxl, ui
 
 static QXLDrawable *red_replay_native_drawable(SpiceReplay *replay, uint32_t 
flags)
 {
-QXLDrawable *qxl = spice_malloc0(sizeof(QXLDrawable)); // TODO - this is 
too large usually
+QXLDrawable *qxl = replay_malloc0(replay, sizeof(QXLDrawable)); // TODO - 
this is too large usually
 int i;
 int temp;
 
@@ -919,7 +938,7 @@ static void red_replay_native_drawable_free(SpiceReplay 
*replay, QXLDrawable *qx
 static QXLCompatDrawable *red_replay_compat_drawable(SpiceReplay *replay, 
uint32_t flags)
 {
 int temp;
-QXLCompatDrawable *qxl = spice_malloc0(sizeof(QXLCompatDrawable)); // TODO 
- too large usually
+QXLCompatDrawable *qxl = replay_malloc0(replay, 
sizeof(QXLCompatDrawable)); // TODO - too large usually
 
 red_replay_rect_ptr(replay, "bbox", >bbox);
 red_replay_clip_ptr(replay, >clip);
@@ -994,7 +1013,7 @@ static QXLPHYSICAL red_replay_drawable(SpiceReplay 
*replay, uint32_t flags)
 
 static QXLUpdateCmd *red_replay_update_cmd(SpiceReplay *replay)
 {
-QXLUpdateCmd *qxl = spice_malloc0(sizeof(QXLUpdateCmd));
+QXLUpdateCmd *qxl = replay_malloc0(replay, sizeof(QXLUpdateCmd));
 
 replay_fscanf(replay, "update\n");
 red_replay_rect_ptr(replay, "area", >area);
@@ -1019,7 +1038,7 @@ static QXLSurfaceCmd *red_replay_surface_cmd(SpiceReplay 
*replay)
 size_t size;
 size_t read_size;
 int temp;
-QXLSurfaceCmd *qxl = spice_malloc0(sizeof(QXLSurfaceCmd));
+QXLSurfaceCmd *qxl = 

Re: [Spice-devel] [PATCH spice v3 2/2] reds: Simplify vdi_port_read_buf_process

[Frediano Ziglio]

> 
> Reuse and handle the return value from agent_msg_filter_process_data
> ---
> v3: discard AGENT_MSG_FILTER_MONITORS_CONFIG
> ---
>  server/reds.c | 66
>  ++-
>  1 file changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 3addc1e..8e9dc7b 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -763,36 +763,23 @@ static void vdi_port_read_buf_release(uint8_t *data,
> void *opaque)
>  red_pipe_item_unref((RedPipeItem *)opaque);
>  }
>  
> -/* returns TRUE if the buffer can be forwarded */
> -static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
> -  RedVDIReadBuf *buf, gboolean
> *error)
> +/*
> +returns the #AgentMsgFilterResult value:
> +AGENT_MSG_FILTER_OK if the buffer can be forwarded,
> +AGENT_MSG_FILTER_PROTO_ERROR on error
> +other values can be discarded
> +*/
> +static AgentMsgFilterResult vdi_port_read_buf_process(RedCharDeviceVDIPort
> *dev,
> +  RedVDIReadBuf *buf)
>  {
> -AgentMsgFilterResult res;
> -
> -*error = FALSE;
> -
>  switch (dev->priv->vdi_chunk_header.port) {
> -case VDP_CLIENT_PORT: {
> -res = agent_msg_filter_process_data(>priv->read_filter,
> -buf->data, buf->len);
> -switch (res) {
> -case AGENT_MSG_FILTER_OK:
> -return TRUE;
> -case AGENT_MSG_FILTER_MONITORS_CONFIG:
> -/* fall through */
> -case AGENT_MSG_FILTER_DISCARD:
> -return FALSE;
> -case AGENT_MSG_FILTER_PROTO_ERROR:
> -*error = TRUE;
> -return FALSE;
> -}
> -}
> +case VDP_CLIENT_PORT:
> +return agent_msg_filter_process_data(>priv->read_filter,
> buf->data, buf->len);
>  case VDP_SERVER_PORT:
> -return FALSE;
> +return AGENT_MSG_FILTER_DISCARD;
>  default:
>  spice_warning("invalid port");
> -*error = TRUE;
> -return FALSE;
> +return AGENT_MSG_FILTER_PROTO_ERROR;
>  }
>  }
>  
> @@ -883,7 +870,6 @@ static RedPipeItem
> *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
>  dev->priv->read_state = VDI_PORT_READ_STATE_READ_DATA;
>  }
>  case VDI_PORT_READ_STATE_READ_DATA: {
> -gboolean error = FALSE;
>  n = sif->read(reds->vdagent, dev->priv->receive_pos,
>  dev->priv->receive_len);
>  if (!n) {
>  return NULL;
> @@ -902,12 +888,15 @@ static RedPipeItem
> *vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
>  } else {
>  dev->priv->read_state = VDI_PORT_READ_STATE_GET_BUFF;
>  }
> -if (vdi_port_read_buf_process(reds->agent_dev, dispatch_buf,
> )) {
> +switch (vdi_port_read_buf_process(reds->agent_dev,
> dispatch_buf)) {
> +case AGENT_MSG_FILTER_OK:
>  return _buf->base;
> -} else {
> -if (error) {
> -reds_agent_remove(reds);
> -}
> +case AGENT_MSG_FILTER_PROTO_ERROR:
> +reds_agent_remove(reds);
> +/* fall through */
> +case AGENT_MSG_FILTER_MONITORS_CONFIG:
> +/* fall through */
> +case AGENT_MSG_FILTER_DISCARD:
>  red_pipe_item_unref(_buf->base);
>  }
>  }
> @@ -1277,22 +1266,25 @@ void reds_on_main_channel_migrate(RedsState *reds,
> MainChannelClient *mcc)
>  if (agent_dev->priv->read_filter.msg_data_to_read ||
>  read_data_len > sizeof(VDAgentMessage)) { /* msg header has been
>  read */
>  RedVDIReadBuf *read_buf = agent_dev->priv->current_read_buf;
> -gboolean error = FALSE;
>  
>  spice_debug("push partial read %u (msg first chunk? %d)",
>  read_data_len,
>  !agent_dev->priv->read_filter.msg_data_to_read);
>  
>  read_buf->len = read_data_len;
> -if (vdi_port_read_buf_process(reds->agent_dev, read_buf, )) {
> +switch (vdi_port_read_buf_process(reds->agent_dev, read_buf)) {
> +case AGENT_MSG_FILTER_OK:
>  main_channel_client_push_agent_data(mcc,
>  read_buf->data,
>  read_buf->len,
>  vdi_port_read_buf_release,
>  read_buf);
> -} else {
> -if (error) {
> -   reds_agent_remove(reds);
> -}
> +break;
> +case AGENT_MSG_FILTER_PROTO_ERROR:
> +reds_agent_remove(reds);
> +/* fall through */
> +case AGENT_MSG_FILTER_MONITORS_CONFIG:
> +/* fall 

Re: [Spice-devel] [PATCH spice v3 1/2] agent-filter: Use enum as return value

[Frediano Ziglio]

> 
> Explicitely discard AGENT_MSG_FILTER_MONITORS_CONFIG messages
> from the agent.
> 
> Also remove unused AGENT_MSG_FILTER_END
> ---
> v3: discard AGENT_MSG_FILTER_MONITORS_CONFIG
> ---
>  server/agent-msg-filter.c |  4 ++--
>  server/agent-msg-filter.h | 11 +--
>  server/reds.c |  6 --
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
> index a11f624..7921fe7 100644
> --- a/server/agent-msg-filter.c
> +++ b/server/agent-msg-filter.c
> @@ -48,8 +48,8 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
>  filter->discard_all = discard_all;
>  }
>  
> -int agent_msg_filter_process_data(AgentMsgFilter *filter,
> -  const uint8_t *data, uint32_t len)
> +AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
> +   const uint8_t *data,
> uint32_t len)
>  {
>  struct VDAgentMessage msg_header;
>  
> diff --git a/server/agent-msg-filter.h b/server/agent-msg-filter.h
> index d61f8d4..b4d8e72 100644
> --- a/server/agent-msg-filter.h
> +++ b/server/agent-msg-filter.h
> @@ -25,17 +25,16 @@
>  #include 
>  
>  /* Possible return values for agent_msg_filter_process_data */
> -enum {
> +typedef enum {
>  AGENT_MSG_FILTER_OK,
>  AGENT_MSG_FILTER_DISCARD,
>  AGENT_MSG_FILTER_PROTO_ERROR,
>  AGENT_MSG_FILTER_MONITORS_CONFIG,
> -AGENT_MSG_FILTER_END
> -};
> +} AgentMsgFilterResult;
>  
>  typedef struct AgentMsgFilter {
>  int msg_data_to_read;
> -int result;
> +AgentMsgFilterResult result;
>  gboolean copy_paste_enabled;
>  gboolean file_xfer_enabled;
>  gboolean use_client_monitors_config;
> @@ -49,7 +48,7 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
>  void agent_msg_filter_config(AgentMsgFilter *filter,
>   gboolean copy_paste, gboolean file_xfer,
>   gboolean use_client_monitors_config);
> -int agent_msg_filter_process_data(AgentMsgFilter *filter,
> -  const uint8_t *data, uint32_t len);
> +AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
> +   const uint8_t *data,
> uint32_t len);
>  
>  #endif
> diff --git a/server/reds.c b/server/reds.c
> index a387eeb..3addc1e 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -767,7 +767,7 @@ static void vdi_port_read_buf_release(uint8_t *data, void
> *opaque)
>  static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
>RedVDIReadBuf *buf, gboolean
>*error)
>  {
> -int res;
> +AgentMsgFilterResult res;
>  
>  *error = FALSE;
>  
> @@ -778,6 +778,8 @@ static gboolean
> vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
>  switch (res) {
>  case AGENT_MSG_FILTER_OK:
>  return TRUE;
> +case AGENT_MSG_FILTER_MONITORS_CONFIG:
> +/* fall through */
>  case AGENT_MSG_FILTER_DISCARD:
>  return FALSE;
>  case AGENT_MSG_FILTER_PROTO_ERROR:
> @@ -1199,7 +1201,7 @@ void reds_on_main_agent_data(RedsState *reds,
> MainChannelClient *mcc, void *mess
>  {
>  RedCharDeviceVDIPort *dev = reds->agent_dev;
>  VDIChunkHeader *header;
> -int res;
> +AgentMsgFilterResult res;
>  
>  res =
>  agent_msg_filter_process_data(>agent_dev->priv->write_filter,
>  message, size);

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [PATCH v2 8/8] replay: Propagate error correctly in replay_fread

[Pavel Grunt]

On Thu, 2016-09-15 at 23:19 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 73f9cd4..fe4f7a9 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -57,16 +57,15 @@ struct SpiceReplay {
>  pthread_cond_t cond;
>  };
>  
> -static int replay_fread(SpiceReplay *replay, uint8_t *buf, size_t
> size)
> +static ssize_t replay_fread(SpiceReplay *replay, uint8_t *buf,
> size_t size)
>  {
> -if (replay->error) {
> -return 0;
> -}
> -if (feof(replay->fd)) {
> +if (replay->error
> +|| feof(replay->fd)
> +|| fread(buf, 1, size, replay->fd) != size) {

it doesn't follow our style:
https://www.spice-space.org/spice-project-coding-style-and-coding-conv
entions.html#_branching_indentation

>  replay->error = TRUE;
>  return 0;
>  }
> -return fread(buf, size, 1, replay->fd);
> +return size;
What is the point of returning size - unchanged by the function ?

If the function is boolean, return type should be boolean as well

Pavel

>  }
>  
>  __attribute__((format(scanf, 2, 3)))
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 6/8] replay: Assure read_binary receives a NULL pointer

[Pavel Grunt]

On Thu, 2016-09-15 at 23:19 +0100, Frediano Ziglio wrote:
> read_binary do not allocate a buffer for no-NULL pointers.
> Avoid using uninitialized data and allocate proper buffer.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/red-replay-qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index ef5569e..45c105c 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -1089,7 +1089,7 @@ static QXLUpdateCmd
> *red_replay_update_cmd(SpiceReplay *replay)
>  
>  static QXLMessage *red_replay_message(SpiceReplay *replay)
>  {
> -QXLMessage *qxl;
> +QXLMessage *qxl = NULL;
>  size_t size;
>  
>  read_binary(replay, "message", , (uint8_t**),
> sizeof(QXLMessage));
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 1/8] replay: Rename eof to error

[Pavel Grunt]

On Thu, 2016-09-15 at 23:19 +0100, Frediano Ziglio wrote:
> The eof variable and enumeration will be used for all errors
> so avoid confusion.
Ok, ack

Pavel
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 38 +++---
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index b17c38b..7170292 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -36,12 +36,12 @@
>  
>  typedef enum {
>  REPLAY_OK = 0,
> -REPLAY_EOF,
> +REPLAY_ERROR,
>  } replay_t;
>  
>  struct SpiceReplay {
>  FILE *fd;
> -int eof;
> +gboolean error;
>  int counter;
>  bool created_primary;
>  
> @@ -56,11 +56,11 @@ struct SpiceReplay {
>  
>  static int replay_fread(SpiceReplay *replay, uint8_t *buf, size_t
> size)
>  {
> -if (replay->eof) {
> +if (replay->error) {
>  return 0;
>  }
>  if (feof(replay->fd)) {
> -replay->eof = 1;
> +replay->error = TRUE;
>  return 0;
>  }
>  return fread(buf, size, 1, replay->fd);
> @@ -72,20 +72,20 @@ static replay_t replay_fscanf(SpiceReplay
> *replay, const char *fmt, ...)
>  va_list ap;
>  int ret;
>  
> -if (replay->eof) {
> -return REPLAY_EOF;
> +if (replay->error) {
> +return REPLAY_ERROR;
>  }
>  if (feof(replay->fd)) {
> -replay->eof = 1;
> -return REPLAY_EOF;
> +replay->error = TRUE;
> +return REPLAY_ERROR;
>  }
>  va_start(ap, fmt);
>  ret = vfscanf(replay->fd, fmt, ap);
>  va_end(ap);
>  if (ret == EOF) {
> -replay->eof = 1;
> +replay->error = TRUE;
>  }
> -return replay->eof ? REPLAY_EOF : REPLAY_OK;
> +return replay->error ? REPLAY_ERROR : REPLAY_OK;
>  }
>  
>  static uint32_t replay_id_get(SpiceReplay *replay, uint32_t id)
> @@ -195,8 +195,8 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>  z_stream strm;
>  
>  snprintf(template, sizeof(template), "binary %%d %s %%ld:",
> prefix);
> -if (replay_fscanf(replay, template, _zlib, size) ==
> REPLAY_EOF)
> -return REPLAY_EOF;
> +if (replay_fscanf(replay, template, _zlib, size) ==
> REPLAY_ERROR)
> +return REPLAY_ERROR;
>  
>  if (*buf == NULL) {
>  *buf = spice_malloc(*size + base_size);
> @@ -208,7 +208,7 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>  hexdump(*buf + base_size, *size);
>  }
>  #else
> -spice_return_val_if_fail(with_zlib != -1, REPLAY_EOF);
> +spice_return_val_if_fail(with_zlib != -1, REPLAY_ERROR);
>  if (with_zlib) {
>  int ret;
>  
> @@ -234,7 +234,7 @@ static replay_t read_binary(SpiceReplay *replay,
> const char *prefix, size_t *siz
>   * it seems it may kill the red_worker thread (so a
> chunk is
>   * left hanging and the rest of the message is
> never written).
>   * Let it pass */
> -return REPLAY_EOF;
> +return REPLAY_ERROR;
>  }
>  if (ret != Z_OK) {
>  spice_warn_if_reached();
> @@ -263,7 +263,7 @@ static size_t red_replay_data_chunks(SpiceReplay
> *replay, const char *prefix,
>  base_size = sizeof(QXLDataChunk);
>  }
>  
> -if (read_binary(replay, prefix, _data_size, mem,
> base_size) == REPLAY_EOF) {
> +if (read_binary(replay, prefix, _data_size, mem,
> base_size) == REPLAY_ERROR) {
>  return 0;
>  }
>  cur = (QXLDataChunk*)(*mem + base_size - sizeof(QXLDataChunk));
> @@ -272,7 +272,7 @@ static size_t red_replay_data_chunks(SpiceReplay
> *replay, const char *prefix,
>  cur->next_chunk = cur->prev_chunk = 0;
>  while (count_chunks-- > 0) {
>  if (read_binary(replay, prefix, _data_size,
> (uint8_t**)>next_chunk,
> -sizeof(QXLDataChunk)) == REPLAY_EOF) {
> +sizeof(QXLDataChunk)) == REPLAY_ERROR) {
>  return 0;
>  }
>  data_size += next_data_size;
> @@ -981,7 +981,7 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
>  
>  static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay,
> uint32_t flags)
>  {
> -if (replay->eof) {
> +if (replay->error) {
>  return 0;
>  }
>  replay_fscanf(replay, "drawable\n");
> @@ -1194,7 +1194,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
>  while (what != 0) {
>  replay_fscanf(replay, "event %d %d %d %"PRIu64"\n",
> ,
>  , , );
> -if (replay->eof) {
> +if (replay->error) {
>  return NULL;
>  }
>  if (what == 1) {
> @@ -1296,7 +1296,7 @@ SpiceReplay *spice_replay_new(FILE *file, int
> nsurfaces)
>  
>  replay = spice_malloc0(sizeof(SpiceReplay));
>  
> -

Re: [Spice-devel] [PATCH v2 5/8] replay: Remove useless check

[Pavel Grunt]

On Thu, 2016-09-15 at 23:19 +0100, Frediano Ziglio wrote:
> Same check is done inside replay_fscanf

But removing it will not prevent calling red_replay_compat_drawable or
red_replay_native_drawable

Maybe you want to check the return value of replay_fscanf

Pavel

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 28da13d..ef5569e 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -1063,9 +1063,6 @@ static QXLCompatDrawable
> *red_replay_compat_drawable(SpiceReplay *replay, uint32
>  
>  static QXLPHYSICAL red_replay_drawable(SpiceReplay *replay,
> uint32_t flags)
>  {
> -if (replay->error) {
> -return 0;
> -}
>  replay_fscanf(replay, "drawable\n");
>  if (flags & QXL_COMMAND_FLAG_COMPAT) {
>  return
> QXLPHYSICAL_FROM_PTR(red_replay_compat_drawable(replay, flags));
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice v3 1/2] agent-filter: Use enum as return value

[Pavel Grunt]

Explicitely discard AGENT_MSG_FILTER_MONITORS_CONFIG messages
from the agent.

Also remove unused AGENT_MSG_FILTER_END
---
v3: discard AGENT_MSG_FILTER_MONITORS_CONFIG
---
 server/agent-msg-filter.c |  4 ++--
 server/agent-msg-filter.h | 11 +--
 server/reds.c |  6 --
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/server/agent-msg-filter.c b/server/agent-msg-filter.c
index a11f624..7921fe7 100644
--- a/server/agent-msg-filter.c
+++ b/server/agent-msg-filter.c
@@ -48,8 +48,8 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
 filter->discard_all = discard_all;
 }
 
-int agent_msg_filter_process_data(AgentMsgFilter *filter,
-  const uint8_t *data, uint32_t len)
+AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
+   const uint8_t *data, 
uint32_t len)
 {
 struct VDAgentMessage msg_header;
 
diff --git a/server/agent-msg-filter.h b/server/agent-msg-filter.h
index d61f8d4..b4d8e72 100644
--- a/server/agent-msg-filter.h
+++ b/server/agent-msg-filter.h
@@ -25,17 +25,16 @@
 #include 
 
 /* Possible return values for agent_msg_filter_process_data */
-enum {
+typedef enum {
 AGENT_MSG_FILTER_OK,
 AGENT_MSG_FILTER_DISCARD,
 AGENT_MSG_FILTER_PROTO_ERROR,
 AGENT_MSG_FILTER_MONITORS_CONFIG,
-AGENT_MSG_FILTER_END
-};
+} AgentMsgFilterResult;
 
 typedef struct AgentMsgFilter {
 int msg_data_to_read;
-int result;
+AgentMsgFilterResult result;
 gboolean copy_paste_enabled;
 gboolean file_xfer_enabled;
 gboolean use_client_monitors_config;
@@ -49,7 +48,7 @@ void agent_msg_filter_init(AgentMsgFilter *filter,
 void agent_msg_filter_config(AgentMsgFilter *filter,
  gboolean copy_paste, gboolean file_xfer,
  gboolean use_client_monitors_config);
-int agent_msg_filter_process_data(AgentMsgFilter *filter,
-  const uint8_t *data, uint32_t len);
+AgentMsgFilterResult agent_msg_filter_process_data(AgentMsgFilter *filter,
+   const uint8_t *data, 
uint32_t len);
 
 #endif
diff --git a/server/reds.c b/server/reds.c
index a387eeb..3addc1e 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -767,7 +767,7 @@ static void vdi_port_read_buf_release(uint8_t *data, void 
*opaque)
 static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
   RedVDIReadBuf *buf, gboolean *error)
 {
-int res;
+AgentMsgFilterResult res;
 
 *error = FALSE;
 
@@ -778,6 +778,8 @@ static gboolean 
vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
 switch (res) {
 case AGENT_MSG_FILTER_OK:
 return TRUE;
+case AGENT_MSG_FILTER_MONITORS_CONFIG:
+/* fall through */
 case AGENT_MSG_FILTER_DISCARD:
 return FALSE;
 case AGENT_MSG_FILTER_PROTO_ERROR:
@@ -1199,7 +1201,7 @@ void reds_on_main_agent_data(RedsState *reds, 
MainChannelClient *mcc, void *mess
 {
 RedCharDeviceVDIPort *dev = reds->agent_dev;
 VDIChunkHeader *header;
-int res;
+AgentMsgFilterResult res;
 
 res = agent_msg_filter_process_data(>agent_dev->priv->write_filter,
 message, size);
-- 
2.10.0

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

[Spice-devel] [PATCH spice v3 2/2] reds: Simplify vdi_port_read_buf_process

[Pavel Grunt]

Reuse and handle the return value from agent_msg_filter_process_data
---
v3: discard AGENT_MSG_FILTER_MONITORS_CONFIG
---
 server/reds.c | 66 ++-
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 3addc1e..8e9dc7b 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -763,36 +763,23 @@ static void vdi_port_read_buf_release(uint8_t *data, void 
*opaque)
 red_pipe_item_unref((RedPipeItem *)opaque);
 }
 
-/* returns TRUE if the buffer can be forwarded */
-static gboolean vdi_port_read_buf_process(RedCharDeviceVDIPort *dev,
-  RedVDIReadBuf *buf, gboolean *error)
+/*
+returns the #AgentMsgFilterResult value:
+AGENT_MSG_FILTER_OK if the buffer can be forwarded,
+AGENT_MSG_FILTER_PROTO_ERROR on error
+other values can be discarded
+*/
+static AgentMsgFilterResult vdi_port_read_buf_process(RedCharDeviceVDIPort 
*dev,
+  RedVDIReadBuf *buf)
 {
-AgentMsgFilterResult res;
-
-*error = FALSE;
-
 switch (dev->priv->vdi_chunk_header.port) {
-case VDP_CLIENT_PORT: {
-res = agent_msg_filter_process_data(>priv->read_filter,
-buf->data, buf->len);
-switch (res) {
-case AGENT_MSG_FILTER_OK:
-return TRUE;
-case AGENT_MSG_FILTER_MONITORS_CONFIG:
-/* fall through */
-case AGENT_MSG_FILTER_DISCARD:
-return FALSE;
-case AGENT_MSG_FILTER_PROTO_ERROR:
-*error = TRUE;
-return FALSE;
-}
-}
+case VDP_CLIENT_PORT:
+return agent_msg_filter_process_data(>priv->read_filter, 
buf->data, buf->len);
 case VDP_SERVER_PORT:
-return FALSE;
+return AGENT_MSG_FILTER_DISCARD;
 default:
 spice_warning("invalid port");
-*error = TRUE;
-return FALSE;
+return AGENT_MSG_FILTER_PROTO_ERROR;
 }
 }
 
@@ -883,7 +870,6 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
 dev->priv->read_state = VDI_PORT_READ_STATE_READ_DATA;
 }
 case VDI_PORT_READ_STATE_READ_DATA: {
-gboolean error = FALSE;
 n = sif->read(reds->vdagent, dev->priv->receive_pos, 
dev->priv->receive_len);
 if (!n) {
 return NULL;
@@ -902,12 +888,15 @@ static RedPipeItem 
*vdi_port_read_one_msg_from_device(SpiceCharDeviceInstance *s
 } else {
 dev->priv->read_state = VDI_PORT_READ_STATE_GET_BUFF;
 }
-if (vdi_port_read_buf_process(reds->agent_dev, dispatch_buf, 
)) {
+switch (vdi_port_read_buf_process(reds->agent_dev, dispatch_buf)) {
+case AGENT_MSG_FILTER_OK:
 return _buf->base;
-} else {
-if (error) {
-reds_agent_remove(reds);
-}
+case AGENT_MSG_FILTER_PROTO_ERROR:
+reds_agent_remove(reds);
+/* fall through */
+case AGENT_MSG_FILTER_MONITORS_CONFIG:
+/* fall through */
+case AGENT_MSG_FILTER_DISCARD:
 red_pipe_item_unref(_buf->base);
 }
 }
@@ -1277,22 +1266,25 @@ void reds_on_main_channel_migrate(RedsState *reds, 
MainChannelClient *mcc)
 if (agent_dev->priv->read_filter.msg_data_to_read ||
 read_data_len > sizeof(VDAgentMessage)) { /* msg header has been read 
*/
 RedVDIReadBuf *read_buf = agent_dev->priv->current_read_buf;
-gboolean error = FALSE;
 
 spice_debug("push partial read %u (msg first chunk? %d)", 
read_data_len,
 !agent_dev->priv->read_filter.msg_data_to_read);
 
 read_buf->len = read_data_len;
-if (vdi_port_read_buf_process(reds->agent_dev, read_buf, )) {
+switch (vdi_port_read_buf_process(reds->agent_dev, read_buf)) {
+case AGENT_MSG_FILTER_OK:
 main_channel_client_push_agent_data(mcc,
 read_buf->data,
 read_buf->len,
 vdi_port_read_buf_release,
 read_buf);
-} else {
-if (error) {
-   reds_agent_remove(reds);
-}
+break;
+case AGENT_MSG_FILTER_PROTO_ERROR:
+reds_agent_remove(reds);
+/* fall through */
+case AGENT_MSG_FILTER_MONITORS_CONFIG:
+/* fall through */
+case AGENT_MSG_FILTER_DISCARD:
 red_pipe_item_unref(_buf->base);
 }
 
-- 
2.10.0

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

[Spice-devel] [PATCH 1/2] fixup! Add DisplayChannelPrivate struct

[Frediano Ziglio]

Remove unneeded header inclusion.

Signed-off-by: Frediano Ziglio 
---
 server/stream.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/stream.c b/server/stream.c
index c3877c7..4732dee 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -20,7 +20,6 @@
 
 #include "stream.h"
 #include "display-channel.h"
-#include "main-channel-client.h"
 
 #define FPS_TEST_INTERVAL 1
 #define FOREACH_STREAMS(display, item)  \
-- 
2.7.4

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

[Spice-devel] [PATCH 2/2] fixup! Add DisplayChannelPrivate struct

[Frediano Ziglio]

Remove added FIXME introducing an helper function.

Signed-off-by: Frediano Ziglio 
---
 server/display-channel.c | 12 
 server/display-channel.h |  2 ++
 server/red-worker.c  | 16 ++--
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index d193925..6682d77 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2099,3 +2099,15 @@ void display_channel_reset_image_cache(DisplayChannel 
*self)
 {
 image_cache_reset(>priv->image_cache);
 }
+
+void display_channel_debug_oom(DisplayChannel *display, const char *msg)
+{
+RedChannel *channel = RED_CHANNEL(display);
+
+spice_debug("%s #draw=%u, #glz_draw=%u current %u pipes %u",
+msg,
+display->priv->drawable_count,
+display->priv->encoder_shared_data.glz_drawable_count,
+display->priv->current_size,
+red_channel_sum_pipes_size(channel));
+}
diff --git a/server/display-channel.h b/server/display-channel.h
index ce5d419..ab344fa 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -306,6 +306,8 @@ void set_monitors_config_to_primary(DisplayChannel 
*display);
 gboolean display_channel_validate_surface(DisplayChannel *display, uint32_t 
surface_id);
 void display_channel_reset_image_cache(DisplayChannel *self);
 
+void display_channel_debug_oom(DisplayChannel *display, const char *msg);
+
 static inline int is_equal_path(SpicePath *path1, SpicePath *path2)
 {
 SpicePathSeg *seg1, *seg2;
diff --git a/server/red-worker.c b/server/red-worker.c
index 92ab59c..2cde7fd 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -773,13 +773,7 @@ static void handle_dev_oom(void *opaque, void *payload)
 
 spice_return_if_fail(worker->running);
 // streams? but without streams also leak
-#if FIXME
-spice_debug("OOM1 #draw=%u, #glz_draw=%u current %u pipes %u",
-display->drawable_count,
-display->encoder_shared_data.glz_drawable_count,
-display->current_size,
-red_channel_sum_pipes_size(display_red_channel));
-#endif
+display_channel_debug_oom(display, "OOM1");
 while (red_process_display(worker, _is_empty)) {
 red_channel_push(display_red_channel);
 }
@@ -787,13 +781,7 @@ static void handle_dev_oom(void *opaque, void *payload)
 display_channel_free_some(worker->display_channel);
 red_qxl_flush_resources(worker->qxl);
 }
-#if FIXME
-spice_debug("OOM2 #draw=%u, #glz_draw=%u current %u pipes %u",
-display->drawable_count,
-display->encoder_shared_data.glz_drawable_count,
-display->current_size,
-red_channel_sum_pipes_size(display_red_channel));
-#endif
+display_channel_debug_oom(display, "OOM2");
 red_qxl_clear_pending(worker->qxl->st, RED_DISPATCHER_PENDING_OOM);
 }
 
-- 
2.7.4

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

Re: [Spice-devel] [PATCH v2 2/2] Add DisplayChannelPrivate struct

[Frediano Ziglio]

> 
> On Thu, 2016-09-15 at 15:06 -0500, Jonathon Jongsma wrote:
> > On Thu, 2016-09-15 at 11:22 -0500, Jonathon Jongsma wrote:
> > > Move all of the DisplayChannel data memembers into a private
> > > struct
...

> > > @@ -783,11 +787,13 @@ static void handle_dev_oom(void *opaque,
> > > void
> > > *payload)
> > >  display_channel_free_some(worker->display_channel);
> > >  red_qxl_flush_resources(worker->qxl);
> > >  }
> > > +#if FIXME
> > >  spice_debug("OOM2 #draw=%u, #glz_draw=%u current %u pipes
> > > %u",
> > >  display->drawable_count,
> > >  display->encoder_shared_data.glz_drawable_count,
> > >  display->current_size,
> > >  red_channel_sum_pipes_size(display_red_channel));
> > > +#endif
> 
> I am ok to remove these debugs, do you have an idea how to fix them
> 

I would add a display_channel_debug_oom function.
I'll try to send some fixup.

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

Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel

[Frediano Ziglio]

> 
> Add a couple new functions to the header so that they can be called by
> other objects rather than poking into the internals of the struct.
> ---
>  server/dcc-send.c| 16 +--
>  server/display-channel.c | 71
>  
>  server/display-channel.h | 37 +
>  server/red-worker.c  | 44 ++
>  server/stream.c  | 18 ++--
>  5 files changed, 109 insertions(+), 77 deletions(-)
> 

> diff --git a/server/display-channel.c b/server/display-channel.c
> index 108e69b..56bb029 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -203,6 +203,13 @@ void display_channel_surface_unref(DisplayChannel
> *display, uint32_t surface_id)
>  spice_warn_if_fail(ring_is_empty(>depend_on_me));
>  }
>  
> +/* TODO: perhaps rename to "ready" or "realized" ? */
> +bool display_channel_surface_has_canvas(DisplayChannel *display,
> +uint32_t surface_id)
> +{
> +return display->surfaces[surface_id].context.canvas != NULL;
> +}
> +

I honestly prefer bool and true/false but we decided to use gboolean and not 
bool.
This function is mainly doing the same think of validate_surface without
the logging stuff.
Perhaps adding a flag "warnings" to validate_surface is enough?


...

> diff --git a/server/display-channel.h b/server/display-channel.h
> index 7b71480..022cd93 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -208,10 +208,17 @@ struct DisplayChannel {
>  ImageEncoderSharedData encoder_shared_data;
>  };
>  
> -static inline int get_stream_id(DisplayChannel *display, Stream *stream)
> -{
> -return (int)(stream - display->streams_buf);
> -}
> +
> +#define FOREACH_DCC(channel, _link, _next, _data)   \
> +for (_link = (channel ? RED_CHANNEL(channel)->clients : NULL), \
> + _next = (_link ? _link->next : NULL), \
> + _data = (_link ? _link->data : NULL); \
> + _link; \
> + _link = _next, \
> + _next = (_link ? _link->next : NULL), \
> + _data = (_link ? _link->data : NULL))
> +
> +int display_channel_get_stream_id(DisplayChannel *display, Stream *stream);
>  
>  typedef struct RedSurfaceDestroyItem {
>  RedPipeItem pipe_item;
> @@ -258,6 +265,8 @@ void
> display_channel_compress_stats_print  (const Disp
>  void   display_channel_compress_stats_reset
>  (DisplayChannel *display);
>  void   display_channel_surface_unref
>  (DisplayChannel *display,
>
> uint32_t
>
> surface_id);
> +bool   display_channel_surface_has_canvas
> (DisplayChannel *display,
> +

Although this is consistent with the rest it does not follow our style.

> uint32_t
> surface_id);
>  void   display_channel_current_flush
>  (DisplayChannel *display,
>int
>
> surface_id);
>  intdisplay_channel_wait_for_migrate_data
>  (DisplayChannel *display);
...

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

[Spice-devel] [PATCH] fixup! RedChannelClient: store pipe items in a GQueue

[Frediano Ziglio]

Minor changes:
- use same name for dcc_add_surface_area_image argument in header
  and source;
- avoid using 2 variable in a for loop, is not much readable and
  confusing. This also was fixing a regression quite hard to spot
  so make sure code is less easy to break in the future;
- remove check in red_drawable_pipe_item_free. Now RedDrawablePipeItem
  is not forced to know in which container is it and the check should
  be up to container. Also this was potentially O(n) expensive with
  the GQueue implementation.

Signed-off-by: Frediano Ziglio 
---
 server/dcc.c | 31 +++
 server/dcc.h |  2 +-
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 184a944..cca3ce5 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -68,7 +68,7 @@ int dcc_drawable_is_in_pipe(DisplayChannelClient *dcc, 
Drawable *drawable)
 int dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int 
surface_id,
   int wait_if_used)
 {
-GList *l, *item_pos = NULL;
+GList *l;
 int x;
 RedChannelClient *rcc;
 
@@ -77,13 +77,13 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
no other drawable depends on them */
 
 rcc = RED_CHANNEL_CLIENT(dcc);
-for (l = rcc->priv->pipe.head; l != NULL; item_pos = NULL) {
+for (l = rcc->priv->pipe.head; l != NULL; ) {
 Drawable *drawable;
 RedDrawablePipeItem *dpi = NULL;
 int depend_found = FALSE;
 RedPipeItem *item = l->data;
+GList *item_pos = l;
 
-item_pos = l;
 l = l->next;
 if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
 dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem, dpi_pipe_item);
@@ -108,11 +108,11 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
 
 if (depend_found) {
 spice_debug("surface %d dependent item found %p, %p", surface_id, 
drawable, item);
-if (wait_if_used) {
-break;
-} else {
+if (!wait_if_used) {
 return TRUE;
 }
+return red_channel_client_wait_pipe_item_sent(rcc, item_pos,
+  
COMMON_CLIENT_TIMEOUT);
 }
 }
 
@@ -120,18 +120,11 @@ int 
dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int surface
 return TRUE;
 }
 
-if (item_pos) {
-return red_channel_client_wait_pipe_item_sent(RED_CHANNEL_CLIENT(dcc), 
item_pos,
-  COMMON_CLIENT_TIMEOUT);
-} else {
-/*
- * in case that the pipe didn't contain any item that is dependent on 
the surface, but
- * there is one during sending. Use a shorter timeout, since it is 
just one item
- */
-return red_channel_client_wait_outgoing_item(RED_CHANNEL_CLIENT(dcc),
- 
DISPLAY_CLIENT_SHORT_TIMEOUT);
-}
-return TRUE;
+/*
+ * in case that the pipe didn't contain any item that is dependent on the 
surface, but
+ * there is one during sending. Use a shorter timeout, since it is just 
one item
+ */
+return red_channel_client_wait_outgoing_item(rcc, 
DISPLAY_CLIENT_SHORT_TIMEOUT);
 }
 
 void dcc_create_surface(DisplayChannelClient *dcc, int surface_id)
@@ -284,8 +277,6 @@ static void red_drawable_pipe_item_free(RedPipeItem *item)
  dpi_pipe_item);
 spice_assert(item->refcount == 0);
 
-
spice_warn_if_fail(!red_channel_client_pipe_item_is_linked(RED_CHANNEL_CLIENT(dpi->dcc),
-   
>dpi_pipe_item));
 if (ring_item_is_linked(>base)) {
 ring_remove(>base);
 }
diff --git a/server/dcc.h b/server/dcc.h
index 7f93186..a2478b9 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -129,7 +129,7 @@ void   dcc_push_surface_image   
 (DisplayCha
 RedImageItem * dcc_add_surface_area_image
(DisplayChannelClient *dcc,
   int 
surface_id,
   
SpiceRect *area,
-  GList 
*pos,
+  GList 
*pipe_item_pos,
   int 
can_lossy);
 void   dcc_palette_cache_reset   
(DisplayChannelClient *dcc);
 void   dcc_palette_cache_palette 
(DisplayChannelClient *dcc,
-- 
2.7.4

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

Re: [Spice-devel] [PATCH v2 2/2] Add DisplayChannelPrivate struct

[Pavel Grunt]

On Thu, 2016-09-15 at 15:06 -0500, Jonathon Jongsma wrote:
> On Thu, 2016-09-15 at 11:22 -0500, Jonathon Jongsma wrote:
> > Move all of the DisplayChannel data memembers into a private
> > struct
> > to
> > encapsulate things better. This necessitated a few new 'public'
> > methods
> > and a small bit of refactoring to avoid poking into DisplayChannel
> > internals from too many places. The DisplayChannel and the
> > DisplayChannelClient are still far too intertwined to completely
> > avoid
> > accessing private data, so at the moment the private struct is
> > defined
> > in display-channel.h and the DisplayChannelClient implementation
> > still accesses it sometimes.
> > ---
> >  server/dcc-send.c|  16 +--
> >  server/dcc.c |  33 ++---
> >  server/display-channel.c | 314 
> > -
> > --
> >  server/display-channel.h |  13 +-
> >  server/red-worker.c  |   6 +
> >  server/stream.c  |  53 
> >  6 files changed, 227 insertions(+), 208 deletions(-)
> > 
> > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > index 0afa25c..0635afa 100644
> > --- a/server/dcc-send.c
> > +++ b/server/dcc-send.c
> > @@ -96,7 +96,7 @@ static int
> > is_surface_area_lossy(DisplayChannelClient *dcc, uint32_t
> > surface_id,
> >  
> >  spice_return_val_if_fail(display_channel_validate_surface(dis
> > pla
> > y, surface_id), FALSE);
> >  
> > -surface = >surfaces[surface_id];
> > +surface = >priv->surfaces[surface_id];
> >  surface_lossy_region = >priv-
> > > surface_client_lossy_region[surface_id];
> > 
> >  
> >  if (!area) {
> > @@ -208,13 +208,13 @@ static void
> > red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> >  io_image->descriptor.flags |=
> > SPICE_IMAGE_FLAGS_CACHE_ME;
> >  dcc->priv->send_data.pixmap_cache_items[dcc-
> > >priv-
> > > send_data.num_pixmap_cache_items++] =
> > 
> >   
> >    
> >    image->descriptor.id;
> > -stat_inc_counter(reds, display_channel-
> > > add_to_cache_counter, 1);
> > 
> > +stat_inc_counter(reds, display_channel->priv-
> > > add_to_cache_counter, 1);
> > 
> >  }
> >  }
> >  }
> >  
> >  if (!(io_image->descriptor.flags &
> > SPICE_IMAGE_FLAGS_CACHE_ME))
> > {
> > -stat_inc_counter(reds, display_channel-
> > >non_cache_counter,
> > 1);
> > +stat_inc_counter(reds, display_channel->priv-
> > > non_cache_counter, 1);
> > 
> >  }
> >  }
> >  
> > @@ -385,7 +385,7 @@ static FillBitsType
> > fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> >  dcc->priv->send_data.pixmap_cache_items[dcc->priv-
> > > send_data.num_pixmap_cache_items++] =
> > 
> >  image.descriptor.id;
> >  if (can_lossy || !lossy_cache_item) {
> > -if (!display->enable_jpeg || lossy_cache_item) {
> > +if (!display->priv->enable_jpeg ||
> > lossy_cache_item)
> > {
> >  image.descriptor.type =
> > SPICE_IMAGE_TYPE_FROM_CACHE;
> >  } else {
> >  // making sure, in multiple monitor scenario,
> > that lossy items that
> > @@ -397,7 +397,7 @@ static FillBitsType
> > fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> >   _palette_out,
> > _palette_out);
> >  spice_assert(bitmap_palette_out == NULL);
> >  spice_assert(lzplt_palette_out == NULL);
> > -stat_inc_counter(reds, display-
> > >cache_hits_counter,
> > 1);
> > +stat_inc_counter(reds, display->priv-
> > > cache_hits_counter, 1);
> > 
> >  pthread_mutex_unlock(>priv->pixmap_cache-
> > > lock);
> > 
> >  return FILL_BITS_TYPE_CACHE;
> >  } else {
> > @@ -420,7 +420,7 @@ static FillBitsType
> > fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> >  return FILL_BITS_TYPE_SURFACE;
> >  }
> >  
> > -surface = >surfaces[surface_id];
> > +surface = >priv->surfaces[surface_id];
> >  image.descriptor.type = SPICE_IMAGE_TYPE_SURFACE;
> >  image.descriptor.flags = 0;
> >  image.descriptor.width = surface->context.width;
> > @@ -1862,7 +1862,7 @@ static void
> > display_channel_marshall_migrate_data(RedChannelClient *rcc,
> >  spice_marshaller_add(base_marshaller,
> >   (uint8_t *)_data,
> > sizeof(display_data) - sizeof(uint32_t));
> >  display_channel_marshall_migrate_data_surfaces(dcc,
> > base_marshaller,
> > -   display_channe
> > l-
> > > enable_jpeg);
> > 
> > +   display_channe
> > l-
> > > priv->enable_jpeg);
> > 
> >  }
> >  
> >  static void display_channel_marshall_pixmap_sync(RedChannelClient
> > *rcc,
> > @@ 

Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel

[Pavel Grunt]

On Thu, 2016-09-15 at 11:22 -0500, Jonathon Jongsma wrote:
> Add a couple new functions to the header so that they can be called
> by
> other objects rather than poking into the internals of the struct.
> ---
>  server/dcc-send.c| 16 +--
>  server/display-channel.c | 71
> 
>  server/display-channel.h | 37 +
>  server/red-worker.c  | 44 ++
>  server/stream.c  | 18 ++--
>  5 files changed, 109 insertions(+), 77 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 521e6a2..0afa25c 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -94,7 +94,7 @@ static int
> is_surface_area_lossy(DisplayChannelClient *dcc, uint32_t
> surface_id,
>  QRegion lossy_region;
>  DisplayChannel *display = DCC_TO_DC(dcc);
>  
> -spice_return_val_if_fail(validate_surface(display, surface_id),
> FALSE);
> +spice_return_val_if_fail(display_channel_validate_surface(displ
> ay, surface_id), FALSE);
>  
>  surface = >surfaces[surface_id];
>  surface_lossy_region = >priv-
> >surface_client_lossy_region[surface_id];
> @@ -414,7 +414,7 @@ static FillBitsType
> fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
>  RedSurface *surface;
>  
>  surface_id = simage->u.surface.surface_id;
> -if (!validate_surface(display, surface_id)) {
> +if (!display_channel_validate_surface(display, surface_id))
> {
>  spice_warning("Invalid surface in
> SPICE_IMAGE_TYPE_SURFACE");
>  pthread_mutex_unlock(>priv->pixmap_cache->lock);
>  return FILL_BITS_TYPE_SURFACE;
> @@ -1706,7 +1706,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
>  return FALSE;
>  }
>  
> -StreamAgent *agent = >priv-
> >stream_agents[get_stream_id(display, stream)];
> +StreamAgent *agent = >priv-
> >stream_agents[display_channel_get_stream_id(display, stream)];
>  uint64_t time_now = spice_get_monotonic_time_ns();
>  
>  if (!dcc->priv->use_video_encoder_rate_control) {
> @@ -1752,7 +1752,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
>  
>  red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DATA, NULL);
>  
> -stream_data.base.id = get_stream_id(display, stream);
> +stream_data.base.id =
> display_channel_get_stream_id(display, stream);
>  stream_data.base.multi_media_time = frame_mm_time;
>  stream_data.data_size = outbuf->size;
>  
> @@ -1762,7 +1762,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
>  
>  red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, NULL);
>  
> -stream_data.base.id = get_stream_id(display, stream);
> +stream_data.base.id =
> display_channel_get_stream_id(display, stream);
>  stream_data.base.multi_media_time = frame_mm_time;
>  stream_data.data_size = outbuf->size;
>  stream_data.width = copy->src_area.right - copy-
> >src_area.left;
> @@ -2171,7 +2171,7 @@ static void
> marshall_stream_start(RedChannelClient *rcc,
>  SpiceClipRects clip_rects;
>  
>  stream_create.surface_id = 0;
> -stream_create.id = get_stream_id(DCC_TO_DC(dcc), stream);
> +stream_create.id =
> display_channel_get_stream_id(DCC_TO_DC(dcc), stream);
>  stream_create.flags = stream->top_down ?
> SPICE_STREAM_FLAGS_TOP_DOWN : 0;
>  stream_create.codec_type = agent->video_encoder->codec_type;
>  
> @@ -2207,7 +2207,7 @@ static void
> marshall_stream_clip(RedChannelClient *rcc,
>  red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_CLIP, >base);
>  SpiceMsgDisplayStreamClip stream_clip;
>  
> -stream_clip.id = get_stream_id(DCC_TO_DC(dcc), agent->stream);
> +stream_clip.id = display_channel_get_stream_id(DCC_TO_DC(dcc),
> agent->stream);
>  stream_clip.clip.type = item->clip_type;
>  stream_clip.clip.rects = item->rects;
>  
> @@ -2221,7 +2221,7 @@ static void
> marshall_stream_end(RedChannelClient *rcc,
>  SpiceMsgDisplayStreamDestroy destroy;
>  
>  red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DESTROY, NULL);
> -destroy.id = get_stream_id(DCC_TO_DC(dcc), agent->stream);
> +destroy.id = display_channel_get_stream_id(DCC_TO_DC(dcc),
> agent->stream);
>  stream_agent_stop(agent);
>  spice_marshall_msg_display_stream_destroy(base_marshaller,
> );
>  }
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 108e69b..56bb029 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -203,6 +203,13 @@ void
> display_channel_surface_unref(DisplayChannel *display, uint32_t
> surface_id)
>  spice_warn_if_fail(ring_is_empty(>depend_on_me));
>  }
>  
> +/* TODO: perhaps rename to "ready" or "realized" ? */
> +bool display_channel_surface_has_canvas(DisplayChannel *display,
> +  

Re: [Spice-devel] [PATCH 1/3] Remove unused drawable parameter

[Pavel Grunt]

On Fri, 2016-09-16 at 08:30 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/dcc-send.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 521e6a2..317d906 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -131,7 +131,7 @@ static int
> is_surface_area_lossy(DisplayChannelClient *dcc, uint32_t
> surface_id,
> all the surface is considered. out_lossy_data will hold info
> about the bitmap, and its lossy
> area in case it is lossy and part of a surface. */
>  static int is_bitmap_lossy(RedChannelClient *rcc, SpiceImage
> *image, SpiceRect *area,
> -   Drawable *drawable, BitmapData
> *out_data)
> +   BitmapData *out_data)
>  {
>  DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
>  
> @@ -176,11 +176,11 @@ static int is_bitmap_lossy(RedChannelClient
> *rcc, SpiceImage *image, SpiceRect *
>  }
>  
>  static int is_brush_lossy(RedChannelClient *rcc, SpiceBrush *brush,
> -  Drawable *drawable, BitmapData *out_data)
> +  BitmapData *out_data)
>  {
>  if (brush->type == SPICE_BRUSH_TYPE_PATTERN) {
>  return is_bitmap_lossy(rcc, brush->u.pattern.pat, NULL,
> -   drawable, out_data);
> +   out_data);
>  } else {
>  out_data->type = BITMAP_DATA_TYPE_INVALID;
>  return FALSE;
> @@ -843,7 +843,7 @@ static void
> red_lossy_marshall_qxl_draw_fill(RedChannelClient *rcc,
> (rop & SPICE_ROPD_OP_AND) ||
> (rop & SPICE_ROPD_OP_XOR));
>  
> -brush_is_lossy = is_brush_lossy(rcc, >u.fill.brush,
> item,
> +brush_is_lossy = is_brush_lossy(rcc, >u.fill.brush,
>  _bitmap_data);
>  if (!dest_allowed_lossy) {
>  dest_is_lossy = is_surface_area_lossy(dcc, item-
> >surface_id, >bbox,
> @@ -933,13 +933,12 @@ static void
> red_lossy_marshall_qxl_draw_opaque(RedChannelClient *rcc,
>    (rop & SPICE_ROPD_OP_AND) ||
>    (rop & SPICE_ROPD_OP_XOR));
>  
> -brush_is_lossy = is_brush_lossy(rcc, >u.opaque.brush, 
> item,
> +brush_is_lossy = is_brush_lossy(rcc, >u.opaque.brush,
>  _bitmap_data);
>  
>  if (!src_allowed_lossy) {
>  src_is_lossy = is_bitmap_lossy(rcc, drawable-
> >u.opaque.src_bitmap,
> 
> >u.opaque.src_area,
> -   item,
> _bitmap_data);
>  }
>  
> @@ -1018,7 +1017,7 @@ static void
> red_lossy_marshall_qxl_draw_copy(RedChannelClient *rcc,
>  FillBitsType src_send_type;
>  
>  src_is_lossy = is_bitmap_lossy(rcc, drawable-
> >u.copy.src_bitmap,
> -   >u.copy.src_area,
> item, _bitmap_data);
> +   >u.copy.src_area,
> _bitmap_data);
>  
>  src_send_type = red_marshall_qxl_draw_copy(rcc,
> base_marshaller, dpi, TRUE);
>  if (src_send_type == FILL_BITS_TYPE_COMPRESS_LOSSY) {
> @@ -1060,7 +1059,7 @@ static void
> red_lossy_marshall_qxl_draw_transparent(RedChannelClient *rcc,
>  BitmapData src_bitmap_data;
>  
>  src_is_lossy = is_bitmap_lossy(rcc, drawable-
> >u.transparent.src_bitmap,
> -   
> >u.transparent.src_area, item, _bitmap_data);
> +   
> >u.transparent.src_area, _bitmap_data);
>  
>  if (!src_is_lossy || (src_bitmap_data.type !=
> BITMAP_DATA_TYPE_SURFACE)) {
>  red_marshall_qxl_draw_transparent(rcc, base_marshaller,
> dpi);
> @@ -1114,7 +1113,7 @@ static void
> red_lossy_marshall_qxl_draw_alpha_blend(RedChannelClient *rcc,
>  FillBitsType src_send_type;
>  
>  src_is_lossy = is_bitmap_lossy(rcc, drawable-
> >u.alpha_blend.src_bitmap,
> -   
> >u.alpha_blend.src_area, item, _bitmap_data);
> +   
> >u.alpha_blend.src_area, _bitmap_data);
>  
>  src_send_type = red_marshall_qxl_draw_alpha_blend(rcc,
> base_marshaller, dpi, TRUE);
>  
> @@ -1210,7 +1209,7 @@ static void
> red_lossy_marshall_qxl_draw_blend(RedChannelClient *rcc,
>  SpiceRect dest_lossy_area;
>  
>  src_is_lossy = is_bitmap_lossy(rcc, drawable-
> >u.blend.src_bitmap,
> -   >u.blend.src_area,
> item, _bitmap_data);
> +   >u.blend.src_area,
> _bitmap_data);
>  dest_is_lossy = is_surface_area_lossy(dcc, drawable-
> >surface_id,
>    >bbox,
> _lossy_area);
>  
> @@ -1377,8 +1376,8 @@ static void
> red_lossy_marshall_qxl_draw_rop3(RedChannelClient *rcc,
>  SpiceRect 

Re: [Spice-devel] [PATCH 3/3] Reduce indentation

[Pavel Grunt]

On Fri, 2016-09-16 at 08:30 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/dcc-send.c | 26 --
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 89c3dc5..1d32276 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -102,28 +102,26 @@ static int
> is_surface_area_lossy(DisplayChannelClient *dcc, uint32_t
> surface_id,
>  if (!area) {
>  if (region_is_empty(surface_lossy_region)) {
>  return FALSE;
> -} else {
> -out_lossy_area->top = 0;
> -out_lossy_area->left = 0;
> -out_lossy_area->bottom = surface->context.height;
> -out_lossy_area->right = surface->context.width;
> -return TRUE;
>  }
> +out_lossy_area->top = 0;
> +out_lossy_area->left = 0;
> +out_lossy_area->bottom = surface->context.height;
> +out_lossy_area->right = surface->context.width;
> +return TRUE;
>  }
>  
>  region_init(_region);
>  region_add(_region, area);
>  region_and(_region, surface_lossy_region);
> -if (!region_is_empty(_region)) {
> -out_lossy_area->left = lossy_region.extents.x1;
> -out_lossy_area->top = lossy_region.extents.y1;
> -out_lossy_area->right = lossy_region.extents.x2;
> -out_lossy_area->bottom = lossy_region.extents.y2;
> -region_destroy(_region);
> -return TRUE;
> -} else {
> +if (region_is_empty(_region)) {
>  return FALSE;
>  }
> +out_lossy_area->left = lossy_region.extents.x1;
> +out_lossy_area->top = lossy_region.extents.y1;
> +out_lossy_area->right = lossy_region.extents.x2;
> +out_lossy_area->bottom = lossy_region.extents.y2;
> +region_destroy(_region);
> +return TRUE;
>  }
>  
>  /* returns if the bitmap was already sent lossy to the client. If
> the bitmap hasn't been sent yet
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 6/9] Change Drawable->pipes from Ring to GList

[Frediano Ziglio]

> 
> This improves the readability of the code and keeps things
> encapsulated better.
> ---
> Changes in v2:
>  - changed some loops from while to for
>  - moved some declarations within loop scope
> 
>  server/dcc.c | 12 +---
>  server/dcc.h |  1 -
>  server/display-channel.c | 34 +-
>  server/display-channel.h |  6 +-
>  server/stream.c  |  8 ++--
>  5 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index ddc1a59..922fbef 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -50,9 +50,10 @@ static RedSurfaceCreateItem
> *red_surface_create_item_new(RedChannel* channel,
>  int dcc_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable *drawable)
>  {
>  RedDrawablePipeItem *dpi;
> -RingItem *dpi_link, *dpi_next;
> +GList *l;
>  
> -DRAWABLE_FOREACH_DPI_SAFE(drawable, dpi_link, dpi_next, dpi) {
> +for (l = drawable->pipes; l != NULL; l = l->next) {
> +dpi = l->data;
>  if (dpi->dcc == dcc) {
>  return TRUE;
>  }
> @@ -286,9 +287,7 @@ static void red_drawable_pipe_item_free(RedPipeItem
> *item)
>  
>  
> spice_warn_if_fail(!red_channel_client_pipe_item_is_linked(RED_CHANNEL_CLIENT(dpi->dcc),
> 
> >dpi_pipe_item));
> -if (ring_item_is_linked(>base)) {
> -ring_remove(>base);
> -}
> +dpi->drawable->pipes = g_list_remove(dpi->drawable->pipes, dpi);
>  drawable_unref(dpi->drawable);
>  free(dpi);
>  }
> @@ -301,8 +300,7 @@ static RedDrawablePipeItem
> *red_drawable_pipe_item_new(DisplayChannelClient *dcc
>  dpi = spice_malloc0(sizeof(*dpi));
>  dpi->drawable = drawable;
>  dpi->dcc = dcc;
> -ring_item_init(>base);
> -ring_add(>pipes, >base);
> +drawable->pipes = g_list_prepend(drawable->pipes, dpi);
>  red_pipe_item_init_full(>dpi_pipe_item, RED_PIPE_ITEM_TYPE_DRAW,
>  red_drawable_pipe_item_free);
>  drawable->refs++;
> diff --git a/server/dcc.h b/server/dcc.h
> index 7f93186..9dac8a3 100644
> --- a/server/dcc.h
> +++ b/server/dcc.h
> @@ -91,7 +91,6 @@ typedef struct RedImageItem {
>  } RedImageItem;
>  
>  typedef struct RedDrawablePipeItem {
> -RingItem base;  /* link for a list of pipe items held by Drawable */
>  RedPipeItem dpi_pipe_item; /* link for the client's pipe itself */
>  Drawable *drawable;
>  DisplayChannelClient *dcc;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e37b5ae..426b9c0 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -255,7 +255,7 @@ static void pipes_add_drawable(DisplayChannel *display,
> Drawable *drawable)
>  DisplayChannelClient *dcc;
>  GList *link, *next;
>  
> -spice_warn_if_fail(ring_is_empty(>pipes));
> +spice_warn_if_fail(drawable->pipes == NULL);
>  FOREACH_CLIENT(display, link, next, dcc) {
>  dcc_prepend_drawable(dcc, drawable);
>  }
> @@ -265,14 +265,17 @@ static void pipes_add_drawable_after(DisplayChannel
> *display,
>   Drawable *drawable, Drawable
>   *pos_after)
>  {
>  RedDrawablePipeItem *dpi_pos_after;
> -RingItem *dpi_link, *dpi_next;
>  DisplayChannelClient *dcc;
>  int num_other_linked = 0;
> +GList *l;
> +
> +for (l = pos_after->pipes; l != NULL; l = l->next) {
> +dpi_pos_after = l->data;
>  
> -DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next, dpi_pos_after)
> {
>  num_other_linked++;
>  dcc_add_drawable_after(dpi_pos_after->dcc, drawable,
>  _pos_after->dpi_pipe_item);
>  }
> +
>  if (num_other_linked == 0) {
>  pipes_add_drawable(display, drawable);
>  return;
> @@ -282,12 +285,15 @@ static void pipes_add_drawable_after(DisplayChannel
> *display,
>  spice_debug("TODO: not O(n^2)");
>  FOREACH_CLIENT(display, link, next, dcc) {
>  int sent = 0;
> -DRAWABLE_FOREACH_DPI_SAFE(pos_after, dpi_link, dpi_next,
> dpi_pos_after) {
> +GList *l;
> +for (l = pos_after->pipes; l != NULL; l = l->next) {
> +dpi_pos_after = l->data;
>  if (dpi_pos_after->dcc == dcc) {
>  sent = 1;
>  break;
>  }
>  }
> +
>  if (!sent) {
>  dcc_prepend_drawable(dcc, drawable);
>  }
> @@ -324,14 +330,17 @@ static void current_remove_drawable(DisplayChannel
> *display, Drawable *item)
>  static void drawable_remove_from_pipes(Drawable *drawable)
>  {
>  RedDrawablePipeItem *dpi;
> -RingItem *item, *next;
> +GList *l;
>  
> -RING_FOREACH_SAFE(item, next, >pipes) {
> +l = drawable->pipes;
> +while (l) {
> +GList *next = l->next;
>  RedChannelClient 

Re: [Spice-devel] [PATCH 2/3] Simplify some boolean arithmetic

[Pavel Grunt]

On Fri, 2016-09-16 at 08:30 +0100, Frediano Ziglio wrote:
> Pass boolean directly.
> 
> Signed-off-by: Frediano Ziglio 

Acked-by: Pavel Grunt 

> ---
>  server/dcc-send.c | 15 +++
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 317d906..89c3dc5 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -147,11 +147,7 @@ static int is_bitmap_lossy(RedChannelClient
> *rcc, SpiceImage *image, SpiceRect *
>  out_data->id = image->descriptor.id;
>  if (dcc_pixmap_cache_hit(dcc, image->descriptor.id,
> _hit_lossy)) {
>  out_data->type = BITMAP_DATA_TYPE_CACHE;
> -if (is_hit_lossy) {
> -return TRUE;
> -} else {
> -return FALSE;
> -}
> +return is_hit_lossy;
>  } else {
>  out_data->type = BITMAP_DATA_TYPE_BITMAP_TO_CACHE;
>  }
> @@ -166,13 +162,8 @@ static int is_bitmap_lossy(RedChannelClient
> *rcc, SpiceImage *image, SpiceRect *
>  out_data->type = BITMAP_DATA_TYPE_SURFACE;
>  out_data->id = image->u.surface.surface_id;
>  
> -if (is_surface_area_lossy(dcc, out_data->id,
> -  area, _data->lossy_rect))
> -{
> -return TRUE;
> -} else {
> -return FALSE;
> -}
> +return is_surface_area_lossy(dcc, out_data->id,
> + area, _data->lossy_rect);
>  }
>  
>  static int is_brush_lossy(RedChannelClient *rcc, SpiceBrush *brush,
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 4/9] Replace a couple Rings with GList

[Frediano Ziglio]

> 
> Make RedsState::mig_target_clients into a GList to improve encapsulation
> and maintainability. Also RedsMigTargetClient::pending_links. With
> GList, a type implementation can be ignorant of whether they're
> contained within a list or not.

Acked-by: Frediano Ziglio 

Frediano

> ---
> Changes in v2:
>  - don't change memory allocator -- use spice_new0() not g_new0()
> 
>  server/reds-private.h |  6 ++
>  server/reds.c | 56
>  ++-
>  2 files changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/server/reds-private.h b/server/reds-private.h
> index 0408f25..29645bf 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -61,16 +61,14 @@ typedef struct RedsStatValue {
>  #endif
>  
>  typedef struct RedsMigPendingLink {
> -RingItem ring_link; // list of links that belongs to the same client
>  SpiceLinkMess *link_msg;
>  RedsStream *stream;
>  } RedsMigPendingLink;
>  
>  typedef struct RedsMigTargetClient {
>  RedsState *reds;
> -RingItem link;
>  RedClient *client;
> -Ring pending_links;
> +GList *pending_links;
>  } RedsMigTargetClient;
>  
>  /* Intermediate state for on going monitors config message from a single
> @@ -122,7 +120,7 @@ struct RedsState {
>  between the 2 servers */
>  int dst_do_seamless_migrate; /* per migration. Updated after the
>  migration handshake
>  between the 2 servers */
> -Ring mig_target_clients;
> +GList *mig_target_clients;
>  
>  int num_of_channels;
>  Ring channels;
> diff --git a/server/reds.c b/server/reds.c
> index 800107b..a387eeb 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -295,7 +295,7 @@ static RedCharDeviceVDIPort
> *red_char_device_vdi_port_new(RedsState *reds);
>  
>  static void migrate_timeout(void *opaque);
>  static RedsMigTargetClient* reds_mig_target_client_find(RedsState *reds,
>  RedClient *client);
> -static void reds_mig_target_client_free(RedsMigTargetClient *mig_client);
> +static void reds_mig_target_client_free(RedsState *reds, RedsMigTargetClient
> *mig_client);
>  static void reds_mig_cleanup_wait_disconnect(RedsState *reds);
>  static void reds_mig_remove_wait_disconnect_client(RedsState *reds,
>  RedClient *client);
>  static void reds_add_char_device(RedsState *reds, RedCharDevice *dev);
> @@ -596,7 +596,7 @@ void reds_client_disconnect(RedsState *reds, RedClient
> *client)
>  
>  mig_client = reds_mig_target_client_find(reds, client);
>  if (mig_client) {
> -reds_mig_target_client_free(mig_client);
> +reds_mig_target_client_free(reds, mig_client);
>  }
>  
>  if (reds->mig_wait_disconnect) {
> @@ -1678,24 +1678,21 @@ static void reds_mig_target_client_add(RedsState
> *reds, RedClient *client)
>  {
>  RedsMigTargetClient *mig_client;
>  
> -spice_assert(reds);
> +g_return_if_fail(reds);
>  spice_info(NULL);
> -mig_client = spice_malloc0(sizeof(RedsMigTargetClient));
> +mig_client = spice_new0(RedsMigTargetClient, 1);
>  mig_client->client = client;
>  mig_client->reds = reds;
> -ring_init(_client->pending_links);
> -ring_add(>mig_target_clients, _client->link);
> -
> +reds->mig_target_clients = g_list_append(reds->mig_target_clients,
> mig_client);
>  }
>  
>  static RedsMigTargetClient* reds_mig_target_client_find(RedsState *reds,
>  RedClient *client)
>  {
> -RingItem *item;
> +GList *l;
>  
> -RING_FOREACH(item, >mig_target_clients) {
> -RedsMigTargetClient *mig_client;
> +for (l = reds->mig_target_clients; l != NULL; l = l->next) {
> +RedsMigTargetClient *mig_client = l->data;
>  
> -mig_client = SPICE_CONTAINEROF(item, RedsMigTargetClient, link);
>  if (mig_client->client == client) {
>  return mig_client;
>  }
> @@ -1710,34 +1707,30 @@ static void
> reds_mig_target_client_add_pending_link(RedsMigTargetClient *client,
>  RedsMigPendingLink *mig_link;
>  
>  spice_assert(client);
> -mig_link = spice_malloc0(sizeof(RedsMigPendingLink));
> +mig_link = spice_new0(RedsMigPendingLink, 1);
>  mig_link->link_msg = link_msg;
>  mig_link->stream = stream;
>  
> -ring_add(>pending_links, _link->ring_link);
> +client->pending_links = g_list_append(client->pending_links, mig_link);
>  }
>  
> -static void reds_mig_target_client_free(RedsMigTargetClient *mig_client)
> +static void reds_mig_target_client_free(RedsState *reds, RedsMigTargetClient
> *mig_client)
>  {
> -RingItem *now, *next;
> -
> -ring_remove(_client->link);
> -
> -RING_FOREACH_SAFE(now, next, _client->pending_links) {
> -RedsMigPendingLink *mig_link = SPICE_CONTAINEROF(now,
> RedsMigPendingLink, ring_link);
> -ring_remove(now);
> -free(mig_link);
> -}
> +reds->mig_target_clients = g_list_remove(reds->mig_target_clients,
> 

[Spice-devel] [PATCH 3/3] Reduce indentation

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 89c3dc5..1d32276 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -102,28 +102,26 @@ static int is_surface_area_lossy(DisplayChannelClient 
*dcc, uint32_t surface_id,
 if (!area) {
 if (region_is_empty(surface_lossy_region)) {
 return FALSE;
-} else {
-out_lossy_area->top = 0;
-out_lossy_area->left = 0;
-out_lossy_area->bottom = surface->context.height;
-out_lossy_area->right = surface->context.width;
-return TRUE;
 }
+out_lossy_area->top = 0;
+out_lossy_area->left = 0;
+out_lossy_area->bottom = surface->context.height;
+out_lossy_area->right = surface->context.width;
+return TRUE;
 }
 
 region_init(_region);
 region_add(_region, area);
 region_and(_region, surface_lossy_region);
-if (!region_is_empty(_region)) {
-out_lossy_area->left = lossy_region.extents.x1;
-out_lossy_area->top = lossy_region.extents.y1;
-out_lossy_area->right = lossy_region.extents.x2;
-out_lossy_area->bottom = lossy_region.extents.y2;
-region_destroy(_region);
-return TRUE;
-} else {
+if (region_is_empty(_region)) {
 return FALSE;
 }
+out_lossy_area->left = lossy_region.extents.x1;
+out_lossy_area->top = lossy_region.extents.y1;
+out_lossy_area->right = lossy_region.extents.x2;
+out_lossy_area->bottom = lossy_region.extents.y2;
+region_destroy(_region);
+return TRUE;
 }
 
 /* returns if the bitmap was already sent lossy to the client. If the bitmap 
hasn't been sent yet
-- 
2.7.4

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

[Spice-devel] [PATCH 2/3] Simplify some boolean arithmetic

[Frediano Ziglio]

Pass boolean directly.

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 317d906..89c3dc5 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -147,11 +147,7 @@ static int is_bitmap_lossy(RedChannelClient *rcc, 
SpiceImage *image, SpiceRect *
 out_data->id = image->descriptor.id;
 if (dcc_pixmap_cache_hit(dcc, image->descriptor.id, _hit_lossy)) {
 out_data->type = BITMAP_DATA_TYPE_CACHE;
-if (is_hit_lossy) {
-return TRUE;
-} else {
-return FALSE;
-}
+return is_hit_lossy;
 } else {
 out_data->type = BITMAP_DATA_TYPE_BITMAP_TO_CACHE;
 }
@@ -166,13 +162,8 @@ static int is_bitmap_lossy(RedChannelClient *rcc, 
SpiceImage *image, SpiceRect *
 out_data->type = BITMAP_DATA_TYPE_SURFACE;
 out_data->id = image->u.surface.surface_id;
 
-if (is_surface_area_lossy(dcc, out_data->id,
-  area, _data->lossy_rect))
-{
-return TRUE;
-} else {
-return FALSE;
-}
+return is_surface_area_lossy(dcc, out_data->id,
+ area, _data->lossy_rect);
 }
 
 static int is_brush_lossy(RedChannelClient *rcc, SpiceBrush *brush,
-- 
2.7.4

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

[Spice-devel] [PATCH 1/3] Remove unused drawable parameter

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 521e6a2..317d906 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -131,7 +131,7 @@ static int is_surface_area_lossy(DisplayChannelClient *dcc, 
uint32_t surface_id,
all the surface is considered. out_lossy_data will hold info about the 
bitmap, and its lossy
area in case it is lossy and part of a surface. */
 static int is_bitmap_lossy(RedChannelClient *rcc, SpiceImage *image, SpiceRect 
*area,
-   Drawable *drawable, BitmapData *out_data)
+   BitmapData *out_data)
 {
 DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
 
@@ -176,11 +176,11 @@ static int is_bitmap_lossy(RedChannelClient *rcc, 
SpiceImage *image, SpiceRect *
 }
 
 static int is_brush_lossy(RedChannelClient *rcc, SpiceBrush *brush,
-  Drawable *drawable, BitmapData *out_data)
+  BitmapData *out_data)
 {
 if (brush->type == SPICE_BRUSH_TYPE_PATTERN) {
 return is_bitmap_lossy(rcc, brush->u.pattern.pat, NULL,
-   drawable, out_data);
+   out_data);
 } else {
 out_data->type = BITMAP_DATA_TYPE_INVALID;
 return FALSE;
@@ -843,7 +843,7 @@ static void 
red_lossy_marshall_qxl_draw_fill(RedChannelClient *rcc,
(rop & SPICE_ROPD_OP_AND) ||
(rop & SPICE_ROPD_OP_XOR));
 
-brush_is_lossy = is_brush_lossy(rcc, >u.fill.brush, item,
+brush_is_lossy = is_brush_lossy(rcc, >u.fill.brush,
 _bitmap_data);
 if (!dest_allowed_lossy) {
 dest_is_lossy = is_surface_area_lossy(dcc, item->surface_id, 
>bbox,
@@ -933,13 +933,12 @@ static void 
red_lossy_marshall_qxl_draw_opaque(RedChannelClient *rcc,
   (rop & SPICE_ROPD_OP_AND) ||
   (rop & SPICE_ROPD_OP_XOR));
 
-brush_is_lossy = is_brush_lossy(rcc, >u.opaque.brush, item,
+brush_is_lossy = is_brush_lossy(rcc, >u.opaque.brush,
 _bitmap_data);
 
 if (!src_allowed_lossy) {
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.opaque.src_bitmap,
>u.opaque.src_area,
-   item,
_bitmap_data);
 }
 
@@ -1018,7 +1017,7 @@ static void 
red_lossy_marshall_qxl_draw_copy(RedChannelClient *rcc,
 FillBitsType src_send_type;
 
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.copy.src_bitmap,
-   >u.copy.src_area, item, 
_bitmap_data);
+   >u.copy.src_area, 
_bitmap_data);
 
 src_send_type = red_marshall_qxl_draw_copy(rcc, base_marshaller, dpi, 
TRUE);
 if (src_send_type == FILL_BITS_TYPE_COMPRESS_LOSSY) {
@@ -1060,7 +1059,7 @@ static void 
red_lossy_marshall_qxl_draw_transparent(RedChannelClient *rcc,
 BitmapData src_bitmap_data;
 
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.transparent.src_bitmap,
-   >u.transparent.src_area, item, 
_bitmap_data);
+   >u.transparent.src_area, 
_bitmap_data);
 
 if (!src_is_lossy || (src_bitmap_data.type != BITMAP_DATA_TYPE_SURFACE)) {
 red_marshall_qxl_draw_transparent(rcc, base_marshaller, dpi);
@@ -1114,7 +1113,7 @@ static void 
red_lossy_marshall_qxl_draw_alpha_blend(RedChannelClient *rcc,
 FillBitsType src_send_type;
 
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.alpha_blend.src_bitmap,
-   >u.alpha_blend.src_area, item, 
_bitmap_data);
+   >u.alpha_blend.src_area, 
_bitmap_data);
 
 src_send_type = red_marshall_qxl_draw_alpha_blend(rcc, base_marshaller, 
dpi, TRUE);
 
@@ -1210,7 +1209,7 @@ static void 
red_lossy_marshall_qxl_draw_blend(RedChannelClient *rcc,
 SpiceRect dest_lossy_area;
 
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.blend.src_bitmap,
-   >u.blend.src_area, item, 
_bitmap_data);
+   >u.blend.src_area, 
_bitmap_data);
 dest_is_lossy = is_surface_area_lossy(dcc, drawable->surface_id,
   >bbox, _lossy_area);
 
@@ -1377,8 +1376,8 @@ static void 
red_lossy_marshall_qxl_draw_rop3(RedChannelClient *rcc,
 SpiceRect dest_lossy_area;
 
 src_is_lossy = is_bitmap_lossy(rcc, drawable->u.rop3.src_bitmap,
-   >u.rop3.src_area, item, 
_bitmap_data);
-brush_is_lossy = is_brush_lossy(rcc, >u.rop3.brush, item,
+   >u.rop3.src_area, 
_bitmap_data);
+brush_is_lossy = is_brush_lossy(rcc, >u.rop3.brush,