[Spice-devel] [PATCH 03/18] worker: improve some pre-conditions

[Frediano Ziglio]

> 
> On Fri, Nov 20, 2015 at 12:17 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> >
> > ---
> >  server/red_worker.c | 35 ++---------------------------------
> >  1 file changed, 2 insertions(+), 33 deletions(-)
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 6e859fe..f6dfe28 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -223,8 +223,8 @@ void drawable_pipe_item_unref(DrawablePipeItem *dpi)
> >          return;
> >      }
> >
> > -    spice_warn_if_fail(!ring_item_is_linked(&dpi->dpi_pipe_item.link));
> > -    spice_warn_if_fail(!ring_item_is_linked(&dpi->base));
> > +    spice_return_if_fail(!ring_item_is_linked(&dpi->dpi_pipe_item.link));
> > +    spice_return_if_fail(!ring_item_is_linked(&dpi->base));
> 
> Again changing the spice_warn_if_fail() for spice_return_if_fail().
> 

I prefer to post the patch more or less unchanged (beside rebase and some style)
for authorship considerations.
Actually in this case is a bit different than usual.
Usually assert are changed to spice_return.
In this case conditions leads to small leaks.
Personally in this case I would prefer an assert. The item is still attached to
the list to surely there will be some issues.
Another safety code would be to unlink if linked and give a warning.

But these solutions/improvements would require a different patch.

> >      display_channel_drawable_unref(display, dpi->drawable);
> >      free(dpi);
> >  }
> > @@ -236,33 +236,6 @@ QXLInstance* red_worker_get_qxl(RedWorker *worker)
> >      return worker->qxl;
> >  }
> >
> > -static int validate_drawable_bbox(RedWorker *worker, RedDrawable
> > *drawable)
> > -{
> > -        DrawContext *context;
> > -        uint32_t surface_id = drawable->surface_id;
> > -
> > -        /* surface_id must be validated before calling into
> > -         * validate_drawable_bbox
> > -         */
> > -        VALIDATE_SURFACE_RETVAL(worker->display_channel, surface_id,
> > FALSE);
> > -        context = &worker->display_channel->surfaces[surface_id].context;
> > -
> > -        if (drawable->bbox.top < 0)
> > -                return FALSE;
> > -        if (drawable->bbox.left < 0)
> > -                return FALSE;
> > -        if (drawable->bbox.bottom < 0)
> > -                return FALSE;
> > -        if (drawable->bbox.right < 0)
> > -                return FALSE;
> > -        if (drawable->bbox.bottom > context->height)
> > -                return FALSE;
> > -        if (drawable->bbox.right > context->width)
> > -                return FALSE;
> > -
> > -        return TRUE;
> > -}
> > -
> >  static inline int validate_surface(DisplayChannel *display, uint32_t
> >  surface_id)
> >  {
> >      if SPICE_UNLIKELY(surface_id >= display->n_surfaces) {
> > @@ -1098,10 +1071,6 @@ static Drawable *get_drawable(RedWorker *worker,
> > uint8_t effect, RedDrawable *re
> >      int x;
> >
> >      VALIDATE_SURFACE_RETVAL(display, red_drawable->surface_id, NULL)
> > -    if (!validate_drawable_bbox(worker, red_drawable)) {
> > -        rendering_incorrect(__func__);
> > -        return NULL;
> > -    }
> 
> I don't understand how we are improving the pre-conditions removing
> the validate_drawable_bbox().
> 
> >      for (x = 0; x < 3; ++x) {
> >          if (red_drawable->surface_deps[x] != -1) {
> >              VALIDATE_SURFACE_RETVAL(display,
> >              red_drawable->surface_deps[x], NULL)
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> NACK  from me.
> --
> Fabiano Fidêncio
> 

Also Christophe commented out (in the spreadsheet)

"revert of upstream e270edcbfd9, could be a bad rebase conflict resolution"

Could be there were some rebase problems, this area was changed a lot (surface
checks) but even old patches have this hunk.

I agree to NACK this.

Frediano