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@xxxxxxxxxx> wrote: > > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > > > > > > --- > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel