Re: [Spice-devel] [spice-server v2 7/9] qxl: Release QXL resource in red_put_message

[Frediano Ziglio]

> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-parse-qxl.c | 6 --
>  server/red-parse-qxl.h | 3 ++-
>  server/red-worker.c| 3 +--
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index b0c47cfeb..a5e363579 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1266,7 +1266,7 @@ void red_put_update_cmd(RedUpdateCmd *red)
>  /* nothing yet */
>  }
>  
> -bool red_get_message(RedMemSlotInfo *slots, int group_id,
> +bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int
> group_id,
>   RedMessage *red, QXLPHYSICAL addr)

Maybe for consistency adding a "RedMessage *red_message_get" function?
I think Drawable and Cursor are much more "hot" than surface/messages
(messages are actually mostly deprecated).

>  {
>  QXLMessage *qxl;
> @@ -1285,6 +1285,7 @@ bool red_get_message(RedMemSlotInfo *slots, int
> group_id,
>  if (error) {
>  return false;
>  }
> +red->qxl = qxl_instance;
>  red->release_info_ext.info  = >release_info;
>  red->release_info_ext.group_id  = group_id;
>  red->data   = qxl->data;
> @@ -1301,7 +1302,8 @@ bool red_get_message(RedMemSlotInfo *slots, int
> group_id,
>  
>  void red_put_message(RedMessage *red)
>  {
> -/* nothing yet */
> +   if (red->qxl != NULL)
> +   red_qxl_release_resource(red->qxl, red->release_info_ext);

bracket missing, also looks like indented with 3 spaces instead of 4.

>  }
>  
>  static unsigned int surface_format_to_bpp(uint32_t format)
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 231844e96..c73462e0e 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -75,6 +75,7 @@ typedef struct RedUpdateCmd {
>  } RedUpdateCmd;
>  
>  typedef struct RedMessage {
> +QXLInstance *qxl;
>  QXLReleaseInfoExt release_info_ext;
>  int len;
>  uint8_t *data;
> @@ -127,7 +128,7 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int
> group_id,
>  RedUpdateCmd *red, QXLPHYSICAL addr);
>  void red_put_update_cmd(RedUpdateCmd *red);
>  
> -bool red_get_message(RedMemSlotInfo *slots, int group_id,
> +bool red_get_message(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
>   RedMessage *red, QXLPHYSICAL addr);
>  void red_put_message(RedMessage *red);
>  
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 2dcf2b0ef..762443f7c 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -238,14 +238,13 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
>  case QXL_CMD_MESSAGE: {
>  RedMessage message;
>  
> -if (!red_get_message(>mem_slots, ext_cmd.group_id,
> +if (!red_get_message(worker->qxl, >mem_slots,
> ext_cmd.group_id,
>   , ext_cmd.cmd.data)) {
>  break;
>  }
>  #ifdef DEBUG
>  spice_warning("MESSAGE: %.*s", message.len, message.data);
>  #endif
> -red_qxl_release_resource(worker->qxl, message.release_info_ext);
>  red_put_message();
>  break;
>  }

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

Re: [Spice-devel] [spice-server v2 6/9] qxl: Add red_cursor_cmd_new and red_cursor_cmd_free helpers

[Frediano Ziglio]

> 
> Currently, the cursor channel is allocating RedCursorCmd instances itself,
> and then
> calling into red-parse-qxl.h to initialize it, and doing something
> similar when releasing the data. This commit moves this common code to
> red-parse-qxl.[ch]
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/cursor-channel.c |  6 +-
>  server/red-parse-qxl.c  | 24 +---
>  server/red-parse-qxl.h  |  6 +++---
>  server/red-worker.c | 10 +-
>  server/tests/test-qxl-parsing.c | 37 -
>  5 files changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index ada1d62a7..c826627bf 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -69,13 +69,9 @@ static RedCursorPipeItem
> *cursor_pipe_item_new(RedCursorCmd *cmd)
>  
>  static void cursor_pipe_item_free(RedPipeItem *base)
>  {
> -RedCursorCmd *cursor_cmd;
>  RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);
>  
> -cursor_cmd = pipe_item->red_cursor;
> -red_put_cursor_cmd(cursor_cmd);
> -g_free(cursor_cmd);
> -
> +red_cursor_cmd_free(pipe_item->red_cursor);
>  g_free(pipe_item);
>  }
>  
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index ccb01d92d..b0c47cfeb 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1443,8 +1443,9 @@ static void red_put_cursor(SpiceCursor *red)
>  g_free(red->data);
>  }
>  
> -bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> -RedCursorCmd *red, QXLPHYSICAL addr)
> +static bool red_get_cursor_cmd(QXLInstance *qxl_instance, RedMemSlotInfo
> *slots,
> +   int group_id, RedCursorCmd *red,
> +   QXLPHYSICAL addr)
>  {
>  QXLCursorCmd *qxl;
>  int error;
> @@ -1453,6 +1454,7 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int
> group_id,
>  if (error) {
>  return false;
>  }
> +red->qxl = qxl_instance;
>  red->release_info_ext.info  = >release_info;
>  red->release_info_ext.group_id  = group_id;
>  
> @@ -1470,10 +1472,25 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int
> group_id,
>  red->u.trail.frequency = qxl->u.trail.frequency;
>  break;
>  }
> +
>  return true;
>  }
>  
> -void red_put_cursor_cmd(RedCursorCmd *red)
> +RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
> + int group_id, QXLPHYSICAL addr)

red_cursor_cmd_get ?

> +{
> +RedCursorCmd *cmd;
> +
> +cmd = g_new0(RedCursorCmd, 1);
> +if (!red_get_cursor_cmd(qxl, slots, group_id, cmd, addr)) {
> +g_free(cmd);
> +return NULL;
> +}
> +
> +return cmd;
> +}
> +
> +void red_cursor_cmd_free(RedCursorCmd *red)
>  {
>  switch (red->type) {
>  case QXL_CURSOR_SET:
> @@ -1483,6 +1500,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
>  if (red->qxl) {
>  red_qxl_release_resource(red->qxl, red->release_info_ext);
>  }
> +g_free(red);
>  }
>  
>  void red_drawable_unref(RedDrawable *red_drawable)
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 882657e88..231844e96 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -138,8 +138,8 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
> group_id,
>   RedSurfaceCmd *red, QXLPHYSICAL addr);
>  void red_put_surface_cmd(RedSurfaceCmd *red);
>  
> -bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> -RedCursorCmd *red, QXLPHYSICAL addr);
> -void red_put_cursor_cmd(RedCursorCmd *red);
> +RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
> + int group_id, QXLPHYSICAL addr);

OT: all these calls pass group_id and addr from a QXLCommandExt,
maybe we could pass a "const QXLCommandExt*" instead?

> +void red_cursor_cmd_free(RedCursorCmd *cmd);
>  
>  #endif /* RED_PARSE_QXL_H_ */
> diff --git a/server/red-worker.c b/server/red-worker.c
> index ebc6d423d..2dcf2b0ef 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -103,13 +103,14 @@ static gboolean red_process_cursor_cmd(RedWorker
> *worker, const QXLCommandExt *e
>  {
>  RedCursorCmd *cursor_cmd;
>  
> -cursor_cmd = g_new0(RedCursorCmd, 1);
> -if (!red_get_cursor_cmd(>mem_slots, ext->group_id, cursor_cmd,
> ext->cmd.data)) {
> -g_free(cursor_cmd);
> +cursor_cmd = red_cursor_cmd_new(worker->qxl, >mem_slots,
> +ext->group_id, ext->cmd.data);
> +if (cursor_cmd == NULL) {
>  return FALSE;
>  }
> -cursor_cmd->qxl = worker->qxl;
> +
>  cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
> +
>  return TRUE;
>  }
>  
> @@ -965,7 +966,6 @@ static bool 

Re: [Spice-devel] [spice-server v2 5/9] qxl: Fix guest resources release in red_put_drawable()

[Frediano Ziglio]

> 
> At the moment, we'll unconditionally release the guest QXL resources in
> red_put_drawable() even if red_get_drawable() failed and did not
> initialize drawable->release_info_ext properly.
> This commit checks the QXLReleaseInfo in release_info_ext is non-0

You forget to update the comment

> before attempting to release it.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-parse-qxl.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index cc6a8b51d..ccb01d92d 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1012,7 +1012,7 @@ static void red_put_clip(SpiceClip *red)
>  }
>  }
>  
> -static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
> +static bool red_get_native_drawable(QXLInstance *qxl_instance,
> RedMemSlotInfo *slots, int group_id,
>  RedDrawable *red, QXLPHYSICAL addr,
>  uint32_t flags)
>  {
>  QXLDrawable *qxl;
> @@ -1023,6 +1023,7 @@ static bool red_get_native_drawable(RedMemSlotInfo
> *slots, int group_id,
>  if (error) {
>  return false;
>  }
> +red->qxl = qxl_instance;

already updated in red_get_drawable

>  red->release_info_ext.info = >release_info;
>  red->release_info_ext.group_id = group_id;
>  
> @@ -1093,7 +1094,7 @@ static bool red_get_native_drawable(RedMemSlotInfo
> *slots, int group_id,
>  return true;
>  }
>  
> -static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
> +static bool red_get_compat_drawable(QXLInstance *qxl_instance,
> RedMemSlotInfo *slots, int group_id,
>  RedDrawable *red, QXLPHYSICAL addr,
>  uint32_t flags)
>  {
>  QXLCompatDrawable *qxl;
> @@ -1103,6 +1104,7 @@ static bool red_get_compat_drawable(RedMemSlotInfo
> *slots, int group_id,
>  if (error) {
>  return false;
>  }
> +red->qxl = qxl_instance;

here too

>  red->release_info_ext.info = >release_info;
>  red->release_info_ext.group_id = group_id;
>  
> @@ -1176,15 +1178,16 @@ static bool red_get_compat_drawable(RedMemSlotInfo
> *slots, int group_id,
>  return true;
>  }
>  
> -static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
> +static bool red_get_drawable(QXLInstance *qxl, RedMemSlotInfo *slots, int
> group_id,
>   RedDrawable *red, QXLPHYSICAL addr, uint32_t
>   flags)
>  {
>  bool ret;
>  
> +red->qxl = qxl;
>  if (flags & QXL_COMMAND_FLAG_COMPAT) {
> -ret = red_get_compat_drawable(slots, group_id, red, addr, flags);
> +ret = red_get_compat_drawable(qxl, slots, group_id, red, addr,
> flags);
>  } else {
> -ret = red_get_native_drawable(slots, group_id, red, addr, flags);
> +ret = red_get_native_drawable(qxl, slots, group_id, red, addr,
> flags);
>  }
>  return ret;
>  }
> @@ -1487,7 +1490,9 @@ void red_drawable_unref(RedDrawable *red_drawable)
>  if (--red_drawable->refs) {
>  return;
>  }
> -red_qxl_release_resource(red_drawable->qxl,
> red_drawable->release_info_ext);
> +if (red_drawable->qxl != NULL) {
> +red_qxl_release_resource(red_drawable->qxl,
> red_drawable->release_info_ext);
> +}

move in red_put_drawable ?

>  red_put_drawable(red_drawable);
>  g_free(red_drawable);
>  }
> @@ -1499,9 +1504,8 @@ RedDrawable *red_drawable_new(QXLInstance *qxl,
> RedMemSlotInfo *slots,
>  RedDrawable *red = g_new0(RedDrawable, 1);
>  
>  red->refs = 1;
> -red->qxl = qxl;
>  
> -if (!red_get_drawable(slots, group_id, red, addr, flags)) {
> +if (!red_get_drawable(qxl, slots, group_id, red, addr, flags)) {
> red_drawable_unref(red);
> return NULL;
>  }

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

Re: [Spice-devel] [spice-server v2 4/9] qxl: Make red_{get, put}_drawable static

[Frediano Ziglio]

> 
> Rather than needing to call red_drawable_new() and then initialize it
> with red_get_drawable(), we can improve slightly red_drawable new so
> that red_drawable_{new,ref,unref} is all which is used by code out of
> red-parse-qxl.c.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-parse-qxl.c | 17 -
>  server/red-parse-qxl.h |  9 -
>  server/red-worker.c| 11 ++-
>  3 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 245235082..cc6a8b51d 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1176,8 +1176,8 @@ static bool red_get_compat_drawable(RedMemSlotInfo
> *slots, int group_id,
>  return true;
>  }
>  
> -bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
> -  RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
> +static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
> + RedDrawable *red, QXLPHYSICAL addr, uint32_t
> flags)
>  {
>  bool ret;
>  
> @@ -1189,7 +1189,7 @@ bool red_get_drawable(RedMemSlotInfo *slots, int
> group_id,
>  return ret;
>  }
>  
> -void red_put_drawable(RedDrawable *red)
> +static void red_put_drawable(RedDrawable *red)
>  {
>  red_put_clip(>clip);
>  if (red->self_bitmap_image) {
> @@ -1492,12 +1492,19 @@ void red_drawable_unref(RedDrawable *red_drawable)
>  g_free(red_drawable);
>  }
>  
> -RedDrawable *red_drawable_new(QXLInstance *qxl)
> +RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
> +  int group_id, QXLPHYSICAL addr,
> +  uint32_t flags)
>  {
> -RedDrawable * red = g_new0(RedDrawable, 1);
> +RedDrawable *red = g_new0(RedDrawable, 1);
>  
>  red->refs = 1;
>  red->qxl = qxl;
>  
> +if (!red_get_drawable(slots, group_id, red, addr, flags)) {
> +   red_drawable_unref(red);
> +   return NULL;
> +}
> +
>  return red;
>  }

red_drawable_new is confusing if is also reading from QXL memory,
red_drawable_get IMHO would be better

> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 0b507b93a..882657e88 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -64,8 +64,6 @@ static inline RedDrawable *red_drawable_ref(RedDrawable
> *drawable)
>  drawable->refs++;
>  return drawable;
>  }
> -RedDrawable *red_drawable_new(QXLInstance *qxl);
> -void red_drawable_unref(RedDrawable *red_drawable);
>  
>  void red_drawable_unref(RedDrawable *red_drawable);
>  
> @@ -120,9 +118,10 @@ typedef struct RedCursorCmd {
>  
>  void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);
>  
> -bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
> -  RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
> -void red_put_drawable(RedDrawable *red);
> +RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
> +  int group_id, QXLPHYSICAL addr,
> +  uint32_t flags);
> +void red_drawable_unref(RedDrawable *red_drawable);
>  
>  bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
>  RedUpdateCmd *red, QXLPHYSICAL addr);
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 8f806e8e3..ebc6d423d 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -205,15 +205,16 @@ static int red_process_display(RedWorker *worker, int
> *ring_is_empty)
>  worker->display_poll_tries = 0;
>  switch (ext_cmd.cmd.type) {
>  case QXL_CMD_DRAW: {
> -RedDrawable *red_drawable = red_drawable_new(worker->qxl); //
> returns with 1 ref
> +RedDrawable *red_drawable;
> +red_drawable = red_drawable_new(worker->qxl, >mem_slots,
> +ext_cmd.group_id,
> ext_cmd.cmd.data,
> +ext_cmd.flags); // returns with
> 1 ref
>  
> -if (red_get_drawable(>mem_slots, ext_cmd.group_id,
> - red_drawable, ext_cmd.cmd.data,
> ext_cmd.flags)) {
> +if (red_drawable != NULL) {
>  display_channel_process_draw(worker->display_channel,
>  red_drawable,
>   
> worker->process_display_generation);
> +red_drawable_unref(red_drawable);
>  }
> -// release the red_drawable

this comment was really... unhelpful :-)

> -red_drawable_unref(red_drawable);
>  break;
>  }
>  case QXL_CMD_UPDATE: {

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

Re: [Spice-devel] [spice-server v2 3/9] qxl: Move red_drawable_unref/red_drawable_new

[Frediano Ziglio]

> 
> RedDrawable really is a RedDrawCmd which is parsed by red-parse-qxl.h
> This commit moves them close to the other functions creating/unref'ing
> QXL commands parsed by red-parse-qxl.h
> 

Does not seem a great reason to have different APIs from red-parse-qxl,
this and next patch seems to mix parsing with allocation, is not clear
why for some structure is separate and for some other bound.
Maybe we should join all (even surface and messages)?

> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-parse-qxl.c | 21 +
>  server/red-parse-qxl.h |  2 ++
>  server/red-worker.c| 20 
>  3 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 4a45c0f76..245235082 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -26,6 +26,7 @@
>  #include "red-common.h"
>  #include "red-qxl.h"
>  #include "memslot.h"
> +#include "red-qxl.h"

duplicate

>  #include "red-parse-qxl.h"
>  
>  /* Max size in bytes for any data field used in a QXL command.
> @@ -1480,3 +1481,23 @@ void red_put_cursor_cmd(RedCursorCmd *red)
>  red_qxl_release_resource(red->qxl, red->release_info_ext);
>  }
>  }
> +
> +void red_drawable_unref(RedDrawable *red_drawable)
> +{
> +if (--red_drawable->refs) {
> +return;
> +}
> +red_qxl_release_resource(red_drawable->qxl,
> red_drawable->release_info_ext);
> +red_put_drawable(red_drawable);
> +g_free(red_drawable);
> +}
> +
> +RedDrawable *red_drawable_new(QXLInstance *qxl)
> +{
> +RedDrawable * red = g_new0(RedDrawable, 1);
> +
> +red->refs = 1;
> +red->qxl = qxl;
> +
> +return red;
> +}
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index a33f36adb..0b507b93a 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -64,6 +64,8 @@ static inline RedDrawable *red_drawable_ref(RedDrawable
> *drawable)
>  drawable->refs++;
>  return drawable;
>  }
> +RedDrawable *red_drawable_new(QXLInstance *qxl);
> +void red_drawable_unref(RedDrawable *red_drawable);
>  

duplicate (unref)

>  void red_drawable_unref(RedDrawable *red_drawable);
>  

here

> diff --git a/server/red-worker.c b/server/red-worker.c
> index eb927f3e0..8f806e8e3 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -99,16 +99,6 @@ static int display_is_connected(RedWorker *worker)
>  red_channel_is_connected(RED_CHANNEL(worker->display_channel));
>  }
>  
> -void red_drawable_unref(RedDrawable *red_drawable)
> -{
> -if (--red_drawable->refs) {
> -return;
> -}
> -red_qxl_release_resource(red_drawable->qxl,
> red_drawable->release_info_ext);
> -red_put_drawable(red_drawable);
> -g_free(red_drawable);
> -}
> -
>  static gboolean red_process_cursor_cmd(RedWorker *worker, const
>  QXLCommandExt *ext)
>  {
>  RedCursorCmd *cursor_cmd;
> @@ -165,16 +155,6 @@ static int red_process_cursor(RedWorker *worker, int
> *ring_is_empty)
>  return n;
>  }
>  
> -static RedDrawable *red_drawable_new(QXLInstance *qxl)
> -{
> -RedDrawable * red = g_new0(RedDrawable, 1);
> -
> -red->refs = 1;
> -red->qxl = qxl;
> -
> -return red;
> -}
> -
>  static gboolean red_process_surface_cmd(RedWorker *worker, QXLCommandExt
>  *ext, gboolean loadvm)
>  {
>  RedSurfaceCmd surface_cmd;

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

Re: [Spice-devel] [spice-server v2 2/9] qxl: Remove 'blackness' and 'invers' put/get methods

[Frediano Ziglio]

> 
> SpiceWhiteness/SpiceBlackness/SpiceInvers are 3 typedef for the same
> type, no need to have 3 identical red_put_xxx/red_get_xxx methods.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/red-parse-qxl.c | 25 -
>  1 file changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index b766e9935..4a45c0f76 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -986,27 +986,10 @@ static void red_put_whiteness(SpiceWhiteness *red)
>  red_put_qmask(>mask);
>  }
>  
> -static void red_get_blackness_ptr(RedMemSlotInfo *slots, int group_id,
> -  SpiceBlackness *red, QXLBlackness *qxl,
> uint32_t flags)
> -{
> -red_get_qmask_ptr(slots, group_id, >mask, >mask, flags);
> -}
> -
> -static void red_put_blackness(SpiceWhiteness *red)
> -{
> -red_put_qmask(>mask);
> -}
> -
> -static void red_get_invers_ptr(RedMemSlotInfo *slots, int group_id,
> -   SpiceInvers *red, QXLInvers *qxl, uint32_t
> flags)
> -{
> -red_get_qmask_ptr(slots, group_id, >mask, >mask, flags);
> -}
> -
> -static void red_put_invers(SpiceWhiteness *red)
> -{
> -red_put_qmask(>mask);
> -}
> +#define red_get_invers_ptr red_get_whiteness_ptr
> +#define red_get_blackness_ptr red_get_whiteness_ptr
> +#define red_put_invers red_put_whiteness
> +#define red_put_blackness red_put_whiteness
>  
>  static void red_get_clip_ptr(RedMemSlotInfo *slots, int group_id,
>   SpiceClip *red, QXLClip *qxl)

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 1/2] Ensure that plugins cannot bypass version check

[Frediano Ziglio]

> 
> > On 17 Apr 2018, at 16:12, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> This change addresses three issues related to plugin version checking:
> >> 
> >> 1. It is possible for plugins to bypass version checking or do it wrong
> >>   (as a matter of fact, the mjpeg fallback sets a bad example)
> >> 
> >> 2. The current plugin version check violates the C++ ODR, i.e.
> >>   it relies on undefined behaviors when the header used to compile
> >>   the plugin and to compile the agent are not identical,
> >> 
> >> 3. A major.minor numbering scheme is not ideal for ABI checks.
> >>   In particular, it makes it difficult to react to an incompatibility
> >>   that was detected post release.
> >> 
> >> [More info]
> >> 
> >> 1. Make it impossible to bypass version checking
> >> 
> >> The current code depends on the plugin implementing the version check
> >> correctly by calling PluginVersionIsCompatible. To make things worse,
> >> the only publicly available example gets this wrong and uses an
> >> ad-hoc version check, so anybody copy-pasting this code will get it
> >> wrong.
> >> 
> >> It is more robust to do the version check in the agent before calling
> >> any method in the plugin. It ensures that version checking cannot be
> >> bypassed, is done consistently and generates consistent error messages.
> >> 
> >> As an indication that the aproach is robust, the new check correctly
> >> refuses to load older plugins that use the old version checking method:
> >> 
> >>spice-streaming-agent[5167]:
> >>error loading plugin [...].so: no version information
> >> 
> > 
> > This is clear to everybody, doing from the agent avoid each plugin to
> > do the check on the version and we can do it safely before executing
> > any code in the plugin (library initialization aside).
> 
> As I pointed out in an early reply, library initialization is not much of an
> issue since it does not have access to the agent yet. So barring static
> entry points in the agent interface that could be called during init, we are
> OK.
> 
> > 
> >> 2. ODR-related problems
> >> 
> >> The C++ One Definition Rule (ODR) states that all translation units
> >> must see the same definitions. In the current code, when we call
> >> Agent::PluginVersionIsCompatible from the plugin, it is an ODR
> >> violation as soon as we have made any change in the Agent class
> >> compared to what the plugin was compiled against.
> >> 
> >> The current code therefore relies on implementation dependent knowlege
> >> of how virtual functions are laid out in the vtable, and puts
> >> unnecessary constraints on the changes allowed in the classes
> >> (e.g. it's not allowed to put anything before some member functions)
> >> 
> >> 3. Major.minor numbering scheme
> >> 
> >> The major.minor numbering scheme initially selected makes it harder
> >> to fixes cases where an incompatibility was detected after release.
> >> 
> >> For example, the major.minor version checking assumes that agent 1.21
> >> is compatible with plugins 1.21, 1.13 or 1.03. If later testing
> >> shows that 1.13 actually introduced an incompatiliy, you have to
> >> special-case 1.13 in the compatibiliy check.
> >> 
> >> An approach that does not have this problem is to rely on incremented
> >> version numbers, with a "current" and "oldest compatible" version
> >> number. This is used for example by libtools [1].
> >> 
> >> Since the change required for #1 and #2  introduces an ABI break,
> >> it is as good a time as any to also change the numbering scheme,
> >> since changing it later would introduce another unnecessary ABI break.
> >> 
> >> [1]
> >> https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> >> 
> > 
> > Reading all the replies looks like to me that what you are basically
> > doing with these changes is basically removing the minor from the
> > versioning
> > schema and making every change binary incompatible.
> 
> I don’t understand how you could reach that conclusion. If that was the case,
> there would be no “PluginInterfaceOldestCompatibleVersion” version in my
> proposal.
> 
> In reality, the numbering change I proposed offers the possibility to decide,
> for any release, what is the oldest compatible version, without having to
> necessarily have that oldest compatible be the latest.
> 
> In the example I gave above, I was indicating that version 20 was compatible
> with version 3, but that version 21 introduced a change that breaks
> compatibility not with version 20, but with versions earlier than 13. This
> can be expressed with my scheme easily by bumping the “oldest compatible”
> from 3 to 13 (without being forced to bump it to 21). By contrast, with the
> major.minor scheme, this kind of scenario requires special treatment.
> 
> When does this kind of scenario happen? Imagine we added support for
> loading/unloading plugins. Version 13 adds a new interface to unload
> 

Re: [Spice-devel] [PATCH spice-server 2/3] red-stream: Implements flush using TCP_CORK

[Frediano Ziglio]

> 
> On 04/13/2018 03:25 PM, Frediano Ziglio wrote:
> > Cork is a system interface implemented by Linux and some *BSD systems to
> > tell the system that other data are expected to be written to a socket.
> > This allows the system to reduce network fragmentation waiting for network
> > packets to be complete.
> > 
> > Using some replay capture and some instrumentation resulted in a
> > bandwith reduction of 11% and a packet reduction of 56%.
> > 
> > The tests was done using replay utility so results could be a bit different
> > from real cases as:
> > - replay goes as fast as it can, for instance packets could
> >be merged by the kernel decreasing packet numbers and a bit
> >byte spent (this actually make the following improves worse);
> > - there are fewer channels (no much cursor, sound, etc).
> > The following tests shows count packet and total bytes from server to
> > client using a real network. I used a direct cable connection using 1gb
> > connection and 2 laptops.
> > 
> > cork: 537 1582240
> > cork: 681 1823754
> > cork: 524 1583287
> > cork: 538 1582350
> > no cork: 1329 1834630
> > no cork: 1290 1829094
> > no cork: 1289 1830164
> > no cork: 1317 1833589
> > no cork: 1320 1835705
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >   server/red-stream.c | 40 +++-
> >   1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-stream.c b/server/red-stream.c
> > index 9a9c1a0f..18c4a935 100644
> > --- a/server/red-stream.c
> > +++ b/server/red-stream.c
> > @@ -24,6 +24,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   
> >   #include 
> >   
> > @@ -37,6 +38,11 @@
> >   #include "red-stream.h"
> >   #include "reds.h"
> >   
> > +// compatibility for *BSD systems
> > +#ifndef TCP_CORK
> > +#define TCP_CORK TCP_NOPUSH
> > +#endif
> > +
> >   struct AsyncRead {
> >   void *opaque;
> >   uint8_t *now;
> > @@ -83,6 +89,8 @@ struct RedStreamPrivate {
> >* deallocated when main_dispatcher handles the
> >SPICE_CHANNEL_EVENT_DISCONNECTED
> >* event, either from same thread or by call back from main thread.
> >*/
> >   SpiceChannelEventInfo* info;
> > +bool use_cork;
> > +bool corked;
> >   
> >   ssize_t (*read)(RedStream *s, void *buf, size_t nbyte);
> >   ssize_t (*write)(RedStream *s, const void *buf, size_t nbyte);
> > @@ -92,6 +100,16 @@ struct RedStreamPrivate {
> >   SpiceCoreInterfaceInternal *core;
> >   };
> >   
> > +/**
> > + * Set TCP_CORK on socket
> > + */
> > +/* NOTE: enabled must be int */
> > +static int socket_set_cork(int socket, int enabled)
> > +{
> > +SPICE_VERIFY(sizeof(enabled) == sizeof(int));
> > +return setsockopt(socket, IPPROTO_TCP, TCP_CORK, ,
> > sizeof(enabled));
> > +}
> > +
> >   static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t
> >   size)
> >   {
> >   return write(s->socket, buf, size);
> > @@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const
> > void *in_buf, size_t n)
> >   
> >   bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)
> >   {
> > -return auto_flush;
> > +if (s->priv->use_cork == !auto_flush) {
> > +return true;
> > +}
> > +
> > +s->priv->use_cork = !auto_flush;
> > +if (s->priv->use_cork) {
> > +if (socket_set_cork(s->socket, 1)) {
> > +s->priv->use_cork = false;
> > +return false;
> > +} else {
> > +s->priv->corked = true;
> > +}
> > +} else if (s->priv->corked) {
> > +socket_set_cork(s->socket, 0);
> > +s->priv->corked = false;
> > +}
> Hi Frediano,
> 
> It would be simpler to use !auto_flash or s->priv->use_cork
> and also only keep one of use_cork and corked.
> Possibly the logic is a bit different and not exactly what you want.
> 
>if (socket_set_cork(s->socket, !auto_flash)) {
>return false;
>}
> 
>s->priv->use_cork = !auto_flash;
> 
> 
> > +return true;
> >   }
> 
> Uri.
> 

Not proper... the exact equivalent is this:


bool red_stream_set_auto_flush_new(RedStream *s, bool auto_flush)
{
if (s->priv->use_cork == !auto_flush) {
return true;
}

if (!auto_flush || s->priv->corked) {
if (socket_set_cork(s->socket, !auto_flush) && !auto_flush) {
return false;
}
s->priv->corked = !auto_flush;
}

s->priv->use_cork = !auto_flush;
return true;
}


but is confusing, maybe easier to remove the "} else {" after a return
and move use_cork change after the ifs as suggested to avoid assigning
to false, like:


--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -227,18 +227,16 @@ bool red_stream_set_auto_flush(RedStream *s, bool 
auto_flush)
 return true;
 }

-s->priv->use_cork = !auto_flush;
-if (s->priv->use_cork) {
+if (!auto_flush) {
 if (socket_set_cork(s->socket, 1)) {
- 

Re: [Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Christophe de Dinechin]

> On 17 Apr 2018, at 16:12, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> This change addresses three issues related to plugin version checking:
>> 
>> 1. It is possible for plugins to bypass version checking or do it wrong
>>   (as a matter of fact, the mjpeg fallback sets a bad example)
>> 
>> 2. The current plugin version check violates the C++ ODR, i.e.
>>   it relies on undefined behaviors when the header used to compile
>>   the plugin and to compile the agent are not identical,
>> 
>> 3. A major.minor numbering scheme is not ideal for ABI checks.
>>   In particular, it makes it difficult to react to an incompatibility
>>   that was detected post release.
>> 
>> [More info]
>> 
>> 1. Make it impossible to bypass version checking
>> 
>> The current code depends on the plugin implementing the version check
>> correctly by calling PluginVersionIsCompatible. To make things worse,
>> the only publicly available example gets this wrong and uses an
>> ad-hoc version check, so anybody copy-pasting this code will get it
>> wrong.
>> 
>> It is more robust to do the version check in the agent before calling
>> any method in the plugin. It ensures that version checking cannot be
>> bypassed, is done consistently and generates consistent error messages.
>> 
>> As an indication that the aproach is robust, the new check correctly
>> refuses to load older plugins that use the old version checking method:
>> 
>>spice-streaming-agent[5167]:
>>error loading plugin [...].so: no version information
>> 
> 
> This is clear to everybody, doing from the agent avoid each plugin to
> do the check on the version and we can do it safely before executing
> any code in the plugin (library initialization aside).

As I pointed out in an early reply, library initialization is not much of an 
issue since it does not have access to the agent yet. So barring static entry 
points in the agent interface that could be called during init, we are OK.

> 
>> 2. ODR-related problems
>> 
>> The C++ One Definition Rule (ODR) states that all translation units
>> must see the same definitions. In the current code, when we call
>> Agent::PluginVersionIsCompatible from the plugin, it is an ODR
>> violation as soon as we have made any change in the Agent class
>> compared to what the plugin was compiled against.
>> 
>> The current code therefore relies on implementation dependent knowlege
>> of how virtual functions are laid out in the vtable, and puts
>> unnecessary constraints on the changes allowed in the classes
>> (e.g. it's not allowed to put anything before some member functions)
>> 
>> 3. Major.minor numbering scheme
>> 
>> The major.minor numbering scheme initially selected makes it harder
>> to fixes cases where an incompatibility was detected after release.
>> 
>> For example, the major.minor version checking assumes that agent 1.21
>> is compatible with plugins 1.21, 1.13 or 1.03. If later testing
>> shows that 1.13 actually introduced an incompatiliy, you have to
>> special-case 1.13 in the compatibiliy check.
>> 
>> An approach that does not have this problem is to rely on incremented
>> version numbers, with a "current" and "oldest compatible" version
>> number. This is used for example by libtools [1].
>> 
>> Since the change required for #1 and #2  introduces an ABI break,
>> it is as good a time as any to also change the numbering scheme,
>> since changing it later would introduce another unnecessary ABI break.
>> 
>> [1]
>> https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>> 
> 
> Reading all the replies looks like to me that what you are basically
> doing with these changes is basically removing the minor from the versioning
> schema and making every change binary incompatible.

I don’t understand how you could reach that conclusion. If that was the case, 
there would be no “PluginInterfaceOldestCompatibleVersion” version in my 
proposal.

In reality, the numbering change I proposed offers the possibility to decide, 
for any release, what is the oldest compatible version, without having to 
necessarily have that oldest compatible be the latest.

In the example I gave above, I was indicating that version 20 was compatible 
with version 3, but that version 21 introduced a change that breaks 
compatibility not with version 20, but with versions earlier than 13. This can 
be expressed with my scheme easily by bumping the “oldest compatible” from 3 to 
13 (without being forced to bump it to 21). By contrast, with the major.minor 
scheme, this kind of scenario requires special treatment.

When does this kind of scenario happen? Imagine we added support for 
loading/unloading plugins. Version 13 adds a new interface to unload plugins, 
and a new “unloadable plugin” entry point. Therefore, starting with version 13, 
the agent would try the new entry point first, and if it’s there, assume it can 
unload the plugin, otherwise 

[Spice-devel] [spice-server v2 7/9] qxl: Release QXL resource in red_put_message

[Christophe Fergeau]

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 6 --
 server/red-parse-qxl.h | 3 ++-
 server/red-worker.c| 3 +--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index b0c47cfeb..a5e363579 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1266,7 +1266,7 @@ void red_put_update_cmd(RedUpdateCmd *red)
 /* nothing yet */
 }
 
-bool red_get_message(RedMemSlotInfo *slots, int group_id,
+bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int 
group_id,
  RedMessage *red, QXLPHYSICAL addr)
 {
 QXLMessage *qxl;
@@ -1285,6 +1285,7 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,
 if (error) {
 return false;
 }
+red->qxl = qxl_instance;
 red->release_info_ext.info  = >release_info;
 red->release_info_ext.group_id  = group_id;
 red->data   = qxl->data;
@@ -1301,7 +1302,8 @@ bool red_get_message(RedMemSlotInfo *slots, int group_id,
 
 void red_put_message(RedMessage *red)
 {
-/* nothing yet */
+   if (red->qxl != NULL)
+   red_qxl_release_resource(red->qxl, red->release_info_ext);
 }
 
 static unsigned int surface_format_to_bpp(uint32_t format)
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 231844e96..c73462e0e 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -75,6 +75,7 @@ typedef struct RedUpdateCmd {
 } RedUpdateCmd;
 
 typedef struct RedMessage {
+QXLInstance *qxl;
 QXLReleaseInfoExt release_info_ext;
 int len;
 uint8_t *data;
@@ -127,7 +128,7 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
 RedUpdateCmd *red, QXLPHYSICAL addr);
 void red_put_update_cmd(RedUpdateCmd *red);
 
-bool red_get_message(RedMemSlotInfo *slots, int group_id,
+bool red_get_message(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
  RedMessage *red, QXLPHYSICAL addr);
 void red_put_message(RedMessage *red);
 
diff --git a/server/red-worker.c b/server/red-worker.c
index 2dcf2b0ef..762443f7c 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -238,14 +238,13 @@ static int red_process_display(RedWorker *worker, int 
*ring_is_empty)
 case QXL_CMD_MESSAGE: {
 RedMessage message;
 
-if (!red_get_message(>mem_slots, ext_cmd.group_id,
+if (!red_get_message(worker->qxl, >mem_slots, 
ext_cmd.group_id,
  , ext_cmd.cmd.data)) {
 break;
 }
 #ifdef DEBUG
 spice_warning("MESSAGE: %.*s", message.len, message.data);
 #endif
-red_qxl_release_resource(worker->qxl, message.release_info_ext);
 red_put_message();
 break;
 }
-- 
2.14.3

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

[Spice-devel] [spice-server v2 9/9] qxl: Improve 'red' and 'qxl' argument names

[Christophe Fergeau]

All the methods related to QXL command parsing have a 'qxl' argument
which designates the guest side resources, and a 'red' argument, which
designates the spice-server side data which has been parsed from 'qxl'.
When reading random functions in red-parse-qxl.c, it's quite easy to
wonder what a 'red' is if you are not familiar with the convention.
This commit adds a suffix to each of these 'red'/'qxl' variables so that
it's clearer what they are referring to.

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 924 -
 1 file changed, 462 insertions(+), 462 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index afd136e43..22ee2e986 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -116,7 +116,7 @@ static uint8_t *red_linearize_chunk(RedDataChunk *head, 
size_t size, bool *free_
 
 static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
   int memslot_id,
-  RedDataChunk *red, QXLDataChunk *qxl)
+  RedDataChunk *red_data_chunk, 
QXLDataChunk *qxl_data_chunk)
 {
 RedDataChunk *red_prev;
 uint64_t data_size = 0;
@@ -125,16 +125,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 QXLPHYSICAL next_chunk;
 unsigned num_chunks = 0;
 
-red->data_size = qxl->data_size;
-data_size += red->data_size;
-red->data = qxl->data;
-red->prev_chunk = red->next_chunk = NULL;
-if (!memslot_validate_virt(slots, (intptr_t)red->data, memslot_id, 
red->data_size, group_id)) {
-red->data = NULL;
+red_data_chunk->data_size = qxl_data_chunk->data_size;
+data_size += red_data_chunk->data_size;
+red_data_chunk->data = qxl_data_chunk->data;
+red_data_chunk->prev_chunk = red_data_chunk->next_chunk = NULL;
+if (!memslot_validate_virt(slots, (intptr_t)red_data_chunk->data, 
memslot_id, red_data_chunk->data_size, group_id)) {
+red_data_chunk->data = NULL;
 return INVALID_SIZE;
 }
 
-while ((next_chunk = qxl->next_chunk) != 0) {
+while ((next_chunk = qxl_data_chunk->next_chunk) != 0) {
 /* somebody is trying to use too much memory using a lot of chunks.
  * Or made a circular list of chunks
  */
@@ -144,8 +144,8 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 }
 
 memslot_id = memslot_get_id(slots, next_chunk);
-qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, sizeof(*qxl),
-   group_id, );
+qxl_data_chunk = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, 
sizeof(*qxl_data_chunk),
+  group_id, );
 if (error)
 goto error;
 
@@ -155,16 +155,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
  * All above cases are handled by the check for number
  * of chunks.
  */
-chunk_data_size = qxl->data_size;
+chunk_data_size = qxl_data_chunk->data_size;
 if (chunk_data_size == 0)
 continue;
 
-red_prev = red;
-red = g_new0(RedDataChunk, 1);
-red->data_size = chunk_data_size;
-red->prev_chunk = red_prev;
-red->data = qxl->data;
-red_prev->next_chunk = red;
+red_prev = red_data_chunk;
+red_data_chunk = g_new0(RedDataChunk, 1);
+red_data_chunk->data_size = chunk_data_size;
+red_data_chunk->prev_chunk = red_prev;
+red_data_chunk->data = qxl_data_chunk->data;
+red_prev->next_chunk = red_data_chunk;
 
 data_size += chunk_data_size;
 /* this can happen if client is sending nested chunks */
@@ -172,69 +172,69 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 spice_warning("too much data inside chunks, avoiding DoS\n");
 goto error;
 }
-if (!memslot_validate_virt(slots, (intptr_t)red->data, memslot_id, 
red->data_size, group_id))
+if (!memslot_validate_virt(slots, (intptr_t)red_data_chunk->data, 
memslot_id, red_data_chunk->data_size, group_id))
 goto error;
 }
 
-red->next_chunk = NULL;
+red_data_chunk->next_chunk = NULL;
 return data_size;
 
 error:
-while (red->prev_chunk) {
-red_prev = red->prev_chunk;
-g_free(red);
-red = red_prev;
+while (red_data_chunk->prev_chunk) {
+red_prev = red_data_chunk->prev_chunk;
+g_free(red_data_chunk);
+red_data_chunk = red_prev;
 }
-red->data_size = 0;
-red->next_chunk = NULL;
-red->data = NULL;
+red_data_chunk->data_size = 0;
+red_data_chunk->next_chunk = NULL;
+red_data_chunk->data = NULL;
 return INVALID_SIZE;
 }
 
 static size_t 

[Spice-devel] [spice-server v2 1/9] qxl: Remove red_put_blend()

[Christophe Fergeau]

SpiceBlend is a typedef to SpiceCopy, and red_put_blend() and
red_put_copy() are identical, so we can add a #define red_put_blend
red_put_copy similar to the one we already have for red_get_blend.

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 9f1303da3..b766e9935 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -712,12 +712,7 @@ static void red_put_copy(SpiceCopy *red)
 
 // these types are really the same thing
 #define red_get_blend_ptr red_get_copy_ptr
-
-static void red_put_blend(SpiceBlend *red)
-{
-red_put_image(red->src_bitmap);
-red_put_qmask(>mask);
-}
+#define red_put_blend red_put_copy
 
 static void red_get_transparent_ptr(RedMemSlotInfo *slots, int group_id,
 SpiceTransparent *red, QXLTransparent *qxl,
-- 
2.14.3

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

[Spice-devel] [spice-server v2 5/9] qxl: Fix guest resources release in red_put_drawable()

[Christophe Fergeau]

At the moment, we'll unconditionally release the guest QXL resources in
red_put_drawable() even if red_get_drawable() failed and did not
initialize drawable->release_info_ext properly.
This commit checks the QXLReleaseInfo in release_info_ext is non-0
before attempting to release it.

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index cc6a8b51d..ccb01d92d 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1012,7 +1012,7 @@ static void red_put_clip(SpiceClip *red)
 }
 }
 
-static bool red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_native_drawable(QXLInstance *qxl_instance, RedMemSlotInfo 
*slots, int group_id,
 RedDrawable *red, QXLPHYSICAL addr, 
uint32_t flags)
 {
 QXLDrawable *qxl;
@@ -1023,6 +1023,7 @@ static bool red_get_native_drawable(RedMemSlotInfo 
*slots, int group_id,
 if (error) {
 return false;
 }
+red->qxl = qxl_instance;
 red->release_info_ext.info = >release_info;
 red->release_info_ext.group_id = group_id;
 
@@ -1093,7 +1094,7 @@ static bool red_get_native_drawable(RedMemSlotInfo 
*slots, int group_id,
 return true;
 }
 
-static bool red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_compat_drawable(QXLInstance *qxl_instance, RedMemSlotInfo 
*slots, int group_id,
 RedDrawable *red, QXLPHYSICAL addr, 
uint32_t flags)
 {
 QXLCompatDrawable *qxl;
@@ -1103,6 +1104,7 @@ static bool red_get_compat_drawable(RedMemSlotInfo 
*slots, int group_id,
 if (error) {
 return false;
 }
+red->qxl = qxl_instance;
 red->release_info_ext.info = >release_info;
 red->release_info_ext.group_id = group_id;
 
@@ -1176,15 +1178,16 @@ static bool red_get_compat_drawable(RedMemSlotInfo 
*slots, int group_id,
 return true;
 }
 
-static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
+static bool red_get_drawable(QXLInstance *qxl, RedMemSlotInfo *slots, int 
group_id,
  RedDrawable *red, QXLPHYSICAL addr, uint32_t 
flags)
 {
 bool ret;
 
+red->qxl = qxl;
 if (flags & QXL_COMMAND_FLAG_COMPAT) {
-ret = red_get_compat_drawable(slots, group_id, red, addr, flags);
+ret = red_get_compat_drawable(qxl, slots, group_id, red, addr, flags);
 } else {
-ret = red_get_native_drawable(slots, group_id, red, addr, flags);
+ret = red_get_native_drawable(qxl, slots, group_id, red, addr, flags);
 }
 return ret;
 }
@@ -1487,7 +1490,9 @@ void red_drawable_unref(RedDrawable *red_drawable)
 if (--red_drawable->refs) {
 return;
 }
-red_qxl_release_resource(red_drawable->qxl, 
red_drawable->release_info_ext);
+if (red_drawable->qxl != NULL) {
+red_qxl_release_resource(red_drawable->qxl, 
red_drawable->release_info_ext);
+}
 red_put_drawable(red_drawable);
 g_free(red_drawable);
 }
@@ -1499,9 +1504,8 @@ RedDrawable *red_drawable_new(QXLInstance *qxl, 
RedMemSlotInfo *slots,
 RedDrawable *red = g_new0(RedDrawable, 1);
 
 red->refs = 1;
-red->qxl = qxl;
 
-if (!red_get_drawable(slots, group_id, red, addr, flags)) {
+if (!red_get_drawable(qxl, slots, group_id, red, addr, flags)) {
red_drawable_unref(red);
return NULL;
 }
-- 
2.14.3

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

[Spice-devel] [spice-server v2 3/9] qxl: Move red_drawable_unref/red_drawable_new

[Christophe Fergeau]

RedDrawable really is a RedDrawCmd which is parsed by red-parse-qxl.h
This commit moves them close to the other functions creating/unref'ing
QXL commands parsed by red-parse-qxl.h

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 21 +
 server/red-parse-qxl.h |  2 ++
 server/red-worker.c| 20 
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 4a45c0f76..245235082 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -26,6 +26,7 @@
 #include "red-common.h"
 #include "red-qxl.h"
 #include "memslot.h"
+#include "red-qxl.h"
 #include "red-parse-qxl.h"
 
 /* Max size in bytes for any data field used in a QXL command.
@@ -1480,3 +1481,23 @@ void red_put_cursor_cmd(RedCursorCmd *red)
 red_qxl_release_resource(red->qxl, red->release_info_ext);
 }
 }
+
+void red_drawable_unref(RedDrawable *red_drawable)
+{
+if (--red_drawable->refs) {
+return;
+}
+red_qxl_release_resource(red_drawable->qxl, 
red_drawable->release_info_ext);
+red_put_drawable(red_drawable);
+g_free(red_drawable);
+}
+
+RedDrawable *red_drawable_new(QXLInstance *qxl)
+{
+RedDrawable * red = g_new0(RedDrawable, 1);
+
+red->refs = 1;
+red->qxl = qxl;
+
+return red;
+}
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index a33f36adb..0b507b93a 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -64,6 +64,8 @@ static inline RedDrawable *red_drawable_ref(RedDrawable 
*drawable)
 drawable->refs++;
 return drawable;
 }
+RedDrawable *red_drawable_new(QXLInstance *qxl);
+void red_drawable_unref(RedDrawable *red_drawable);
 
 void red_drawable_unref(RedDrawable *red_drawable);
 
diff --git a/server/red-worker.c b/server/red-worker.c
index eb927f3e0..8f806e8e3 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -99,16 +99,6 @@ static int display_is_connected(RedWorker *worker)
 red_channel_is_connected(RED_CHANNEL(worker->display_channel));
 }
 
-void red_drawable_unref(RedDrawable *red_drawable)
-{
-if (--red_drawable->refs) {
-return;
-}
-red_qxl_release_resource(red_drawable->qxl, 
red_drawable->release_info_ext);
-red_put_drawable(red_drawable);
-g_free(red_drawable);
-}
-
 static gboolean red_process_cursor_cmd(RedWorker *worker, const QXLCommandExt 
*ext)
 {
 RedCursorCmd *cursor_cmd;
@@ -165,16 +155,6 @@ static int red_process_cursor(RedWorker *worker, int 
*ring_is_empty)
 return n;
 }
 
-static RedDrawable *red_drawable_new(QXLInstance *qxl)
-{
-RedDrawable * red = g_new0(RedDrawable, 1);
-
-red->refs = 1;
-red->qxl = qxl;
-
-return red;
-}
-
 static gboolean red_process_surface_cmd(RedWorker *worker, QXLCommandExt *ext, 
gboolean loadvm)
 {
 RedSurfaceCmd surface_cmd;
-- 
2.14.3

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

[Spice-devel] [spice-server v2 2/9] qxl: Remove 'blackness' and 'invers' put/get methods

[Christophe Fergeau]

SpiceWhiteness/SpiceBlackness/SpiceInvers are 3 typedef for the same
type, no need to have 3 identical red_put_xxx/red_get_xxx methods.

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 25 -
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index b766e9935..4a45c0f76 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -986,27 +986,10 @@ static void red_put_whiteness(SpiceWhiteness *red)
 red_put_qmask(>mask);
 }
 
-static void red_get_blackness_ptr(RedMemSlotInfo *slots, int group_id,
-  SpiceBlackness *red, QXLBlackness *qxl, 
uint32_t flags)
-{
-red_get_qmask_ptr(slots, group_id, >mask, >mask, flags);
-}
-
-static void red_put_blackness(SpiceWhiteness *red)
-{
-red_put_qmask(>mask);
-}
-
-static void red_get_invers_ptr(RedMemSlotInfo *slots, int group_id,
-   SpiceInvers *red, QXLInvers *qxl, uint32_t 
flags)
-{
-red_get_qmask_ptr(slots, group_id, >mask, >mask, flags);
-}
-
-static void red_put_invers(SpiceWhiteness *red)
-{
-red_put_qmask(>mask);
-}
+#define red_get_invers_ptr red_get_whiteness_ptr
+#define red_get_blackness_ptr red_get_whiteness_ptr
+#define red_put_invers red_put_whiteness
+#define red_put_blackness red_put_whiteness
 
 static void red_get_clip_ptr(RedMemSlotInfo *slots, int group_id,
  SpiceClip *red, QXLClip *qxl)
-- 
2.14.3

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

[Spice-devel] [spice-server v2 8/9] qxl: Release QXL resources in red_put_update_cmd

[Christophe Fergeau]

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 11 +++
 server/red-parse-qxl.h |  3 ++-
 server/red-worker.c|  3 +--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index a5e363579..afd136e43 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1241,8 +1241,9 @@ static void red_put_drawable(RedDrawable *red)
 }
 }
 
-bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
-RedUpdateCmd *red, QXLPHYSICAL addr)
+bool red_get_update_cmd(QXLInstance *qxl_instance, RedMemSlotInfo *slots,
+int group_id, RedUpdateCmd *red,
+QXLPHYSICAL addr)
 {
 QXLUpdateCmd *qxl;
 int error;
@@ -1251,10 +1252,10 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int 
group_id,
 if (error) {
 return false;
 }
+red->qxl = qxl_instance;
 red->release_info_ext.info = >release_info;
 red->release_info_ext.group_id = group_id;
 
-
 red_get_rect_ptr(>area, >area);
 red->update_id  = qxl->update_id;
 red->surface_id = qxl->surface_id;
@@ -1263,7 +1264,9 @@ bool red_get_update_cmd(RedMemSlotInfo *slots, int 
group_id,
 
 void red_put_update_cmd(RedUpdateCmd *red)
 {
-/* nothing yet */
+if (red->qxl != NULL) {
+red_qxl_release_resource(red->qxl, red->release_info_ext);
+}
 }
 
 bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, int 
group_id,
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index c73462e0e..c3ad39ea7 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -68,6 +68,7 @@ static inline RedDrawable *red_drawable_ref(RedDrawable 
*drawable)
 void red_drawable_unref(RedDrawable *red_drawable);
 
 typedef struct RedUpdateCmd {
+QXLInstance *qxl;
 QXLReleaseInfoExt release_info_ext;
 SpiceRect area;
 uint32_t update_id;
@@ -124,7 +125,7 @@ RedDrawable *red_drawable_new(QXLInstance *qxl, 
RedMemSlotInfo *slots,
   uint32_t flags);
 void red_drawable_unref(RedDrawable *red_drawable);
 
-bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
+bool red_get_update_cmd(QXLInstance *qxl, RedMemSlotInfo *slots, int group_id,
 RedUpdateCmd *red, QXLPHYSICAL addr);
 void red_put_update_cmd(RedUpdateCmd *red);
 
diff --git a/server/red-worker.c b/server/red-worker.c
index 762443f7c..62ee357ab 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -221,7 +221,7 @@ static int red_process_display(RedWorker *worker, int 
*ring_is_empty)
 case QXL_CMD_UPDATE: {
 RedUpdateCmd update;
 
-if (!red_get_update_cmd(>mem_slots, ext_cmd.group_id,
+if (!red_get_update_cmd(worker->qxl, >mem_slots, 
ext_cmd.group_id,
 , ext_cmd.cmd.data)) {
 break;
 }
@@ -231,7 +231,6 @@ static int red_process_display(RedWorker *worker, int 
*ring_is_empty)
 display_channel_draw(worker->display_channel, , 
update.surface_id);
 red_qxl_notify_update(worker->qxl, update.update_id);
 }
-red_qxl_release_resource(worker->qxl, update.release_info_ext);
 red_put_update_cmd();
 break;
 }
-- 
2.14.3

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

[Spice-devel] [spice-server v2 4/9] qxl: Make red_{get, put}_drawable static

[Christophe Fergeau]

Rather than needing to call red_drawable_new() and then initialize it
with red_get_drawable(), we can improve slightly red_drawable new so
that red_drawable_{new,ref,unref} is all which is used by code out of
red-parse-qxl.c.

Signed-off-by: Christophe Fergeau 
---
 server/red-parse-qxl.c | 17 -
 server/red-parse-qxl.h |  9 -
 server/red-worker.c| 11 ++-
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 245235082..cc6a8b51d 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1176,8 +1176,8 @@ static bool red_get_compat_drawable(RedMemSlotInfo 
*slots, int group_id,
 return true;
 }
 
-bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
-  RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
+static bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
+ RedDrawable *red, QXLPHYSICAL addr, uint32_t 
flags)
 {
 bool ret;
 
@@ -1189,7 +1189,7 @@ bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
 return ret;
 }
 
-void red_put_drawable(RedDrawable *red)
+static void red_put_drawable(RedDrawable *red)
 {
 red_put_clip(>clip);
 if (red->self_bitmap_image) {
@@ -1492,12 +1492,19 @@ void red_drawable_unref(RedDrawable *red_drawable)
 g_free(red_drawable);
 }
 
-RedDrawable *red_drawable_new(QXLInstance *qxl)
+RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+  int group_id, QXLPHYSICAL addr,
+  uint32_t flags)
 {
-RedDrawable * red = g_new0(RedDrawable, 1);
+RedDrawable *red = g_new0(RedDrawable, 1);
 
 red->refs = 1;
 red->qxl = qxl;
 
+if (!red_get_drawable(slots, group_id, red, addr, flags)) {
+   red_drawable_unref(red);
+   return NULL;
+}
+
 return red;
 }
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 0b507b93a..882657e88 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -64,8 +64,6 @@ static inline RedDrawable *red_drawable_ref(RedDrawable 
*drawable)
 drawable->refs++;
 return drawable;
 }
-RedDrawable *red_drawable_new(QXLInstance *qxl);
-void red_drawable_unref(RedDrawable *red_drawable);
 
 void red_drawable_unref(RedDrawable *red_drawable);
 
@@ -120,9 +118,10 @@ typedef struct RedCursorCmd {
 
 void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);
 
-bool red_get_drawable(RedMemSlotInfo *slots, int group_id,
-  RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
-void red_put_drawable(RedDrawable *red);
+RedDrawable *red_drawable_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+  int group_id, QXLPHYSICAL addr,
+  uint32_t flags);
+void red_drawable_unref(RedDrawable *red_drawable);
 
 bool red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
 RedUpdateCmd *red, QXLPHYSICAL addr);
diff --git a/server/red-worker.c b/server/red-worker.c
index 8f806e8e3..ebc6d423d 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -205,15 +205,16 @@ static int red_process_display(RedWorker *worker, int 
*ring_is_empty)
 worker->display_poll_tries = 0;
 switch (ext_cmd.cmd.type) {
 case QXL_CMD_DRAW: {
-RedDrawable *red_drawable = red_drawable_new(worker->qxl); // 
returns with 1 ref
+RedDrawable *red_drawable;
+red_drawable = red_drawable_new(worker->qxl, >mem_slots,
+ext_cmd.group_id, ext_cmd.cmd.data,
+ext_cmd.flags); // returns with 1 
ref
 
-if (red_get_drawable(>mem_slots, ext_cmd.group_id,
- red_drawable, ext_cmd.cmd.data, 
ext_cmd.flags)) {
+if (red_drawable != NULL) {
 display_channel_process_draw(worker->display_channel, 
red_drawable,
  
worker->process_display_generation);
+red_drawable_unref(red_drawable);
 }
-// release the red_drawable
-red_drawable_unref(red_drawable);
 break;
 }
 case QXL_CMD_UPDATE: {
-- 
2.14.3

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

[Spice-devel] [spice-server v2 0/9] qxl: Move more guest resource release to red-parse-qxl

[Christophe Fergeau]

Hey,

Currently, after parsing a QXL command through red-parse-qxl, the code which
got the command has to tell red-parse-qxl when it no longer needs the
command, but also to remember to release the command QXL resources
itself. This series moves this 'release resource' logic to
red-parse-qxl.

Changes since v1:
- moved renaming patch to the end, and made it much more extensive
- added a new patch unifying identical methods
- reworked 'qxl: Fix guest resources release in red_put_drawable()' so that 
it's similar to
  the cursor changes

Christophe

Christophe Fergeau (9):
  qxl: Remove red_put_blend()
  qxl: Remove 'blackness' and 'invers' put/get methods
  qxl: Move red_drawable_unref/red_drawable_new
  qxl: Make red_{get,put}_drawable static
  qxl: Fix guest resources release in red_put_drawable()
  qxl: Add red_cursor_cmd_new and red_cursor_cmd_free helpers
  qxl: Release QXL resource in red_put_message
  qxl: Release QXL resources in red_put_update_cmd
  qxl: Improve 'red' and 'qxl' argument names

 server/cursor-channel.c |   6 +-
 server/red-parse-qxl.c  | 993 +---
 server/red-parse-qxl.h  |  19 +-
 server/red-worker.c |  47 +-
 server/tests/test-qxl-parsing.c |  37 +-
 5 files changed, 558 insertions(+), 544 deletions(-)

-- 
2.14.3

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

[Spice-devel] [spice-server v2 6/9] qxl: Add red_cursor_cmd_new and red_cursor_cmd_free helpers

[Christophe Fergeau]

Currently, the cursor channel is allocating RedCursorCmd instances itself, and 
then
calling into red-parse-qxl.h to initialize it, and doing something
similar when releasing the data. This commit moves this common code to
red-parse-qxl.[ch]

Signed-off-by: Christophe Fergeau 
---
 server/cursor-channel.c |  6 +-
 server/red-parse-qxl.c  | 24 +---
 server/red-parse-qxl.h  |  6 +++---
 server/red-worker.c | 10 +-
 server/tests/test-qxl-parsing.c | 37 -
 5 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index ada1d62a7..c826627bf 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -69,13 +69,9 @@ static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd 
*cmd)
 
 static void cursor_pipe_item_free(RedPipeItem *base)
 {
-RedCursorCmd *cursor_cmd;
 RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);
 
-cursor_cmd = pipe_item->red_cursor;
-red_put_cursor_cmd(cursor_cmd);
-g_free(cursor_cmd);
-
+red_cursor_cmd_free(pipe_item->red_cursor);
 g_free(pipe_item);
 }
 
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index ccb01d92d..b0c47cfeb 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1443,8 +1443,9 @@ static void red_put_cursor(SpiceCursor *red)
 g_free(red->data);
 }
 
-bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
-RedCursorCmd *red, QXLPHYSICAL addr)
+static bool red_get_cursor_cmd(QXLInstance *qxl_instance, RedMemSlotInfo 
*slots,
+   int group_id, RedCursorCmd *red,
+   QXLPHYSICAL addr)
 {
 QXLCursorCmd *qxl;
 int error;
@@ -1453,6 +1454,7 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int 
group_id,
 if (error) {
 return false;
 }
+red->qxl = qxl_instance;
 red->release_info_ext.info  = >release_info;
 red->release_info_ext.group_id  = group_id;
 
@@ -1470,10 +1472,25 @@ bool red_get_cursor_cmd(RedMemSlotInfo *slots, int 
group_id,
 red->u.trail.frequency = qxl->u.trail.frequency;
 break;
 }
+
 return true;
 }
 
-void red_put_cursor_cmd(RedCursorCmd *red)
+RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr)
+{
+RedCursorCmd *cmd;
+
+cmd = g_new0(RedCursorCmd, 1);
+if (!red_get_cursor_cmd(qxl, slots, group_id, cmd, addr)) {
+g_free(cmd);
+return NULL;
+}
+
+return cmd;
+}
+
+void red_cursor_cmd_free(RedCursorCmd *red)
 {
 switch (red->type) {
 case QXL_CURSOR_SET:
@@ -1483,6 +1500,7 @@ void red_put_cursor_cmd(RedCursorCmd *red)
 if (red->qxl) {
 red_qxl_release_resource(red->qxl, red->release_info_ext);
 }
+g_free(red);
 }
 
 void red_drawable_unref(RedDrawable *red_drawable)
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index 882657e88..231844e96 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -138,8 +138,8 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int 
group_id,
  RedSurfaceCmd *red, QXLPHYSICAL addr);
 void red_put_surface_cmd(RedSurfaceCmd *red);
 
-bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
-RedCursorCmd *red, QXLPHYSICAL addr);
-void red_put_cursor_cmd(RedCursorCmd *red);
+RedCursorCmd *red_cursor_cmd_new(QXLInstance *qxl, RedMemSlotInfo *slots,
+ int group_id, QXLPHYSICAL addr);
+void red_cursor_cmd_free(RedCursorCmd *cmd);
 
 #endif /* RED_PARSE_QXL_H_ */
diff --git a/server/red-worker.c b/server/red-worker.c
index ebc6d423d..2dcf2b0ef 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -103,13 +103,14 @@ static gboolean red_process_cursor_cmd(RedWorker *worker, 
const QXLCommandExt *e
 {
 RedCursorCmd *cursor_cmd;
 
-cursor_cmd = g_new0(RedCursorCmd, 1);
-if (!red_get_cursor_cmd(>mem_slots, ext->group_id, cursor_cmd, 
ext->cmd.data)) {
-g_free(cursor_cmd);
+cursor_cmd = red_cursor_cmd_new(worker->qxl, >mem_slots,
+ext->group_id, ext->cmd.data);
+if (cursor_cmd == NULL) {
 return FALSE;
 }
-cursor_cmd->qxl = worker->qxl;
+
 cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
+
 return TRUE;
 }
 
@@ -965,7 +966,6 @@ static bool loadvm_command(RedWorker *worker, QXLCommandExt 
*ext)
 switch (ext->cmd.type) {
 case QXL_CMD_CURSOR:
 return red_process_cursor_cmd(worker, ext);
-
 case QXL_CMD_SURFACE:
 return red_process_surface_cmd(worker, ext, TRUE);
 
diff --git a/server/tests/test-qxl-parsing.c b/server/tests/test-qxl-parsing.c
index 47139a48c..f2b12ad07 100644
--- a/server/tests/test-qxl-parsing.c
+++ 

Re: [Spice-devel] [spice-server 04/10] qxl: Fix guest resources release in red_put_drawable()

[Christophe Fergeau]

On Mon, Apr 16, 2018 at 06:51:52PM +0300, Uri Lublin wrote:
> On 04/16/2018 01:13 PM, Christophe Fergeau wrote:
> > At the moment, we'll unconditionally release the guest QXL resources in
> > red_put_drawable() even if red_get_drawable() failed and did not
> > initialize drawable->release_info_ext properly.
> > This commit checks the QXLReleaseInfo in release_info_ext is non-0
> > before attempting to release it.
> > ---
> >   server/red-parse-qxl.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > index dc99d1f30..80746ecbb 100644
> > --- a/server/red-parse-qxl.c
> > +++ b/server/red-parse-qxl.c
> > @@ -1504,7 +1504,9 @@ void red_drawable_unref(RedDrawable *red_drawable)
> >   if (--red_drawable->refs) {
> >   return;
> >   }
> > -red_qxl_release_resource(red_drawable->qxl, 
> > red_drawable->release_info_ext);
> > +if (red_drawable->release_info_ext.info != NULL) {
> > +red_qxl_release_resource(red_drawable->qxl, 
> > red_drawable->release_info_ext);
> 
> Hi Christophe,
> 
> Would it not be better to do the check in red_qxl_release_resource ?
> (and also set ".info" to NULL after it is released)

After implementing Frediano's suggestion, this test is gone ;)

Christophe


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

Re: [Spice-devel] [PATCH spice-server 2/3] red-stream: Implements flush using TCP_CORK

[Uri Lublin]

On 04/13/2018 03:25 PM, Frediano Ziglio wrote:

Cork is a system interface implemented by Linux and some *BSD systems to
tell the system that other data are expected to be written to a socket.
This allows the system to reduce network fragmentation waiting for network
packets to be complete.

Using some replay capture and some instrumentation resulted in a
bandwith reduction of 11% and a packet reduction of 56%.

The tests was done using replay utility so results could be a bit different
from real cases as:
- replay goes as fast as it can, for instance packets could
   be merged by the kernel decreasing packet numbers and a bit
   byte spent (this actually make the following improves worse);
- there are fewer channels (no much cursor, sound, etc).
The following tests shows count packet and total bytes from server to
client using a real network. I used a direct cable connection using 1gb
connection and 2 laptops.

cork: 537 1582240
cork: 681 1823754
cork: 524 1583287
cork: 538 1582350
no cork: 1329 1834630
no cork: 1290 1829094
no cork: 1289 1830164
no cork: 1317 1833589
no cork: 1320 1835705

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

diff --git a/server/red-stream.c b/server/red-stream.c
index 9a9c1a0f..18c4a935 100644
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -24,6 +24,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 
  
@@ -37,6 +38,11 @@

  #include "red-stream.h"
  #include "reds.h"
  
+// compatibility for *BSD systems

+#ifndef TCP_CORK
+#define TCP_CORK TCP_NOPUSH
+#endif
+
  struct AsyncRead {
  void *opaque;
  uint8_t *now;
@@ -83,6 +89,8 @@ struct RedStreamPrivate {
   * deallocated when main_dispatcher handles the 
SPICE_CHANNEL_EVENT_DISCONNECTED
   * event, either from same thread or by call back from main thread. */
  SpiceChannelEventInfo* info;
+bool use_cork;
+bool corked;
  
  ssize_t (*read)(RedStream *s, void *buf, size_t nbyte);

  ssize_t (*write)(RedStream *s, const void *buf, size_t nbyte);
@@ -92,6 +100,16 @@ struct RedStreamPrivate {
  SpiceCoreInterfaceInternal *core;
  };
  
+/**

+ * Set TCP_CORK on socket
+ */
+/* NOTE: enabled must be int */
+static int socket_set_cork(int socket, int enabled)
+{
+SPICE_VERIFY(sizeof(enabled) == sizeof(int));
+return setsockopt(socket, IPPROTO_TCP, TCP_CORK, , 
sizeof(enabled));
+}
+
  static ssize_t stream_write_cb(RedStream *s, const void *buf, size_t size)
  {
  return write(s->socket, buf, size);
@@ -205,11 +223,31 @@ bool red_stream_write_all(RedStream *stream, const void 
*in_buf, size_t n)
  
  bool red_stream_set_auto_flush(RedStream *s, bool auto_flush)

  {
-return auto_flush;
+if (s->priv->use_cork == !auto_flush) {
+return true;
+}
+
+s->priv->use_cork = !auto_flush;
+if (s->priv->use_cork) {
+if (socket_set_cork(s->socket, 1)) {
+s->priv->use_cork = false;
+return false;
+} else {
+s->priv->corked = true;
+}
+} else if (s->priv->corked) {
+socket_set_cork(s->socket, 0);
+s->priv->corked = false;
+}

Hi Frediano,

It would be simpler to use !auto_flash or s->priv->use_cork
and also only keep one of use_cork and corked.
Possibly the logic is a bit different and not exactly what you want.

  if (socket_set_cork(s->socket, !auto_flash)) {
  return false;
  }

  s->priv->use_cork = !auto_flash;



+return true;
  }


Uri.

  
  void red_stream_flush(RedStream *s)

  {
+if (s->priv->corked) {
+socket_set_cork(s->socket, 0);
+socket_set_cork(s->socket, 1);
+}
  }
  
  #if HAVE_SASL




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

Re: [Spice-devel] [spice-gtk PATCH 2/2] gst_decoder_queue_frame: free frame when returning false

[Uri Lublin]

On 04/17/2018 03:13 PM, Frediano Ziglio wrote:


The decoder_queue_frame now owns frame.



The queue is named decoding_queue and actually the reason you have to free frame
is that is not owned by decoding_queue in this path of code.


I was talking about the function not the queue itself.

I'll rewrite the commit log.



Note that the same issue happens some lines below in the

#if GST_CHECK_VERSION(1,9,0)
 if (decoder->appsrc == NULL) {
 spice_warning("Error: Playbin has not yet initialized the Appsrc 
element");
 stream_dropped_frame_on_playback(decoder->base.stream);
 return TRUE;
 }
#endif

does not matter if the function returns TRUE or FALSE, the caller never free 
frame
so either we free or we retain it.


Yes, I missed that return TRUE. I assumed all cases of return TRUE
were already handled.
I'll add it to v2.

Thanks,
Uri.





Signed-off-by: Uri Lublin 
---
  src/channel-display-gst.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 0d7aabb..ae59292 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -544,6 +544,7 @@ static gboolean
spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
  if (decoder->pipeline == NULL) {
  /* An error occurred, causing the GStreamer pipeline to be freed */
  spice_warning("An error occurred, stopping the video stream");
+frame->free(frame);
  return FALSE;
  }
  


Frediano



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

Re: [Spice-devel] [PATCH spice-server 3/3] common-graphics-channel: Use manual flushing on stream to decrease packet fragmentation

[Christophe Fergeau]

On Fri, Apr 13, 2018 at 01:25:08PM +0100, Frediano Ziglio wrote:
> In order to use the new TCP_CORK feature, disable auto flush.
> 
> Depending on channel implementation and purpose of the channel enabling
> blindly for all channels could cause performance issues, specifically if
> flush is not done at the right time.
> CommonGraphicsChannel channels were tested to make sure is not that case.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/common-graphics-channel.c | 17 +
>  server/red-channel-client.c  |  5 +
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/server/common-graphics-channel.c 
> b/server/common-graphics-channel.c
> index ce6b5e57..083ab3eb 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -83,14 +83,15 @@ bool common_channel_client_config_socket(RedChannelClient 
> *rcc)
>  
>  // TODO - this should be dynamic, not one time at channel creation
>  is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> -/* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
> - * on the delayed ack timeout on the other side.
> - * Instead of using Nagle's, we need to implement message buffering on
> - * the application level.
> - * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
> - */
> -red_stream_set_no_delay(stream, !is_low_bandwidth);
> -
> +if (!red_stream_set_auto_flush(stream, false)) {
> +/* FIXME: Using Nagle's Algorithm can lead to apparent delays, 
> depending
> + * on the delayed ack timeout on the other side.
> + * Instead of using Nagle's, we need to implement message buffering 
> on
> + * the application level.
> + * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
> + */
> +red_stream_set_no_delay(stream, !is_low_bandwidth);
> +}
>  // TODO: move wide/narrow ack setting to red_channel.
>  red_channel_client_ack_set_client_window(rcc,
>  is_low_bandwidth ?
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index f154c5c6..fe912802 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1328,6 +1328,11 @@ void red_channel_client_push(RedChannelClient *rcc)
>  if ((red_channel_client_no_item_being_sent(rcc) && 
> g_queue_is_empty(>priv->pipe)) ||
>  red_channel_client_waiting_for_ack(rcc)) {
>  red_channel_client_watch_update_mask(rcc, SPICE_WATCH_EVENT_READ);
> +/* channel has no pending data to send so now we can flush data in
> + * order to avoid data stall into buffers in case of manual
> + * flushing
> + */
> +red_stream_flush(rcc->priv->stream);

I finally convinced myself that having it here makes sense, so
Acked-by: Christophe Fergeau  for the series.

Christophe


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

Re: [Spice-devel] [PATCH 1/2] Ensure that plugins cannot bypass version check

[Frediano Ziglio]

>
> From: Christophe de Dinechin 
> 
> This change addresses three issues related to plugin version checking:
> 
> 1. It is possible for plugins to bypass version checking or do it wrong
>(as a matter of fact, the mjpeg fallback sets a bad example)
> 
> 2. The current plugin version check violates the C++ ODR, i.e.
>it relies on undefined behaviors when the header used to compile
>the plugin and to compile the agent are not identical,
> 
> 3. A major.minor numbering scheme is not ideal for ABI checks.
>In particular, it makes it difficult to react to an incompatibility
>that was detected post release.
> 
> [More info]
> 
> 1. Make it impossible to bypass version checking
> 
> The current code depends on the plugin implementing the version check
> correctly by calling PluginVersionIsCompatible. To make things worse,
> the only publicly available example gets this wrong and uses an
> ad-hoc version check, so anybody copy-pasting this code will get it
> wrong.
> 
> It is more robust to do the version check in the agent before calling
> any method in the plugin. It ensures that version checking cannot be
> bypassed, is done consistently and generates consistent error messages.
> 
> As an indication that the aproach is robust, the new check correctly
> refuses to load older plugins that use the old version checking method:
> 
> spice-streaming-agent[5167]:
> error loading plugin [...].so: no version information
> 

This is clear to everybody, doing from the agent avoid each plugin to
do the check on the version and we can do it safely before executing
any code in the plugin (library initialization aside).

> 2. ODR-related problems
> 
> The C++ One Definition Rule (ODR) states that all translation units
> must see the same definitions. In the current code, when we call
> Agent::PluginVersionIsCompatible from the plugin, it is an ODR
> violation as soon as we have made any change in the Agent class
> compared to what the plugin was compiled against.
> 
> The current code therefore relies on implementation dependent knowlege
> of how virtual functions are laid out in the vtable, and puts
> unnecessary constraints on the changes allowed in the classes
> (e.g. it's not allowed to put anything before some member functions)
> 
> 3. Major.minor numbering scheme
> 
> The major.minor numbering scheme initially selected makes it harder
> to fixes cases where an incompatibility was detected after release.
> 
> For example, the major.minor version checking assumes that agent 1.21
> is compatible with plugins 1.21, 1.13 or 1.03. If later testing
> shows that 1.13 actually introduced an incompatiliy, you have to
> special-case 1.13 in the compatibiliy check.
> 
> An approach that does not have this problem is to rely on incremented
> version numbers, with a "current" and "oldest compatible" version
> number. This is used for example by libtools [1].
> 
> Since the change required for #1 and #2  introduces an ABI break,
> it is as good a time as any to also change the numbering scheme,
> since changing it later would introduce another unnecessary ABI break.
> 
> [1]
> https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> 

Reading all the replies looks like to me that what you are basically
doing with these changes is basically removing the minor from the versioning
schema and making every change binary incompatible.

libtool by itself uses major, minor to implement its numbering schema
that is the major minor schema is more flexible than libtool schema.
Major minor allows for instance to have different plugins or executables 
installed
each in its major private directory or having multiple binary library
installed in a system.

If a plugin is compiled with version X and the agent if version Y (obviously
having X != Y) a full ODR would require the definition of "Agent" (for instance)
in version X would be the same of version Y. That is each version is not
compatible.

> Signed-off-by: Christophe de Dinechin 
> ---
>  include/spice-streaming-agent/plugin.hpp | 50
>  +---
>  src/concrete-agent.cpp   | 35 +++---
>  src/concrete-agent.hpp   |  4 ---
>  src/mjpeg-fallback.cpp   |  3 --
>  4 files changed, 45 insertions(+), 47 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index e08e3a6..0ec5040 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -23,11 +23,22 @@ namespace streaming_agent {
>  class FrameCapture;
> 
>  /*!
> - * Plugin version, only using few bits, schema is 0xMMmm
> - * where MM is major and mm is the minor, can be easily expanded
> - * using more bits in the future
> + * Plugins use a versioning system similar to that implemented by libtool
> + *
> 

Re: [Spice-devel] [PATCH spice-gtk] spice-channel: Use atomic operations for SpiceMsgIn::refcount

[Frediano Ziglio]

> 
> On Tue, Apr 17, 2018 at 01:32:53PM +0100, Frediano Ziglio wrote:
> > This structure is potentially used in multiple thread.
> > Currently in Gstreamer thread using streaming data and coroutine
> > thread.
> 
> But only GStreamer uses another thread, so this might be
> suboptimal for all other channels.
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > I would avoid the atomic penalty but better safe then sorry

Yes, that is my comment too... do you have another safe solution?

I was thinking about adding an atomic reference counting for
SpiceFrame instead and using it for instance, but I'm not sure
it fixes all the cases. It would need to remove ref_data/unref_data
and have ownership from SpiceFrame to "data" (raw message basically)
but this would mean that refcount would be 2 in queue_frame and
decremented by coroutine thread and lately by GStreamer thread which
in theory is racy too.

OT: not much familiar with the threading model of spice-gtk.
I supposed (beside GStreamer) there were a main thread and a
coroutine thread. Is the main thread the same as coroutine one?

> > ---
> >  src/spice-channel-priv.h | 2 +-
> >  src/spice-channel.c  | 5 ++---
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> > index b431037..706df9f 100644
> > --- a/src/spice-channel-priv.h
> > +++ b/src/spice-channel-priv.h
> > @@ -55,7 +55,7 @@ struct _SpiceMsgOut {
> >  };
> >  
> >  struct _SpiceMsgIn {
> > -int   refcount;
> > +gint  refcount;
> >  SpiceChannel  *channel;
> >  uint8_t   header[MAX_SPICE_DATA_HEADER_SIZE];
> >  uint8_t   *data;
> > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > index 7e3e3b7..49897b7 100644
> > --- a/src/spice-channel.c
> > +++ b/src/spice-channel.c
> > @@ -531,7 +531,7 @@ void spice_msg_in_ref(SpiceMsgIn *in)
> >  {
> >  g_return_if_fail(in != NULL);
> >  
> > -in->refcount++;
> > +g_atomic_int_inc(>refcount);
> >  }
> >  
> >  G_GNUC_INTERNAL
> > @@ -539,8 +539,7 @@ void spice_msg_in_unref(SpiceMsgIn *in)
> >  {
> >  g_return_if_fail(in != NULL);
> >  
> > -in->refcount--;
> > -if (in->refcount > 0)
> > +if (!g_atomic_int_dec_and_test(>refcount))
> >  return;
> >  if (in->parsed)
> >  in->pfree(in->parsed);

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

Re: [Spice-devel] [PATCH spice-gtk] spice-channel: Use atomic operations for SpiceMsgIn::refcount

[Victor Toso]

On Tue, Apr 17, 2018 at 01:32:53PM +0100, Frediano Ziglio wrote:
> This structure is potentially used in multiple thread.
> Currently in Gstreamer thread using streaming data and coroutine
> thread.

But only GStreamer uses another thread, so this might be
suboptimal for all other channels.

> 
> Signed-off-by: Frediano Ziglio 
> ---
> I would avoid the atomic penalty but better safe then sorry
> ---
>  src/spice-channel-priv.h | 2 +-
>  src/spice-channel.c  | 5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> index b431037..706df9f 100644
> --- a/src/spice-channel-priv.h
> +++ b/src/spice-channel-priv.h
> @@ -55,7 +55,7 @@ struct _SpiceMsgOut {
>  };
>  
>  struct _SpiceMsgIn {
> -int   refcount;
> +gint  refcount;
>  SpiceChannel  *channel;
>  uint8_t   header[MAX_SPICE_DATA_HEADER_SIZE];
>  uint8_t   *data;
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 7e3e3b7..49897b7 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -531,7 +531,7 @@ void spice_msg_in_ref(SpiceMsgIn *in)
>  {
>  g_return_if_fail(in != NULL);
>  
> -in->refcount++;
> +g_atomic_int_inc(>refcount);
>  }
>  
>  G_GNUC_INTERNAL
> @@ -539,8 +539,7 @@ void spice_msg_in_unref(SpiceMsgIn *in)
>  {
>  g_return_if_fail(in != NULL);
>  
> -in->refcount--;
> -if (in->refcount > 0)
> +if (!g_atomic_int_dec_and_test(>refcount))
>  return;
>  if (in->parsed)
>  in->pfree(in->parsed);
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-common 0/3] Fix some possible overflows in client

[Frediano Ziglio]

ping

> 
> ping
> 
> > 
> > This fix https://bugzilla.redhat.com/show_bug.cgi?id=1501200.
> > 
> > Frediano Ziglio (3):
> >   Fix integer overflows computing sizes
> >   Write a small test to test possible crash
> >   Avoid integer overflow computing image sizes
> > 
> >  python_modules/demarshal.py | 52 ++--
> >  python_modules/marshal.py   |  7 ++--
> >  tests/Makefile.am   | 14 
> >  tests/test-overflow.c   | 83
> >  +
> >  4 files changed, 125 insertions(+), 31 deletions(-)
> >  create mode 100644 tests/test-overflow.c
> > 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2] usb-device-widget: Fix crash on no USB devices

[Frediano Ziglio]

> 
> On 13/04/18 18:20, Victor Toso wrote:
> > Hi,
> > 
> > On Fri, Apr 13, 2018 at 03:14:56PM -0300, Eduardo Lima (Etrunko) wrote:
> >> On 13/04/18 05:50, Victor Toso wrote:
> >>> From: goldengdeng <907246...@qq.com>
> >>>
> >>> The spice_usb_device_manager_get_devices() is only checking for NULL
> >>> while the program can crash when no USB devices are available.
> >>>
> >>> Signed-off-by: Victor Toso 
> >>> ---
> >>>  src/usb-device-widget.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> >>> index a3c0910..1be80ae 100644
> >>> --- a/src/usb-device-widget.c
> >>> +++ b/src/usb-device-widget.c
> >>> @@ -218,8 +218,9 @@ static void
> >>> spice_usb_device_widget_constructed(GObject *gobject)
> >>>   G_CALLBACK(device_error_cb), self);
> >>>  
> >>>  devices = spice_usb_device_manager_get_devices(priv->manager);
> >>> -if (!devices)
> >>> +if (devices == NULL || devices->len == 0) {
> >>>  goto end;
> >>> +}
> >>
> >> Does it mean that the crash is happening on g_ptr_array_unref() call
> >> (which happens after the loop below)?
> > 
> > Even if we call g_ptr_array_unref() with NULL, it just log some
> > criticals.
> > 
> >> Would be interesting to see the backtrace for this supposed
> >> crash, because this patch does not seem correct to me.
> > 
> > Yeah, I agree. I've asked in the original email too although the
> > change itself is not complex I don't want to dive into 'where
> > could it crash' but for sure we should check if devices is NULL
> > (the original patch removed that).
> > 
> 
> I agree with the check for NULL, but the new conditional for
> devices->len is already done in the loop, thus the reason I asked for
> the trace.
> 
> Regards, Eduardo
> 

Agreed, looks like this patch is just adding a leak if the returned
array has 0 elements.
From the last patch from Victor looks like this patch is silently
nacked.

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

[Spice-devel] [PATCH spice-gtk] spice-channel: Use atomic operations for SpiceMsgIn::refcount

[Frediano Ziglio]

This structure is potentially used in multiple thread.
Currently in Gstreamer thread using streaming data and coroutine
thread.

Signed-off-by: Frediano Ziglio 
---
I would avoid the atomic penalty but better safe then sorry
---
 src/spice-channel-priv.h | 2 +-
 src/spice-channel.c  | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
index b431037..706df9f 100644
--- a/src/spice-channel-priv.h
+++ b/src/spice-channel-priv.h
@@ -55,7 +55,7 @@ struct _SpiceMsgOut {
 };
 
 struct _SpiceMsgIn {
-int   refcount;
+gint  refcount;
 SpiceChannel  *channel;
 uint8_t   header[MAX_SPICE_DATA_HEADER_SIZE];
 uint8_t   *data;
diff --git a/src/spice-channel.c b/src/spice-channel.c
index 7e3e3b7..49897b7 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -531,7 +531,7 @@ void spice_msg_in_ref(SpiceMsgIn *in)
 {
 g_return_if_fail(in != NULL);
 
-in->refcount++;
+g_atomic_int_inc(>refcount);
 }
 
 G_GNUC_INTERNAL
@@ -539,8 +539,7 @@ void spice_msg_in_unref(SpiceMsgIn *in)
 {
 g_return_if_fail(in != NULL);
 
-in->refcount--;
-if (in->refcount > 0)
+if (!g_atomic_int_dec_and_test(>refcount))
 return;
 if (in->parsed)
 in->pfree(in->parsed);
-- 
2.14.3

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

Re: [Spice-devel] [spice-gtk PATCH 2/2] gst_decoder_queue_frame: free frame when returning false

[Frediano Ziglio]

> 
> The decoder_queue_frame now owns frame.
> 

The queue is named decoding_queue and actually the reason you have to free frame
is that is not owned by decoding_queue in this path of code.

Note that the same issue happens some lines below in the

#if GST_CHECK_VERSION(1,9,0)
if (decoder->appsrc == NULL) {
spice_warning("Error: Playbin has not yet initialized the Appsrc 
element");
stream_dropped_frame_on_playback(decoder->base.stream);
return TRUE;
}
#endif

does not matter if the function returns TRUE or FALSE, the caller never free 
frame
so either we free or we retain it.

> Signed-off-by: Uri Lublin 
> ---
>  src/channel-display-gst.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 0d7aabb..ae59292 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -544,6 +544,7 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>  if (decoder->pipeline == NULL) {
>  /* An error occurred, causing the GStreamer pipeline to be freed */
>  spice_warning("An error occurred, stopping the video stream");
> +frame->free(frame);
>  return FALSE;
>  }
>  

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

Re: [Spice-devel] [spice-gtk PATCH 1/2] mjpeg_decoder_queue_frame: free frame when dropping the frame

[Frediano Ziglio]

> 
> Signed-off-by: Uri Lublin 
> ---
>  src/channel-display-mjpeg.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 8c4c0aa..7b2f775 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -262,6 +262,7 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
> *video_decoder,
>   * So drop late frames as early as possible to save on processing time.
>   */
>  if (latency < 0) {
> +frame->free(frame);
>  return TRUE;
>  }
>  

Patch looks good, but I would write something more in the commit, something 
like:

We own the frame and we didn't add to any queue so if we don't free the
frame we are having a leak.

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

[Spice-devel] [spice-gtk PATCH 1/2] mjpeg_decoder_queue_frame: free frame when dropping the frame

[Uri Lublin]

Signed-off-by: Uri Lublin 
---
 src/channel-display-mjpeg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 8c4c0aa..7b2f775 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -262,6 +262,7 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder 
*video_decoder,
  * So drop late frames as early as possible to save on processing time.
  */
 if (latency < 0) {
+frame->free(frame);
 return TRUE;
 }
 
-- 
2.14.3

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

[Spice-devel] [spice-gtk PATCH 2/2] gst_decoder_queue_frame: free frame when returning false

[Uri Lublin]

The decoder_queue_frame now owns frame.

Signed-off-by: Uri Lublin 
---
 src/channel-display-gst.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 0d7aabb..ae59292 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -544,6 +544,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder 
*video_decoder,
 if (decoder->pipeline == NULL) {
 /* An error occurred, causing the GStreamer pipeline to be freed */
 spice_warning("An error occurred, stopping the video stream");
+frame->free(frame);
 return FALSE;
 }
 
-- 
2.14.3

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

Re: [Spice-devel] [spice-gtk v1 09/11] channel-display: don't debug latency for each frame

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> Becomes quite hard to find meaning on something that is printed every
> time. Only print latency value if it is a new min/max or if average
> latency is 10% bigger/lower then usual.
> 
> Not aiming to perfect statistics in latency here, just to avoid too
> verbose logging. Removing latency debug isn't cool as we could infer
> issues with streaming based on it.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display-priv.h |  3 +++
>  src/channel-display.c  | 18 +-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 95ad7d8..e7758cc 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -136,6 +136,9 @@ struct display_stream {
>  drops_sequence_stats cur_drops_seq_stats;
>  GArray   *drops_seqs_stats_arr;
>  uint32_t num_drops_seqs;
> +uint32_t latency_min;
> +uint32_t latency_max;
> +uint32_t latency_avg;
>  

In the style documentation is explicitly state that we should not
column align.

>  uint32_t playback_sync_drops_seq_len;
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 4757aa6..3901cd1 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1547,6 +1547,10 @@ static void display_stream_stats_save(display_stream
> *st,
>guint32 client_mmtime)
>  {
>  gint32 latency = server_mmtime - client_mmtime;
> +gint32 min_latency = st->latency_min == 0 ? latency :
> MIN(st->latency_min, latency);

why not initializing latency_min with INT32_MAX?

> +gint32 max_latency = MAX(st->latency_max, latency);

as latency can be <0 latency_max should be initialized to INT32_MIN, not 0.

> +gdouble avg_latency = (st->latency_avg * st->num_input_frames + latency)
> /
> +  ((double) (st->num_input_frames + 1));
>  

I would use a latency_total in the display_stream structure. I think int64_t is
safe.

>  if (!st->num_input_frames) {
>  st->first_frame_mm_time = server_mmtime;
> @@ -1567,7 +1571,19 @@ static void display_stream_stats_save(display_stream
> *st,
>  return;
>  }
>  
> -CHANNEL_DEBUG(st->channel, "video latency: %d", latency);
> +/* Only debug latency value if it matters otherwise it can be too
> verbose */
> +if (min_latency != st->latency_min ||
> +max_latency != st->latency_max ||
> +avg_latency < 0.90 * st->latency_avg ||
> +avg_latency > 1.10 * st->latency_avg) {
> +CHANNEL_DEBUG(st->channel,
> +  "video latency: %d | (%d , %0.2f , %d)",
> +  latency, min_latency, avg_latency, max_latency);
> +st->latency_min = min_latency;
> +st->latency_max = max_latency;
> +}
> +st->latency_avg = avg_latency;
> +
>  if (st->cur_drops_seq_stats.len) {
>  st->cur_drops_seq_stats.duration = server_mmtime -
> 
> st->cur_drops_seq_stats.start_mm_time;

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

Re: [Spice-devel] [spice-gtk v1 10/11] channel-display-gst: summarize number of frames dropped

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> For example, this has produced 9 lines of debug below instead of 31.
> 
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 5
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 4
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 3
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 2
> frames
> GSpice-DEBUG: channel-display-gst.c:247 the GStreamer pipeline dropped 1
> frames
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display-gst.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index d2847ec..72b5a16 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -207,6 +207,7 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
> gpointer video_decoder)
>  GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
>  if (sample) {
>  GstBuffer *buffer = gst_sample_get_buffer(sample);
> +guint num_frames_dropped = 0;
>  g_mutex_lock(>queues_mutex);
>  
>  /* gst_app_sink_pull_sample() sometimes returns the same buffer
>  twice
> @@ -235,13 +236,16 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
> gpointer video_decoder)
>  /* The GStreamer pipeline dropped the corresponding
>   * buffer.
>   */
> -SPICE_DEBUG("the GStreamer pipeline dropped a frame");
> +num_frames_dropped++;
>  free_gst_frame(gstframe);
>  }
>  break;
>  }
>  l = l->next;
>  }
> +if (num_frames_dropped != 0) {
> +SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> num_frames_dropped);
> +}
>  if (!l) {
>  spice_warning("got an unexpected decoded buffer!");
>  gst_sample_unref(sample);

I think is more a bug (or lack) of protocol specification!

The protocol state (comments included):

message {
StreamDataHeader base;
uint32 data_size;
uint8 data[data_size] @end @nomarshal;
} stream_data;

now... where is the "frame" ?? The client is expecting the "stream_data" to 
contain
a full frame information and calls the stream_data message "frame" and report 
the
problem. The server is currently splitting the byte stream into multiple chunks
which is fine from the protocol specification and the client is complaining.
I think we should start putting more efforts on protocol specification and
avoid to having hidden assumption all over!

I would propose to just drop the SPICE_DEBUG line.

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

Re: [Spice-devel] [PATCH spice-server v2] ci: Workaround bug in Valgrind detecting memcpy instead of memmove

[Frediano Ziglio]

> 
> 
> Acked-by: Christophe Fergeau 
> 

Just for ML reference.

I discovered this issue quite long time ago.
In another project and with Ubuntu I encountered it. I tried to understand,
proved Valgrind had the issue (disassembly and so on).
Did some search on internet and the issue occurred time ago and was fixed.
Apparently however this happened again.
Looks like this happens with some combination of Glibc (don't ask me why).
I tried to fix the issue in Valgrind code but the hooking code was quite
complicated (not even C but some specific script if I remember) and decided
was not worth for me effort and by the way there was already some report/bug
opened so I found the fortify workaround.
Unfortunately even after an Ubuntu upgrade the problem seems still there :-(
Some days ago Snir reported to me the problem with our gitlab CI, I proposed
my workaround and we both tested that is working.
I think Snir did some search on internet too, don't know if he pinged some
report/bug or opened a new one.

Frediano

> On Tue, Apr 17, 2018 at 11:19:16AM +0100, Frediano Ziglio wrote:
> > Due to a bug in current packaged Valgrind in the CI (1:3.13.0-13.fc27)
> > check-valgrind is failing with:
> > 
> > ==17986== Source and destination overlap in memcpy_chk(0x72c060, 0x72c068,
> > 33)
> > ==17986==at 0x4C344F0: __memcpy_chk (vg_replace_strmem.c:1581)
> > ==17986==by 0x40E7E9: check_vmc_error_message
> > (test-stream-device.c:166)
> > ==17986==by 0x40EFD4: test_stream_device_format_after_data
> > (test-stream-device.c:349)
> > ==17986==by 0x7A012E9: test_case_run (gtestutils.c:2157)
> > ==17986==by 0x7A012E9: g_test_run_suite_internal (gtestutils.c:2241)
> > ==17986==by 0x7A0121A: g_test_run_suite_internal (gtestutils.c:2253)
> > ==17986==by 0x7A014C1: g_test_run_suite (gtestutils.c:2329)
> > ==17986==by 0x7A014E0: g_test_run (gtestutils.c:1594)
> > ==17986==by 0x40951A: main (test-stream-device.c:410)
> > ==17986==
> > 
> > By default during CI build _FORTIFY_SOURCE is enabled, which turns memmove
> > into __memmove_chk, which is wrongly turned into __memcpy_chk when running
> > under Valgrind.
> > Setting _FORTIFY_SOURCE to 0 prevents the use of __memmove_chk, and avoids
> > triggering the Valgrind bug.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > This should be temporary till Valgrind is fixed in Fedora
> > 
> > Changes since v1:
> > - update commit message
> > ---
> >  .gitlab-ci.yml | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index 535d220d..655232c2 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -34,7 +34,9 @@ check-valgrind:
> >  dnf install valgrind
> >  gstreamer1-libav gstreamer1-plugins-ugly gstreamer1-plugins-good
> >  gstreamer1-plugins-bad-free
> >  -y
> > -  - ./autogen.sh --enable-valgrind --enable-extra-checks
> > +  - >
> > +CFLAGS='-O2 -pipe -g -D_FORTIFY_SOURCE=0'
> > +./autogen.sh --enable-valgrind --enable-extra-checks
> >- make
> >- make check-valgrind || (cat server/tests/test-suite-memcheck.log &&
> >exit 1)
> >  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 11/11] channel-display-mjpeg: remove verbose logs

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> Those don't add any useful information.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display-mjpeg.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 66a6eb8..380df7a 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -181,7 +181,6 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
>  
>  static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>  {
> -SPICE_DEBUG("%s", __FUNCTION__);
>  if (decoder->timer_id) {
>  return;
>  }
> @@ -234,8 +233,6 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
> *video_decoder,
>  MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
>  SpiceFrame *last_frame;
>  
> -SPICE_DEBUG("%s", __FUNCTION__);
> -
>  last_frame = g_queue_peek_tail(decoder->msgq);
>  if (last_frame) {
>  if (spice_mmtime_diff(frame->mm_time, last_frame->mm_time) < 0) {

Acked

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

Re: [Spice-devel] [spice-gtk v1 06/11] channel-display: add spice_frame_new() helper

[Frediano Ziglio]

> From: Victor Toso 
> 
> As it makes easier to track what is allocated in its own function and
> ultimately what needs to be freed later on.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index a134516..2ee761e 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1485,6 +1485,27 @@ static void
> display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
>  
>  #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
>  
> +static SpiceFrame *spice_frame_new(display_stream *st,
> +   SpiceMsgIn *in,
> +   guint32 server_mmtime)
> +{
> +SpiceFrame *frame;
> +guint8 *data_ptr;
> +const SpiceRect *dest_rect = stream_get_dest(st, in);
> +guint32 data_size = spice_msg_in_frame_data(in, _ptr);
> +

why all these variables? I prefer the old syntax.
Also would be more a clean factorization.

> +frame = g_new(SpiceFrame, 1);
> +frame->mm_time = server_mmtime;
> +frame->dest = *dest_rect;
> +frame->data = data_ptr;
> +frame->size = data_size;
> +frame->data_opaque = in;
> +frame->ref_data = (void*)spice_msg_in_ref;
> +frame->unref_data = (void*)spice_msg_in_unref;
> +frame->free = (void*)g_free;
> +return frame;
> +}
> +
>  G_GNUC_INTERNAL
>  void spice_frame_free(SpiceFrame *frame)
>  {
> @@ -1551,14 +1572,7 @@ static void display_handle_stream_data(SpiceChannel
> *channel, SpiceMsgIn *in)
>   * decoding and best decide if/when to drop them when they are late,
>   * taking into account the impact on later frames.
>   */
> -frame = g_new(SpiceFrame, 1);
> -frame->mm_time = op->multi_media_time;
> -frame->dest = *stream_get_dest(st, in);
> -frame->size = spice_msg_in_frame_data(in, >data);
> -frame->data_opaque = in;
> -frame->ref_data = (void*)spice_msg_in_ref;
> -frame->unref_data = (void*)spice_msg_in_unref;
> -frame->free = (void*)g_free;
> +frame = spice_frame_new(st, in, op->multi_media_time);
>  if (!st->video_decoder->queue_frame(st->video_decoder, frame, latency))
>  {
>  destroy_stream(channel, op->id);
>  report_invalid_stream(channel, op->id);

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

Re: [Spice-devel] [spice-gtk v1 05/11] channel-display: add spice_frame_free() helper

[Frediano Ziglio]

> 
> From: Victor Toso 
> 
> The SpiceFrame is created in channel-display.c but it is currently
> freed at each decoders' end. A helper function can reduce some code
> and makes it easier to check if the function is called, what time was
> called, etc.
> 
> In channel-display-mjpeg.c this means removing free_spice_frame()
> function;
> 
> In channel-display-gst.c, SpiceFrame is under SpiceGstFrame so we just
> need to be careful to call spice_frame_free() once by removing the
> unref function parameter to gst_buffer_new_wrapped_full();
> 
> Note that I'm using g_clear_pointer() everywhere that makes sense
> (checking for NULL before calling free and setting pointer to NULL
> afterwards)
> 
> Signed-off-by: Victor Toso 

Reading this patch I realize how this structure is weird and that
due to it you are introducing a bug.

The problem with SpiceFrame is that the "data" (so data, data_size and
data_opaque fields) are valid until in VideoDecoder::queue_frame OR
SpiceFrame::ref_data is called. The ownership of SpiceFrame is passed
to VideoDecoder::queue_frame. Usually in VideoDecoder::queue_frame
the pointer is retained but as soon of VideoDecoder::queue_frame exit
if you didn't call SpiceFrame::ref_data all the above mentioned fields
became invalid (dangling pointers!).
That's the reason why SpiceFrame::free is just a g_free and not free
the data message, because SpiceFrame does not own the data (but have
a pointer to it!).
I think would be a good way to have SpiceFrame::free to unreference
data and obviously to own it (in spice_frame_new).

> ---
>  src/channel-display-gst.c   |  8 +++-
>  src/channel-display-mjpeg.c | 21 -
>  src/channel-display-priv.h  |  1 +
>  src/channel-display.c   | 11 +++
>  4 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 8b23036..d2847ec 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -91,10 +91,8 @@ static SpiceGstFrame *create_gst_frame(GstBuffer *buffer,
> SpiceFrame *frame)
>  
>  static void free_gst_frame(SpiceGstFrame *gstframe)
>  {
> -gstframe->frame->free(gstframe->frame);
> -if (gstframe->sample) {
> -gst_sample_unref(gstframe->sample);
> -}
> +g_clear_pointer(>frame, spice_frame_free);

Note that previously you didn't unreference the data.

> +g_clear_pointer(>sample, gst_sample_unref);
>  g_free(gstframe);
>  }
>  
> @@ -553,7 +551,7 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>  frame->ref_data(frame->data_opaque);
>  GstBuffer *buffer =
>  gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
>  frame->data,
>  frame->size, 0,
>  frame->size,
> -frame->data_opaque,
> frame->unref_data);
> +frame->data_opaque,
> NULL);

Bug: you are retaining a pointer without ownership, looking for trouble!

>  
>  GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>  GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index f0d55f6..66a6eb8 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -75,15 +75,6 @@ static void mjpeg_src_term(struct jpeg_decompress_struct
> *cinfo)
>  }
>  
>  
> -/* -- A SpiceFrame helper -- */
> -
> -static void free_spice_frame(SpiceFrame *frame)
> -{
> -frame->unref_data(frame->data_opaque);
> -frame->free(frame);
> -}
> -

This was correct as the code added the ownership calling ref_data, in previous
file this was done differently.

> -
>  /* -- Decoder proper -- */
>  
>  static void mjpeg_decoder_schedule(MJpegDecoder *decoder);
> @@ -177,8 +168,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
>  /* Display the frame and dispose of it */
>  stream_display_frame(decoder->base.stream, decoder->cur_frame,
>   width, height, SPICE_UNKNOWN_STRIDE,
>   decoder->out_frame);
> -free_spice_frame(decoder->cur_frame);
> -decoder->cur_frame = NULL;
> +g_clear_pointer(>cur_frame, spice_frame_free);
>  decoder->timer_id = 0;
>  
>  /* Schedule the next frame */
> @@ -212,7 +202,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>  __FUNCTION__, time - frame->mm_time,
>  frame->mm_time, time);
>  stream_dropped_frame_on_playback(decoder->base.stream);
> -free_spice_frame(frame);
> +spice_frame_free(frame);
>  }
>  frame = g_queue_pop_head(decoder->msgq);
>  } while 

Re: [Spice-devel] [spice-gtk v1 04/11] channel-display: use GHashTable to keep stream's structure

[Victor Toso]

Hi,

On Tue, Apr 17, 2018 at 06:54:27AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Tue, Apr 10, 2018 at 07:46:46PM +0300, Uri Lublin wrote:
> > > On 04/10/2018 02:58 PM, Victor Toso wrote:
> > > > On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
> > > > > Some thoughts:
> > > > > 1. It make sense to make it 64 on the server side.
> > > > 
> > > > Why? Just so we can alloc a power of two?
> > > 
> > > Yes, there are 14 entries that are allocated but not used.
> > > It probably does not matter much, as mostly 50 is not reached.
> > 
> > Indeed, I don't think we use that much.
> > 
> > If we would limit that in the protocol and state that ids are
> > always between 0-63 for instance (so we can drop ids < 0 and >
> > 63), I'd be happy to stay with an array
> > 
> 
> I would rather goes on this patch, maybe a 256 limit to start, just
> to be sure.
> Allowing 32 bit for ID potentially requires more than 1TB of RAM,
> looks like not a good idea to allow all the range, there's no point,
> the current server limit is 50 and I never manage to get further than
> 3/4 using the "all" option.
> 
> OT: Maybe would be great to add a @max specification in the protocol
> file to allow to limit some constants (very OT).

My point with hash table is basically due the fact that we don't
have that in the protocol...

I'll address some non-existing-in-the-protocol limitation while
keeping the array (instead of the hashtable), I hope to send that
later this week...

Cheers,

>  
> > > > > Is it safer to keep clear_streams similar to clear_surfaces ?
> > > > > There is a difference, clear_surfaces also emits a signal.
> > > > 
> > > > The main use for clear_streams() is to clear the streams, which
> > > > does that by calling g_hash_table_remove_all(). In the finalize
> > > > method we are destroying the object so I think g_clear_pointer()
> > > > with unref for the GHashTable works fine.
> > > > 
> > > > Maybe I did not understand what you mean with clear_surfaces. I
> > > > did not touch that part but as you said clear_surfaces() is more
> > > > elaborated then clear_streams(). Still, I'd mind to change the
> > > > g_hash_table_unref(c->surfaces) to be a g_clear_pointer(..) too.
> > > 
> > > What I meant is that the hash table entries are being released only
> > > once anyway, so the overhead is not that big.
> > > 
> > > I read in g_hash_table_new_full() documentation that sometimes
> > > it's better to call g_hash_table_remove_all before _unref.
> > > However, I think in this case it's not required.
> > > 
> > > Also I saw that for surfaces too, the entries are freed
> > > before the g_hash_table_unref.
> > > 
> > > You can leave it as is.
> > 
> > Ok
> > 
> > > 
> > > > 
> > > > > >g_clear_pointer(>palettes, cache_free);
> > > > > >if
> > > > > >
> > > > > > (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> > > > > > @@ -863,6 +862,7 @@ static void
> > > > > > spice_display_channel_init(SpiceDisplayChannel *channel)
> > > > > >c = channel->priv =
> > > > > >SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
> > > > > >c->surfaces = g_hash_table_new_full(NULL, NULL, NULL,
> > > > > >destroy_surface);
> > > > > > +c->streams = g_hash_table_new_full(g_direct_hash,
> > > > > > g_direct_equal, NULL, display_stream_destroy);
> > > > > 
> > > > > - Stream id is an integer, why not keep integer keys
> > > > >(g_int_hash, g_int_equal)?
> > > > >Probably it does not matter much.
> > > > 
> > > > It is a unsigned integer. Yes, I was using it in v1 but it wasn't
> > > > working properly while with g_direct_hash() I never had a
> > > > problem. Probably my fault.
> > > > 
> > > > > - Why make it different than the line above (using NULL instead of
> > > > >specifying the default functions)
> > > > 
> > > > Because the there is not allocation to the hash table's key
> > > > (pointer to uint);
> > > 
> > > Either I do not understand you or vice versa (or both).
> > 
> > Now that I re-read what I said, yeah, not sure what I meant
> > either. Sorry :)
> > 
> > > I meant, why not
> > >  c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
> > > +c->streams  = g_hash_table_new_full(NULL, NULL, NULL,
> > > display_stream_destroy);
> > > 
> > > It does not really matter, just more consistent with current code.
> > 
> > NULL doesn't say much. With NULL one needs to check the
> > documentation and see what's going on (this is the first time I
> > saw using NULL for instance).
> > 
> > I'd change the surface's hash table new to use g_direct_* instead
> > but I don't mind to change my patch to NULL instead if are common
> > practice.
> > 
> > Best,
> > toso
> > > 
> > > Uri.
> > > 
> > > > Thanks for the review, let me know if there anything you want me
> > > > to change ;) >
> > > > Cheers,
> > > >  toso
> > > > > 
> > > > > >c->image_cache.ops = _cache_ops;
> > > > > >c->palette_cache.ops = 

Re: [Spice-devel] [PATCH spice-server v2] ci: Workaround bug in Valgrind detecting memcpy instead of memmove

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Tue, Apr 17, 2018 at 11:19:16AM +0100, Frediano Ziglio wrote:
> Due to a bug in current packaged Valgrind in the CI (1:3.13.0-13.fc27)
> check-valgrind is failing with:
> 
> ==17986== Source and destination overlap in memcpy_chk(0x72c060, 0x72c068, 33)
> ==17986==at 0x4C344F0: __memcpy_chk (vg_replace_strmem.c:1581)
> ==17986==by 0x40E7E9: check_vmc_error_message (test-stream-device.c:166)
> ==17986==by 0x40EFD4: test_stream_device_format_after_data 
> (test-stream-device.c:349)
> ==17986==by 0x7A012E9: test_case_run (gtestutils.c:2157)
> ==17986==by 0x7A012E9: g_test_run_suite_internal (gtestutils.c:2241)
> ==17986==by 0x7A0121A: g_test_run_suite_internal (gtestutils.c:2253)
> ==17986==by 0x7A014C1: g_test_run_suite (gtestutils.c:2329)
> ==17986==by 0x7A014E0: g_test_run (gtestutils.c:1594)
> ==17986==by 0x40951A: main (test-stream-device.c:410)
> ==17986==
> 
> By default during CI build _FORTIFY_SOURCE is enabled, which turns memmove
> into __memmove_chk, which is wrongly turned into __memcpy_chk when running
> under Valgrind.
> Setting _FORTIFY_SOURCE to 0 prevents the use of __memmove_chk, and avoids
> triggering the Valgrind bug.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> This should be temporary till Valgrind is fixed in Fedora
> 
> Changes since v1:
> - update commit message
> ---
>  .gitlab-ci.yml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 535d220d..655232c2 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -34,7 +34,9 @@ check-valgrind:
>  dnf install valgrind
>  gstreamer1-libav gstreamer1-plugins-ugly gstreamer1-plugins-good 
> gstreamer1-plugins-bad-free
>  -y
> -  - ./autogen.sh --enable-valgrind --enable-extra-checks
> +  - >
> +CFLAGS='-O2 -pipe -g -D_FORTIFY_SOURCE=0'
> +./autogen.sh --enable-valgrind --enable-extra-checks
>- make
>- make check-valgrind || (cat server/tests/test-suite-memcheck.log && exit 
> 1)
>  
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk v1 04/11] channel-display: use GHashTable to keep stream's structure

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Apr 10, 2018 at 07:46:46PM +0300, Uri Lublin wrote:
> > On 04/10/2018 02:58 PM, Victor Toso wrote:
> > > On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
> > > > Some thoughts:
> > > > 1. It make sense to make it 64 on the server side.
> > > 
> > > Why? Just so we can alloc a power of two?
> > 
> > Yes, there are 14 entries that are allocated but not used.
> > It probably does not matter much, as mostly 50 is not reached.
> 
> Indeed, I don't think we use that much.
> 
> If we would limit that in the protocol and state that ids are
> always between 0-63 for instance (so we can drop ids < 0 and >
> 63), I'd be happy to stay with an array
> 

I would rather goes on this patch, maybe a 256 limit to start, just
to be sure.
Allowing 32 bit for ID potentially requires more than 1TB of RAM,
looks like not a good idea to allow all the range, there's no point,
the current server limit is 50 and I never manage to get further than
3/4 using the "all" option.

OT: Maybe would be great to add a @max specification in the protocol
file to allow to limit some constants (very OT).
 
> > > > Is it safer to keep clear_streams similar to clear_surfaces ?
> > > > There is a difference, clear_surfaces also emits a signal.
> > > 
> > > The main use for clear_streams() is to clear the streams, which
> > > does that by calling g_hash_table_remove_all(). In the finalize
> > > method we are destroying the object so I think g_clear_pointer()
> > > with unref for the GHashTable works fine.
> > > 
> > > Maybe I did not understand what you mean with clear_surfaces. I
> > > did not touch that part but as you said clear_surfaces() is more
> > > elaborated then clear_streams(). Still, I'd mind to change the
> > > g_hash_table_unref(c->surfaces) to be a g_clear_pointer(..) too.
> > 
> > What I meant is that the hash table entries are being released only
> > once anyway, so the overhead is not that big.
> > 
> > I read in g_hash_table_new_full() documentation that sometimes
> > it's better to call g_hash_table_remove_all before _unref.
> > However, I think in this case it's not required.
> > 
> > Also I saw that for surfaces too, the entries are freed
> > before the g_hash_table_unref.
> > 
> > You can leave it as is.
> 
> Ok
> 
> > 
> > > 
> > > > >g_clear_pointer(>palettes, cache_free);
> > > > >if
> > > > >(G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
> > > > > @@ -863,6 +862,7 @@ static void
> > > > > spice_display_channel_init(SpiceDisplayChannel *channel)
> > > > >c = channel->priv =
> > > > >SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
> > > > >c->surfaces = g_hash_table_new_full(NULL, NULL, NULL,
> > > > >destroy_surface);
> > > > > +c->streams = g_hash_table_new_full(g_direct_hash,
> > > > > g_direct_equal, NULL, display_stream_destroy);
> > > > 
> > > > - Stream id is an integer, why not keep integer keys
> > > >(g_int_hash, g_int_equal)?
> > > >Probably it does not matter much.
> > > 
> > > It is a unsigned integer. Yes, I was using it in v1 but it wasn't
> > > working properly while with g_direct_hash() I never had a
> > > problem. Probably my fault.
> > > 
> > > > - Why make it different than the line above (using NULL instead of
> > > >specifying the default functions)
> > > 
> > > Because the there is not allocation to the hash table's key
> > > (pointer to uint);
> > 
> > Either I do not understand you or vice versa (or both).
> 
> Now that I re-read what I said, yeah, not sure what I meant
> either. Sorry :)
> 
> > I meant, why not
> >  c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
> > +c->streams  = g_hash_table_new_full(NULL, NULL, NULL,
> > display_stream_destroy);
> > 
> > It does not really matter, just more consistent with current code.
> 
> NULL doesn't say much. With NULL one needs to check the
> documentation and see what's going on (this is the first time I
> saw using NULL for instance).
> 
> I'd change the surface's hash table new to use g_direct_* instead
> but I don't mind to change my patch to NULL instead if are common
> practice.
> 
> Best,
> toso
> > 
> > Uri.
> > 
> > > Thanks for the review, let me know if there anything you want me
> > > to change ;) >
> > > Cheers,
> > >  toso
> > > > 
> > > > >c->image_cache.ops = _cache_ops;
> > > > >c->palette_cache.ops = _cache_ops;
> > > > >c->image_surfaces.ops = _surfaces_ops;
> > > > > @@ -1207,15 +1207,18 @@ static void
> > > > > report_invalid_stream(SpiceChannel *channel, uint32_t id)
> > > > >static display_stream *get_stream_by_id(SpiceChannel *channel,
> > > > >uint32_t id)
> > > > >{
> > > > > +display_stream *st;
> > > > >SpiceDisplayChannelPrivate *c =
> > > > >SPICE_DISPLAY_CHANNEL(channel)->priv;
> > > > > -if (c != NULL && c->streams != NULL && id < c->nstreams &&
> > > > > -c->streams[id] != NULL) {
> > > > > -

Re: [Spice-devel] [RFC spice-gtk v2 0/1] Direct rendering

[Snir Sheriber]

Hi,


On 04/16/2018 12:37 PM, Frediano Ziglio wrote:

Differences from v1
-Recognize streaming mode by the streaming-mode surface flag

The propagation of this property (as English word, not gobject) is weird.
Seems that is a SpiceDisplay property but in reality is a surface propery
(which is stored in display_surface structure in spice-gtk), the previous
patch version stored the property on display_surface which seems more
correct.


Storing it in display_surface was suggested in your patch,
i agree it's more suitable as surface property but then it
wouldn't be accessible from the widget\SpiceDisplay, that
needs to know if streaming mode is enabled in order to
prepare the area and extract the handle from the window.
(maybe will be more suitable under SpiceDisplay.canvas?)



Storing the "handle" (currently xid) in channel seems more of an hack
than the proper way, a channel in theory can have multiple primary
surfaces (which was a request to implement). Also the handle keep
to be set even if the streaming surface is reset (this can happen

True, and needs to be checked.

AFAIU the idea was to separate the actual window (widget) from the
display-channel related to it, so that the only interface connected
between them is a canvas (pre-defined size buffer) and a
few gobject pre-defined status signals.
The thing is that decoding is currently done on the channel-side and
displaying/rendering is done on the widget side, while using the
gstvideooverlay can let gstreamer handle both decoding and
displaying/rendering and "breaks" the client's architecture (this was
mentioned before)

That's why it's kind of hackish now, I have several other ideas, all
of them requires pretty massive changes, not sure what is the proper
way, any thoughts are welcome.


for Virgl, although not implemented currently by the code). Similar,
what happen if the "Display" (in client terminology, the client window)
is closed? Does the xid is still valid? Is there a way to reduce the

yes, no xid in this case, but when could it happen?


CPU/GPU usage in this case (not strict requirement, mainly OT).


-Modifying the streaming mode signal

Feels more confusing that previous version.
Looking at current code the code (master) calls stream_display_frame
which emit a "display-invalidate" signal, captured by SpiceDisplay
(OT: why SpiceDisplay is implemented in spice-widget.c and not
in a spice-display.c ??). I imagine all SpiceDisplay(s) attached
to the same channel will handle the update filtering when the
rectangle does not intersect.
Maybe a signal like that emitted from channel-display-gst.c to
get the xid handle would make more sense and would support multiple
monitors?

True, it's indeed make more sense..
Although if we are necessarily going to stream each display separately
it should be equal i guess.




-Applying patches from Frediano (sent on v1 thread)
-Applying Uri's patch fixing a memory leak
-This feature can forced to be disabled now by setting the
  DISABLE_GSTVIDEOOVERLAY environment variable

Small note: I would avoid to call g_getenv every time but
cache the value.


Can be done, although It's only when realize so it shouldn't
be called too many times.




-Does not create a new drawing area

Some issues
- Canvas is allocated although it's not always used

I won't consider this an issue, is not even a regression and
not strictly related to direct rendering.


- Needs to be tested with different plugins, environments...

I tested with Intel card with and without vaapi drivers and
with Xorg and Wayland.
Maybe would be useful to get a matrix?


- Not sure what is needed in order to make it to support
   multi-monitor in the future.
- Currently works only with x (xid is transferred from
   spice-widget to spice-gst-decoder which sets the overlay)
- There is no synchronization with audio! (decodes and
renders AFAP)


On my tests the PTS is taken into account so seems to
sync audio and video... weird!


Shouldn't be, it's in use only when appsink is in use, try to suspend
the client for a few seconds to get it out of sync




I'd be happy to hear more comments, ideas, patches :)


The results with this patch are great! With drivers installed
I'm getting 10% CPU usage using full HD!

Hope to see this patch in master soon!

\o/

Thanks for your patches, testing and reviewing




Thanks, Snir.


Snir Sheriber (1):
   Gstreamer: Use GstVideoOverlay if possible

  src/channel-display-gst.c | 99
  ++-
  src/channel-display.c | 55 ++
  src/channel-display.h |  3 ++
  src/spice-widget-priv.h   |  1 +
  src/spice-widget.c| 40 ++-
  5 files changed, 179 insertions(+), 19 deletions(-)


Frediano


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

[Spice-devel] [PATCH spice-server v2] ci: Workaround bug in Valgrind detecting memcpy instead of memmove

[Frediano Ziglio]

Due to a bug in current packaged Valgrind in the CI (1:3.13.0-13.fc27)
check-valgrind is failing with:

==17986== Source and destination overlap in memcpy_chk(0x72c060, 0x72c068, 33)
==17986==at 0x4C344F0: __memcpy_chk (vg_replace_strmem.c:1581)
==17986==by 0x40E7E9: check_vmc_error_message (test-stream-device.c:166)
==17986==by 0x40EFD4: test_stream_device_format_after_data 
(test-stream-device.c:349)
==17986==by 0x7A012E9: test_case_run (gtestutils.c:2157)
==17986==by 0x7A012E9: g_test_run_suite_internal (gtestutils.c:2241)
==17986==by 0x7A0121A: g_test_run_suite_internal (gtestutils.c:2253)
==17986==by 0x7A014C1: g_test_run_suite (gtestutils.c:2329)
==17986==by 0x7A014E0: g_test_run (gtestutils.c:1594)
==17986==by 0x40951A: main (test-stream-device.c:410)
==17986==

By default during CI build _FORTIFY_SOURCE is enabled, which turns memmove
into __memmove_chk, which is wrongly turned into __memcpy_chk when running
under Valgrind.
Setting _FORTIFY_SOURCE to 0 prevents the use of __memmove_chk, and avoids
triggering the Valgrind bug.

Signed-off-by: Frediano Ziglio 
---
This should be temporary till Valgrind is fixed in Fedora

Changes since v1:
- update commit message
---
 .gitlab-ci.yml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 535d220d..655232c2 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -34,7 +34,9 @@ check-valgrind:
 dnf install valgrind
 gstreamer1-libav gstreamer1-plugins-ugly gstreamer1-plugins-good 
gstreamer1-plugins-bad-free
 -y
-  - ./autogen.sh --enable-valgrind --enable-extra-checks
+  - >
+CFLAGS='-O2 -pipe -g -D_FORTIFY_SOURCE=0'
+./autogen.sh --enable-valgrind --enable-extra-checks
   - make
   - make check-valgrind || (cat server/tests/test-suite-memcheck.log && exit 1)
 
-- 
2.14.3

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

Re: [Spice-devel] [spice-server 05/10] qxl: Rename 'qxl' to 'qxl_cmd' in red_get_cursor_cmd()

[Lukáš Hrázký]

On Tue, 2018-04-17 at 10:49 +0200, Christophe Fergeau wrote:
> On Mon, Apr 16, 2018 at 08:38:00AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Mon, Apr 16, 2018 at 06:58:18AM -0400, Frediano Ziglio wrote:
> > > > Don't like this, lot of structures in this file use "qxl", for coherence
> > > > I would change all or nothing but changing all would mean a lot of 
> > > > changes
> > > > with not much value
> > > 
> > > Imo 'red' and 'qxl' are not very good names, I'd prefer to have them
> > > named red_cmd and qxl_cmd. I agree it's quite a few changes though. I
> > > can send an initial patch changing all of these if you prefer.
> > > 
> > > Christophe
> > > 
> > 
> > I would personally keep red and qxl and add a qxl_instance.
> 
> 
> Fwiw, here is what renaming everything would look like.
> Quite some churn indeed :(

Fwiw, skimming through, the naming would be much better :) I'd try to
get it in despite the churn, not sure how many people have patches
against the code in question...

Cheers,
Lukas

> From 22a4eb242a7b5da0c37a5ebf9aaf8156be423785 Mon Sep 17 00:00:00 2001
> From: Christophe Fergeau 
> Date: Tue, 17 Apr 2018 10:36:09 +0200
> Subject: [spice-server] improved arg names in red-parse-qxl
> 
> ---
>  server/red-parse-qxl.c | 906 
> +
>  1 file changed, 455 insertions(+), 451 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 7a6805e76..18dcbb2f6 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -115,7 +115,7 @@ static uint8_t *red_linearize_chunk(RedDataChunk *head, 
> size_t size, bool *free_
>  
>  static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
>int memslot_id,
> -  RedDataChunk *red, QXLDataChunk *qxl)
> +  RedDataChunk *red_data_chunk, 
> QXLDataChunk *qxl_data_chunk)
>  {
>  RedDataChunk *red_prev;
>  uint64_t data_size = 0;
> @@ -124,16 +124,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
> *slots, int group_id,
>  QXLPHYSICAL next_chunk;
>  unsigned num_chunks = 0;
>  
> -red->data_size = qxl->data_size;
> -data_size += red->data_size;
> -red->data = qxl->data;
> -red->prev_chunk = red->next_chunk = NULL;
> -if (!memslot_validate_virt(slots, (intptr_t)red->data, memslot_id, 
> red->data_size, group_id)) {
> -red->data = NULL;
> +red_data_chunk->data_size = qxl_data_chunk->data_size;
> +data_size += red_data_chunk->data_size;
> +red_data_chunk->data = qxl_data_chunk->data;
> +red_data_chunk->prev_chunk = red_data_chunk->next_chunk = NULL;
> +if (!memslot_validate_virt(slots, (intptr_t)red_data_chunk->data, 
> memslot_id, red_data_chunk->data_size, group_id)) {
> +red_data_chunk->data = NULL;
>  return INVALID_SIZE;
>  }
>  
> -while ((next_chunk = qxl->next_chunk) != 0) {
> +while ((next_chunk = qxl_data_chunk->next_chunk) != 0) {
>  /* somebody is trying to use too much memory using a lot of chunks.
>   * Or made a circular list of chunks
>   */
> @@ -143,7 +143,7 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
> *slots, int group_id,
>  }
>  
>  memslot_id = memslot_get_id(slots, next_chunk);
> -qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, 
> sizeof(*qxl),
> +qxl_data_chunk = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, 
> sizeof(*qxl_data_chunk),
> group_id, );
>  if (error)
>  goto error;
> @@ -154,16 +154,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
> *slots, int group_id,
>   * All above cases are handled by the check for number
>   * of chunks.
>   */
> -chunk_data_size = qxl->data_size;
> +chunk_data_size = qxl_data_chunk->data_size;
>  if (chunk_data_size == 0)
>  continue;
>  
> -red_prev = red;
> -red = g_new0(RedDataChunk, 1);
> -red->data_size = chunk_data_size;
> -red->prev_chunk = red_prev;
> -red->data = qxl->data;
> -red_prev->next_chunk = red;
> +red_prev = red_data_chunk;
> +red_data_chunk = g_new0(RedDataChunk, 1);
> +red_data_chunk->data_size = chunk_data_size;
> +red_data_chunk->prev_chunk = red_prev;
> +red_data_chunk->data = qxl_data_chunk->data;
> +red_prev->next_chunk = red_data_chunk;
>  
>  data_size += chunk_data_size;
>  /* this can happen if client is sending nested chunks */
> @@ -171,69 +171,69 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
> *slots, int group_id,
>  spice_warning("too much data inside chunks, avoiding DoS\n");
>  goto error;
>  }
> -if (!memslot_validate_virt(slots, (intptr_t)red->data, 

Re: [Spice-devel] [spice-server 05/10] qxl: Rename 'qxl' to 'qxl_cmd' in red_get_cursor_cmd()

[Christophe Fergeau]

On Mon, Apr 16, 2018 at 08:38:00AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Apr 16, 2018 at 06:58:18AM -0400, Frediano Ziglio wrote:
> > > Don't like this, lot of structures in this file use "qxl", for coherence
> > > I would change all or nothing but changing all would mean a lot of changes
> > > with not much value
> > 
> > Imo 'red' and 'qxl' are not very good names, I'd prefer to have them
> > named red_cmd and qxl_cmd. I agree it's quite a few changes though. I
> > can send an initial patch changing all of these if you prefer.
> > 
> > Christophe
> > 
> 
> I would personally keep red and qxl and add a qxl_instance.


Fwiw, here is what renaming everything would look like.
Quite some churn indeed :(

From 22a4eb242a7b5da0c37a5ebf9aaf8156be423785 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau 
Date: Tue, 17 Apr 2018 10:36:09 +0200
Subject: [spice-server] improved arg names in red-parse-qxl

---
 server/red-parse-qxl.c | 906 +
 1 file changed, 455 insertions(+), 451 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 7a6805e76..18dcbb2f6 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -115,7 +115,7 @@ static uint8_t *red_linearize_chunk(RedDataChunk *head, 
size_t size, bool *free_
 
 static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
   int memslot_id,
-  RedDataChunk *red, QXLDataChunk *qxl)
+  RedDataChunk *red_data_chunk, 
QXLDataChunk *qxl_data_chunk)
 {
 RedDataChunk *red_prev;
 uint64_t data_size = 0;
@@ -124,16 +124,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 QXLPHYSICAL next_chunk;
 unsigned num_chunks = 0;
 
-red->data_size = qxl->data_size;
-data_size += red->data_size;
-red->data = qxl->data;
-red->prev_chunk = red->next_chunk = NULL;
-if (!memslot_validate_virt(slots, (intptr_t)red->data, memslot_id, 
red->data_size, group_id)) {
-red->data = NULL;
+red_data_chunk->data_size = qxl_data_chunk->data_size;
+data_size += red_data_chunk->data_size;
+red_data_chunk->data = qxl_data_chunk->data;
+red_data_chunk->prev_chunk = red_data_chunk->next_chunk = NULL;
+if (!memslot_validate_virt(slots, (intptr_t)red_data_chunk->data, 
memslot_id, red_data_chunk->data_size, group_id)) {
+red_data_chunk->data = NULL;
 return INVALID_SIZE;
 }
 
-while ((next_chunk = qxl->next_chunk) != 0) {
+while ((next_chunk = qxl_data_chunk->next_chunk) != 0) {
 /* somebody is trying to use too much memory using a lot of chunks.
  * Or made a circular list of chunks
  */
@@ -143,7 +143,7 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 }
 
 memslot_id = memslot_get_id(slots, next_chunk);
-qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, sizeof(*qxl),
+qxl_data_chunk = (QXLDataChunk *)memslot_get_virt(slots, next_chunk, 
sizeof(*qxl_data_chunk),
group_id, );
 if (error)
 goto error;
@@ -154,16 +154,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
  * All above cases are handled by the check for number
  * of chunks.
  */
-chunk_data_size = qxl->data_size;
+chunk_data_size = qxl_data_chunk->data_size;
 if (chunk_data_size == 0)
 continue;
 
-red_prev = red;
-red = g_new0(RedDataChunk, 1);
-red->data_size = chunk_data_size;
-red->prev_chunk = red_prev;
-red->data = qxl->data;
-red_prev->next_chunk = red;
+red_prev = red_data_chunk;
+red_data_chunk = g_new0(RedDataChunk, 1);
+red_data_chunk->data_size = chunk_data_size;
+red_data_chunk->prev_chunk = red_prev;
+red_data_chunk->data = qxl_data_chunk->data;
+red_prev->next_chunk = red_data_chunk;
 
 data_size += chunk_data_size;
 /* this can happen if client is sending nested chunks */
@@ -171,69 +171,69 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo 
*slots, int group_id,
 spice_warning("too much data inside chunks, avoiding DoS\n");
 goto error;
 }
-if (!memslot_validate_virt(slots, (intptr_t)red->data, memslot_id, 
red->data_size, group_id))
+if (!memslot_validate_virt(slots, (intptr_t)red_data_chunk->data, 
memslot_id, red_data_chunk->data_size, group_id))
 goto error;
 }
 
-red->next_chunk = NULL;
+red_data_chunk->next_chunk = NULL;
 return data_size;
 
 error:
-while (red->prev_chunk) {
-red_prev = red->prev_chunk;
-g_free(red);
-red = red_prev;
+while (red_data_chunk->prev_chunk) {
+red_prev = 

Re: [Spice-devel] [PATCH] build: Remove check for pnp.ids path

[Victor Toso]

Hi,

On Mon, Apr 16, 2018 at 05:44:14PM -0300, Eduardo Lima (Etrunko) wrote:
> This looks like a leftover from the following commit, which removed the
> only reference for this file.
> 
> commit 30986505ba6041c293c38cb4b7f4b618a59f4716
> Author: Marc-André Lureau 
> Date:   Fri May 10 17:05:49 2013 +0200
> 
> Remove GnomeRR code
> 
> Changing client resolution is a bad idea, and never took up.
> Remove some unmaintained experimental code.
> 
> Signed-off-by: Eduardo Lima (Etrunko) 

Acked-by: Victor Toso 

> ---
>  configure.ac| 15 ---
>  src/Makefile.am |  1 -
>  2 files changed, 16 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 17799c1..a9a7eb9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -164,21 +164,6 @@ CFLAGS="$old_CFLAGS"
>  PKG_CHECK_EXISTS([gtk+-x11-$with_gtk], [PKG_CHECK_MODULES(X11, x11)])
>  AC_CHECK_HEADERS([X11/XKBlib.h])
>  
> -AC_ARG_WITH([pnp-ids-path],
> -  AC_HELP_STRING([--with-pnp-ids-path],
> - [Specify the path to pnp.ids @<:@default=(internal)@:>@]),
> -  [],
> -  [with_pnp_ids_path="\${pnpdatadir}/pnp.ids"])
> -
> -AM_CONDITIONAL(USE_INTERNAL_PNP_IDS, test "x$with_pnp_ids_path" = 
> "x\${pnpdatadir}/pnp.ids")
> -PNP_IDS=$with_pnp_ids_path
> -AC_SUBST(PNP_IDS)
> -if test "x$with_pnp_ids_path" = "x\${pnpdatadir}/pnp.ids"; then
> -EXTERNAL_PNP_IDS="no (internal)"
> -else
> -EXTERNAL_PNP_IDS="$with_pnp_ids_path"
> -fi
> -
>  AC_CHECK_FUNCS(clearenv strtok_r)
>  
>  # Keep these two definitions in agreement.
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1701cc6..f49c766 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -67,7 +67,6 @@ KEYMAP_CSV = keycodemapdb/data/keymaps.csv
>  SPICE_COMMON_CPPFLAGS =  \
>   -DSPICE_COMPILATION \
>   -DG_LOG_DOMAIN=\"GSpice\"   \
> - -DPNP_IDS=\""$(PNP_IDS)"\"  \
>   -DUSB_IDS=\""$(USB_IDS)"\"  \
>   -I$(top_srcdir) \
>   $(COMMON_CFLAGS)\
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [spice-gtk v1] usb-device-widget: remove goto/label

[Victor Toso]

From: Victor Toso 

The 'end' label is used only once and can be replaced by moving the
code into the existing 'if (!devices)'.

For convenience this patch also:
* Explicit check against NULL
* Added curly brackets to the moved 'for'
* Moved variable 'i' to inner scope

Signed-off-by: Victor Toso 
---
 src/usb-device-widget.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
index a3c0910..6dd3617 100644
--- a/src/usb-device-widget.c
+++ b/src/usb-device-widget.c
@@ -187,7 +187,6 @@ static void spice_usb_device_widget_constructed(GObject 
*gobject)
 GPtrArray *devices = NULL;
 GError *err = NULL;
 gchar *str;
-int i;
 
 self = SPICE_USB_DEVICE_WIDGET(gobject);
 priv = self->priv;
@@ -218,15 +217,15 @@ static void spice_usb_device_widget_constructed(GObject 
*gobject)
  G_CALLBACK(device_error_cb), self);
 
 devices = spice_usb_device_manager_get_devices(priv->manager);
-if (!devices)
-goto end;
-
-for (i = 0; i < devices->len; i++)
-device_added_cb(NULL, g_ptr_array_index(devices, i), self);
+if (devices != NULL) {
+int i;
+for (i = 0; i < devices->len; i++) {
+device_added_cb(NULL, g_ptr_array_index(devices, i), self);
+}
 
-g_ptr_array_unref(devices);
+g_ptr_array_unref(devices);
+}
 
-end:
 spice_usb_device_widget_update_status(self);
 }
 
-- 
2.16.2

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

Re: [Spice-devel] [PATCH spice-server] ci: Workaround bug in Valgrind detecting memcpy instead of memmove

[Christophe Fergeau]

On Mon, Apr 16, 2018 at 02:07:57PM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Apr 16, 2018 at 05:02:33PM +0100, Frediano Ziglio wrote:
> > > Due to a bug in current packaged Valgrind check-valgrind is failing
> > > with:
> > > 
> > > ==17986== Source and destination overlap in memcpy_chk(0x72c060, 0x72c068,
> > > 33)
> > > ==17986==at 0x4C344F0: __memcpy_chk (vg_replace_strmem.c:1581)
> > > ==17986==by 0x40E7E9: check_vmc_error_message
> > > (test-stream-device.c:166)
> > > ==17986==by 0x40EFD4: test_stream_device_format_after_data
> > > (test-stream-device.c:349)
> > > ==17986==by 0x7A012E9: test_case_run (gtestutils.c:2157)
> > > ==17986==by 0x7A012E9: g_test_run_suite_internal (gtestutils.c:2241)
> > > ==17986==by 0x7A0121A: g_test_run_suite_internal (gtestutils.c:2253)
> > > ==17986==by 0x7A014C1: g_test_run_suite (gtestutils.c:2329)
> > > ==17986==by 0x7A014E0: g_test_run (gtestutils.c:1594)
> > > ==17986==by 0x40951A: main (test-stream-device.c:410)
> > > ==17986==
> > > 
> > > Note that source code calls memmove instead of memcpy (memmove
> > > supports overlapping).
> > > Disable call to __memmove_chk to avoid the issue.
> > 
> > __memcpy_chk? Not clear how this is achieved from the log. By disabling
> > _FORTIFY_SOURCE I assume?
> > 
> > Christophe
> > 
> 
> The bug is this:
> 

I'd add in the log that in the CI, we build with _FORTIFY_SOURCE by
default, which turns memmove into __memmove_chk, which is wrongly
turned into __memcpy_chk when running under valgrind, and that setting
FORTIFY_SOURCE to 0 prevents the use of __memmove_chk, and avoids
triggering the valgrind bug.
But hopefully the fedora fix will come soon? Can you add the exact name
of the buggy valgrind package so that it's easier later to check whether
this was fixed or not?

Christophe


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