>
> On Fri, Nov 20, 2015 at 12:17 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel