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

[Srivardhan]

> -----Original Message-----
> From: wayland-devel [mailto:wayland-devel-
> bounces at lists.freedesktop.org] On Behalf Of Pekka Paalanen
> Sent: Monday, May 12, 2014 2:48 PM
> To: Bryce W. Harrington
> Cc: Srivardhan Hebbar; wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH V2] event: assert wl_event_source pointer is NULL.
> 
> 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.ht
> > ml
> >
> > 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.
> 

Sorry... My bad... I went through the assert now. I replaced the if with
assert!!

Yes... There are many functions in the library which dereference pointers
without checking NULL... 

I read the Kristian mail after sending the patch. We can ignore this I feel.

Thank-you,
Hebbar

> 
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel