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 <[email protected]> > > --- > > 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 > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
