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

[Kristian Høgsberg]

On Mon, May 12, 2014 at 08:31:14AM +0000, Bryce W. Harrington 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);
> 
> 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.
> 
> Also, looking through event-loop.c there's many other routines that take
> a pointer argument to a struct and reference members without first
> checking the pointer's validity.  So I'm gathering that this code
> expects source's validity to be verified before calling into
> wl_event_source_remove()?  In which case, there's no need for a NULL
> pointer check.  (Or, conversely, all the incoming pointers in
> event-loop.c need checked.)

Yes, we expect the input to be non-NULL, will crash the caller if they
pass a NULL (or otherwise invalid) pointer.  Similar to strdup, for example.

> I'd hazard a guess that the reason the functions in event-loop.c don't
> validate their inputs is for performance reasons.

Not really, it's not a hotpath at all, it's just redundant.  Of course,
asserts are always redundant, that's the point, but we don't generally use
many asserts in the code.

Kristian

> >  	/* We need to explicitly remove the fd, since closing the fd
> >  	 * isn't enough in case we've dup'ed the fd. */
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel