[Spice-devel] [PATCH 1/7] worker: remove current_add assert

[Christophe Fergeau]

On Thu, Nov 12, 2015 at 10:36:59AM -0500, Frediano Ziglio wrote:
>  
> > On Thu, Nov 12, 2015 at 3:00 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >
> > > ---
> > >  server/red_worker.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 6bc015f..d33a352 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -2254,7 +2254,7 @@ static inline int current_add(RedWorker *worker, Ring
> > > *ring, Drawable *drawable)
> > >      QRegion exclude_rgn;
> > >      RingItem *exclude_base = NULL;
> > >
> > > -    spice_assert(!region_is_empty(&item->base.rgn));
> > > +    spice_return_val_if_fail(!region_is_empty(&item->base.rgn), FALSE);
> > >      region_init(&exclude_rgn);
> > >      now = ring_next(ring, ring);
> > >
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > ACK!
> > 
> 
> On this and 4/7 (asserts and so on)
> 
> There has been some discussion going on (see "spice-server, logging and style"
> on ML)
> - me: I'd like to have some reasoning and comments and not changed silently
>   (this is not hidden but there is no much reasoning).

Agreed that longer log would be needed here.

>   I don't want to continue on memory errors but stop (ie: spice-assert);
> - Marc: prefer to not have assert so to not stop spice-server. I don't agree
>   for memory errors;

Note that Marc-André position and yours are not necessarily mutually
exclusive ;) For this commit, I don't think the change is related to
memory errors, so if we can ignore the unexpected situation rather than
asserting, it's better. We can indeed be extra careful when we detect
inconsistencies in reference counting and assert, we can change it
anytime change it if we realize it's causing more harm than
good.

> - Marc, Christophe: like to have logs. I pointed out that too log cause DoS,

This is not really related to assert VS return, but more whether to use
if() return or g_return_if_fail()

> - Pavel: pointed out that some of these hidden changes are not good (don't
>   remember exactly);
> - spice-server mainly reflect glib macro beside critical is fatal causing
>   return_if macro to abort;
> - somebody suggested to use glib macro directly. But they are not 100% compatible
>   so we decided to not do it now. However Pavel (and I agree with him) does
>   not like to do a change that we need to review later (that is: let's doing once
>   in the right ways);

We indeed cannot do a whole tree
s/spice_return_if_fail/g_return_if_fail/ as there are most likely some
places where it's not going to be safe to do so. However, in my option
we can (and should!) use g_return_if_fail() for new code when we made
sure it's safe.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151119/6cda11e5/attachment.sig>