[PATCH V2] event: assert wl_event_source pointer is NULL.

[Pekka Paalanen]

On Mon, 12 May 2014 08:31:14 +0000
"Bryce W. Harrington" <b.harrington at samsung.com> wrote:

> On Mon, May 12, 2014 at 11:26:01AM +0530, Srivardhan Hebbar wrote:
> > Signed-off-by: Srivardhan Hebbar <sri.hebbar at samsung.com>
> > ---
> >  src/event-loop.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/event-loop.c b/src/event-loop.c
> > index 9790cde..57e3fed 100644
> > --- a/src/event-loop.c
> > +++ b/src/event-loop.c
> > @@ -312,7 +312,11 @@ wl_event_source_check(struct wl_event_source *source)
> >  WL_EXPORT int
> >  wl_event_source_remove(struct wl_event_source *source)
> >  {
> > -	struct wl_event_loop *loop = source->loop;
> > +	struct wl_event_loop *loop;
> > +
> > +	assert(!source);

Like already pointed out, both the condition in assert and the patch
subject are negated of what they should be. I suppose this was never
actually tested.

> While I notice event-loop.c does include assert.h, there are no other
> asserts in this file; most other calls are returning NULL or -1 on error
> conditions, which I'm guessing might be better?  Especially for cleanup
> routines, it's not uncommon to return success when the provided object
> is already null or otherwise doesn't need cleanup.  Generally in library
> you want to bubble errors up for the library user to decide how to
> handle rather than exiting or asserting.

For the reference:
http://lists.freedesktop.org/archives/wayland-devel/2014-May/014786.html

A guaranteed crash or abort is good in this case.


Thanks,
pq