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

[Pekka Paalanen]

On Mon, 12 May 2014 11:49:46 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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.

Or, well, if someone wants to take it all the way, there would be an
assert plus a failure path with logging to avoid the crash on NDEBUG
builds.

Here's a wacky idea:

#define ASSERT_FAILED(x) ({ \
	assert(x); \
	if (!(x)) { \
		wl_log("Assertion failure in %s:%d: '%s'\n", \
		       __FILE__, __LINE__, #x); \
		1; \
	} else { \
		0; \
	} \
})

To be used like:

int
myfunc(struct foo *p)
{
	if (ASSERT_FAILED(p))
		return -99;

	...
}

Something like that, you get the idea. I wonder if that'd be for the
better or worse... I mean I don't know if Kristian would like this kind
of code.


Thanks,
pq