[Spice-devel] [PATCH 05/10] worker: move red_destroy_surface_item()

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> ---
>  server/display-channel.c | 29 ++++++++++++++++++++++++++++
>  server/display-channel.h | 19 ++++++++++++++++++
>  server/red_worker.c      | 50
>  +-----------------------------------------------
>  3 files changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 3df6a31..359b0bf 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -222,6 +222,35 @@ void dcc_push_monitors_config(DisplayChannelClient *dcc)
>      red_channel_client_push(&dcc->common.base);
>  }
>  
> +static SurfaceDestroyItem *surface_destroy_item_new(RedChannel *channel,
> +                                                    uint32_t surface_id)
> +{
> +    SurfaceDestroyItem *destroy;
> +
> +    destroy = spice_malloc(sizeof(SurfaceDestroyItem));
> +    destroy->surface_destroy.surface_id = surface_id;
> +    red_channel_pipe_item_init(channel, &destroy->pipe_item,
> +                               PIPE_ITEM_TYPE_DESTROY_SURFACE);
> +
> +    return destroy;
> +}
> +
> +void dcc_push_destroy_surface(DisplayChannelClient *dcc, uint32_t
> surface_id)
> +{
> +    DisplayChannel *display = DCC_TO_DC(dcc);
> +    RedChannel *channel = RED_CHANNEL(display);
> +    SurfaceDestroyItem *destroy;
> +
> +    if (!dcc || COMMON_CHANNEL(display)->during_target_migrate ||
> +        !dcc->surface_client_created[surface_id]) {
> +        return;
> +    }
> +

Although it seems just a move this code introduce some maintaining
problems.
Before dcc was checked for NULL before any possible access.
Now DCC_TO_DC and RED_CHANNEL are used before this check.
This assume that the macro does not dereference the dcc pointer which
is not good to assume. I'll change the code to

   DisplayChannel *display;
   RedChannel *channel;
   SurfaceDestroyItem *destroy;

   if (!dcc) {
       return;
   }

   display = DCC_TO_DC(dcc);
   channel = RED_CHANNEL(display);

   if (COMMON_CHANNEL(display)->during_target_migrate ||
       !dcc->surface_client_created[surface_id]) {
       return;
   }

> +    dcc->surface_client_created[surface_id] = FALSE;
> +    destroy = surface_destroy_item_new(channel, surface_id);
> +    red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc),
> &destroy->pipe_item);
> +}
> +
>  int display_channel_get_streams_timeout(DisplayChannel *display)
>  {
>      int timeout = INT_MAX;
> diff --git a/server/display-channel.h b/server/display-channel.h
> index b92bc5c..254c3d0 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -236,6 +236,8 @@ DisplayChannelClient*      dcc_new
> (DisplayCha
>                                                                        uint32_t
>                                                                        *caps,
>                                                                        int
>                                                                        num_caps);
>  void                       dcc_push_monitors_config
>  (DisplayChannelClient *dcc);
> +void                       dcc_push_destroy_surface
> (DisplayChannelClient *dcc,
> +
> uint32_t
> surface_id);
>  
>  typedef struct DrawablePipeItem {
>      RingItem base;  /* link for a list of pipe items held by Drawable */
> @@ -311,6 +313,23 @@ struct DisplayChannel {
>      stat_info_t lz4_stat;
>  #endif
>  };
> +typedef struct SurfaceDestroyItem {
> +    SpiceMsgSurfaceDestroy surface_destroy;
> +    PipeItem pipe_item;
> +} SurfaceDestroyItem;
> +
> +typedef struct SurfaceCreateItem {
> +    SpiceMsgSurfaceCreate surface_create;
> +    PipeItem pipe_item;
> +} SurfaceCreateItem;
> +
> +typedef struct UpgradeItem {
> +    PipeItem base;
> +    int refs;
> +    Drawable *drawable;
> +    SpiceClipRects *rects;
> +} UpgradeItem;
> +
>  
>  void                       display_channel_set_stream_video
>  (DisplayChannel *display,
>                                                                        int
>                                                                        stream_video);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 93e0efb..34bc85c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -217,16 +217,6 @@ struct SpiceWatch {
>  
>  #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS
>  
> -typedef struct SurfaceCreateItem {
> -    SpiceMsgSurfaceCreate surface_create;
> -    PipeItem pipe_item;
> -} SurfaceCreateItem;
> -
> -typedef struct SurfaceDestroyItem {
> -    SpiceMsgSurfaceDestroy surface_destroy;
> -    PipeItem pipe_item;
> -} SurfaceDestroyItem;
> -
>  #define MAX_PIPE_SIZE 50
>  
>  #define WIDE_CLIENT_ACK_WINDOW 40
> @@ -318,13 +308,6 @@ struct _Drawable {
>      } u;
>  };
>  
> -typedef struct UpgradeItem {
> -    PipeItem base;
> -    int refs;
> -    Drawable *drawable;
> -    SpiceClipRects *rects;
> -} UpgradeItem;
> -
>  typedef struct DrawContext {
>      SpiceCanvas *canvas;
>      int canvas_draws_on_surface;
> @@ -966,37 +949,6 @@ static void drawables_init(RedWorker *worker)
>  }
>  
>  
> -static SurfaceDestroyItem *get_surface_destroy_item(RedChannel *channel,
> -                                                    uint32_t surface_id)
> -{
> -    SurfaceDestroyItem *destroy;
> -
> -    destroy = spice_malloc(sizeof(SurfaceDestroyItem));
> -
> -    destroy->surface_destroy.surface_id = surface_id;
> -
> -    red_channel_pipe_item_init(channel,
> -        &destroy->pipe_item, PIPE_ITEM_TYPE_DESTROY_SURFACE);
> -
> -    return destroy;
> -}
> -
> -static inline void red_destroy_surface_item(RedWorker *worker,
> -    DisplayChannelClient *dcc, uint32_t surface_id)
> -{
> -    SurfaceDestroyItem *destroy;
> -    RedChannel *channel;
> -
> -    if (!dcc || worker->display_channel->common.during_target_migrate ||
> -        !dcc->surface_client_created[surface_id]) {
> -        return;
> -    }
> -    dcc->surface_client_created[surface_id] = FALSE;
> -    channel = RED_CHANNEL(worker->display_channel);
> -    destroy = get_surface_destroy_item(channel, surface_id);
> -    red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc),
> &destroy->pipe_item);
> -}
> -
>  static void stop_streams(DisplayChannel *display)
>  {
>      Ring *ring = &display->streams;
> @@ -1043,7 +995,7 @@ static void red_surface_unref(RedWorker *worker,
> uint32_t surface_id)
>      region_destroy(&surface->draw_dirty_region);
>      surface->context.canvas = NULL;
>      FOREACH_DCC(worker->display_channel, link, next, dcc) {
> -        red_destroy_surface_item(worker, dcc, surface_id);
> +        dcc_push_destroy_surface(dcc, surface_id);
>      }
>  
>      spice_warn_if(!ring_is_empty(&surface->depend_on_me));
> --
> 2.4.3

Frediano