Re: [PATCH 09/15] worker: don't process drawable if it can't be allocated

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

 



On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>
> ---
>  server/red_worker.c | 131 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 69 insertions(+), 62 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 45750f9..d50d4dd 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -3280,15 +3280,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;
> @@ -3302,47 +3303,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++;
>      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;
> +
>      return drawable;
>  }
>
> @@ -3418,24 +3404,41 @@ static inline void red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
>      }
>  }
>
> -static inline void red_process_drawable(RedWorker *worker, RedDrawable *red_drawable,
> -                                        uint32_t group_id)
> +static RedDrawable *red_drawable_new(RedWorker *worker)
>  {
> -    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_drawable(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;
>
> @@ -3444,6 +3447,19 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *red_draw
>          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 (!validate_surface(worker, drawable->surfaces_dest[x]))
> +            goto end;
> +    }
> +
> +    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.
> @@ -3453,19 +3469,19 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *red_draw
>      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;
>      }
>
>      if (red_current_add_qxl(worker, &worker->surfaces[surface_id].current, drawable,
> @@ -3475,8 +3491,15 @@ static inline void red_process_drawable(RedWorker *worker, RedDrawable *red_draw
>          }
>          red_pipes_add_drawable(worker, drawable);
>      }
> -cleanup:
> -    release_drawable(worker, drawable);
> +
> +    success = TRUE;
> +
> +end:
> +    if (drawable != NULL)
> +        release_drawable(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,
> @@ -3972,16 +3995,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;
> @@ -4021,14 +4034,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_drawable(worker, red_drawable, ext_cmd.group_id);
> -            }
> -            // release the red_drawable
> -            put_red_drawable(worker, red_drawable, ext_cmd.group_id);
> +            if (!red_process_drawable(worker, &ext_cmd))
> +                break;
>              break;
>          }
>          case QXL_CMD_UPDATE: {
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

NACK!

Trying to run a RHEL-7.VM and I got:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff901ff700 (LWP 16684)]
release_drawable (worker=worker@entry=0x555557e44000,
drawable=drawable@entry=0x555558005560) at red_worker.c:1246
1246            put_red_drawable(worker, drawable->red_drawable,
drawable->group_id);
(gdb) bt
#0  0x00007ffff7ab73a6 in release_drawable
(worker=worker@entry=0x555557e44000,
drawable=drawable@entry=0x555558005560)
    at red_worker.c:1246
#1  0x00007ffff7abb733 in red_process_commands (drawable=<optimized
out>, worker=0x555557e44000) at red_worker.c:3545
#2  0x00007ffff7abb733 in red_process_commands (ext_cmd=<optimized
out>, worker=<optimized out>) at red_worker.c:3497
#3  0x00007ffff7abb733 in red_process_commands
(worker=worker@entry=0x555557e44000,
ring_is_empty=ring_is_empty@entry=0x7fff901fe980, max_pipe_size=50) at
red_worker.c:4035
#4  0x00007ffff7abebb3 in red_worker_main (arg=0x555557e44000) at
red_worker.c:10652
#5  0x00007ffff686060a in start_thread () at /lib64/libpthread.so.0
#6  0x00007fffe846dbbd in clone () at /lib64/libc.so.6

I will try to fix it in the morning.
_______________________________________________
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]