Hi
On Tue, Nov 10, 2015 at 3:16 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> 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
Please wait until Christophe confirm he wrote this patch. (I bet it
was rather me looking at the style of the patch ;)
> ---
> 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 5daca87..e0d9593 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -3462,16 +3462,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);
> }
> @@ -3506,66 +3517,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
> *******************************************************/
> @@ -3890,9 +3841,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;
> @@ -4152,7 +4101,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) {
> @@ -4173,7 +4122,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);
> @@ -4188,7 +4137,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) {
> @@ -4203,7 +4152,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
> @@ -4211,7 +4160,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;
> }
> @@ -4250,7 +4199,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) {
> @@ -4264,7 +4213,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;
> }
> @@ -4278,7 +4227,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
> @@ -4353,7 +4302,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) {
> @@ -4368,7 +4317,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;
> }
> @@ -4391,7 +4340,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
> @@ -4417,7 +4366,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;
>
> @@ -4553,13 +4502,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;
>
> @@ -4567,7 +4511,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;
> }
> @@ -5047,7 +4991,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));
> @@ -7274,14 +7217,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
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel