[Spice-devel] [PATCH 09/15] worker: don't process drawable if it can't be allocated
Frediano Ziglio
fziglio at redhat.com
Wed Nov 4 06:39:00 PST 2015
>
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>
> ---
> server/red_worker.c | 136
> ++++++++++++++++++++++++++++------------------------
> 1 file changed, 73 insertions(+), 63 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 9697532..5f1bbaf 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -1179,7 +1179,9 @@ static void red_worker_drawable_unref(RedWorker
> *worker, Drawable *drawable)
> SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable =
> NULL;
> ring_remove(item);
> }
> - put_red_drawable(worker, drawable->red_drawable, drawable->group_id);
> + if (drawable->red_drawable) {
> + put_red_drawable(worker, drawable->red_drawable,
> drawable->group_id);
> + }
> free_drawable(worker, drawable);
> worker->drawable_count--;
> }
This piece of change is mine. Is basically allow to free a structure not
entirely initialized. This happen if some validation in the middle fails.
Could be the code can be rewritten in order to avoid this check.
> @@ -3213,15 +3215,16 @@ static inline int red_handle_self_bitmap(RedWorker
> *worker, Drawable *drawable)
> return TRUE;
> }
>
> -static void free_one_drawable(RedWorker *worker, int force_glz_free)
> +static bool free_one_drawable(RedWorker *worker, int force_glz_free)
> {
> RingItem *ring_item = ring_get_tail(&worker->current_list);
> Drawable *drawable;
> Container *container;
>
> if (!ring_item) {
> - return;
> + return FALSE;
> }
> +
> drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> if (force_glz_free) {
> RingItem *glz_item, *next_item;
> @@ -3235,47 +3238,32 @@ static void free_one_drawable(RedWorker *worker, int
> force_glz_free)
>
> current_remove_drawable(worker, drawable);
> container_cleanup(worker, container);
> + return TRUE;
> }
>
> -static Drawable *get_drawable(RedWorker *worker, uint8_t effect, RedDrawable
> *red_drawable,
> - uint32_t group_id)
> +static Drawable *drawable_try_new(RedWorker *worker, int group_id)
> {
> Drawable *drawable;
> - int x;
> -
> - VALIDATE_SURFACE_RETVAL(worker, red_drawable->surface_id, NULL)
> - if (!validate_drawable_bbox(worker, red_drawable)) {
> - rendering_incorrect(__func__);
> - return NULL;
> - }
> - for (x = 0; x < 3; ++x) {
> - if (red_drawable->surfaces_dest[x] != -1) {
> - VALIDATE_SURFACE_RETVAL(worker, red_drawable->surfaces_dest[x],
> NULL)
> - }
> - }
>
> while (!(drawable = alloc_drawable(worker))) {
> - free_one_drawable(worker, FALSE);
> + if (!free_one_drawable(worker, FALSE))
> + return NULL;
> }
> - worker->drawable_count++;
> - memset(drawable, 0, sizeof(Drawable));
> +
> + bzero(drawable, sizeof(Drawable));
> drawable->refs = 1;
> + worker->drawable_count++;
Just style. Mainly all spice-server code uses memset so I won't use bzero.
> drawable->creation_time = red_get_monotonic_time();
> ring_item_init(&drawable->list_link);
> ring_item_init(&drawable->surface_list_link);
> ring_item_init(&drawable->tree_item.base.siblings_link);
> drawable->tree_item.base.type = TREE_ITEM_TYPE_DRAWABLE;
> region_init(&drawable->tree_item.base.rgn);
> - drawable->tree_item.effect = effect;
> - drawable->red_drawable = ref_red_drawable(red_drawable);
> - drawable->group_id = group_id;
> -
> - drawable->surface_id = red_drawable->surface_id;
> - memcpy(drawable->surfaces_dest, red_drawable->surfaces_dest,
> sizeof(drawable->surfaces_dest));
> ring_init(&drawable->pipes);
> ring_init(&drawable->glz_ring);
> -
> drawable->process_commands_generation =
> worker->process_commands_generation;
> + drawable->group_id = group_id;
This line can be moved in the original place.
> +
> return drawable;
> }
>
> @@ -3351,24 +3339,41 @@ static inline void
> red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
> }
> }
>
> -static inline void red_process_draw(RedWorker *worker, RedDrawable
> *red_drawable,
> - uint32_t group_id)
> +static RedDrawable *red_drawable_new(RedWorker *worker)
This function was moved, move it back to reduce patch.
> {
> - int surface_id;
> - Drawable *drawable = get_drawable(worker, red_drawable->effect,
> red_drawable, group_id);
> + RedDrawable * red = spice_new0(RedDrawable, 1);
>
> - if (!drawable) {
> - rendering_incorrect("failed to get_drawable");
> - return;
> - }
> + red->refs = 1;
> + worker->red_drawable_count++;
>
> - red_drawable->mm_time = reds_get_mm_time();
> - surface_id = drawable->surface_id;
> + return red;
> +}
>
> - worker->surfaces[surface_id].refs++;
> +static gboolean red_process_draw(RedWorker *worker, QXLCommandExt *ext_cmd)
> +{
> + RedDrawable *red_drawable = NULL;
> + Drawable *drawable = NULL;
> + int surface_id, x;
> + gboolean success = FALSE;
>
> - region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
> + drawable = drawable_try_new(worker, ext_cmd->group_id);
> + if (!drawable)
> + goto end;
> +
> + red_drawable = red_drawable_new(worker);
> + if (red_get_drawable(&worker->mem_slots, ext_cmd->group_id,
> + red_drawable, ext_cmd->cmd.data, ext_cmd->flags) !=
> 0)
> + goto end;
> +
> + if (!validate_drawable_bbox(worker, red_drawable)) {
> + rendering_incorrect(__func__);
> + goto end;
> + }
>
> + drawable->tree_item.effect = red_drawable->effect;
> + drawable->red_drawable = ref_red_drawable(red_drawable);
> + drawable->surface_id = red_drawable->surface_id;
> + region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
> if (red_drawable->clip.type == SPICE_CLIP_TYPE_RECTS) {
> QRegion rgn;
>
> @@ -3377,6 +3382,20 @@ static inline void red_process_draw(RedWorker *worker,
> RedDrawable *red_drawable
> region_and(&drawable->tree_item.base.rgn, &rgn);
> region_destroy(&rgn);
> }
> +
> + if (!validate_surface(worker, drawable->surface_id))
> + goto end;
> + for (x = 0; x < 3; ++x) {
> + drawable->surfaces_dest[x] = red_drawable->surfaces_dest[x];
> + if (drawable->surfaces_dest[x] != -1
> + && !validate_surface(worker, drawable->surfaces_dest[x]))
> + goto end;
> + }
> +
I changed this if checking for -1. This make the patch work again.
> + red_drawable->mm_time = reds_get_mm_time();
> + surface_id = drawable->surface_id;
> + worker->surfaces[surface_id].refs++;
> +
> /*
> surface->refs is affected by a drawable (that is
> dependent on the surface) as long as the drawable is alive.
> @@ -3386,19 +3405,19 @@ static inline void red_process_draw(RedWorker
> *worker, RedDrawable *red_drawable
> red_inc_surfaces_drawable_dependencies(worker, drawable);
>
> if (region_is_empty(&drawable->tree_item.base.rgn)) {
> - goto cleanup;
> + goto end;
> }
>
> if (!red_handle_self_bitmap(worker, drawable)) {
> - goto cleanup;
> + goto end;
> }
>
> if (!red_handle_depends_on_target_surface(worker, surface_id)) {
> - goto cleanup;
> + goto end;
> }
>
> if (!red_handle_surfaces_dependencies(worker, drawable)) {
> - goto cleanup;
> + goto end;
> }
>
I don't see the reason why changing label name.
> if (red_current_add_qxl(worker, &worker->surfaces[surface_id].current,
> drawable,
> @@ -3408,8 +3427,15 @@ static inline void red_process_draw(RedWorker *worker,
> RedDrawable *red_drawable
> }
> red_pipes_add_drawable(worker, drawable);
> }
> -cleanup:
> - red_worker_drawable_unref(worker, drawable);
> +
> + success = TRUE;
> +
> +end:
> + if (drawable != NULL)
> + red_worker_drawable_unref(worker, drawable);
> + if (red_drawable != NULL)
> + put_red_drawable(worker, red_drawable, ext_cmd->group_id);
> + return success;
> }
>
> static inline void red_create_surface(RedWorker *worker, uint32_t
> surface_id,uint32_t width,
> @@ -3900,16 +3926,6 @@ static int red_process_cursor(RedWorker *worker,
> uint32_t max_pipe_size, int *ri
> return n;
> }
>
> -static RedDrawable *red_drawable_new(RedWorker *worker)
> -{
> - RedDrawable * red = spice_new0(RedDrawable, 1);
> -
> - red->refs = 1;
> - worker->red_drawable_count++;
> -
> - return red;
> -}
> -
> static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size,
> int *ring_is_empty)
> {
> QXLCommandExt ext_cmd;
> @@ -3949,14 +3965,8 @@ static int red_process_commands(RedWorker *worker,
> uint32_t max_pipe_size, int *
> worker->display_poll_tries = 0;
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_DRAW: {
> - RedDrawable *red_drawable = red_drawable_new(worker); // returns
> with 1 ref
> -
> - if (!red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
> - red_drawable, ext_cmd.cmd.data,
> ext_cmd.flags)) {
> - red_process_draw(worker, red_drawable, ext_cmd.group_id);
> - }
> - // release the red_drawable
> - put_red_drawable(worker, red_drawable, ext_cmd.group_id);
> + if (!red_process_draw(worker, &ext_cmd))
> + break;
> break;
This is a
if (cond)
break;
break;
which is the same as
cond;
break;
so function can be still be void, no reason to change to gboolean.
> }
> case QXL_CMD_UPDATE: {
> --
> 2.4.3
Looks like to me this patch is bigger than it should be.
I understand the reasoning, implementation looks to change too much stuff.
I'll try to rewrite it.
Frediano
More information about the Spice-devel
mailing list