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

[Fabiano Fidêncio]

On Thu, Nov 12, 2015 at 4:36 PM, Frediano Ziglio <fziglio at redhat.com> 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).
>   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;
> - Marc, Christophe: like to have logs. I pointed out that too log cause DoS,
>   Christophe sent a link that Qemu guys are worried of logs too;
> - 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);
> - Uri is mainly (this is a big sum up!) suggesting to use spice-assert for
>   internal conditions and something not aborting for others. This patch is against
>   this rule as item are created by spice-server.
>
> Surely I have forgot something or blame somebody for stuff others said.
>
> I think is better to move these changes to later time when we have more
> clearer idea. I'm not sure however we all agree so I posted the patches anyway.

Okay. So, let's skip the patches that are changing only changing the
asserts (and try to split the assert changes from the other patches)
at least till we have a final conclusion.


>
> Frediano