Re: [Spice-devel] [PATCH 3.6/12] common_channel_client_create -> common_channel_new_client

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Rename and re-order the initial arguments to make this function look and
> act more like a method of the CommonChannel class.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 18 +-
>  server/red_worker.c | 24 
>  server/red_worker.h | 22 +++---
>  3 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index ad0998e..24b7362 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -393,15 +393,15 @@ CursorChannelClient
> *cursor_channel_client_new(CommonChannel *common,
> uint32_t *caps, int num_caps)
>  {
>  CursorChannelClient *ccc =
> -(CursorChannelClient*)common_channel_client_create(
> -sizeof(CursorChannelClient), common, client, stream,
> -mig_target,
> -FALSE,
> -common_caps,
> -num_common_caps,
> -caps,
> -num_caps);
> -
> +(CursorChannelClient*)common_channel_new_client(common,
> +
> sizeof(CursorChannelClient),
> +client, stream,
> +mig_target,
> +FALSE,
> +common_caps,
> +num_common_caps,
> +caps,
> +num_caps);
>  if (!ccc) {
>  return NULL;
>  }
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 0631efb..0c8ba4c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9506,16 +9506,16 @@ SpiceCoreInterface worker_core = {
>  .watch_remove = worker_watch_remove,
>  };
>  
> -CommonChannelClient *common_channel_client_create(int size,
> -  CommonChannel *common,
> -  RedClient *client,
> -  RedsStream *stream,
> -  int mig_target,
> -  int monitor_latency,
> -  uint32_t *common_caps,
> -  int num_common_caps,
> -  uint32_t *caps,
> -  int num_caps)
> +CommonChannelClient *common_channel_new_client(CommonChannel *common,
> +   int size,
> +   RedClient *client,
> +   RedsStream *stream,
> +   int mig_target,
> +   int monitor_latency,
> +   uint32_t *common_caps,
> +   int num_common_caps,
> +   uint32_t *caps,
> +   int num_caps)
>  {
>  RedChannelClient *rcc =
>  red_channel_client_create(size, >base, client, stream,
>  monitor_latency,
> @@ -9543,8 +9543,8 @@ DisplayChannelClient
> *display_channel_client_create(CommonChannel *common,
>  uint32_t *caps, int
>  num_caps)
>  {
>  DisplayChannelClient *dcc =
> -(DisplayChannelClient*)common_channel_client_create(
> -sizeof(DisplayChannelClient), common, client, stream,
> +(DisplayChannelClient*)common_channel_new_client(
> +common, sizeof(DisplayChannelClient), client, stream,
>  mig_target,
>  TRUE,
>  common_caps, num_common_caps,
> diff --git a/server/red_worker.h b/server/red_worker.h
> index c828d99..795959d 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -118,17 +118,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher);
>  bool   red_worker_run(RedWorker *worker);
>  QXLInstance* red_worker_get_qxl(RedWorker *worker);
>  
> -CommonChannelClient *common_channel_client_create(int size,
> -  CommonChannel *common,
> -  RedClient *client,
> -  RedsStream *stream,
> -  int mig_target,
> -  int monitor_latency,
> - 

Re: [Spice-devel] [PATCH 3.5/12] Change red_marshall_verb() to accept a VerbItem

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Instead of passing a verb enumeration value, pass the verb pipe item
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 4 ++--
>  server/red_worker.c | 4 ++--
>  server/red_worker.h | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 2ea00c8..ad0998e 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -314,7 +314,7 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item)
>  red_cursor_marshall_inval(rcc, m, (CacheItem *)pipe_item);
>  break;
>  case PIPE_ITEM_TYPE_VERB:
> -red_marshall_verb(rcc, ((VerbItem*)pipe_item)->verb);
> +red_marshall_verb(rcc, (VerbItem*)pipe_item);
>  break;
>  case PIPE_ITEM_TYPE_CURSOR_INIT:
>  red_reset_cursor_cache(rcc);
> @@ -322,7 +322,7 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item)
>  break;
>  case PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE:
>  red_reset_cursor_cache(rcc);
> -red_marshall_verb(rcc, SPICE_MSG_CURSOR_INVAL_ALL);
> +red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_INVAL_ALL,
> NULL);
>  break;
>  default:
>  spice_error("invalid pipe item type");
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 7b3eee2..0631efb 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8392,7 +8392,7 @@ static void display_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item
>  red_display_marshall_upgrade(rcc, m, (UpgradeItem *)pipe_item);
>  break;
>  case PIPE_ITEM_TYPE_VERB:
> -red_marshall_verb(rcc, ((VerbItem*)pipe_item)->verb);
> +red_marshall_verb(rcc, (VerbItem*)pipe_item);
>  break;
>  case PIPE_ITEM_TYPE_MIGRATE_DATA:
>  display_channel_marshall_migrate_data(rcc, m);
> @@ -8408,7 +8408,7 @@ static void display_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item
>  break;
>  case PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE:
>  red_reset_palette_cache(dcc);
> -red_marshall_verb(rcc, SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES);
> +red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES, NULL);
>  break;
>  case PIPE_ITEM_TYPE_CREATE_SURFACE: {
>  SurfaceCreateItem *surface_create = SPICE_CONTAINEROF(pipe_item,
>  SurfaceCreateItem,
> diff --git a/server/red_worker.h b/server/red_worker.h
> index 502283e..c828d99 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -74,10 +74,10 @@ typedef struct VerbItem {
>  uint16_t verb;
>  } VerbItem;
>  
> -static inline void red_marshall_verb(RedChannelClient *rcc, uint16_t verb)
> +static inline void red_marshall_verb(RedChannelClient *rcc, VerbItem *item)
>  {
>  spice_assert(rcc);
> -red_channel_client_init_send_data(rcc, verb, NULL);
> +red_channel_client_init_send_data(rcc, item->verb, NULL);
>  }
>  
>  static inline void red_pipe_add_verb(RedChannelClient* rcc, uint16_t verb)
> --
> 2.4.3
> 

Acked

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

Re: [Spice-devel] [PATCH 3.8/12] CursorChannel* arg in cursor_channel_client_new()

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Instead of passing a CommonChannel* argument, use CursorChannel* since
> this function is only valid for CursorChannels.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 5 ++---
>  server/cursor-channel.h | 2 +-
>  server/red_worker.c | 2 +-
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 9c4bf1e..4811b79 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -385,14 +385,13 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
>  return cursor;
>  }
>  
> -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> -   RedClient *client, RedsStream
> *stream,
> +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> RedClient *client, RedsStream *stream,
> int mig_target,
> uint32_t *common_caps, int
> num_common_caps,
> uint32_t *caps, int num_caps)
>  {
>  CursorChannelClient *ccc =
> -(CursorChannelClient*)common_channel_new_client(common,
> +(CursorChannelClient*)common_channel_new_client(>common,
>  
> sizeof(CursorChannelClient),
>  client, stream,
>  mig_target,
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 213f124..9a22c19 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -84,7 +84,7 @@ void cursor_channel_reset
> (CursorChannel *cursor);
>  void cursor_channel_process_cmd (CursorChannel *cursor,
>  RedCursorCmd *cursor_cmd,
>   uint32_t group_id);
>  
> -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> RedClient *client, RedsStream
> *stream,
> int mig_target,
> uint32_t *common_caps, int
> num_common_caps,
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 52fe132..495e452 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9921,7 +9921,7 @@ static void red_connect_cursor(RedWorker *worker,
> RedClient *client, RedsStream
>  }
>  channel = worker->cursor_channel;
>  spice_info("add cursor channel client");
> -ccc = cursor_channel_client_new(>common, client, stream,
> +ccc = cursor_channel_client_new(channel, client, stream,
>  migrate,
>  common_caps, num_common_caps,
>  caps, num_caps);
> --
> 2.4.3
> 

Acked

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

Re: [Spice-devel] [PATCH 3.7/12] __new_channel -> red_worker_new_channel()

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Rename and lightly refactor the function that creates new common
> channels for RedWorker (essentially Cursor and Display channels).
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 25 ++---
>  server/red_worker.c | 59
>  ++---
>  server/red_worker.h | 14 
>  3 files changed, 42 insertions(+), 56 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 24b7362..9c4bf1e 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -364,22 +364,21 @@ static void
> cursor_channel_release_item(RedChannelClient *rcc, PipeItem *item, i
>  
>  CursorChannel* cursor_channel_new(RedWorker *worker)
>  {
> -CursorChannel* cursor;
> +CursorChannel *cursor;

I would prefer cursor_channel or channel.

> +RedChannel *channel = NULL;
> +ChannelCbs cbs = {
> +.on_disconnect =  cursor_channel_client_on_disconnect,
> +.send_item = cursor_channel_send_item,
> +.hold_item = cursor_channel_hold_pipe_item,
> +.release_item = cursor_channel_release_item
> +};
>  
>  spice_info("create cursor channel");
> -cursor = (CursorChannel *)__new_channel(
> -worker, sizeof(CursorChannel),
> -SPICE_CHANNEL_CURSOR,
> -0,
> -cursor_channel_client_on_disconnect,
> -cursor_channel_send_item,
> -cursor_channel_hold_pipe_item,
> -cursor_channel_release_item,
> -red_channel_client_handle_message,
> -NULL,
> -NULL,
> -NULL);
> +channel = red_worker_new_channel(worker, sizeof(CursorChannel),
> + SPICE_CHANNEL_CURSOR, 0,
> + ,
> red_channel_client_handle_message);
>  
> +cursor = (CursorChannel *)channel;
>  cursor->cursor_visible = TRUE;
>  cursor->mouse_mode = SPICE_MOUSE_MODE_SERVER;
>  
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 0c8ba4c..52fe132 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9558,38 +9558,30 @@ DisplayChannelClient
> *display_channel_client_create(CommonChannel *common,
>  return dcc;
>  }
>  
> -RedChannel *__new_channel(RedWorker *worker, int size, uint32_t
> channel_type,
> -  int migration_flags,
> -  channel_disconnect_proc on_disconnect,
> -  channel_send_pipe_item_proc send_item,
> -  channel_hold_pipe_item_proc hold_item,
> -  channel_release_pipe_item_proc release_item,
> -  channel_handle_parsed_proc handle_parsed,
> -  channel_handle_migrate_flush_mark_proc
> handle_migrate_flush_mark,
> -  channel_handle_migrate_data_proc
> handle_migrate_data,
> -  channel_handle_migrate_data_get_serial_proc
> migrate_get_serial)
> +RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> +   uint32_t channel_type, int
> migration_flags,
> +   ChannelCbs *channel_cbs,
> +   channel_handle_parsed_proc handle_parsed)
>  {
>  RedChannel *channel = NULL;
>  CommonChannel *common;
> -ChannelCbs channel_cbs = { NULL, };
> -
> -channel_cbs.config_socket = common_channel_config_socket;
> -channel_cbs.on_disconnect = on_disconnect;
> -channel_cbs.send_item = send_item;
> -channel_cbs.hold_item = hold_item;
> -channel_cbs.release_item = release_item;
> -channel_cbs.alloc_recv_buf = common_alloc_recv_buf;
> -channel_cbs.release_recv_buf = common_release_recv_buf;
> -channel_cbs.handle_migrate_flush_mark = handle_migrate_flush_mark;
> -channel_cbs.handle_migrate_data = handle_migrate_data;
> -channel_cbs.handle_migrate_data_get_serial = migrate_get_serial;
> +
> +spice_return_val_if_fail(worker, NULL);
> +spice_return_val_if_fail(channel_cbs, NULL);
> +spice_return_val_if_fail(!channel_cbs->config_socket, NULL);
> +spice_return_val_if_fail(!channel_cbs->alloc_recv_buf, NULL);
> +spice_return_val_if_fail(!channel_cbs->release_recv_buf, NULL);
> +
> +channel_cbs->config_socket = common_channel_config_socket;
> +channel_cbs->alloc_recv_buf = common_alloc_recv_buf;
> +channel_cbs->release_recv_buf = common_release_recv_buf;
>  
>  channel = red_channel_create_parser(size, _core,
>  channel_type, worker->qxl->id,
>  TRUE /* handle_acks */,
>  
> spice_get_client_channel_parser(channel_type,
>  NULL),
>  handle_parsed,
> -   

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

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Doing so allows us to remove the extra QXLInstance parameter from
> cursor_item_unref() and makes the code a bit cleaner.
> 
> Also add cursor_item_ref().
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 70
>  +++--
>  server/cursor-channel.h |  9 ---
>  2 files changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 6cc2b93..2d7afc6 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -25,55 +25,58 @@
>  #include "cache_item.tmpl.c"
>  #undef CLIENT_CURSOR_CACHE
>  
> -static inline CursorItem *alloc_cursor_item(void)
> +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t
> group_id)
>  {
>  CursorItem *cursor_item;
>  
> +spice_return_val_if_fail(cmd != NULL, NULL);
> +
>  cursor_item = g_slice_new0(CursorItem);
> +cursor_item->qxl = qxl;
>  cursor_item->refs = 1;
> +cursor_item->group_id = group_id;
> +cursor_item->red_cursor = cmd;
>  
>  return cursor_item;
>  }
>  
> -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> +CursorItem *cursor_item_ref(CursorItem *item)
>  {
> -CursorItem *cursor_item;
> -
> -spice_return_val_if_fail(cmd != NULL, NULL);
> -cursor_item = alloc_cursor_item();
> +spice_return_val_if_fail(item != NULL, NULL);
> +spice_return_val_if_fail(item->refs > 0, NULL);

This catch a memory corruption, should abort the program,
I suggest spice_error, spice_critical or spice_assert.

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

Are these tests worth doing or can we assume that a NULL pointer is expected 
(like
unref(NULL) and ref(NULL) do nothing) ? This would make cursor_set_item like

static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
{
cursor_item_unref(cursor->item);

cursor->item = cursor_item_ref(item);
}


On naming, sometimes is cursor_item sometimes item. I would prefer cursor_item.

>  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int
>  num)
> @@ -155,7 +158,7 @@ static void put_cursor_pipe_item(CursorChannelClient
> *ccc, CursorPipeItem *pipe_
>  
>  spice_assert(!pipe_item_is_linked(_item->base));
>  
> -cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> pipe_item->cursor_item);
> +cursor_item_unref(pipe_item->cursor_item);
>  free(pipe_item);
>  }
>  
> @@ -411,7 +414,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  CursorItem *cursor_item;
>  int cursor_show = FALSE;
>  
> -cursor_item = cursor_item_new(cursor_cmd, group_id);
> +spice_return_if_fail(cursor);
> +spice_return_if_fail(cursor_cmd);
> +
> +cursor_item = cursor_item_new(red_worker_get_qxl(cursor->common.worker),
> +  cursor_cmd, group_id);
>  
>  switch (cursor_cmd->type) {
>  case QXL_CURSOR_SET:
> @@ -439,7 +446,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  red_channel_pipes_new_add(>common.base,
>  new_cursor_pipe_item,
>(void*)cursor_item);
>  }
> -cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> 

Re: [Spice-devel] [PATCH 3.2/12] Make cursor_channel_disconnect a CursorChannel method

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> The first argument should be CursorChannel* rather than RedChannel*
> since it's essentially a CursorChannel method.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 4 +++-
>  server/cursor-channel.h | 2 +-
>  server/red_worker.c | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 2d7afc6..219c1da 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -138,8 +138,10 @@ static void red_reset_cursor_cache(RedChannelClient
> *rcc)
>  red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
>  }
>  
> -void cursor_channel_disconnect(RedChannel *channel)
> +void cursor_channel_disconnect(CursorChannel *cursor)
>  {
> +RedChannel *channel = (RedChannel *)cursor;
> +
>  if (!channel || !red_channel_is_connected(channel)) {
>  return;
>  }

On naming, I would prefer cursor_channel or channel but cursor is quite
misleading as variable name for CursorChannel.

> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 8b49df7..c7eee81 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -79,7 +79,7 @@ typedef struct CursorChannel {
>  G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
>  
>  CursorChannel*   cursor_channel_new (RedWorker *worker, int
>  migrate);
> -void cursor_channel_disconnect  (RedChannel *channel);
> +void cursor_channel_disconnect  (CursorChannel *cursor);
>  void cursor_channel_reset   (CursorChannel *cursor);
>  void cursor_channel_process_cmd (CursorChannel *cursor,
>  RedCursorCmd *cursor_cmd,
>   uint32_t group_id);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 96c0f14..2543713 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8908,7 +8908,7 @@ static inline void flush_cursor_commands(RedWorker
> *worker)
>  red_channel_send(channel);
>  if (red_now() >= end_time) {
>  spice_warning("flush cursor timeout");
> -cursor_channel_disconnect(channel);
> +cursor_channel_disconnect(worker->cursor_channel);
>  worker->cursor_channel = NULL;
>  } else {
>  sleep_count++;

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

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

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Move the cursor-specific pipe item types to cursor-channel.h, and the
> display-specific types to red_worker.c. Only leave the common
> definitions in red_worker.h. This prepares for splitting the display
> channel into a separate file.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.h |  6 ++
>  server/red_worker.c | 17 +
>  server/red_worker.h | 21 +++--
>  3 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 9a22c19..1bdf236 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -32,6 +32,12 @@
>  #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
>  #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
>  
> +enum {
> +PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST,
> +PIPE_ITEM_TYPE_CURSOR_INIT,
> +PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> +};
> +
>  typedef struct CursorItem {
>  QXLInstance *qxl;
>  uint32_t group_id;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 495e452..2428daf 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -265,6 +265,23 @@ struct SpiceWatch {
>  void *watch_func_opaque;
>  };
>  
> +enum {
> +PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST,
> +PIPE_ITEM_TYPE_IMAGE,
> +PIPE_ITEM_TYPE_STREAM_CREATE,
> +PIPE_ITEM_TYPE_STREAM_CLIP,
> +PIPE_ITEM_TYPE_STREAM_DESTROY,
> +PIPE_ITEM_TYPE_UPGRADE,
> +PIPE_ITEM_TYPE_MIGRATE_DATA,
> +PIPE_ITEM_TYPE_PIXMAP_SYNC,
> +PIPE_ITEM_TYPE_PIXMAP_RESET,
> +PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> +PIPE_ITEM_TYPE_CREATE_SURFACE,
> +PIPE_ITEM_TYPE_DESTROY_SURFACE,
> +PIPE_ITEM_TYPE_MONITORS_CONFIG,
> +PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> +};
> +
>  #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS
>  
>  typedef struct SurfaceCreateItem {
> diff --git a/server/red_worker.h b/server/red_worker.h
> index 76502b6..aa97707 100644
> --- a/server/red_worker.h
> +++ b/server/red_worker.h
> @@ -48,25 +48,10 @@ typedef struct CommonChannel {
>  } CommonChannel;
>  
>  enum {
> -PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE,
> +PIPE_ITEM_TYPE_VERB = PIPE_ITEM_TYPE_CHANNEL_BASE,
>  PIPE_ITEM_TYPE_INVAL_ONE,
> -PIPE_ITEM_TYPE_CURSOR,
> -PIPE_ITEM_TYPE_CURSOR_INIT,
> -PIPE_ITEM_TYPE_IMAGE,
> -PIPE_ITEM_TYPE_STREAM_CREATE,
> -PIPE_ITEM_TYPE_STREAM_CLIP,
> -PIPE_ITEM_TYPE_STREAM_DESTROY,
> -PIPE_ITEM_TYPE_UPGRADE,
> -PIPE_ITEM_TYPE_VERB,
> -PIPE_ITEM_TYPE_MIGRATE_DATA,
> -PIPE_ITEM_TYPE_PIXMAP_SYNC,
> -PIPE_ITEM_TYPE_PIXMAP_RESET,
> -PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
> -PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE,
> -PIPE_ITEM_TYPE_CREATE_SURFACE,
> -PIPE_ITEM_TYPE_DESTROY_SURFACE,
> -PIPE_ITEM_TYPE_MONITORS_CONFIG,
> -PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> +
> +PIPE_ITEM_TYPE_COMMON_LAST
>  };
>  
>  typedef struct VerbItem {
> --
> 2.4.3

Acked

OT: I started wondering if all these Pipe stuff would be better implemented 
with callbacks instead
of lot of switches here and there. I'll write something on my notes.

Frediano

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

Re: [Spice-devel] [PATCH 3.10/12] Remove a couple single-use static functions

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> red_cursor_marshall_inval(), red_migrate_cursor() and
> on_new_cursor_channel() were short functions that were each only called
> from a single location, so there's no need for them to be separate
> functions.
> 
> Signed-off-by: Jonathon Jongsma 

Acked

OT: I don't fully agree with the reasoning. Today compilers are really
good inlining single used static functions. Putting even some lines in
small functions can improve readability if function name is well choosed,
future maintenance and type safety.

Frediano


> ---
>  server/cursor-channel.c |  9 +
>  server/red_worker.c | 40 ++--
>  2 files changed, 15 insertions(+), 34 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 4811b79..5222bd8 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -294,13 +294,6 @@ static inline void red_marshall_inval(RedChannelClient
> *rcc,
>  spice_marshall_msg_cursor_inval_one(base_marshaller, _one);
>  }
>  
> -static void red_cursor_marshall_inval(RedChannelClient *rcc,
> -SpiceMarshaller *m, CacheItem *cach_item)
> -{
> -spice_assert(rcc);
> -red_marshall_inval(rcc, m, cach_item);
> -}
> -
>  static void cursor_channel_send_item(RedChannelClient *rcc, PipeItem
>  *pipe_item)
>  {
>  SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> @@ -311,7 +304,7 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item)
>  cursor_marshall(rcc, m, SPICE_CONTAINEROF(pipe_item, CursorPipeItem,
>  base));
>  break;
>  case PIPE_ITEM_TYPE_INVAL_ONE:
> -red_cursor_marshall_inval(rcc, m, (CacheItem *)pipe_item);
> +red_marshall_inval(rcc, m, (CacheItem *)pipe_item);
>  break;
>  case PIPE_ITEM_TYPE_VERB:
>  red_marshall_verb(rcc, (VerbItem*)pipe_item);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2428daf..2967c91 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9901,29 +9901,6 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>  on_new_display_channel_client(dcc);
>  }
>  
> -static void red_migrate_cursor(RedWorker *worker, RedChannelClient *rcc)
> -{
> -if (red_channel_client_is_connected(rcc)) {
> -red_channel_client_pipe_add_type(rcc,
> - PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> -red_channel_client_default_migrate(rcc);
> -}
> -}
> -
> -static void on_new_cursor_channel(RedWorker *worker, RedChannelClient *rcc)
> -{
> -CursorChannel *channel = worker->cursor_channel;
> -
> -spice_assert(channel);
> -red_channel_client_ack_zero_messages_window(rcc);
> -red_channel_client_push_set_ack(rcc);
> -// TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> -// and test it's canvas? this is just a test to see if there is an
> active renderer?
> -if (worker->surfaces[0].context.canvas &&
> !channel->common.during_target_migrate) {
> -red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT);
> -}
> -}
> -
>  static void red_connect_cursor(RedWorker *worker, RedClient *client,
>  RedsStream *stream,
> int migrate,
> uint32_t *common_caps, int num_common_caps,
> @@ -9949,7 +9926,15 @@ static void red_connect_cursor(RedWorker *worker,
> RedClient *client, RedsStream
>  channel->stat = stat_add_node(worker->stat, "cursor_channel", TRUE);
>  channel->common.base.out_bytes_counter = stat_add_counter(channel->stat,
>  "out_bytes", TRUE);
>  #endif
> -on_new_cursor_channel(worker, >common.base);
> +
> +RedChannelClient *rcc = >common.base;
> +red_channel_client_ack_zero_messages_window(rcc);
> +red_channel_client_push_set_ack(rcc);
> +// TODO: why do we check for context.canvas? defer this to after display
> cc is connected
> +// and test it's canvas? this is just a test to see if there is an
> active renderer?
> +if (worker->surfaces[0].context.canvas &&
> !channel->common.during_target_migrate) {
> +red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT);
> +}
>  }
>  
>  static void surface_dirty_region_to_rects(RedSurface *surface,
> @@ -10674,12 +10659,15 @@ void handle_dev_cursor_disconnect(void *opaque,
> void *payload)
>  void handle_dev_cursor_migrate(void *opaque, void *payload)
>  {
>  RedWorkerMessageCursorMigrate *msg = payload;
> -RedWorker *worker = opaque;
>  RedChannelClient *rcc = msg->rcc;
>  
>  spice_info("migrate cursor client");
>  spice_assert(rcc);
> -red_migrate_cursor(worker, rcc);
> +if (!red_channel_client_is_connected(rcc))
> +return;
> +
> +red_channel_client_pipe_add_type(rcc,
> PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);

Re: [Spice-devel] [PATCH 3.11/12] Change some asserts to warnings

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Various changes in RedWorker and CursorChannel related to error and
> warning messages.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 28 
>  server/red_worker.c | 22 ++
>  server/red_worker.h |  1 -
>  3 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 5222bd8..ecb49b2 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -152,7 +152,8 @@ void cursor_channel_disconnect(CursorChannel *cursor)
>  
>  static void put_cursor_pipe_item(CursorChannelClient *ccc, CursorPipeItem
>  *pipe_item)
>  {
> -spice_assert(pipe_item);
> +spice_return_if_fail(pipe_item);
> +spice_return_if_fail(pipe_item->refs > 0);

Second test should be more strict, we are detecting memory corruption!

>  
>  if (--pipe_item->refs) {
>  return;
> @@ -239,7 +240,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  PipeItem *pipe_item = _pipe_item->base;
>  RedCursorCmd *cmd;
>  
> -spice_assert(cursor_channel);
> +spice_return_if_fail(cursor_channel);

Test for NULL should be done on rcc, there is no reason as rcc == NULL or 
rcc->channel == NULL
imply cursor_channel NULL (assuming just second condition is not good either).
Would even better to set ccc and cursor_channel after the test.

>  
>  cmd = cursor->red_cursor;
>  switch (cmd->type) {
> @@ -327,7 +328,9 @@ static void cursor_channel_send_item(RedChannelClient
> *rcc, PipeItem *pipe_item)
>  
>  static CursorPipeItem *cursor_pipe_item_ref(CursorPipeItem *item)
>  {
> -spice_assert(item);
> +spice_return_val_if_fail(item, NULL);
> +spice_return_val_if_fail(item->refs > 0, NULL);
> +

See above.

>  item->refs++;
>  return item;
>  }
> @@ -336,7 +339,9 @@ static CursorPipeItem
> *cursor_pipe_item_ref(CursorPipeItem *item)
>  static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem
>  *item)
>  {
>  CursorPipeItem *cursor_pipe_item;
> -spice_assert(item);
> +
> +spice_return_if_fail(item);
> +
>  cursor_pipe_item = SPICE_CONTAINEROF(item, CursorPipeItem, base);
>  cursor_pipe_item_ref(cursor_pipe_item);
>  }
> @@ -383,6 +388,12 @@ CursorChannelClient*
> cursor_channel_client_new(CursorChannel *cursor, RedClient
> uint32_t *common_caps, int
> num_common_caps,
> uint32_t *caps, int num_caps)
>  {
> +spice_return_val_if_fail(cursor, NULL);
> +spice_return_val_if_fail(client, NULL);
> +spice_return_val_if_fail(stream, NULL);
> +spice_return_val_if_fail(!num_common_caps || common_caps, NULL);
> +spice_return_val_if_fail(!num_caps || caps, NULL);
> +
>  CursorChannelClient *ccc =
>  (CursorChannelClient*)common_channel_new_client(>common,
>  
> sizeof(CursorChannelClient),
> @@ -393,11 +404,11 @@ CursorChannelClient*
> cursor_channel_client_new(CursorChannel *cursor, RedClient
>  num_common_caps,
>  caps,
>  num_caps);
> -if (!ccc) {
> -return NULL;
> -}
> +spice_return_val_if_fail(ccc != NULL, NULL);
> +
>  ring_init(>cursor_cache_lru);
>  ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
> +
>  return ccc;
>  }
>  
> @@ -431,7 +442,8 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  cursor->cursor_trail_frequency = cursor_cmd->u.trail.frequency;
>  break;
>  default:
> -spice_error("invalid cursor command %u", cursor_cmd->type);
> +spice_warning("invalid cursor command %u", cursor_cmd->type);
> +return;
>  }
>  

Not fully agree with this change. Is cursor command internally generated?

>  if (red_channel_is_connected(>common.base) &&
>  (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2967c91..b93676e 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -4214,7 +4214,7 @@ static int red_process_cursor(RedWorker *worker,
> uint32_t max_pipe_size, int *ri
>  break;
>  }
>  default:
> -spice_error("bad command type");
> +spice_warning("bad command type");
>  }
>  n++;
>  }

Same as previous comment.

> @@ -9909,19 +9909,15 @@ static void red_connect_cursor(RedWorker *worker,
> RedClient *client, RedsStream
>  CursorChannel *channel;
>  CursorChannelClient *ccc;
>  
> -if (worker->cursor_channel == NULL) {
> -

Re: [Spice-devel] [PATCH 3.12/12] Various minor style changes to worker and cursor channel

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 30 +-
>  server/red_worker.c |  7 ---
>  2 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index ecb49b2..9935c00 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -232,17 +232,17 @@ static void red_marshall_cursor_init(RedChannelClient
> *rcc, SpiceMarshaller *bas
>  }
>  
>  static void cursor_marshall(RedChannelClient *rcc,
> -   SpiceMarshaller *m, CursorPipeItem *cursor_pipe_item)
> +SpiceMarshaller *m, CursorPipeItem
> *cursor_pipe_item)
>  {
>  CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc->channel,
>  CursorChannel, common.base);
>  CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> -CursorItem *cursor = cursor_pipe_item->cursor_item;
> +CursorItem *item = cursor_pipe_item->cursor_item;
>  PipeItem *pipe_item = _pipe_item->base;
>  RedCursorCmd *cmd;
>  
>  spice_return_if_fail(cursor_channel);
>  
> -cmd = cursor->red_cursor;
> +cmd = item->red_cursor;
>  switch (cmd->type) {
>  case QXL_CURSOR_MOVE:
>  {
> @@ -261,7 +261,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  cursor_set.position = cmd->u.set.position;
>  cursor_set.visible = cursor_channel->cursor_visible;
>  
> -cursor_fill(ccc, _set.cursor, cursor, );
> +cursor_fill(ccc, _set.cursor, item, );
>  spice_marshall_msg_cursor_set(m, _set);
>  add_buf_from_info(m, );
>  break;
> @@ -285,7 +285,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  }
>  
>  static inline void red_marshall_inval(RedChannelClient *rcc,
> -SpiceMarshaller *base_marshaller, CacheItem *cach_item)
> +  SpiceMarshaller *base_marshaller,
> CacheItem *cach_item)
>  {
>  SpiceMsgDisplayInvalOne inval_one;
>  
> @@ -446,10 +446,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  return;
>  }
>  
> -if (red_channel_is_connected(>common.base) &&
> (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> -   cursor_cmd->type != QXL_CURSOR_MOVE ||
> cursor_show)) {
> -red_channel_pipes_new_add(>common.base,
> new_cursor_pipe_item,
> -  (void*)cursor_item);
> +if (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
> +|| cursor_cmd->type != QXL_CURSOR_MOVE
> +|| cursor_show) {
> +red_channel_pipes_new_add(>common.base,
> +  new_cursor_pipe_item, cursor_item);
>  }

Why silently remove the red_channel_is_connected check?

>  
>  cursor_item_unref(cursor_item);
> @@ -457,21 +458,24 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  
>  void cursor_channel_reset(CursorChannel *cursor)
>  {
> +RedChannel *channel = (RedChannel *)cursor;
> +

I would prefer channel = >common.base.

> +spice_return_if_fail(cursor);
> +
>  cursor_set_item(cursor, NULL);
>  cursor->cursor_visible = TRUE;
>  cursor->cursor_position.x = cursor->cursor_position.y = 0;
>  cursor->cursor_trail_length = cursor->cursor_trail_frequency = 0;
>  
> -if (red_channel_is_connected(>common.base)) {
> -red_channel_pipes_add_type(>common.base,
> -   PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> +if (red_channel_is_connected(channel)) {
> +red_channel_pipes_add_type(>common.base,

change both to use channel or none here.

> PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
>  if (!cursor->common.during_target_migrate) {
>  red_pipes_add_verb(>common.base,
>  SPICE_MSG_CURSOR_RESET);
>  }
>  if (!red_channel_wait_all_sent(>common.base,

another occurrence.

> DISPLAY_CLIENT_TIMEOUT)) {
>  red_channel_apply_clients(>common.base,
> -
> red_channel_client_disconnect_if_pending_send);
> +
> red_channel_client_disconnect_if_pending_send);
>  }
>  }
>  }
> diff --git a/server/red_worker.c b/server/red_worker.c
> index b93676e..8bb048b 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -10272,10 +10272,9 @@ static void dev_create_primary_surface(RedWorker
> *worker, uint32_t surface_id,
>  red_channel_push(>display_channel->common.base);
>  }
>  
> -if (cursor_is_connected(worker) &&
> !worker->cursor_channel->common.during_target_migrate) {
> +if (!worker->cursor_channel->common.during_target_migrate)

check removed without explanation. Also I think mostly if use blocks so
I don't agree with this style change.

>  

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

[Jeremy White]

We do this by auto detecting the inbound http(s) 'GET' and probing
for a well formulated WebSocket binary connection, such as used
by the spice-html5 client.  If detected, we implement a set of
cover functions that abstract the read/write/writev functions,
in a fashion similar to the SASL implemented.

This includes a limited implementation of the WebSocket protocol,
sufficient for our purposes.

Signed-off-by: Jeremy White 
---
 server/Makefile.am   |   2 +
 server/reds.c|   6 +
 server/reds_stream.c |  98 
 server/reds_stream.h |   2 +
 server/websocket.c   | 432 +++
 server/websocket.h   |  62 
 6 files changed, 602 insertions(+)
 create mode 100644 server/websocket.c
 create mode 100644 server/websocket.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 87288cc..e2f5452 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -113,6 +113,8 @@ libspice_server_la_SOURCES =\
reds-private.h  \
reds_stream.c   \
reds_stream.h   \
+   websocket.c \
+   websocket.h \
reds_sw_canvas.c\
reds_sw_canvas.h\
snd_worker.c\
diff --git a/server/reds.c b/server/reds.c
index 1f6774e..701ac9d 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2172,6 +2172,7 @@ static void reds_handle_link_error(void *opaque, int err)
 reds_link_free(link);
 }
 
+static void reds_handle_new_link(RedLinkInfo *link);
 static void reds_handle_read_header_done(void *opaque)
 {
 RedLinkInfo *link = (RedLinkInfo *)opaque;
@@ -2182,6 +2183,11 @@ static void reds_handle_read_header_done(void *opaque)
 header->size = GUINT32_FROM_LE(header->size);
 
 if (header->magic != SPICE_MAGIC) {
+if (reds_stream_is_websocket(link->stream,
+(guchar *) header, sizeof(*header))) {
+reds_handle_new_link(link);
+return;
+}
 reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
 reds_link_free(link);
 return;
diff --git a/server/reds_stream.c b/server/reds_stream.c
index 3b47391..9f813cf 100644
--- a/server/reds_stream.c
+++ b/server/reds_stream.c
@@ -30,6 +30,7 @@
 #include 
 
 #include 
+#include "websocket.h"
 
 #include 
 
@@ -76,6 +77,17 @@ typedef struct RedsSASL {
 } RedsSASL;
 #endif
 
+typedef struct {
+int closed;
+
+websocket_frame_t read_frame;
+guint64 write_remainder;
+
+ssize_t (*raw_read)(RedsStream *s, void *buf, size_t nbyte);
+ssize_t (*raw_write)(RedsStream *s, const void *buf, size_t nbyte);
+ssize_t (*raw_writev)(RedsStream *s, const struct iovec *iov, int iovcnt);
+} RedsWebSocket;
+
 struct RedsStreamPrivate {
 SSL *ssl;
 
@@ -85,6 +97,8 @@ struct RedsStreamPrivate {
 
 AsyncRead async_read;
 
+RedsWebSocket *ws;
+
 /* life time of info:
  * allocated when creating RedsStream.
  * deallocated when main_dispatcher handles the 
SPICE_CHANNEL_EVENT_DISCONNECTED
@@ -1068,3 +1082,87 @@ error:
 return FALSE;
 }
 #endif
+
+static ssize_t stream_websocket_read(RedsStream *s, void *buf, size_t size)
+{
+int rc;
+
+if (s->priv->ws->closed)
+return 0;
+
+rc = websocket_read((void *)s, buf, size, >priv->ws->read_frame,
+(websocket_write_cb_t) s->priv->ws->raw_read,
+(websocket_write_cb_t) s->priv->ws->raw_write);
+
+if (rc == 0)
+s->priv->ws->closed = 1;
+
+return rc;
+}
+
+static ssize_t stream_websocket_write(RedsStream *s, const void *buf, size_t 
size)
+{
+if (s->priv->ws->closed) {
+errno = EPIPE;
+return -1;
+}
+return websocket_write((void *)s, buf, size, >priv->ws->write_remainder,
+(websocket_write_cb_t) s->priv->ws->raw_write);
+}
+
+static ssize_t stream_websocket_writev(RedsStream *s, const struct iovec *iov, 
int iovcnt)
+{
+if (s->priv->ws->closed) {
+errno = EPIPE;
+return -1;
+}
+return websocket_writev((void *)s, (struct iovec *) iov, iovcnt, 
>priv->ws->write_remainder,
+(websocket_writev_cb_t) s->priv->ws->raw_writev);
+}
+
+/*
+If we detect that a newly opened stream appears to be using
+the WebSocket protocol, we will put in place cover functions
+that will speak WebSocket to the client, but allow the server
+to continue to use normal stream read/write/writev semantics.
+*/
+bool reds_stream_is_websocket(RedsStream *stream, unsigned char *buf, int len)
+{
+char rbuf[4096];
+int rc;
+
+if (stream->priv->ws) {
+return FALSE;
+}
+
+memcpy(rbuf, buf, len);
+rc = stream->priv->read(stream, rbuf + len, sizeof(rbuf) - len);
+if (rc <= 0)
+return FALSE;
+len += rc;
+
+if (websocket_is_start(rbuf, len)) {
+

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

[Gerd Hoffmann]

  Hi,

[ context for spice folks: patch was added to qemu increasing usb-audio 
  default buffer size ]

> > What bothers me is that you have no qualms about making latency on
> > everyone's system worse.
> 
> How do you know it makes sound on other people's systems worse? If you have
> actually done any testing, I would like to see the results. 

It's real.  With that change we *do* actually trade latency for better
sound quality.

You probably wouldn't notice with pure music playback.

Higher chance is with video playback.  lip sync issues might show up,
although you probably still have to watch carefully to actually notice.

Anything sending audio both ways and expecting it to run with low
latency (VoIP phone, music jam as mentioned by stefan) is affected even
more.

And we *do* actually just paper over the root cause.  Problem is the
real root cause can is very hard to track down.  It can be pretty much
anywhere in qemu, and even outside qemu.

One known issue actually is in spice-server (added spice-devel because
of that).  It does audio processing in the qemu iothread (instead of a
separate thread like it is done for the display channel).  If you turn
off audio compression in spice sound quality suddenly becomes better. I
think this this happens because the latency spikes caused by audio
compression go away.  Happens with intel-hda and windows guests (which
use very small audio buffers).  Not (yet) investigated in detail though.

cheers,
  Gerd


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

Re: [Spice-devel] [PATCH 3.2/12] Make cursor_channel_disconnect a CursorChannel method

[Frediano Ziglio]

> 
> On Thu, Oct 29, 2015 at 04:54:39PM -0500, Jonathon Jongsma wrote:
> > From: Marc-André Lureau 
> > 
> > The first argument should be CursorChannel* rather than RedChannel*
> > since it's essentially a CursorChannel method.
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.c | 4 +++-
> >  server/cursor-channel.h | 2 +-
> >  server/red_worker.c | 2 +-
> >  3 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 2d7afc6..219c1da 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -138,8 +138,10 @@ static void red_reset_cursor_cache(RedChannelClient
> > *rcc)
> >  red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
> >  }
> >  
> > -void cursor_channel_disconnect(RedChannel *channel)
> > +void cursor_channel_disconnect(CursorChannel *cursor)
> >  {
> > +RedChannel *channel = (RedChannel *)cursor;
> > +
> >  if (!channel || !red_channel_is_connected(channel)) {
> >  return;
> >  }
> > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > index 8b49df7..c7eee81 100644
> > --- a/server/cursor-channel.h
> > +++ b/server/cursor-channel.h
> > @@ -79,7 +79,7 @@ typedef struct CursorChannel {
> >  G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
> >  
> >  CursorChannel*   cursor_channel_new (RedWorker *worker, int
> >  migrate);
> > -void cursor_channel_disconnect  (RedChannel *channel);
> > +void cursor_channel_disconnect  (CursorChannel *cursor);
> >  void cursor_channel_reset   (CursorChannel *cursor);
> >  void cursor_channel_process_cmd (CursorChannel *cursor,
> >  RedCursorCmd *cursor_cmd,
> >   uint32_t group_id);
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 96c0f14..2543713 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -8908,7 +8908,7 @@ static inline void flush_cursor_commands(RedWorker
> > *worker)
> >  red_channel_send(channel);
> >  if (red_now() >= end_time) {
> >  spice_warning("flush cursor timeout");
> > -cursor_channel_disconnect(channel);
> > +cursor_channel_disconnect(worker->cursor_channel);
> >  worker->cursor_channel = NULL;
> >  } else {
> >  sleep_count++;
> 
> Fwiw, this method has 'channel', 'cursor_red_channel' (both are
> RedChannel *) and it also references 'worker->cursor_channel' as a
> CursorChannel. Ultimately these 3 variables are referencing to the same
> memory location, would be less messy to have 2 (one RedChannel, one
> CursorChannel) defined together, or not to have any and to use
> worker->cursor_channel + cast if needed everywhere.
> 
> Christophe

Sorry, was confusing.
I was referring at the "cursor" name. In order lines is cursor_channel.

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

Re: [Spice-devel] [PATCH v6 01/26] server: Remove an unnecessary cast in encode_frame()

[Frediano Ziglio]

> 
> On Wed, 21 Oct 2015, Christophe Fergeau wrote:
> 
> > ACK.
> > 
> > On Wed, Oct 14, 2015 at 05:30:35PM +0200, Francois Gouget wrote:
> > > Signed-off-by: Francois Gouget 
> > > ---
> > >  server/red_worker.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index babb597..616be72 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -8512,8 +8512,7 @@ static int encode_frame(DisplayChannelClient *dcc,
> > > const SpiceRect *src,
> > >  const unsigned int stream_width = src->right - src->left;
> > >  
> > >  for (i = 0; i < stream_height; i++) {
> > > -uint8_t *src_line =
> > > -(uint8_t *)red_get_image_line(chunks, , ,
> > > image_stride);
> > > +uint8_t *src_line = red_get_image_line(chunks, , ,
> > > image_stride);
> > >  
> > >  if (!src_line) {
> > >  return FALSE;
> > > --
> > > 2.6.1
> 
> Is anything blocking this?
> 

Nothing, pushed.

Probably just a mistake, thanks for reminds it.

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

Re: [Spice-devel] [PATCH 3.8/12] CursorChannel* arg in cursor_channel_client_new()

[Frediano Ziglio]

> 
> On Fri, Oct 30, 2015 at 03:30:40AM -0400, Frediano Ziglio wrote:
> > 
> > > 
> > > From: Marc-André Lureau 
> > > 
> > > Instead of passing a CommonChannel* argument, use CursorChannel* since
> > > this function is only valid for CursorChannels.
> > > 
> > > Signed-off-by: Jonathon Jongsma 
> > > ---
> > >  server/cursor-channel.c | 5 ++---
> > >  server/cursor-channel.h | 2 +-
> > >  server/red_worker.c | 2 +-
> > >  3 files changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index 9c4bf1e..4811b79 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -385,14 +385,13 @@ CursorChannel* cursor_channel_new(RedWorker
> > > *worker)
> > >  return cursor;
> > >  }
> > >  
> > > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> > > -   RedClient *client,
> > > RedsStream
> > > *stream,
> > > +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> > > RedClient *client, RedsStream *stream,
> > > int mig_target,
> > > uint32_t *common_caps,
> > > int
> > > num_common_caps,
> > > uint32_t *caps, int
> > > num_caps)
> > >  {
> > >  CursorChannelClient *ccc =
> > > -(CursorChannelClient*)common_channel_new_client(common,
> > > +(CursorChannelClient*)common_channel_new_client(>common,
> > >  
> > > sizeof(CursorChannelClient),
> > >  client, stream,
> > >  mig_target,
> > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > index 213f124..9a22c19 100644
> > > --- a/server/cursor-channel.h
> > > +++ b/server/cursor-channel.h
> > > @@ -84,7 +84,7 @@ void cursor_channel_reset
> > > (CursorChannel *cursor);
> > >  void cursor_channel_process_cmd (CursorChannel *cursor,
> > >  RedCursorCmd *cursor_cmd,
> > >   uint32_t group_id);
> > >  
> > > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> > > +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> > > RedClient *client,
> > > RedsStream
> > > *stream,
> > > int mig_target,
> > > uint32_t *common_caps,
> > > int
> > > num_common_caps,
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 52fe132..495e452 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -9921,7 +9921,7 @@ static void red_connect_cursor(RedWorker *worker,
> > > RedClient *client, RedsStream
> > >  }
> > >  channel = worker->cursor_channel;
> > >  spice_info("add cursor channel client");
> > > -ccc = cursor_channel_client_new(>common, client, stream,
> > > +ccc = cursor_channel_client_new(channel, client, stream,
> > >  migrate,
> > >  common_caps, num_common_caps,
> > >  caps, num_caps);
> > > --
> > > 2.4.3
> > > 
> > 
> > Acked
> 
> NB: This adds a 'CursorChannel *cursor' which you commented on in other
> patches.
> 
> Christophe
> 

Opps... not a good reason to repeat same mistake again...

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

Re: [Spice-devel] [PATCH 3.10/12] Remove a couple single-use static functions

[Christophe Fergeau]

On Fri, Oct 30, 2015 at 03:44:05AM -0400, Frediano Ziglio wrote:
> OT: I don't fully agree with the reasoning. Today compilers are really
> good inlining single used static functions. Putting even some lines in
> small functions can improve readability if function name is well choosed,
> future maintenance and type safety.

Same feeling here, save for red_cursor_marshall_inval() in that patch.

Christophe


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

[Spice-devel] spice-gtk: Fix error handling in stream_get_current_frame()

[Francois Gouget]

*data must always be set to NULL on error.

Signed-off-by: Francois Gouget 
---
 src/channel-display.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/channel-display.c b/src/channel-display.c
index 46e9829..9e42dd9 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1081,20 +1081,21 @@ uint32_t stream_get_current_frame(display_stream *st, 
uint8_t **data)
 return 0;
 }
 
-if (spice_msg_in_type(st->msg_data) == SPICE_MSG_DISPLAY_STREAM_DATA) {
+switch (spice_msg_in_type(st->msg_data)) {
+case SPICE_MSG_DISPLAY_STREAM_DATA: {
 SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(st->msg_data);
-
 *data = op->data;
 return op->data_size;
-} else {
+}
+case SPICE_MSG_DISPLAY_STREAM_DATA_SIZED: {
 SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
-
-g_return_val_if_fail(spice_msg_in_type(st->msg_data) ==
- SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, 0);
 *data = op->data;
 return op->data_size;
-   }
-
+}
+default:
+*data = NULL;
+g_return_val_if_reached(0);
+}
 }
 
 G_GNUC_INTERNAL
-- 
2.6.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Anton D . Kachalov]

Hello, Jeremy.

I've miss support for SPICE_MSG_DISPLAY_DRAW_ALPHA_BLEND in spice-html5. The 
image type that is passed from the server that we've use is a incremental 
updates (compressed full-size frame) on top of the surface. Do you have some 
roadmap for implementation more SPIDE_MSG_DISPLAY_* message types?

29.10.2015, 19:12, "Jeremy White" :
> On 10/29/2015 06:38 AM, j...@eyeos.com wrote:
>>  Hi,
>>
>>  We are very glad to announce that today we have released under AGPL3
>>  license our full featured (audio, video, clipboard, qxl advanced
>>  graphics, etc) spice web client.
>>
>>  The github of the project:
>>
>>  https://github.com/eyeos/spice-web-client
>>
>>  Comments are welcome!
>
[…]
>
> I am a bit frustrated that you've sprung this, fully formed, upon us.
> The spice-html5 client has been developed, in public, and collaboration
> has been very welcome these past 3 years. But I do prefer your
> contribution to you keeping it closed instead :-).
>
> Do I guess correctly that this has been your company's main user
> interface for the past several years? Does that imply that it's been in
> fairly heavy use for some time now?
>
> If you don't mind my asking, what are your intentions with this? Are
> you hoping to have this become the defacto html5 client for Spice? Are
> you willing to maintain it and take on community contributions?
>
> I note the attribution clause in your license. As it stands now, I
> don't think that would cause any trouble (because of the 'however' of 5
> (d) of the agpl). But it would probably be useful to get a clear
> expression of your intent - is it the case that you are willing to
> contribute this only so long as the eyeos logo is prominently displayed
> by any one choosing to deploy it?
>
> Cheers,
>
> Jeremy

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

[Spice-devel] [spice 1/5] build-sys: Use AC_MSG_NOTICE()

[Francois Gouget]

Signed-off-by: Francois Gouget 
---
 configure.ac | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

This brings it more in line with the spice-gtk notice.

This is also a test of the email threading.


diff --git a/configure.ac b/configure.ac
index dfb967b..ad76467 100644
--- a/configure.ac
+++ b/configure.ac
@@ -289,7 +289,7 @@ docs/manual/Makefile
 ])
 
 dnl ==
-echo "
+AC_MSG_NOTICE([
 
 Spice $VERSION
 ==
@@ -300,23 +300,16 @@ echo "
 python:   ${PYTHON}
 
 OpenGL:   ${enable_opengl}
-
 LZ4 support:  ${enable_lz4}
-
 Smartcard:${have_smartcard}
-
 SASL support: ${enable_sasl}
-
 Automated tests:  ${enable_automated_tests}
-
 Manual:   ${have_asciidoc}
-"
+
+Now type 'make' to build $PACKAGE
+])
 
 if test x"$arch_warn" != x; then
 AC_MSG_WARN([$arch_warn])
 echo ""
 fi
-
-echo \
-"Now type 'make' to build $PACKAGE
-"
-- 
2.6.1

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

[Spice-devel] [client 5/5] build-sys: Use SPICE_WARNING() to issue the DBus warning

[Francois Gouget]

Signed-off-by: Francois Gouget 
---
 configure.ac | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Of course this one depends on 'common 3/5' but not on 'spice 4/5'.

diff --git a/configure.ac b/configure.ac
index 7033cbb..db2891f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -671,6 +671,8 @@ have_dbus=no
 if test "x$enable_dbus" != "xno"; then
   AC_DEFINE([USE_GDBUS], [1], [Define if supporting gdbus])
   have_dbus=yes
+else
+  SPICE_WARNING([No D-Bus support, desktop integration and USB redirection may 
not work properly])
 fi
 
 SPICE_CHECK_LZ4
@@ -750,6 +752,4 @@ AC_MSG_NOTICE([
 Now type 'make' to build $PACKAGE
 
 ])
-if test "x$have_dbus" = "xno"; then
-  AC_MSG_WARN([No D-Bus support, desktop integration and USB redirection may 
not work properly])
-fi
+SPICE_PRINT_MESSAGES
-- 
2.6.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice 4/5] build-sys: Use SPICE_WARNING() to issue the architecture warning

[Francois Gouget]

Signed-off-by: Francois Gouget 
---
 configure.ac | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index ad76467..95e35a4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -64,10 +64,9 @@ m4_ifndef([AS_VAR_APPEND],
 # Check for the CPU we are using
 case $host_cpu in
   x86_64)
-arch_warn=""
 ;;
   *)
-arch_warn="spice-server on non-x86_64 architectures hasn't been 
extensively tested"
+SPICE_WARNING([spice-server on non-x86_64 architectures hasn't been 
extensively tested])
 esac
 
 dnl =
@@ -308,8 +307,4 @@ AC_MSG_NOTICE([
 
 Now type 'make' to build $PACKAGE
 ])
-
-if test x"$arch_warn" != x; then
-AC_MSG_WARN([$arch_warn])
-echo ""
-fi
+SPICE_PRINT_MESSAGES
-- 
2.6.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 09/11] utils: add red_get_monotonic_time()

[Frediano Ziglio]

> 
> On Thu, 29 Oct 2015, Frediano Ziglio wrote:
> [...]
> > +/* FIXME: consider g_get_monotonic_time (), but in microseconds */
> > +static inline red_time_t red_get_monotonic_time(void)
> > +{
> > +struct timespec time;
> > +
> > +clock_gettime(CLOCK_MONOTONIC, );
> > +return (red_time_t) time.tv_sec * (1000 * 1000 * 1000) + time.tv_nsec;
> > +}
> 
> I think the name should be more generic as I see no reason why this
> function could not also replace get_time_stamp() (the signed/unsigned
> difference does not seem insurmountable), and various clock_gettime()
> calls in mjpeg_encoder.c and in red_channel.c.
>

I agree, this code (clock_gettime(MONOTONIC) + computation) is duplicated
quite a lot. Yes, get_time_stamp is exactly the same. int64_t/uint64_t in
this case is not that different but for a generic function I would prefer
the signed version as unsigned could cause some overflows (well, is much
likely to cause them). The range is enough to cover any possible value from
red_get_monotonic_time/get_time_stamp.
 
> It would also be nice to have a NANO_SECOND constant instead of '1000 *
> 1000 * 1000' in one place, '10ULL' in another (notice the LL
> btw), etc.
> 
> Also '30 * NANO_SECOND' would be clearer than '300ULL'.
>

I don't think is much more readable. Personally NANO_SECOND seems a time
measure instead of a time conversion constant I would prefer something like
NANOSECONDS_PER_SECOND but well, it's obviously 1000 * 1000 * 1000 !

Note that in the patch the LL is useless as the conversion is explicit
on the first factor.
 
> I'm open to naming variations but it should be something that can
> readily be adapted to get the duration of one second in milliseconds,
> MILLI_SECOND, in microseconds, MICRO_SECOND, or the duration of a
> millisecond in nanoseconds (for conversions), NANO_MS or
> NANO_MILLISECOND.
>

The patch was pushed but I don't see any drawback in improving it.

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

Re: [Spice-devel] [PATCH 3.2/12] Make cursor_channel_disconnect a CursorChannel method

[Christophe Fergeau]

On Thu, Oct 29, 2015 at 04:54:39PM -0500, Jonathon Jongsma wrote:
> From: Marc-André Lureau 
> 
> The first argument should be CursorChannel* rather than RedChannel*
> since it's essentially a CursorChannel method.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/cursor-channel.c | 4 +++-
>  server/cursor-channel.h | 2 +-
>  server/red_worker.c | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 2d7afc6..219c1da 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -138,8 +138,10 @@ static void red_reset_cursor_cache(RedChannelClient *rcc)
>  red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
>  }
>  
> -void cursor_channel_disconnect(RedChannel *channel)
> +void cursor_channel_disconnect(CursorChannel *cursor)
>  {
> +RedChannel *channel = (RedChannel *)cursor;
> +
>  if (!channel || !red_channel_is_connected(channel)) {
>  return;
>  }
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 8b49df7..c7eee81 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -79,7 +79,7 @@ typedef struct CursorChannel {
>  G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
>  
>  CursorChannel*   cursor_channel_new (RedWorker *worker, int 
> migrate);
> -void cursor_channel_disconnect  (RedChannel *channel);
> +void cursor_channel_disconnect  (CursorChannel *cursor);
>  void cursor_channel_reset   (CursorChannel *cursor);
>  void cursor_channel_process_cmd (CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
>   uint32_t group_id);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 96c0f14..2543713 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8908,7 +8908,7 @@ static inline void flush_cursor_commands(RedWorker 
> *worker)
>  red_channel_send(channel);
>  if (red_now() >= end_time) {
>  spice_warning("flush cursor timeout");
> -cursor_channel_disconnect(channel);
> +cursor_channel_disconnect(worker->cursor_channel);
>  worker->cursor_channel = NULL;
>  } else {
>  sleep_count++;

Fwiw, this method has 'channel', 'cursor_red_channel' (both are
RedChannel *) and it also references 'worker->cursor_channel' as a
CursorChannel. Ultimately these 3 variables are referencing to the same
memory location, would be less messy to have 2 (one RedChannel, one
CursorChannel) defined together, or not to have any and to use
worker->cursor_channel + cast if needed everywhere.

Christophe



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

Re: [Spice-devel] [PATCH v6 01/26] server: Remove an unnecessary cast in encode_frame()

[Francois Gouget]

On Wed, 21 Oct 2015, Christophe Fergeau wrote:

> ACK.
> 
> On Wed, Oct 14, 2015 at 05:30:35PM +0200, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget 
> > ---
> >  server/red_worker.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index babb597..616be72 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -8512,8 +8512,7 @@ static int encode_frame(DisplayChannelClient *dcc, 
> > const SpiceRect *src,
> >  const unsigned int stream_width = src->right - src->left;
> >  
> >  for (i = 0; i < stream_height; i++) {
> > -uint8_t *src_line =
> > -(uint8_t *)red_get_image_line(chunks, , , 
> > image_stride);
> > +uint8_t *src_line = red_get_image_line(chunks, , , 
> > image_stride);
> >  
> >  if (!src_line) {
> >  return FALSE;
> > -- 
> > 2.6.1

Is anything blocking this?


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

Re: [Spice-devel] [PATCH 3.8/12] CursorChannel* arg in cursor_channel_client_new()

[Christophe Fergeau]

On Fri, Oct 30, 2015 at 03:30:40AM -0400, Frediano Ziglio wrote:
> 
> > 
> > From: Marc-André Lureau 
> > 
> > Instead of passing a CommonChannel* argument, use CursorChannel* since
> > this function is only valid for CursorChannels.
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.c | 5 ++---
> >  server/cursor-channel.h | 2 +-
> >  server/red_worker.c | 2 +-
> >  3 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 9c4bf1e..4811b79 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -385,14 +385,13 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
> >  return cursor;
> >  }
> >  
> > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> > -   RedClient *client, 
> > RedsStream
> > *stream,
> > +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> > RedClient *client, RedsStream *stream,
> > int mig_target,
> > uint32_t *common_caps, int
> > num_common_caps,
> > uint32_t *caps, int 
> > num_caps)
> >  {
> >  CursorChannelClient *ccc =
> > -(CursorChannelClient*)common_channel_new_client(common,
> > +(CursorChannelClient*)common_channel_new_client(>common,
> >  
> > sizeof(CursorChannelClient),
> >  client, stream,
> >  mig_target,
> > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > index 213f124..9a22c19 100644
> > --- a/server/cursor-channel.h
> > +++ b/server/cursor-channel.h
> > @@ -84,7 +84,7 @@ void cursor_channel_reset
> > (CursorChannel *cursor);
> >  void cursor_channel_process_cmd (CursorChannel *cursor,
> >  RedCursorCmd *cursor_cmd,
> >   uint32_t group_id);
> >  
> > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common,
> > +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
> > RedClient *client, 
> > RedsStream
> > *stream,
> > int mig_target,
> > uint32_t *common_caps, int
> > num_common_caps,
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 52fe132..495e452 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -9921,7 +9921,7 @@ static void red_connect_cursor(RedWorker *worker,
> > RedClient *client, RedsStream
> >  }
> >  channel = worker->cursor_channel;
> >  spice_info("add cursor channel client");
> > -ccc = cursor_channel_client_new(>common, client, stream,
> > +ccc = cursor_channel_client_new(channel, client, stream,
> >  migrate,
> >  common_caps, num_common_caps,
> >  caps, num_caps);
> > --
> > 2.4.3
> > 
> 
> Acked

NB: This adds a 'CursorChannel *cursor' which you commented on in other
patches.

Christophe


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

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

[Jeremy White]

On 10/30/2015 07:24 AM, j...@eyeos.com wrote:
>> Wow!  That is astonishing, and well done, congratulations.
>>
>>
>> I've played with it a bit; I had some issues with keyboard problems, and
>> it wasn't immediately obvious how to use passwords or ssl.  Of course, I
>> suspect those are issues that are relatively easy to address.
>>
> Keyboard support is one of the most advanced and developed part of the
> spice web client. By default it is using spanish keyboard, maybe this is
> the reason you experienced keyboard issues?

Editing run.js and switching layout to 'us' got me my precious '/' key
.

Beyond that, though, there seem to be some issues with stuck key states.
 For example, typing ^U (as I do all too frequently), gets me the source
and then a stream of u's.  I'm also readily able to get the keyboard in
a stuck state, where I can't type.  (Haven't worked it out yet; some
combination of clicking and typing does it every time, though).

Again, I suspect that will be easy to figure out and correct.

[snip]

> There are three reasons why it is faster. The first one is because of
> the image handling optimization. The second one is because it works with
> qxl activated. The third one is because of the graphical pipeline with
> almost zero copy. For example, in the queue for the receiving socket, if
> look at it (socketqueue.js, inside network) there a shadow copy
> implemented in javascript to avoid copy and gc work.

I'm curious about the details, but may have to wait until I get some
time to dig in and look carefully for myself.

>  
> 
> Wow, thats very interesting. With a linux client using the last chromium
> connecting to a remote windows guest the performance of video is way
> better with spice web client than with spice-html5. Maybe you have
> optimized more for Xspice? We have stil not tested Xspice, neither
> optimized for it.

Yeah, I focus on only on Xspice.

The key feature I added was stream reports, which kicks in some of the
rate control logic in spice, which then, in turn, really helps smooth
playback.  The related patches are here:
  http://lists.freedesktop.org/archives/spice-devel/2015-June/020123.html
(although I now realize, with some embarrassment, that despite ACKs, I
never pushed.  Partly because I was considering redoing them to do the
reports at a later trigger time.)

This thread about video performance may also be of interest to you; it
sure would be great to collaborate with you on the subject:
  http://lists.freedesktop.org/archives/spice-devel/2015-June/020147.html

And while I'm linking interesting work, I hope you will eventually
really come to like this patch :
  http://lists.freedesktop.org/archives/spice-devel/2015-October/022960.html

> 
> I don't remember how spice-html5 handles video frames, we do it by using
> createObjectUrl to not having to encode the jpeg blob with urlencode,
> this increased the video performance a lot.

Mmm.  I do it with an Image and a data blob; I recall it being quite
fast.  But you may be taking better advantage of recent improvements in
html5; spice-html5 is largely set at 2012 vintage html5; things were
really pretty shaky back then.  (Well, okay, shakier; I wouldn't claim
that things are necessarily robust right now ).

[helpful history snipped]

> I hope this clarified a bit our intentions and position, i know this has
> been done a bit late :)

Thanks for the explanation, and thanks for being here now!

I think Daniel expressed the licensing concerns well; I'll let you
respond in that thread, and presume we can work that out.

From a technical perspective, the most straight forward path would seem
to be to deprecate the current spice-html5 and use your implementation
as a replacement.  I don't see any clean way to merge either work into
the other.  Your code appears to be more developed, and does have some
nice features the current implementation lacks (e.g. better name space
handling, unit tests).

That's somewhat painful for me to contemplate; as you know, it's a lot
of fun to write an html5 spice client .

I'll try to free up some time to look at your work in more detail next
week; perhaps I can try adopting your code and see how the process feels
and form a stronger opinion with more in depth knowledge.

If others lurking have an opinion, or advice, I think that would be
welcome; Jose and I are pretty much the definition of biased parties.

Cheers,

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

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

[jose]

 

> Wow! That is astonishing, and well done, congratulations. 
> 
> I've played with it a bit; I had some issues with keyboard problems, and
> it wasn't immediately obvious how to use passwords or ssl. Of course, I
> suspect those are issues that are relatively easy to address.

Keyboard support is one of the most advanced and developed part of the
spice web client. By default it is using spanish keyboard, maybe this is
the reason you experienced keyboard issues? 

In fact, we have used this in production (in spain) with no keyboard
problems at all, even with special characters, but as i said, always
with spanish layouts. 

We have included a english layout, but is not as tested and developed as
the spanish one. 

To use passwords you need to edit run.js to set the password, IIRC there
is no url parameter for the token, but can be aded easily. 

> Rough edges aside, I did note that it was visibly faster than the
> current html5 client for normal usage. I presume that all your
> optimization of image handling pays off nicely. I also presume it has
> better support for Windows guests (that would not be too difficult :-/).

There are three reasons why it is faster. The first one is because of
the image handling optimization. The second one is because it works with
qxl activated. The third one is because of the graphical pipeline with
almost zero copy. For example, in the queue for the receiving socket, if
look at it (socketqueue.js, inside network) there a shadow copy
implemented in javascript to avoid copy and gc work. 

It has been optimized a lot for windows with qxl enabled, in fact with
windows + qxl and using chromium, the performance is quite close to the
native spice client implementation. 

In linux, it is a bit slow on gnome with compositing because of obvious
reasons, but is very fast with kde4 with compositing disabled. In fact
with kde4 without compositing and using the xorg render the performance
is even better than in windows. 

One of our current products use kde4 without compositing and the other
one uses windows with qxl enabled, this is why we optimized a lot for
this environments/configurations. 

> It's lacking some recent developments; notably support for Opus and
> support for file transfer, although I don't think either of those would
> be especially hard to add. My most recent video work around rate
> control seems to give the current spice-html5 an edge in video
> performance, at least with Xspice, but that, too, would likely be easy
> to add to your client.

Wow, thats very interesting. With a linux client using the last chromium
connecting to a remote windows guest the performance of video is way
better with spice web client than with spice-html5. Maybe you have
optimized more for Xspice? We have stil not tested Xspice, neither
optimized for it. 

I don't remember how spice-html5 handles video frames, we do it by using
createObjectUrl to not having to encode the jpeg blob with urlencode,
this increased the video performance a lot. 

You are right about recent spice features, we would like to have support
for the most recent features, but we invested a lot in performance
because of requirements of our customers... They were more interested in
better performance than in more features, even to a crazy extent. 

> I am a bit frustrated that you've sprung this, fully formed, upon us.
> The spice-html5 client has been developed, in public, and collaboration
> has been very welcome these past 3 years. But I do prefer your
> contribution to you keeping it closed instead :-).

Personally I would have prefered to work with you guys improving the
spice-html5. However we started this is a startup and our investors
disliked opensource and free software a lot... So for us it was
impossible to collaborate with you. 

Later, we were acquired by Telefonica, the biggest Spanish Telco. We
continued to develop our products as telefonica owned company.
Telefonica is an active contributor in the Open Source community, its a
red hat partner and has a positive view about FOSS. 

It took us some time and meetings with Telefonica before we reached an
agreement for opensourcing a piece of software like this. I understand
that this could have been done better. 

Of course, as a developer myself, I really love to be able to contribute
with my work of the last 2 years in optimizations of all kinds and
support for lots of spice internals (glyphs, scaling, clipping, masking,
etc). 

> Do I guess correctly that this has been your company's main user
> interface for the past several years? Does that imply that it's been in
> fairly heavy use for some time now?

More or less. We have developed a eyeos-agent (like spice-agent) that
uses rabbitmq and through this we send coordinates and positions of
remote windows. Depending on the customer, we display the entire
linux/desktop or just the application that is running there. 

But yes, it is has been used heavily for the last 2 years. 

> If you don't mind my asking, 

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

[Marc-André Lureau]

Hi

- Original Message -
> Wow! That is astonishing, and well done, congratulations.

I second this, big thanks :)

> More or less. We have developed a eyeos-agent (like spice-agent) that uses
> rabbitmq and through this we send coordinates and positions of remote
> windows. Depending on the customer, we display the entire linux/desktop or
> just the application that is running there.

That's cool! do you cut the application windows on the client side, or you did 
some heavier work on the server side? Any plan to open-source this too? :)

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

Re: [Spice-devel] [PATCH 07/11] server: move display_channel_client_new()

[Christophe Fergeau]

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

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

[Daniel P. Berrange]

On Fri, Oct 30, 2015 at 01:24:36PM +0100, j...@eyeos.com wrote:
>  
> > I note the attribution clause in your license. As it stands now, I
> > don't think that would cause any trouble (because of the 'however' of 5
> > (d) of the agpl). But it would probably be useful to get a clear
> > expression of your intent - is it the case that you are willing to
> > contribute this only so long as the eyeos logo is prominently displayed
> > by any one choosing to deploy it?
> 
> Our intentions are very simple. We want this to become the defacto html5
> client for spice. Of course we want to maintain it and develop it
> openly. Aside from our obvious personal preferences for FOSS the main
> reason to opensource it is to develop it in community and of course, we
> are very excited to discuss the next steps with you and anyone
> interested in web support for spice :) 
> 
> And no, we are not willing to have an eyeos logo everywhere this client
> is used. In fact there is no need for you to display an eyeos logo to
> use this. 
> 
> We choosed AGPL because it protects the spice web client from the ASP
> loophole. We don't want en eyeos logo or something like this displayed
> every time this client is used, we just don't want for example a third
> party company adding support for opus in a private product of its own
> and not contributing it since this can be used from a service provider
> (ASP loophole in gpl and similar licenses). 
> 
> However, we are open to discussions about the license if you feel this
> can be a problem for reasonable scenarios.

Your rationale for using the AGPL as a core license makes sense to me.

Adding any additional terms to any license though, generally makes me
concerned, as it is very easy to add seemingly innocuous text that in
fact has non-obvious legal consequences. So I am a bit conerned about
the additionl terms wrt attribution & logo display. IANAL though, so
I will see if I can get any clarity / expert opinion on whether these
terms are likely to cause any problems for acceptance in distros such
as Fedora (or Debian) which are quite strict about interpretation of
licensing.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

Hey,

On Thu, Oct 29, 2015 at 04:54:40PM -0500, Jonathon Jongsma wrote:
> From: Marc-André Lureau 
> 
> The specific item type that was not being handled was
> PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor
> channel, but the analogous item for the display channel is
> PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> 
> The exact warning follows:
> 
>  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
>  ../../server/dcc-send.c:2442:dcc_send_item: should not be reached
>  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
>  ../../server/dcc.c:1595:release_item_before_push: invalid item type

Maybe I'm missing something, but it looks like the red_worker.c changes
in this patch would cause these warnings if they are not accompanied by
the cache_item.tmpl.c changes. INVAL_ONE seems to mean "remove one cache
item"
INVAL_PALETTE_CACHE seems to be whole cache invalidation VS INVAL_ONE
which invalidates just one element in the cache.

All in all, I'm not sure this patch is correct.

Christophe


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

Re: [Spice-devel] [PATCH v6 08/26] server: Add VP8 support to the GStreamer encoder

[Christophe Fergeau]

On Mon, Oct 26, 2015 at 09:06:03PM +0100, Francois Gouget wrote:
> On Thu, 22 Oct 2015, Christophe Fergeau wrote:
> 
> > On Wed, Oct 14, 2015 at 05:32:03PM +0200, Francois Gouget wrote:
> > > The Spice server administrator can specify the preferred encoder and
> > > codec preferences to optimize for CPU or bandwidth usage. Preferences
> > > are described in a semi-colon separated list of encoder:codec pairs.
> > > For instance 'gstreamer:vp8;spice:mjpeg' to pick first the GStreamer
> > > VP8 video encoder and the original MJPEG video encoder as a fallback.
> > > The server has a default preference list which can also be selected by
> > > specifying 'auto' as the preferences list.
> > 
> > Note: All this paragraph describes a very different thing than what the
> > shortlog says "server: Add VP8 support to the GStreamer encoder". Imo
> > they are 2 distinct things, the addition of
> > spice_server_set_video_codecs() and the addition of vp8 support to the
> > gstreamer encoder, ie they should be (at least) 2 different commits. It
> > might even make sense to move the client capability checks to a 3rd
> > commit.
> 
> These are technically independent but conceptually they all depend on 
> each other:
>  * Adding VP8 support without a way to request using VP8 instead of 
>MJPEG would be equivalent to adding dead code. Furthermore if 
>compiling in GStreamer support forced use of VP8 then that server 
>would be incompatible with most clients so checking the client caps 
>is really necessary.
> 
>  * Committing a way to pick the encoder and codec when the only 
>codec is MJPEG does not make sense either. This could degenerate to 
>code that only lets the administrator pick the encoder (i.e. builting 
>or gstreamer), but the next patch would have to add support for the 
>codec and would essentially be just as big.
> 

I would add the whole parsing code from red_dispatcher.c in separate
commit(s). They are quite pointless without VP8 support, but this is
still some preparatory work which is logically independant from the VP8
support. It's better to have it separate imo, better when reviewing,
better when bisecting, ...

>  * Committing the CAPS checking code first could work, though it would 
>be somewhat pointless and would not even shave a couple of lines off 
>this one.
> 
> 
> > When adding use of SPICE_DISPLAY_CAP_MULTI_CODEC , I would raise the
> > spice-protocol requirement in configure.ac to 0.12.11 (and do a
> > post-release bump in spice-protocol git).
> 
> I have bumped the requirement to 0.12.11 in configure.ac but I believe 
> the post-release bump in spice-protocol git has to be left to you.

It was actually done a while ago in spice-protocol git
http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=017ddbe7a7c73816f068527531

> 
> > > @@ -1349,10 +1349,12 @@ static void mjpeg_encoder_get_stats(VideoEncoder 
> > > *video_encoder,
> > >  stats->avg_quality = (double)encoder->avg_quality / 
> > > encoder->num_frames;
> > >  }
> > >  
> > > -VideoEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
> > > +VideoEncoder *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> > > +uint64_t starting_bit_rate,
> > >  VideoEncoderRateControlCbs *cbs,
> > >  void *cbs_opaque)
> > >  {
> > > +spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
> > 
> > I'd make that g_return_val_if_fail(codec_type ==
> > SPICE_VIDEO_CODEC_TYPE_MJPEG);
> 
> mjpeg_encoder.c does not depend on glib.h currently. The g_return_xxx() 
> functions are also very rarely used in the Spice server: only twice in 
> reds_stream.c. Furthermore calling mjpeg_encoder_new() for anything but 
> the MJPEG codec can only result from a coding error. So I think 
> spice_assert() is appropriate here.


spice_return_if_fail() can be used then. xx_return_if_fail() are used to
indicate programming errors, ie if they are triggered, then the code is
buggy. spice-server being a library, the less assert() it contains, the
better. So if the rest of the code can handle it just fine, better to
tag programming errors with xx_return_if_fail() rather than
assert'ing().

Christophe


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

Re: [Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets

[Victor Toso]

Hi,

On Fri, Oct 23, 2015 at 03:28:52PM +0200, Hans de Goede wrote:
> Hi,
> applications *pending writes* buffer size (in bytes).
>
> (so add the pending writes, drop the "that are ... isoc data", since the 
> application
> is not aware which data is isoc data and which is not.
>
> Otherwise looks good, no need to do a new version, just push it with the 
> above change:
>
> Reviewed-by: Hans de Goede 

Thanks this is now pushed as:
a88e197b18785d6de2322b5f26484c4130a6f2b9

with the follow up pre-release bump to 0.7.1 as:
e1a7e3dbbe091bfdc568372ff5ab18ed7eae972e

Many thanks for the help,
  Victor Toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v6 04/26] server: Hide the MJPEG encoder internals from red_worker.c

[Christophe Fergeau]

On Wed, Oct 21, 2015 at 03:27:02PM +0200, Francois Gouget wrote:
> On Wed, 21 Oct 2015, Christophe Fergeau wrote:
> 
> > ACK.
> > 
> > Since the changes up to now are useful cleanups regardless of the
> > addition of gstreamer, I suggest we land the patches up to this one
> > right now, hopefully this will save some rebasing pain (haven't looked
> > at rest of the patches yet, I'll do that now).
> 
> Thanks!

Sorry for the delay, I've pushed these after fixing a few conflict,s I
hope I got these right.

Christophe


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

Re: [Spice-devel] [spice-gtk PATCH v6 2/2] channel-usbredir: drop isoc packets on low bandwidth

[Victor Toso]

Hi,

On Fri, Oct 23, 2015 at 03:37:47PM +0200, Hans de Goede wrote:
> Ack, I believe the usbredir patch is ready, feel free to push that
> with my suggested doc change, then do a follow up patch to bump
> the version in usbredirproto.h and configure.ac to 0.7.1 and
> update the Changelog file with changes which have landed since
> 0.7 while you are at it.
>
> Then you can make the usbredirhost_set_buffered_output_size_cb
> call in spice-gtk:
>
> #if USBREDIR_VERSION >= 0x000701
> usbredirhost_set_buffered_output_size_cb(priv->host, 
> usbredir_buffered_output_size_callback);
> #endif
>
> And then once David Alan's "Allow missing capabilities from source host"
> patch is also merged we should probably do an official 0.7.1 release.
>
> Regards,
>
> Hans

This is now pushed as 36c7db9a38cc5335727c2abbe7968112eb6667e0
Thanks once again :)
  Victor Toso
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client 2/5] build-sys: Remove some dead configure.ac DBus code

[Christophe Fergeau]

ACK, this was made obsolete when dropping glib < 2.28 support.


On Fri, Oct 30, 2015 at 11:45:57AM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget 
> ---
>  configure.ac | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 5790a37..7033cbb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -671,10 +671,6 @@ have_dbus=no
>  if test "x$enable_dbus" != "xno"; then
>AC_DEFINE([USE_GDBUS], [1], [Define if supporting gdbus])
>have_dbus=yes
> -
> -  if test "x$enable_dbus" = "xyes" && test "x$have_dbus" = "xno"; then
> -AC_MSG_ERROR([D-Bus support explicitly requested, but some required 
> packages are not available])
> -  fi
>  fi
>  
>  SPICE_CHECK_LZ4
> -- 
> 2.6.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel


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

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

[Christophe Fergeau]

Hey,

This looks good, but I was wondering whether this is being copied/pasted
from some other configure.ac/m4 file, or if this is the initial
implementation?
We'll need to readd the AS_VAR_APPEND fallback which was removed in
f7ec855af3d , otherwise looks fine

Christophe

On Fri, Oct 30, 2015 at 11:46:59AM +0100, Francois Gouget wrote:
> A call to SPICE_WARNING() anywhere in the configure file results in the
> warning being printed at the end of the configure run where it will be
> be visible. This makes it possible to keep the SPICE_WARNING() calls
> together with the related feature checks instead of having to put a
> separate AC_MSG_WARN() call near the end.
> 
> Signed-off-by: Francois Gouget 
> ---
>  m4/spice-deps.m4 | 15 +++
>  1 file changed, 15 insertions(+)
> 
> If it is accepted I also plan to use it for the updated Spice GStreamer 
> patches.
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index 59744d2..1c753bf 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -1,3 +1,18 @@
> +# SPICE_WARNING(warning)
> +# SPICE_PRINT_MESSAGES
> +# --
> +# Collect warnings and print them at the end so they are clearly visible.
> +# -
> +AC_DEFUN([SPICE_WARNING],[AS_VAR_APPEND([spice_warnings],["|$1"])])
> +AC_DEFUN([SPICE_PRINT_MESSAGES],[ac_save_IFS="$IFS"
> +IFS="|"
> +for msg in $spice_warnings; do
> +IFS="$ac_save_IFS"
> +AS_VAR_IF([msg],[],,[echo >&2
> +AC_MSG_WARN([$msg])])
> +done
> +IFS="$ac_save_IFS"])# SPICE_WARNING
> +
>  # SPICE_CHECK_SYSDEPS()
>  # -
>  # Checks for header files and library functions needed by spice-common.
> -- 
> 2.6.1
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel


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

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

[Jonathon Jongsma]

On Fri, 2015-10-30 at 03:24 -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau 
> > 
> > Doing so allows us to remove the extra QXLInstance parameter from
> > cursor_item_unref() and makes the code a bit cleaner.
> > 
> > Also add cursor_item_ref().
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.c | 70
> >  +++--
> >  server/cursor-channel.h |  9 ---
> >  2 files changed, 44 insertions(+), 35 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 6cc2b93..2d7afc6 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -25,55 +25,58 @@
> >  #include "cache_item.tmpl.c"
> >  #undef CLIENT_CURSOR_CACHE
> >  
> > -static inline CursorItem *alloc_cursor_item(void)
> > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd,
> > uint32_t
> > group_id)
> >  {
> >  CursorItem *cursor_item;
> >  
> > +spice_return_val_if_fail(cmd != NULL, NULL);
> > +
> >  cursor_item = g_slice_new0(CursorItem);
> > +cursor_item->qxl = qxl;
> >  cursor_item->refs = 1;
> > +cursor_item->group_id = group_id;
> > +cursor_item->red_cursor = cmd;
> >  
> >  return cursor_item;
> >  }
> >  
> > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id)
> > +CursorItem *cursor_item_ref(CursorItem *item)
> >  {
> > -CursorItem *cursor_item;
> > -
> > -spice_return_val_if_fail(cmd != NULL, NULL);
> > -cursor_item = alloc_cursor_item();
> > +spice_return_val_if_fail(item != NULL, NULL);
> > +spice_return_val_if_fail(item->refs > 0, NULL);
> 
> This catch a memory corruption, should abort the program,
> I suggest spice_error, spice_critical or spice_assert.

It can suggest memory corruption, yes. But I'm not sure that it should
be fatal. This is exactly the same behavior as g_object_ref(), so I
don't see any good reason to make this warning fatal when the exact
same problem with a gobject will not be fatal. 

> 
> >  
> > -cursor_item->group_id = group_id;
> > -cursor_item->red_cursor = cmd;
> > +item->refs++;
> >  
> > -return cursor_item;
> > +return item;
> >  }
> >  
> > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > +void cursor_item_unref(CursorItem *item)
> >  {
> > -if (!--cursor->refs) {
> > -QXLReleaseInfoExt release_info_ext;
> > -RedCursorCmd *cursor_cmd;
> > -
> > -cursor_cmd = cursor->red_cursor;
> > -release_info_ext.group_id = cursor->group_id;
> > -release_info_ext.info = cursor_cmd->release_info;
> > -qxl->st->qif->release_resource(qxl, release_info_ext);
> > -red_put_cursor_cmd(cursor_cmd);
> > -free(cursor_cmd);
> > -
> > -g_slice_free(CursorItem, cursor);
> > -}
> > +QXLReleaseInfoExt release_info_ext;
> > +RedCursorCmd *cursor_cmd;
> > +
> > +spice_return_if_fail(item != NULL);
> > +
> > +if (--item->refs)
> > +return;
> > +
> > +cursor_cmd = item->red_cursor;
> > +release_info_ext.group_id = item->group_id;
> > +release_info_ext.info = cursor_cmd->release_info;
> > +item->qxl->st->qif->release_resource(item->qxl,
> > release_info_ext);
> > +red_put_cursor_cmd(cursor_cmd);
> > +free(cursor_cmd);
> > +
> > +g_slice_free(CursorItem, item);
> > +
> >  }
> >  
> >  static void cursor_set_item(CursorChannel *cursor, CursorItem
> > *item)
> >  {
> >  if (cursor->item)
> > -cursor_item_unref(red_worker_get_qxl(cursor
> > ->common.worker),
> > cursor->item);
> > -
> > -if (item)
> > -item->refs++;
> > +cursor_item_unref(cursor->item);
> >  
> > -cursor->item = item;
> > +cursor->item = item ? cursor_item_ref(item) : NULL;
> >  }
> >  
> 
> Are these tests worth doing or can we assume that a NULL pointer is
> expected (like
> unref(NULL) and ref(NULL) do nothing) ? This would make
> cursor_set_item like
> 
> static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
> {
> cursor_item_unref(cursor->item);
> 
> cursor->item = cursor_item_ref(item);
> }


I'd prefer to stay closer to the semantics of g_object_ref/unref(),
which warn if the argument is not a valid GObject.

> 
> 
> On naming, sometimes is cursor_item sometimes item. I would prefer
> cursor_item.
> 
> >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void
> > *data, int
> >  num)
> > @@ -155,7 +158,7 @@ static void
> > put_cursor_pipe_item(CursorChannelClient
> > *ccc, CursorPipeItem *pipe_
> >  
> >  spice_assert(!pipe_item_is_linked(_item->base));
> >  
> > -cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> > pipe_item->cursor_item);
> > +cursor_item_unref(pipe_item->cursor_item);
> >  free(pipe_item);
> >  }
> >  
> > @@ -411,7 +414,11 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >   

Re: [Spice-devel] [PATCH 06/11] server: make more of cursor private

[Christophe Fergeau]

This one looks good to me.
Some 
#define COMMON_CHANNEL_CLIENT(Client) ((CommonChannelClient*)(Client))
macro which we might want to change for now to
#define COMMON_CHANNEL_CLIENT(Client)  ((Client)->base))

and ditto for
#define CURSOR_CHANNEL_CLIENT(Client) ((CursorChannelClient*)(Client))


Christophe

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

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

[Christophe Fergeau]

On Fri, Oct 30, 2015 at 04:45:20PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> This looks good, but I was wondering whether this is being copied/pasted
> from some other configure.ac/m4 file, or if this is the initial
> implementation?
> We'll need to readd the AS_VAR_APPEND fallback which was removed in
> f7ec855af3d , otherwise looks fine

Ah, and maybe an empty line after all the warnings would look a bit
nicer.

Christophe


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

Re: [Spice-devel] [spice 1/5] build-sys: Use AC_MSG_NOTICE()

[Christophe Fergeau]

On Fri, Oct 30, 2015 at 11:45:44AM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget 
> ---
>  configure.ac | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> This brings it more in line with the spice-gtk notice.
> 
> This is also a test of the email threading.

Series looks good to me, thanks!

Christophe


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

[Spice-devel] [PATCH 3.7/12 v2] __new_channel -> red_worker_new_channel()

[Jonathon Jongsma]

From: Marc-André Lureau 

Rename and lightly refactor the function that creates new common
channels for RedWorker (essentially Cursor and Display channels).

Signed-off-by: Jonathon Jongsma 
---

Changes since v1:
 - rename variable from 'cursor' to 'cursor_channel'


 server/cursor-channel.c | 31 +-
 server/red_worker.c | 59 ++---
 server/red_worker.h | 14 
 3 files changed, 45 insertions(+), 59 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 36d65a2..fefbcd2 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -357,26 +357,25 @@ static void cursor_channel_release_item(RedChannelClient 
*rcc, PipeItem *item, i
 
 CursorChannel* cursor_channel_new(RedWorker *worker)
 {
-CursorChannel* cursor;
+CursorChannel *cursor_channel;
+RedChannel *channel = NULL;
+ChannelCbs cbs = {
+.on_disconnect =  cursor_channel_client_on_disconnect,
+.send_item = cursor_channel_send_item,
+.hold_item = cursor_channel_hold_pipe_item,
+.release_item = cursor_channel_release_item
+};
 
 spice_info("create cursor channel");
-cursor = (CursorChannel *)__new_channel(
-worker, sizeof(CursorChannel),
-SPICE_CHANNEL_CURSOR,
-0,
-cursor_channel_client_on_disconnect,
-cursor_channel_send_item,
-cursor_channel_hold_pipe_item,
-cursor_channel_release_item,
-red_channel_client_handle_message,
-NULL,
-NULL,
-NULL);
+channel = red_worker_new_channel(worker, sizeof(CursorChannel),
+ SPICE_CHANNEL_CURSOR, 0,
+ , red_channel_client_handle_message);
 
-cursor->cursor_visible = TRUE;
-cursor->mouse_mode = SPICE_MOUSE_MODE_SERVER;
+cursor_channel = (CursorChannel *)channel;
+cursor_channel->cursor_visible = TRUE;
+cursor_channel->mouse_mode = SPICE_MOUSE_MODE_SERVER;
 
-return cursor;
+return cursor_channel;
 }
 
 CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, 
RedClient *client, RedsStream *stream,
diff --git a/server/red_worker.c b/server/red_worker.c
index 43cb252..9c271c7 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -9478,38 +9478,30 @@ DisplayChannelClient 
*display_channel_client_create(CommonChannel *common,
 return dcc;
 }
 
-RedChannel *__new_channel(RedWorker *worker, int size, uint32_t channel_type,
-  int migration_flags,
-  channel_disconnect_proc on_disconnect,
-  channel_send_pipe_item_proc send_item,
-  channel_hold_pipe_item_proc hold_item,
-  channel_release_pipe_item_proc release_item,
-  channel_handle_parsed_proc handle_parsed,
-  channel_handle_migrate_flush_mark_proc 
handle_migrate_flush_mark,
-  channel_handle_migrate_data_proc handle_migrate_data,
-  channel_handle_migrate_data_get_serial_proc 
migrate_get_serial)
+RedChannel *red_worker_new_channel(RedWorker *worker, int size,
+   uint32_t channel_type, int migration_flags,
+   ChannelCbs *channel_cbs,
+   channel_handle_parsed_proc handle_parsed)
 {
 RedChannel *channel = NULL;
 CommonChannel *common;
-ChannelCbs channel_cbs = { NULL, };
-
-channel_cbs.config_socket = common_channel_config_socket;
-channel_cbs.on_disconnect = on_disconnect;
-channel_cbs.send_item = send_item;
-channel_cbs.hold_item = hold_item;
-channel_cbs.release_item = release_item;
-channel_cbs.alloc_recv_buf = common_alloc_recv_buf;
-channel_cbs.release_recv_buf = common_release_recv_buf;
-channel_cbs.handle_migrate_flush_mark = handle_migrate_flush_mark;
-channel_cbs.handle_migrate_data = handle_migrate_data;
-channel_cbs.handle_migrate_data_get_serial = migrate_get_serial;
+
+spice_return_val_if_fail(worker, NULL);
+spice_return_val_if_fail(channel_cbs, NULL);
+spice_return_val_if_fail(!channel_cbs->config_socket, NULL);
+spice_return_val_if_fail(!channel_cbs->alloc_recv_buf, NULL);
+spice_return_val_if_fail(!channel_cbs->release_recv_buf, NULL);
+
+channel_cbs->config_socket = common_channel_config_socket;
+channel_cbs->alloc_recv_buf = common_alloc_recv_buf;
+channel_cbs->release_recv_buf = common_release_recv_buf;
 
 channel = red_channel_create_parser(size, _core,
 channel_type, worker->qxl->id,
 TRUE /* handle_acks */,
 
spice_get_client_channel_parser(channel_type, NULL),

Re: [Spice-devel] [PATCH 11/11] worker: remove assertion on alloc_drawable

[Christophe Fergeau]

On Thu, Oct 29, 2015 at 04:24:27PM -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau 
> > 
> > There is no guarantee in the code that this can't be hit, so we should
> > cope with it (the condition can be reached easily by running the server
> > without waiting for blocked clients or pipe size)
> > 
> > The following commit will attempt to address this.
> > ---
> >  server/red_worker.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 0542b29..ca7e8bf 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -3300,7 +3300,8 @@ static void free_one_drawable(RedWorker *worker, int
> > force_glz_free)
> >  Drawable *drawable;
> >  Container *container;
> >  
> > -spice_assert(ring_item);
> > +if (!ring_item)
> > +return;
> >  drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> >  if (force_glz_free) {
> >  RingItem *glz_item, *next_item;
> > --
> > 2.4.3
> > 
> 
> The change make sense. The function is called from different places and not 
> in all path
> we are sure worker->current_list is not empty.
> 
> Style note: looks like we prefer always bracket like
> 
> if (!ring_item) {
> return;
> }
> 
> Beside the style note I would ack the patch.

Brackets would probably be more consistent yes, ack too.

Christophe



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

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

[Christophe Fergeau]

SPICE_CHECK_SMARTCARD documentation ends with a '---' comment, but
the # to start the comment is missing, causing a warning message when
running configure.
---
 m4/spice-deps.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
index 59744d2..0f90cec 100644
--- a/m4/spice-deps.m4
+++ b/m4/spice-deps.m4
@@ -35,7 +35,7 @@ AC_DEFUN([SPICE_CHECK_SYSDEPS], [
 # return the flags to use in the SMARTCARD_CFLAGS and SMARTCARD_LIBS 
variables, and
 # it will define a USE_SMARTCARD preprocessor symbol as well as a 
HAVE_SMARTCARD
 # Makefile conditional.
---
+#--
 AC_DEFUN([SPICE_CHECK_SMARTCARD], [
 AC_ARG_ENABLE([smartcard],
   AS_HELP_STRING([--enable-smartcard=@<:@yes/no/auto@:>@],
-- 
2.5.0

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

[Spice-devel] [PATCH 3.2/12 v2] Make cursor_channel_disconnect a CursorChannel method

[Jonathon Jongsma]

From: Marc-André Lureau 

The first argument should be CursorChannel* rather than RedChannel*
since it's essentially a CursorChannel method.

Signed-off-by: Jonathon Jongsma 
---

Changes since v1:
 - use parameter name cursor_channel instead of cursor


 server/cursor-channel.c | 4 +++-
 server/cursor-channel.h | 2 +-
 server/red_worker.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 81e5860..36d65a2 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -138,8 +138,10 @@ static void red_reset_cursor_cache(RedChannelClient *rcc)
 red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
 }
 
-void cursor_channel_disconnect(RedChannel *channel)
+void cursor_channel_disconnect(CursorChannel *cursor_channel)
 {
+RedChannel *channel = (RedChannel *)cursor_channel;
+
 if (!channel || !red_channel_is_connected(channel)) {
 return;
 }
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index f990d3c..f20001c 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -79,7 +79,7 @@ typedef struct CursorChannel {
 G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
 
 CursorChannel*   cursor_channel_new (RedWorker *worker);
-void cursor_channel_disconnect  (RedChannel *channel);
+void cursor_channel_disconnect  (CursorChannel 
*cursor_channel);
 void cursor_channel_reset   (CursorChannel *cursor);
 void cursor_channel_process_cmd (CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
  uint32_t group_id);
diff --git a/server/red_worker.c b/server/red_worker.c
index 2edfef5..574414c 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -8828,7 +8828,7 @@ static inline void flush_cursor_commands(RedWorker 
*worker)
 red_channel_send(channel);
 if (red_get_monotonic_time() >= end_time) {
 spice_warning("flush cursor timeout");
-cursor_channel_disconnect(channel);
+cursor_channel_disconnect(worker->cursor_channel);
 worker->cursor_channel = NULL;
 } else {
 sleep_count++;
-- 
2.4.3

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

Re: [Spice-devel] [PATCH 3.12/12] Various minor style changes to worker and cursor channel

[Jonathon Jongsma]

On Fri, 2015-10-30 at 03:39 -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau 
> > 
> > Signed-off-by: Jonathon Jongsma 
> > ---
> >  server/cursor-channel.c | 30 +-
> >  server/red_worker.c |  7 ---
> >  2 files changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index ecb49b2..9935c00 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -232,17 +232,17 @@ static void
> > red_marshall_cursor_init(RedChannelClient
> > *rcc, SpiceMarshaller *bas
> >  }
> >  
> >  static void cursor_marshall(RedChannelClient *rcc,
> > -   SpiceMarshaller *m, CursorPipeItem
> > *cursor_pipe_item)
> > +SpiceMarshaller *m, CursorPipeItem
> > *cursor_pipe_item)
> >  {
> >  CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc
> > ->channel,
> >  CursorChannel, common.base);
> >  CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> > -CursorItem *cursor = cursor_pipe_item->cursor_item;
> > +CursorItem *item = cursor_pipe_item->cursor_item;
> >  PipeItem *pipe_item = _pipe_item->base;
> >  RedCursorCmd *cmd;
> >  
> >  spice_return_if_fail(cursor_channel);
> >  
> > -cmd = cursor->red_cursor;
> > +cmd = item->red_cursor;
> >  switch (cmd->type) {
> >  case QXL_CURSOR_MOVE:
> >  {
> > @@ -261,7 +261,7 @@ static void cursor_marshall(RedChannelClient
> > *rcc,
> >  cursor_set.position = cmd->u.set.position;
> >  cursor_set.visible = cursor_channel->cursor_visible;
> >  
> > -cursor_fill(ccc, _set.cursor, cursor, );
> > +cursor_fill(ccc, _set.cursor, item, );
> >  spice_marshall_msg_cursor_set(m, _set);
> >  add_buf_from_info(m, );
> >  break;
> > @@ -285,7 +285,7 @@ static void cursor_marshall(RedChannelClient
> > *rcc,
> >  }
> >  
> >  static inline void red_marshall_inval(RedChannelClient *rcc,
> > -SpiceMarshaller *base_marshaller, CacheItem *cach_item)
> > +  SpiceMarshaller
> > *base_marshaller,
> > CacheItem *cach_item)
> >  {
> >  SpiceMsgDisplayInvalOne inval_one;
> >  
> > @@ -446,10 +446,11 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >  return;
> >  }
> >  
> > -if (red_channel_is_connected(>common.base) &&
> > (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> > -   cursor_cmd->type !=
> > QXL_CURSOR_MOVE ||
> > cursor_show)) {
> > -red_channel_pipes_new_add(>common.base,
> > new_cursor_pipe_item,
> > -  (void*)cursor_item);
> > +if (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
> > +|| cursor_cmd->type != QXL_CURSOR_MOVE
> > +|| cursor_show) {
> > +red_channel_pipes_new_add(>common.base,
> > +  new_cursor_pipe_item,
> > cursor_item);
> >  }
> 
> Why silently remove the red_channel_is_connected check?

Yes, I had the same question and was going to comment on it as well.
Marc-Andre, if you're reading this, do you remember?


> 
> >  
> >  cursor_item_unref(cursor_item);
> > @@ -457,21 +458,24 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >  
> >  void cursor_channel_reset(CursorChannel *cursor)
> >  {
> > +RedChannel *channel = (RedChannel *)cursor;
> > +
> 
> I would prefer channel = >common.base.
> 
> > +spice_return_if_fail(cursor);
> > +
> >  cursor_set_item(cursor, NULL);
> >  cursor->cursor_visible = TRUE;
> >  cursor->cursor_position.x = cursor->cursor_position.y = 0;
> >  cursor->cursor_trail_length = cursor->cursor_trail_frequency =
> > 0;
> >  
> > -if (red_channel_is_connected(>common.base)) {
> > -red_channel_pipes_add_type(>common.base,
> > -  
> >  PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> > +if (red_channel_is_connected(channel)) {
> > +red_channel_pipes_add_type(>common.base,
> 
> change both to use channel or none here.
> 
> > PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> >  if (!cursor->common.during_target_migrate) {
> >  red_pipes_add_verb(>common.base,
> >  SPICE_MSG_CURSOR_RESET);
> >  }
> >  if (!red_channel_wait_all_sent(>common.base,
> 
> another occurrence.
> 
> > DISPLAY_CLIENT_TIMEOUT)) {
> >  red_channel_apply_clients(>common.base,
> > -
> > red_channel_client_disconnect_if_pending_send);
> > +
> > red_channel_client_disconnect_if_pending_send);
> >  }
> >  }
> >  }
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index b93676e..8bb048b 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -10272,10 +10272,9 @@ static void

Re: [Spice-devel] [PATCH 11/11] worker: remove assertion on alloc_drawable

[Frediano Ziglio]

> 
> On Thu, Oct 29, 2015 at 04:24:27PM -0400, Frediano Ziglio wrote:
> > > 
> > > From: Marc-André Lureau 
> > > 
> > > There is no guarantee in the code that this can't be hit, so we should
> > > cope with it (the condition can be reached easily by running the server
> > > without waiting for blocked clients or pipe size)
> > > 
> > > The following commit will attempt to address this.
> > > ---
> > >  server/red_worker.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 0542b29..ca7e8bf 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -3300,7 +3300,8 @@ static void free_one_drawable(RedWorker *worker,
> > > int
> > > force_glz_free)
> > >  Drawable *drawable;
> > >  Container *container;
> > >  
> > > -spice_assert(ring_item);
> > > +if (!ring_item)
> > > +return;
> > >  drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> > >  if (force_glz_free) {
> > >  RingItem *glz_item, *next_item;
> > > --
> > > 2.4.3
> > > 
> > 
> > The change make sense. The function is called from different places and not
> > in all path
> > we are sure worker->current_list is not empty.
> > 
> > Style note: looks like we prefer always bracket like
> > 
> > if (!ring_item) {
> > return;
> > }
> > 
> > Beside the style note I would ack the patch.
> 
> Brackets would probably be more consistent yes, ack too.
> 
> Christophe
> 
> 

Pushed

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

[Spice-devel] [PATCH 3.12/12 v2] Various minor style changes to worker and cursor channel

[Jonathon Jongsma]

From: Marc-André Lureau 

Signed-off-by: Jonathon Jongsma 
---

Changes since v1:
 - added back removed red_channel_is_connected() checks
 - switch from (RedChannel*) cast to using >common.base
 - use local 'channel' variable throughout red_channel_reset()


 server/cursor-channel.c | 35 ---
 server/red_worker.c |  7 +--
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index a2e4979..6a1fcea 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -232,17 +232,17 @@ static void red_marshall_cursor_init(RedChannelClient 
*rcc, SpiceMarshaller *bas
 }
 
 static void cursor_marshall(RedChannelClient *rcc,
-   SpiceMarshaller *m, CursorPipeItem *cursor_pipe_item)
+SpiceMarshaller *m, CursorPipeItem 
*cursor_pipe_item)
 {
 CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc->channel, 
CursorChannel, common.base);
 CursorChannelClient *ccc = RCC_TO_CCC(rcc);
-CursorItem *cursor = cursor_pipe_item->cursor_item;
+CursorItem *item = cursor_pipe_item->cursor_item;
 PipeItem *pipe_item = _pipe_item->base;
 RedCursorCmd *cmd;
 
 spice_return_if_fail(cursor_channel);
 
-cmd = cursor->red_cursor;
+cmd = item->red_cursor;
 switch (cmd->type) {
 case QXL_CURSOR_MOVE:
 {
@@ -261,7 +261,7 @@ static void cursor_marshall(RedChannelClient *rcc,
 cursor_set.position = cmd->u.set.position;
 cursor_set.visible = cursor_channel->cursor_visible;
 
-cursor_fill(ccc, _set.cursor, cursor, );
+cursor_fill(ccc, _set.cursor, item, );
 spice_marshall_msg_cursor_set(m, _set);
 add_buf_from_info(m, );
 break;
@@ -285,7 +285,7 @@ static void cursor_marshall(RedChannelClient *rcc,
 }
 
 static inline void red_marshall_inval(RedChannelClient *rcc,
-SpiceMarshaller *base_marshaller, CacheItem *cach_item)
+  SpiceMarshaller *base_marshaller, 
CacheItem *cach_item)
 {
 SpiceMsgDisplayInvalOne inval_one;
 
@@ -446,10 +446,12 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
 return;
 }
 
-if (red_channel_is_connected(>common.base) && (cursor->mouse_mode 
== SPICE_MOUSE_MODE_SERVER ||
-   cursor_cmd->type != QXL_CURSOR_MOVE || 
cursor_show)) {
-red_channel_pipes_new_add(>common.base, new_cursor_pipe_item,
-  (void*)cursor_item);
+if (red_channel_is_connected(>common.base) &&
+(cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
+ || cursor_cmd->type != QXL_CURSOR_MOVE
+ || cursor_show)) {
+red_channel_pipes_new_add(>common.base,
+  new_cursor_pipe_item, cursor_item);
 }
 
 cursor_item_unref(cursor_item);
@@ -457,21 +459,24 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
 
 void cursor_channel_reset(CursorChannel *cursor)
 {
+RedChannel *channel = >common.base;
+
+spice_return_if_fail(cursor);
+
 cursor_set_item(cursor, NULL);
 cursor->cursor_visible = TRUE;
 cursor->cursor_position.x = cursor->cursor_position.y = 0;
 cursor->cursor_trail_length = cursor->cursor_trail_frequency = 0;
 
-if (red_channel_is_connected(>common.base)) {
-red_channel_pipes_add_type(>common.base,
-   PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
+if (red_channel_is_connected(channel)) {
+red_channel_pipes_add_type(channel, PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
 if (!cursor->common.during_target_migrate) {
-red_pipes_add_verb(>common.base, SPICE_MSG_CURSOR_RESET);
+red_pipes_add_verb(channel, SPICE_MSG_CURSOR_RESET);
 }
 if (!red_channel_wait_all_sent(>common.base,
DISPLAY_CLIENT_TIMEOUT)) {
-red_channel_apply_clients(>common.base,
-   red_channel_client_disconnect_if_pending_send);
+red_channel_apply_clients(channel,
+  
red_channel_client_disconnect_if_pending_send);
 }
 }
 }
diff --git a/server/red_worker.c b/server/red_worker.c
index 46f94ce..ee5199a 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -10192,7 +10192,8 @@ static void dev_create_primary_surface(RedWorker 
*worker, uint32_t surface_id,
 red_channel_push(>display_channel->common.base);
 }
 
-if (cursor_is_connected(worker) && 
!worker->cursor_channel->common.during_target_migrate) {
+if (cursor_is_connected(worker)
+&& !worker->cursor_channel->common.during_target_migrate) {
 red_channel_pipes_add_type(>cursor_channel->common.base,

Re: [Spice-devel] [PATCH 3.12/12] Various minor style changes to worker and cursor channel

[Jonathon Jongsma]

On Fri, 2015-10-30 at 12:21 -0400, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > > > @@ -446,10 +446,11 @@ void
> > > > cursor_channel_process_cmd(CursorChannel
> > > > *cursor,
> > > > RedCursorCmd *cursor_cmd,
> > > >  return;
> > > >  }
> > > >  
> > > > -if (red_channel_is_connected(>common.base) &&
> > > > (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> > > > -   cursor_cmd->type !=
> > > > QXL_CURSOR_MOVE ||
> > > > cursor_show)) {
> > > > -red_channel_pipes_new_add(>common.base,
> > > > new_cursor_pipe_item,
> > > > -  (void*)cursor_item);
> > > > +if (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
> > > > +|| cursor_cmd->type != QXL_CURSOR_MOVE
> > > > +|| cursor_show) {
> > > > +red_channel_pipes_new_add(>common.base,
> > > > +  new_cursor_pipe_item,
> > > > cursor_item);
> > > >  }
> > > 
> > > Why silently remove the red_channel_is_connected check?
> > 
> > Yes, I had the same question and was going to comment on it as
> > well.
> > Marc-Andre, if you're reading this, do you remember?
> 
> I don't remember. It looks quite a useless check to me, if
> process_cmd() is called, it suggests the channel is connected, isn't
> it? Furthermore, such a check could possibly exists in
> red_channel_pipes_new_add(). And even if it didn't, I suppose that
> wouldn't be a big deal to free it later. But anyway, it looks safer
> to add back if we don't know why it's changed.


I assumed that was probably the reason they were removed. But I think
I'll add them back just to be safe, since there's not much downside to
having them there.

Thanks,
Jonathon

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

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

[Jonathon Jongsma]

ACK

On Fri, 2015-10-30 at 16:55 +0100, Christophe Fergeau wrote:
> SPICE_CHECK_SMARTCARD documentation ends with a '---' comment,
> but
> the # to start the comment is missing, causing a warning message when
> running configure.
> ---
>  m4/spice-deps.m4 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> index 59744d2..0f90cec 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -35,7 +35,7 @@ AC_DEFUN([SPICE_CHECK_SYSDEPS], [
>  # return the flags to use in the SMARTCARD_CFLAGS and SMARTCARD_LIBS
> variables, and
>  # it will define a USE_SMARTCARD preprocessor symbol as well as a
> HAVE_SMARTCARD
>  # Makefile conditional.
> ---
> +#--
>  AC_DEFUN([SPICE_CHECK_SMARTCARD], [
>  AC_ARG_ENABLE([smartcard],
>AS_HELP_STRING([--enable-smartcard=@<:@yes/no/auto@:>@],
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3.2/12 v2] Make cursor_channel_disconnect a CursorChannel method

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> The first argument should be CursorChannel* rather than RedChannel*
> since it's essentially a CursorChannel method.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
> 
> Changes since v1:
>  - use parameter name cursor_channel instead of cursor
> 
> 
>  server/cursor-channel.c | 4 +++-
>  server/cursor-channel.h | 2 +-
>  server/red_worker.c | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 81e5860..36d65a2 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -138,8 +138,10 @@ static void red_reset_cursor_cache(RedChannelClient
> *rcc)
>  red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE);
>  }
>  
> -void cursor_channel_disconnect(RedChannel *channel)
> +void cursor_channel_disconnect(CursorChannel *cursor_channel)
>  {
> +RedChannel *channel = (RedChannel *)cursor_channel;
> +
>  if (!channel || !red_channel_is_connected(channel)) {
>  return;
>  }
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index f990d3c..f20001c 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -79,7 +79,7 @@ typedef struct CursorChannel {
>  G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE);
>  
>  CursorChannel*   cursor_channel_new (RedWorker *worker);
> -void cursor_channel_disconnect  (RedChannel *channel);
> +void cursor_channel_disconnect  (CursorChannel
> *cursor_channel);
>  void cursor_channel_reset   (CursorChannel *cursor);
>  void cursor_channel_process_cmd (CursorChannel *cursor,
>  RedCursorCmd *cursor_cmd,
>   uint32_t group_id);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2edfef5..574414c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8828,7 +8828,7 @@ static inline void flush_cursor_commands(RedWorker
> *worker)
>  red_channel_send(channel);
>  if (red_get_monotonic_time() >= end_time) {
>  spice_warning("flush cursor timeout");
> -cursor_channel_disconnect(channel);
> +cursor_channel_disconnect(worker->cursor_channel);
>  worker->cursor_channel = NULL;
>  } else {
>  sleep_count++;
> --
> 2.4.3

Acked

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

Re: [Spice-devel] [PATCH 3.7/12 v2] __new_channel -> red_worker_new_channel()

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Rename and lightly refactor the function that creates new common
> channels for RedWorker (essentially Cursor and Display channels).
> 
> Signed-off-by: Jonathon Jongsma 
> ---
> 
> Changes since v1:
>  - rename variable from 'cursor' to 'cursor_channel'
> 
> 
>  server/cursor-channel.c | 31 +-
>  server/red_worker.c | 59
>  ++---
>  server/red_worker.h | 14 
>  3 files changed, 45 insertions(+), 59 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 36d65a2..fefbcd2 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -357,26 +357,25 @@ static void
> cursor_channel_release_item(RedChannelClient *rcc, PipeItem *item, i
>  
>  CursorChannel* cursor_channel_new(RedWorker *worker)
>  {
> -CursorChannel* cursor;
> +CursorChannel *cursor_channel;
> +RedChannel *channel = NULL;
> +ChannelCbs cbs = {
> +.on_disconnect =  cursor_channel_client_on_disconnect,
> +.send_item = cursor_channel_send_item,
> +.hold_item = cursor_channel_hold_pipe_item,
> +.release_item = cursor_channel_release_item
> +};
>  
>  spice_info("create cursor channel");
> -cursor = (CursorChannel *)__new_channel(
> -worker, sizeof(CursorChannel),
> -SPICE_CHANNEL_CURSOR,
> -0,
> -cursor_channel_client_on_disconnect,
> -cursor_channel_send_item,
> -cursor_channel_hold_pipe_item,
> -cursor_channel_release_item,
> -red_channel_client_handle_message,
> -NULL,
> -NULL,
> -NULL);
> +channel = red_worker_new_channel(worker, sizeof(CursorChannel),
> + SPICE_CHANNEL_CURSOR, 0,
> + ,
> red_channel_client_handle_message);
>  
> -cursor->cursor_visible = TRUE;
> -cursor->mouse_mode = SPICE_MOUSE_MODE_SERVER;
> +cursor_channel = (CursorChannel *)channel;
> +cursor_channel->cursor_visible = TRUE;
> +cursor_channel->mouse_mode = SPICE_MOUSE_MODE_SERVER;
>  
> -return cursor;
> +return cursor_channel;
>  }
>  
>  CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
>  RedClient *client, RedsStream *stream,
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 43cb252..9c271c7 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -9478,38 +9478,30 @@ DisplayChannelClient
> *display_channel_client_create(CommonChannel *common,
>  return dcc;
>  }
>  
> -RedChannel *__new_channel(RedWorker *worker, int size, uint32_t
> channel_type,
> -  int migration_flags,
> -  channel_disconnect_proc on_disconnect,
> -  channel_send_pipe_item_proc send_item,
> -  channel_hold_pipe_item_proc hold_item,
> -  channel_release_pipe_item_proc release_item,
> -  channel_handle_parsed_proc handle_parsed,
> -  channel_handle_migrate_flush_mark_proc
> handle_migrate_flush_mark,
> -  channel_handle_migrate_data_proc
> handle_migrate_data,
> -  channel_handle_migrate_data_get_serial_proc
> migrate_get_serial)
> +RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> +   uint32_t channel_type, int
> migration_flags,
> +   ChannelCbs *channel_cbs,
> +   channel_handle_parsed_proc handle_parsed)
>  {
>  RedChannel *channel = NULL;
>  CommonChannel *common;
> -ChannelCbs channel_cbs = { NULL, };
> -
> -channel_cbs.config_socket = common_channel_config_socket;
> -channel_cbs.on_disconnect = on_disconnect;
> -channel_cbs.send_item = send_item;
> -channel_cbs.hold_item = hold_item;
> -channel_cbs.release_item = release_item;
> -channel_cbs.alloc_recv_buf = common_alloc_recv_buf;
> -channel_cbs.release_recv_buf = common_release_recv_buf;
> -channel_cbs.handle_migrate_flush_mark = handle_migrate_flush_mark;
> -channel_cbs.handle_migrate_data = handle_migrate_data;
> -channel_cbs.handle_migrate_data_get_serial = migrate_get_serial;
> +
> +spice_return_val_if_fail(worker, NULL);
> +spice_return_val_if_fail(channel_cbs, NULL);
> +spice_return_val_if_fail(!channel_cbs->config_socket, NULL);
> +spice_return_val_if_fail(!channel_cbs->alloc_recv_buf, NULL);
> +spice_return_val_if_fail(!channel_cbs->release_recv_buf, NULL);
> +
> +channel_cbs->config_socket = common_channel_config_socket;
> +channel_cbs->alloc_recv_buf = common_alloc_recv_buf;
> +channel_cbs->release_recv_buf = common_release_recv_buf;
>  
>  channel = red_channel_create_parser(size, _core,
>  

Re: [Spice-devel] [PATCH 11/11] worker: remove assertion on alloc_drawable

[Jonathon Jongsma]

On Fri, 2015-10-30 at 13:23 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Oct 29, 2015 at 04:24:27PM -0400, Frediano Ziglio wrote:
> > > > 
> > > > From: Marc-André Lureau 
> > > > 
> > > > There is no guarantee in the code that this can't be hit, so we
> > > > should
> > > > cope with it (the condition can be reached easily by running
> > > > the server
> > > > without waiting for blocked clients or pipe size)
> > > > 
> > > > The following commit will attempt to address this.
> > > > ---
> > > >  server/red_worker.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > index 0542b29..ca7e8bf 100644
> > > > --- a/server/red_worker.c
> > > > +++ b/server/red_worker.c
> > > > @@ -3300,7 +3300,8 @@ static void free_one_drawable(RedWorker
> > > > *worker,
> > > > int
> > > > force_glz_free)
> > > >  Drawable *drawable;
> > > >  Container *container;
> > > >  
> > > > -spice_assert(ring_item);
> > > > +if (!ring_item)
> > > > +return;
> > > >  drawable = SPICE_CONTAINEROF(ring_item, Drawable,
> > > > list_link);
> > > >  if (force_glz_free) {
> > > >  RingItem *glz_item, *next_item;
> > > > --
> > > > 2.4.3
> > > > 
> > > 
> > > The change make sense. The function is called from different
> > > places and not
> > > in all path
> > > we are sure worker->current_list is not empty.
> > > 
> > > Style note: looks like we prefer always bracket like
> > > 
> > > if (!ring_item) {
> > > return;
> > > }
> > > 
> > > Beside the style note I would ack the patch.
> > 
> > Brackets would probably be more consistent yes, ack too.
> > 
> > Christophe
> > 
> > 
> 
> Pushed
> 
> Frediano
> 

Hmm, you pushed already, but it seems that the commit summary doesn't
match the change (alloc_drawable() vs free_one_drawable())

Oh well...

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

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

[Jonathon Jongsma]

On Fri, 2015-10-30 at 14:39 +0100, Christophe Fergeau wrote:
> Hey,
> 
> On Thu, Oct 29, 2015 at 04:54:40PM -0500, Jonathon Jongsma wrote:
> > From: Marc-André Lureau 
> > 
> > The specific item type that was not being handled was
> > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the
> > cursor
> > channel, but the analogous item for the display channel is
> > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead.
> > 
> > The exact warning follows:
> > 
> >  (/usr/bin/qemu-kvm:24458): Spice-Warning **:
> >  ../../server/dcc-send.c:2442:dcc_send_item: should not be
> > reached
> >  (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **:
> >  ../../server/dcc.c:1595:release_item_before_push: invalid item
> > type
> 
> Maybe I'm missing something, but it looks like the red_worker.c
> changes
> in this patch would cause these warnings if they are not accompanied
> by
> the cache_item.tmpl.c changes.
>  INVAL_ONE seems to mean "remove one cache
> item"
> INVAL_PALETTE_CACHE seems to be whole cache invalidation VS INVAL_ONE
> which invalidates just one element in the cache.
> 
> All in all, I'm not sure this patch is correct.
> 
> Christophe


Ugh. I look at this more closely now and I notice that the warning
message from the commit log refers to dcc-send.c and dcc.c, which do
not exist in the master branch. I think I need to investigate a bit
more about what's going on here...


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

Re: [Spice-devel] [PATCH 09/11] utils: add red_get_monotonic_time()

[Francois Gouget]

On Thu, 29 Oct 2015, Frediano Ziglio wrote:
[...]
> +/* FIXME: consider g_get_monotonic_time (), but in microseconds */
> +static inline red_time_t red_get_monotonic_time(void)
> +{
> +struct timespec time;
> +
> +clock_gettime(CLOCK_MONOTONIC, );
> +return (red_time_t) time.tv_sec * (1000 * 1000 * 1000) + time.tv_nsec;
> +}

I think the name should be more generic as I see no reason why this 
function could not also replace get_time_stamp() (the signed/unsigned 
difference does not seem insurmountable), and various clock_gettime() 
calls in mjpeg_encoder.c and in red_channel.c. 

It would also be nice to have a NANO_SECOND constant instead of '1000 * 
1000 * 1000' in one place, '10ULL' in another (notice the LL 
btw), etc.

Also '30 * NANO_SECOND' would be clearer than '300ULL'.

I'm open to naming variations but it should be something that can 
readily be adapted to get the duration of one second in milliseconds, 
MILLI_SECOND, in microseconds, MICRO_SECOND, or the duration of a 
millisecond in nanoseconds (for conversions), NANO_MS or 
NANO_MILLISECOND.


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

Re: [Spice-devel] [PATCH 3.12/12 v2] Various minor style changes to worker and cursor channel

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Signed-off-by: Jonathon Jongsma 
> ---
> 
> Changes since v1:
>  - added back removed red_channel_is_connected() checks
>  - switch from (RedChannel*) cast to using >common.base
>  - use local 'channel' variable throughout red_channel_reset()
> 
> 
>  server/cursor-channel.c | 35 ---
>  server/red_worker.c |  7 +--
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index a2e4979..6a1fcea 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -232,17 +232,17 @@ static void red_marshall_cursor_init(RedChannelClient
> *rcc, SpiceMarshaller *bas
>  }
>  
>  static void cursor_marshall(RedChannelClient *rcc,
> -   SpiceMarshaller *m, CursorPipeItem *cursor_pipe_item)
> +SpiceMarshaller *m, CursorPipeItem
> *cursor_pipe_item)
>  {
>  CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc->channel,
>  CursorChannel, common.base);
>  CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> -CursorItem *cursor = cursor_pipe_item->cursor_item;
> +CursorItem *item = cursor_pipe_item->cursor_item;
>  PipeItem *pipe_item = _pipe_item->base;
>  RedCursorCmd *cmd;
>  
>  spice_return_if_fail(cursor_channel);
>  
> -cmd = cursor->red_cursor;
> +cmd = item->red_cursor;
>  switch (cmd->type) {
>  case QXL_CURSOR_MOVE:
>  {
> @@ -261,7 +261,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  cursor_set.position = cmd->u.set.position;
>  cursor_set.visible = cursor_channel->cursor_visible;
>  
> -cursor_fill(ccc, _set.cursor, cursor, );
> +cursor_fill(ccc, _set.cursor, item, );
>  spice_marshall_msg_cursor_set(m, _set);
>  add_buf_from_info(m, );
>  break;
> @@ -285,7 +285,7 @@ static void cursor_marshall(RedChannelClient *rcc,
>  }
>  
>  static inline void red_marshall_inval(RedChannelClient *rcc,
> -SpiceMarshaller *base_marshaller, CacheItem *cach_item)
> +  SpiceMarshaller *base_marshaller,
> CacheItem *cach_item)
>  {
>  SpiceMsgDisplayInvalOne inval_one;
>  
> @@ -446,10 +446,12 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  return;
>  }
>  
> -if (red_channel_is_connected(>common.base) &&
> (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> -   cursor_cmd->type != QXL_CURSOR_MOVE ||
> cursor_show)) {
> -red_channel_pipes_new_add(>common.base,
> new_cursor_pipe_item,
> -  (void*)cursor_item);
> +if (red_channel_is_connected(>common.base) &&
> +(cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
> + || cursor_cmd->type != QXL_CURSOR_MOVE
> + || cursor_show)) {
> +red_channel_pipes_new_add(>common.base,
> +  new_cursor_pipe_item, cursor_item);
>  }
>  
>  cursor_item_unref(cursor_item);
> @@ -457,21 +459,24 @@ void cursor_channel_process_cmd(CursorChannel *cursor,
> RedCursorCmd *cursor_cmd,
>  
>  void cursor_channel_reset(CursorChannel *cursor)
>  {
> +RedChannel *channel = >common.base;
> +
> +spice_return_if_fail(cursor);
> +
>  cursor_set_item(cursor, NULL);
>  cursor->cursor_visible = TRUE;
>  cursor->cursor_position.x = cursor->cursor_position.y = 0;
>  cursor->cursor_trail_length = cursor->cursor_trail_frequency = 0;
>  
> -if (red_channel_is_connected(>common.base)) {
> -red_channel_pipes_add_type(>common.base,
> -   PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
> +if (red_channel_is_connected(channel)) {
> +red_channel_pipes_add_type(channel,
> PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE);
>  if (!cursor->common.during_target_migrate) {
> -red_pipes_add_verb(>common.base,
> SPICE_MSG_CURSOR_RESET);
> +red_pipes_add_verb(channel, SPICE_MSG_CURSOR_RESET);
>  }
>  if (!red_channel_wait_all_sent(>common.base,
> DISPLAY_CLIENT_TIMEOUT)) {
> -red_channel_apply_clients(>common.base,
> -
> red_channel_client_disconnect_if_pending_send);
> +red_channel_apply_clients(channel,
> +
> red_channel_client_disconnect_if_pending_send);
>  }
>  }
>  }
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 46f94ce..ee5199a 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -10192,7 +10192,8 @@ static void dev_create_primary_surface(RedWorker
> *worker, uint32_t surface_id,
>  red_channel_push(>display_channel->common.base);
>  }
>  
> -if (cursor_is_connected(worker) &&
> !worker->cursor_channel->common.during_target_migrate) {
> +if (cursor_is_connected(worker)
> +  

Re: [Spice-devel] [PATCH 07/11] server: move display_channel_client_new()

[Frediano Ziglio]

> 
> On Thu, Oct 29, 2015 at 03:57:46PM -0400, Frediano Ziglio wrote:
> > 
> > > 
> > > From: Marc-André Lureau 
> > > 
> > > ---
> > >  server/Makefile.am   |  1 +
> > >  server/display-channel.c | 38 +++
> > >  server/display-channel.h | 52
> > >  +---
> > >  server/red_worker.c  | 37 +-
> > >  4 files changed, 89 insertions(+), 39 deletions(-)
> > >  create mode 100644 server/display-channel.c
> > > 
> > > diff --git a/server/Makefile.am b/server/Makefile.am
> > > index dc2fbc5..bd95818 100644
> > > --- a/server/Makefile.am
> > > +++ b/server/Makefile.am
> > > @@ -106,6 +106,7 @@ libspice_server_la_SOURCES =  \
> > >   red_time.h  \
> > >   red_worker.c\
> > >   red_worker.h\
> > > + display-channel.c   \
> > >   display-channel.h   \
> > >   cursor-channel.c\
> > >   cursor-channel.h\
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > new file mode 100644
> > > index 000..643169c
> > > --- /dev/null
> > > +++ b/server/display-channel.c
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > +   Copyright (C) 2009-2015 Red Hat, Inc.
> > > +
> > > +   This library is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU Lesser General Public
> > > +   License as published by the Free Software Foundation; either
> > > +   version 2.1 of the License, or (at your option) any later version.
> > > +
> > > +   This library is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +   Lesser General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU Lesser General Public
> > > +   License along with this library; if not, see
> > > .
> > > +*/
> > > +#include "display-channel.h"
> > > +
> > > +DisplayChannelClient *display_channel_client_new(DisplayChannel
> > > *display,
> > > + RedClient *client,
> > > RedsStream *stream,
> > > + int mig_target,
> > > + uint32_t *common_caps,
> > > int
> > > num_common_caps,
> > > + uint32_t *caps, int
> > > num_caps)
> > > +{
> > > +DisplayChannelClient *dcc;
> > > +
> > > +dcc = (DisplayChannelClient*)common_channel_new_client(
> > > +COMMON_CHANNEL(display), sizeof(DisplayChannelClient),
> > > +client, stream, mig_target, TRUE,
> > > +common_caps, num_common_caps,
> > > +caps, num_caps);
> > > +spice_return_val_if_fail(dcc, NULL);
> > > +
> > > +ring_init(>palette_cache_lru);
> > > +dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> > > +
> > > +return dcc;
> > > +}
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index fb38ee2..1b38932 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -15,15 +15,50 @@
> > > You should have received a copy of the GNU Lesser General Public
> > > License along with this library; if not, see
> > > .
> > >  */
> > > -#ifndef RED_WORKER_CLIENT_H_
> > > -# define RED_WORKER_CLIENT_H_
> > > +#ifndef DISPLAY_CHANNEL_H_
> > > +# define DISPLAY_CHANNEL_H_
> > > +
> > > +#include 
> > >  
> > >  #include "red_worker.h"
> > > +#include "reds_stream.h"
> > >  #include "cache-item.h"
> > >  #include "pixmap-cache.h"
> > > +#ifdef USE_OPENGL
> > > +#include "common/ogl_ctx.h"
> > > +#include "reds_gl_canvas.h"
> > > +#endif /* USE_OPENGL */
> > > +#include "reds_sw_canvas.h"
> > > +#include "glz_encoder_dictionary.h"
> > > +#include "glz_encoder.h"
> > > +#include "stat.h"
> > > +#include "reds.h"
> > > +#include "mjpeg_encoder.h"
> > > +#include "red_memslots.h"
> > > +#include "red_parse_qxl.h"
> > > +#include "red_record_qxl.h"
> > > +#include "jpeg_encoder.h"
> > > +#ifdef USE_LZ4
> > > +#include "lz4_encoder.h"
> > > +#endif
> > > +#include "demarshallers.h"
> > > +#include "zlib_encoder.h"
> > > +#include "red_channel.h"
> > > +#include "red_dispatcher.h"
> > > +#include "dispatcher.h"
> > > +#include "main_channel.h"
> > > +#include "migration_protocol.h"
> > > +#include "main_dispatcher.h"
> > > +#include "spice_server_utils.h"
> > > +#include "red_time.h"
> > > +#include "spice_bitmap_utils.h"
> > > +#include "spice_image_cache.h"
> > > +
> > >  
> > >  typedef int64_t red_time_t;
> > >  
> > > +typedef struct DisplayChannel DisplayChannel;
> > > +
> > >  typedef struct Drawable Drawable;
> 

Re: [Spice-devel] [PATCH 11/11] worker: remove assertion on alloc_drawable

[Frediano Ziglio]

> 
> On Fri, 2015-10-30 at 13:23 -0400, Frediano Ziglio wrote:
> > > 
> > > On Thu, Oct 29, 2015 at 04:24:27PM -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > From: Marc-André Lureau 
> > > > > 
> > > > > There is no guarantee in the code that this can't be hit, so we
> > > > > should
> > > > > cope with it (the condition can be reached easily by running
> > > > > the server
> > > > > without waiting for blocked clients or pipe size)
> > > > > 
> > > > > The following commit will attempt to address this.
> > > > > ---
> > > > >  server/red_worker.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > > index 0542b29..ca7e8bf 100644
> > > > > --- a/server/red_worker.c
> > > > > +++ b/server/red_worker.c
> > > > > @@ -3300,7 +3300,8 @@ static void free_one_drawable(RedWorker
> > > > > *worker,
> > > > > int
> > > > > force_glz_free)
> > > > >  Drawable *drawable;
> > > > >  Container *container;
> > > > >  
> > > > > -spice_assert(ring_item);
> > > > > +if (!ring_item)
> > > > > +return;
> > > > >  drawable = SPICE_CONTAINEROF(ring_item, Drawable,
> > > > > list_link);
> > > > >  if (force_glz_free) {
> > > > >  RingItem *glz_item, *next_item;
> > > > > --
> > > > > 2.4.3
> > > > > 
> > > > 
> > > > The change make sense. The function is called from different
> > > > places and not
> > > > in all path
> > > > we are sure worker->current_list is not empty.
> > > > 
> > > > Style note: looks like we prefer always bracket like
> > > > 
> > > > if (!ring_item) {
> > > > return;
> > > > }
> > > > 
> > > > Beside the style note I would ack the patch.
> > > 
> > > Brackets would probably be more consistent yes, ack too.
> > > 
> > > Christophe
> > > 
> > > 
> > 
> > Pushed
> > 
> > Frediano
> > 
> 
> Hmm, you pushed already, but it seems that the commit summary doesn't
> match the change (alloc_drawable() vs free_one_drawable())
> 
> Oh well...
> 
> 

I think I should add this to my checklist before pushing

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