Re: [PATCH 13/15] worker: simplify RedCompressBuf

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



hi

Although I don't remember writing this, it seems git history attributes this to me. I guess a "based on initial patch from" could be useful some day.


----- Original Message -----
> From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> 
> Make sure an allocated buffer is correctly referenced by the marshaller,
> and can't be free and reused by mistake. Simplify the code by using
> GSlice
> ---
>  server/display-channel.h |   3 --
>  server/red_worker.c      | 117
>  ++++++++++++-----------------------------------
>  2 files changed, 28 insertions(+), 92 deletions(-)
> 
> diff --git a/server/display-channel.h b/server/display-channel.h
> index f1f4e3a..f45860c 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -73,7 +73,6 @@
>  typedef struct RedCompressBuf RedCompressBuf;
>  struct RedCompressBuf {
>      uint8_t buf[64 * 1024];
> -    RedCompressBuf *next;
>      RedCompressBuf *send_next;
>  };
>  
> @@ -179,8 +178,6 @@ struct DisplayChannelClient {
>          uint32_t stream_outbuf_size;
>          uint8_t *stream_outbuf; // caution stream buffer is also used as
>          compress bufs!!!
>  
> -        RedCompressBuf *used_compress_bufs;
> -
>          FreeList free_list;
>          uint64_t pixmap_cache_items[MAX_DRAWABLE_PIXMAP_CACHE_ITEMS];
>          int num_pixmap_cache_items;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 83e0fa0..1c99f01 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -3464,16 +3464,27 @@ static void
> red_push_surface_image(DisplayChannelClient *dcc, int surface_id)
>      red_channel_client_push(RED_CHANNEL_CLIENT(dcc));
>  }
>  
> +static RedCompressBuf *compress_buf_new(void)
> +{
> +    return g_slice_new(RedCompressBuf);
> +}
> +
> +static void compress_buf_free(RedCompressBuf *buf)
> +{
> +    g_slice_free(RedCompressBuf, buf);
> +}
> +
>  static void marshaller_add_compressed(SpiceMarshaller *m,
>                                        RedCompressBuf *comp_buf, size_t size)
>  {
>      size_t max = size;
>      size_t now;
>      do {
> -        spice_assert(comp_buf);
> +        spice_return_if_fail(comp_buf);
>          now = MIN(sizeof(comp_buf->buf), max);
>          max -= now;
> -        spice_marshaller_add_ref(m, (uint8_t*)comp_buf->buf, now);
> +        spice_marshaller_add_ref_full(m, (uint8_t*)comp_buf->buf, now,
> +
> (spice_marshaller_item_free_func)compress_buf_free,
> NULL);
>          comp_buf = comp_buf->send_next;
>      } while (max);
>  }
> @@ -3508,66 +3519,6 @@ static inline void fill_palette(DisplayChannelClient
> *dcc,
>      }
>  }
>  
> -static inline RedCompressBuf
> *red_display_alloc_compress_buf(DisplayChannelClient *dcc)
> -{
> -    DisplayChannel *display_channel = DCC_TO_DC(dcc);
> -    RedCompressBuf *ret;
> -
> -    if (display_channel->free_compress_bufs) {
> -        ret = display_channel->free_compress_bufs;
> -        display_channel->free_compress_bufs = ret->next;
> -    } else {
> -        ret = spice_new(RedCompressBuf, 1);
> -    }
> -
> -    ret->next = dcc->send_data.used_compress_bufs;
> -    dcc->send_data.used_compress_bufs = ret;
> -    return ret;
> -}
> -
> -static inline void __red_display_free_compress_buf(DisplayChannel *dc,
> -                                                   RedCompressBuf *buf)
> -{
> -    buf->next = dc->free_compress_bufs;
> -    dc->free_compress_bufs = buf;
> -}
> -
> -static void red_display_free_compress_buf(DisplayChannelClient *dcc,
> -                                          RedCompressBuf *buf)
> -{
> -    DisplayChannel *display_channel = DCC_TO_DC(dcc);
> -    RedCompressBuf **curr_used = &dcc->send_data.used_compress_bufs;
> -
> -    for (;;) {
> -        spice_assert(*curr_used);
> -        if (*curr_used == buf) {
> -            *curr_used = buf->next;
> -            break;
> -        }
> -        curr_used = &(*curr_used)->next;
> -    }
> -    __red_display_free_compress_buf(display_channel, buf);
> -}
> -
> -static void red_display_reset_compress_buf(DisplayChannelClient *dcc)
> -{
> -    while (dcc->send_data.used_compress_bufs) {
> -        RedCompressBuf *buf = dcc->send_data.used_compress_bufs;
> -        dcc->send_data.used_compress_bufs = buf->next;
> -        __red_display_free_compress_buf(DCC_TO_DC(dcc), buf);
> -    }
> -}
> -
> -static void red_display_destroy_compress_bufs(DisplayChannel
> *display_channel)
> -{
> -    spice_assert(!red_channel_is_connected(RED_CHANNEL(display_channel)));
> -    while (display_channel->free_compress_bufs) {
> -        RedCompressBuf *buf = display_channel->free_compress_bufs;
> -        display_channel->free_compress_bufs = buf->next;
> -        free(buf);
> -    }
> -}
> -
>  /******************************************************
>   *      Global lz red drawables routines
>  *******************************************************/
> @@ -3892,9 +3843,7 @@ static int encoder_usr_more_space(EncoderData
> *enc_data, uint8_t **io_ptr)
>  {
>      RedCompressBuf *buf;
>  
> -    if (!(buf = red_display_alloc_compress_buf(enc_data->dcc))) {
> -        return 0;
> -    }
> +    buf = compress_buf_new();
>      enc_data->bufs_tail->send_next = buf;
>      enc_data->bufs_tail = buf;
>      buf->send_next = NULL;
> @@ -4154,7 +4103,7 @@ static inline int
> red_glz_compress_image(DisplayChannelClient *dcc,
>      int glz_size;
>      int zlib_size;
>  
> -    glz_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> +    glz_data->data.bufs_tail = compress_buf_new();
>      glz_data->data.bufs_head = glz_data->data.bufs_tail;
>  
>      if (!glz_data->data.bufs_head) {
> @@ -4175,7 +4124,7 @@ static inline int
> red_glz_compress_image(DisplayChannelClient *dcc,
>  
>      glz_size = glz_encode(dcc->glz, type, src->x, src->y,
>                            (src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN), NULL,
>                            0,
> -                          src->stride,
> (uint8_t*)glz_data->data.bufs_head->buf,
> +                          src->stride, glz_data->data.bufs_head->buf,
>                            sizeof(glz_data->data.bufs_head->buf),
>                            glz_drawable_instance,
>                            &glz_drawable_instance->glz_instance);
> @@ -4190,7 +4139,7 @@ static inline int
> red_glz_compress_image(DisplayChannelClient *dcc,
>  #endif
>      zlib_data = &worker->zlib_data;
>  
> -    zlib_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> +    zlib_data->data.bufs_tail = compress_buf_new();
>      zlib_data->data.bufs_head = zlib_data->data.bufs_tail;
>  
>      if (!zlib_data->data.bufs_head) {
> @@ -4205,7 +4154,7 @@ static inline int
> red_glz_compress_image(DisplayChannelClient *dcc,
>      zlib_data->data.u.compressed_data.size_left = glz_size;
>  
>      zlib_size = zlib_encode(worker->zlib, display_channel->zlib_level,
> -                            glz_size,
> (uint8_t*)zlib_data->data.bufs_head->buf,
> +                            glz_size, zlib_data->data.bufs_head->buf,
>                              sizeof(zlib_data->data.bufs_head->buf));
>  
>      // the compressed buffer is bigger than the original data
> @@ -4213,7 +4162,7 @@ static inline int
> red_glz_compress_image(DisplayChannelClient *dcc,
>          while (zlib_data->data.bufs_head) {
>              RedCompressBuf *buf = zlib_data->data.bufs_head;
>              zlib_data->data.bufs_head = buf->send_next;
> -            red_display_free_compress_buf(dcc, buf);
> +            compress_buf_free(buf);
>          }
>          goto glz;
>      }
> @@ -4252,7 +4201,7 @@ static inline int
> red_lz_compress_image(DisplayChannelClient *dcc,
>      stat_time_t start_time = stat_now(worker);
>  #endif
>  
> -    lz_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> +    lz_data->data.bufs_tail = compress_buf_new();
>      lz_data->data.bufs_head = lz_data->data.bufs_tail;
>  
>      if (!lz_data->data.bufs_head) {
> @@ -4266,7 +4215,7 @@ static inline int
> red_lz_compress_image(DisplayChannelClient *dcc,
>          while (lz_data->data.bufs_head) {
>              RedCompressBuf *buf = lz_data->data.bufs_head;
>              lz_data->data.bufs_head = buf->send_next;
> -            red_display_free_compress_buf(dcc, buf);
> +            compress_buf_free(buf);
>          }
>          return FALSE;
>      }
> @@ -4280,7 +4229,7 @@ static inline int
> red_lz_compress_image(DisplayChannelClient *dcc,
>      size = lz_encode(lz, type, src->x, src->y,
>                       !!(src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN),
>                       NULL, 0, src->stride,
> -                     (uint8_t*)lz_data->data.bufs_head->buf,
> +                     lz_data->data.bufs_head->buf,
>                       sizeof(lz_data->data.bufs_head->buf));
>  
>      // the compressed buffer is bigger than the original data
> @@ -4355,7 +4304,7 @@ static int red_jpeg_compress_image(DisplayChannelClient
> *dcc, SpiceImage *dest,
>          return FALSE;
>      }
>  
> -    jpeg_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> +    jpeg_data->data.bufs_tail = compress_buf_new();
>      jpeg_data->data.bufs_head = jpeg_data->data.bufs_tail;
>  
>      if (!jpeg_data->data.bufs_head) {
> @@ -4370,7 +4319,7 @@ static int red_jpeg_compress_image(DisplayChannelClient
> *dcc, SpiceImage *dest,
>          while (jpeg_data->data.bufs_head) {
>              RedCompressBuf *buf = jpeg_data->data.bufs_head;
>              jpeg_data->data.bufs_head = buf->send_next;
> -            red_display_free_compress_buf(dcc, buf);
> +            compress_buf_free(buf);
>          }
>          return FALSE;
>      }
> @@ -4393,7 +4342,7 @@ static int red_jpeg_compress_image(DisplayChannelClient
> *dcc, SpiceImage *dest,
>      }
>      jpeg_size = jpeg_encode(jpeg, display_channel->jpeg_quality,
>      jpeg_in_type,
>                              src->x, src->y, NULL,
> -                            0, stride,
> (uint8_t*)jpeg_data->data.bufs_head->buf,
> +                            0, stride, jpeg_data->data.bufs_head->buf,
>                              sizeof(jpeg_data->data.bufs_head->buf));
>  
>      // the compressed buffer is bigger than the original data
> @@ -4419,7 +4368,7 @@ static int red_jpeg_compress_image(DisplayChannelClient
> *dcc, SpiceImage *dest,
>  
>      comp_head_filled = jpeg_size % sizeof(lz_data->data.bufs_head->buf);
>      comp_head_left = sizeof(lz_data->data.bufs_head->buf) -
>      comp_head_filled;
> -    lz_out_start_byte = ((uint8_t *)lz_data->data.bufs_head->buf) +
> comp_head_filled;
> +    lz_out_start_byte = lz_data->data.bufs_head->buf + comp_head_filled;
>  
>      lz_data->data.dcc = dcc;
>  
> @@ -4555,13 +4504,8 @@ static inline int
> red_quic_compress_image(DisplayChannelClient *dcc, SpiceImage
>          return FALSE;
>      }
>  
> -    quic_data->data.bufs_tail = red_display_alloc_compress_buf(dcc);
> +    quic_data->data.bufs_tail = compress_buf_new();
>      quic_data->data.bufs_head = quic_data->data.bufs_tail;
> -
> -    if (!quic_data->data.bufs_head) {
> -        return FALSE;
> -    }
> -
>      quic_data->data.bufs_head->send_next = NULL;
>      quic_data->data.dcc = dcc;
>  
> @@ -4569,7 +4513,7 @@ static inline int
> red_quic_compress_image(DisplayChannelClient *dcc, SpiceImage
>          while (quic_data->data.bufs_head) {
>              RedCompressBuf *buf = quic_data->data.bufs_head;
>              quic_data->data.bufs_head = buf->send_next;
> -            red_display_free_compress_buf(dcc, buf);
> +            compress_buf_free(buf);
>          }
>          return FALSE;
>      }
> @@ -5049,7 +4993,6 @@ static void fill_attr(SpiceMarshaller *m, SpiceLineAttr
> *attr, uint32_t group_id
>  
>  static inline void red_display_reset_send_data(DisplayChannelClient *dcc)
>  {
> -    red_display_reset_compress_buf(dcc);
>      dcc->send_data.free_list.res->count = 0;
>      dcc->send_data.num_pixmap_cache_items = 0;
>      memset(dcc->send_data.free_list.sync, 0,
>      sizeof(dcc->send_data.free_list.sync));
> @@ -7276,14 +7219,10 @@ static void
> display_channel_client_on_disconnect(RedChannelClient *rcc)
>      red_release_glz(dcc);
>      red_reset_palette_cache(dcc);
>      free(dcc->send_data.stream_outbuf);
> -    red_display_reset_compress_buf(dcc);
>      free(dcc->send_data.free_list.res);
>      dcc_destroy_stream_agents(dcc);
>  
>      // this was the last channel client
> -    if (!red_channel_is_connected(rcc->channel)) {
> -        red_display_destroy_compress_bufs(display);
> -    }
>      spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d",
>                  display->drawable_count, worker->red_drawable_count,
>                  worker->glz_drawable_count);
> --
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]