[Spice-devel] [PATCH 1/2] server: display_channel: releasing pipe items resources when the pipe is cleared (on disconnect)

Alon Levy alevy at redhat.com
Tue Jul 5 07:18:00 PDT 2011


On Tue, Jul 05, 2011 at 03:50:40PM +0300, Yonit Halperin wrote:
> fixes "display_channel_release_item: panic: invalid item type"
> 

ACK all.

> Before changing the red_worker to use the red_channel interface, there
> was a devoted red_pipe_clear routine for the display channel and cursor channel.
> However, clearing the pipe in red_channel, uses the release_item callback
> the worker provided. This callback has handled only resources that need to be released
> after the pipe item was enqueued from the pipe, and only for pipe items that were set in
> red_channel_init_send_data.
> This fix changes the display channel release_item callback to handle all types of
> pipe items, and also handles differently pushed and non-pushed pipe items.
> ---
>  server/red_worker.c |   99 ++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 75 insertions(+), 24 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index c0a9760..fc4326c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -934,6 +934,8 @@ static BitmapGradualType _get_bitmap_graduality_level(RedWorker *worker, SpiceBi
>  static inline int _stride_is_extra(SpiceBitmap *bitmap);
>  static void red_disconnect_cursor(RedChannel *channel);
>  static void red_wait_pipe_item_sent(RedChannel *channel, PipeItem *item);
> +static void display_channel_release_item_before_push(DisplayChannel *display_channel, PipeItem *item);
> +static void display_channel_release_item_after_push(DisplayChannel *display_channel, PipeItem *item);
>  
>  #ifdef DUMP_BITMAP
>  static void dump_bitmap(RedWorker *worker, SpiceBitmap *bitmap, uint32_t group_id);
> @@ -8012,89 +8014,75 @@ static void display_channel_send_item(RedChannel *base, PipeItem *pipe_item)
>  {
>      SpiceMarshaller *m = red_channel_get_marshaller(base);
>      DisplayChannel *display_channel = (DisplayChannel *)red_ref_channel(base);
> -    RedWorker *worker = display_channel->common.worker;
>  
>      red_display_reset_send_data(display_channel);
>      switch (pipe_item->type) {
>      case PIPE_ITEM_TYPE_DRAW: {
>          Drawable *drawable = SPICE_CONTAINEROF(pipe_item, Drawable, pipe_item);
>          marshall_qxl_drawable(display_channel, m, drawable);
> -        release_drawable(worker, drawable);
>          break;
>      }
>      case PIPE_ITEM_TYPE_INVAL_ONE:
>          red_display_marshall_inval(display_channel, m, (CacheItem *)pipe_item);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_STREAM_CREATE: {
>          StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, create_item);
>          red_display_marshall_stream_start(display_channel, m, agent);
> -        red_display_release_stream(display_channel, agent);
>          break;
>      }
>      case PIPE_ITEM_TYPE_STREAM_CLIP: {
> -        StreamClipItem* clip_item = (StreamClipItem *)pipe_item;
> -        red_display_marshall_stream_clip(display_channel, m, clip_item);
> -        red_display_release_stream_clip(display_channel, clip_item);
> +        red_display_marshall_stream_clip(display_channel, m, (StreamClipItem *)pipe_item);
>          break;
>      }
>      case PIPE_ITEM_TYPE_STREAM_DESTROY: {
>          StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, destroy_item);
>          red_display_marshall_stream_end(display_channel, m, agent);
> -        red_display_release_stream(display_channel, agent);
>          break;
>      }
>      case PIPE_ITEM_TYPE_UPGRADE:
>          red_display_marshall_upgrade(display_channel, m, (UpgradeItem *)pipe_item);
> -        release_upgrade_item(worker, (UpgradeItem *)pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_VERB:
>          display_marshall_verb(display_channel, ((VerbItem*)pipe_item)->verb);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_MIGRATE:
>          red_printf("PIPE_ITEM_TYPE_MIGRATE");
>          display_channel_marshall_migrate(display_channel, m);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_MIGRATE_DATA:
>          display_channel_marshall_migrate_data(display_channel, m);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_IMAGE:
>          red_marshall_image(display_channel, m, (ImageItem *)pipe_item);
> -        release_image_item((ImageItem *)pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_PIXMAP_SYNC:
>          display_channel_marshall_pixmap_sync(display_channel, m);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_PIXMAP_RESET:
>          display_channel_marshall_reset_cache(display_channel, m);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_INVAL_PALLET_CACHE:
>          red_reset_palette_cache(display_channel);
>          red_marshall_verb((RedChannel *)display_channel, SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES);
> -        free(pipe_item);
>          break;
>      case PIPE_ITEM_TYPE_CREATE_SURFACE: {
>          SurfaceCreateItem *surface_create = SPICE_CONTAINEROF(pipe_item, SurfaceCreateItem,
>                                                                pipe_item);
>          red_marshall_surface_create(display_channel, m, &surface_create->surface_create);
> -        free(surface_create);
>          break;
>      }
>      case PIPE_ITEM_TYPE_DESTROY_SURFACE: {
>          SurfaceDestroyItem *surface_destroy = SPICE_CONTAINEROF(pipe_item, SurfaceDestroyItem,
>                                                                  pipe_item);
>          red_marshall_surface_destroy(display_channel, m, surface_destroy->surface_destroy.surface_id);
> -        free(surface_destroy);
>          break;
>      }
>      default:
>          red_error("invalid pipe item type");
>      }
> +
> +    display_channel_release_item_before_push(display_channel, pipe_item);
> +
>      // a message is pending
>      if (red_channel_send_message_pending(&display_channel->common.base)) {
>          display_begin_send_message(display_channel, m);
> @@ -9082,21 +9070,20 @@ static void display_channel_hold_pipe_item(RedChannel *channel, PipeItem *item)
>      }
>  }
>  
> -static void display_channel_release_item(RedChannel *channel, PipeItem *item, int item_pushed /* ignored */)
> +static void display_channel_release_item_after_push(DisplayChannel *display_channel, PipeItem *item)
>  {
> -    CommonChannel *common = SPICE_CONTAINEROF(channel, CommonChannel, base);
> +    RedWorker *worker = display_channel->common.worker;
>  
> -    ASSERT(item);
>      switch (item->type) {
>      case PIPE_ITEM_TYPE_DRAW:
>      case PIPE_ITEM_TYPE_STREAM_CREATE:
> -        release_drawable(common->worker, SPICE_CONTAINEROF(item, Drawable, pipe_item));
> +        release_drawable(worker, SPICE_CONTAINEROF(item, Drawable, pipe_item));
>          break;
>      case PIPE_ITEM_TYPE_STREAM_CLIP:
> -        red_display_release_stream_clip((DisplayChannel *)channel, (StreamClipItem *)item);
> +        red_display_release_stream_clip(display_channel, (StreamClipItem *)item);
>          break;
>      case PIPE_ITEM_TYPE_UPGRADE:
> -        release_upgrade_item(common->worker, (UpgradeItem *)item);
> +        release_upgrade_item(worker, (UpgradeItem *)item);
>          break;
>      case PIPE_ITEM_TYPE_IMAGE:
>          release_image_item((ImageItem *)item);
> @@ -9106,6 +9093,70 @@ static void display_channel_release_item(RedChannel *channel, PipeItem *item, in
>      }
>  }
>  
> +static void display_channel_release_item_before_push(DisplayChannel *display_channel, PipeItem *item)
> +{
> +    RedWorker *worker = display_channel->common.worker;
> +
> +    switch (item->type) {
> +    case PIPE_ITEM_TYPE_DRAW:
> +        release_drawable(worker, SPICE_CONTAINEROF(item, Drawable, pipe_item));
> +        break;
> +    case PIPE_ITEM_TYPE_STREAM_CREATE: {
> +        StreamAgent *agent = SPICE_CONTAINEROF(item, StreamAgent, create_item);
> +        red_display_release_stream(display_channel, agent);
> +        break;
> +    }
> +    case PIPE_ITEM_TYPE_STREAM_CLIP:
> +        red_display_release_stream_clip(display_channel, (StreamClipItem *)item);
> +        break;
> +    case PIPE_ITEM_TYPE_STREAM_DESTROY: {
> +        StreamAgent *agent = SPICE_CONTAINEROF(item, StreamAgent, destroy_item);
> +        red_display_release_stream(display_channel, agent);
> +        break;
> +    }
> +    case PIPE_ITEM_TYPE_UPGRADE:
> +        release_upgrade_item(worker, (UpgradeItem *)item);
> +        break;
> +    case PIPE_ITEM_TYPE_IMAGE:
> +        release_image_item((ImageItem *)item);
> +        break;
> +    case PIPE_ITEM_TYPE_CREATE_SURFACE: {
> +        SurfaceCreateItem *surface_create = SPICE_CONTAINEROF(item, SurfaceCreateItem,
> +                                                              pipe_item);
> +        free(surface_create);
> +        break;
> +    }
> +    case PIPE_ITEM_TYPE_DESTROY_SURFACE: {
> +        SurfaceDestroyItem *surface_destroy = SPICE_CONTAINEROF(item, SurfaceDestroyItem,
> +                                                                pipe_item);
> +        free(surface_destroy);
> +        break;
> +    }
> +    case PIPE_ITEM_TYPE_INVAL_ONE:
> +    case PIPE_ITEM_TYPE_VERB:
> +    case PIPE_ITEM_TYPE_MIGRATE:
> +    case PIPE_ITEM_TYPE_MIGRATE_DATA:
> +    case PIPE_ITEM_TYPE_PIXMAP_SYNC:
> +    case PIPE_ITEM_TYPE_PIXMAP_RESET:
> +    case PIPE_ITEM_TYPE_INVAL_PALLET_CACHE:
> +        free(item);
> +        break;
> +    default:
> +        PANIC("invalid item type");
> +    }
> +}
> +
> +static void display_channel_release_item(RedChannel *channel, PipeItem *item, int item_pushed)
> +{
> +    ASSERT(item);
> +    if (item_pushed) {
> +        display_channel_release_item_after_push((DisplayChannel *)channel, item);
> +    } else {
> +        red_printf("not pushed");
> +        display_channel_release_item_before_push((DisplayChannel *)channel, item);
> +    }
> +}
> +
>  static void handle_new_display_channel(RedWorker *worker, RedsStream *stream, int migrate)
>  {
>      DisplayChannel *display_channel;
> -- 
> 1.7.4.4
> 


More information about the Spice-devel mailing list