[PATCH] Add generic event cookie handling to libX11. (v3)

[Peter Hutterer]

On Wed, Jul 08, 2009 at 03:10:59PM +0100, Julien Cristau wrote:
> a couple questions below.

thanks for the review! I'm skipping some hunks in this reply for brevity's
sake. The updated patch is below.

> > diff --git a/include/X11/Xlib.h b/include/X11/Xlib.h
> > index 682988c..2e735ee 100644
> > --- a/include/X11/Xlib.h
> > +++ b/include/X11/Xlib.h
> > @@ -966,6 +966,17 @@ typedef struct
> >      int            evtype;       /* actual event type. */
> >      } XGenericEvent;
> >  
> > +typedef struct {
> > +    int            type;         /* of event. Always GenericEvent */
> > +    unsigned long  serial;       /* # of last request processed */
> > +    Bool           send_event;   /* true if from SendEvent request */
> > +    Display        *display;     /* Display the event was read from */
> > +    int            extension;    /* major opcode of extension that caused the event */
> > +    int            evtype;       /* actual event type. */
> > +    unsigned int   cookie;
> > +    void           *data;
> > +} XGenericEventCookie;
> > +
> 
> Any reason this (and XGenericEvent) isn't added to the XEvent union?
> That would avoid a bunch of casts later on.

amended.

> >  #endif /* _XLIB_H_ */
> > diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h
> > index 4f3755f..2acfc76 100644
> > --- a/include/X11/Xlibint.h
> > +++ b/include/X11/Xlibint.h
> > @@ -185,6 +185,20 @@ struct _XDisplay
> >  	struct _XkbInfoRec *xkb_info; /* XKB info */
> >  	struct _XtransConnInfo *trans_conn; /* transport connection object */
> >  	struct _X11XCBPrivate *xcb; /* XCB glue private data */
> > +
> > +	/* Generic event cookie handling */
> > +	unsigned int next_cookie; /* next event cookie */
> 
> what happens when next_cookie wraps?
> (... thinking about it, i guess if you keep 2^32 events around you have
> other issues)

could be an interesting thing to trigger. it essentially means that you must
wait for all these events to arrive in your local EQ and then get a cookie
clash before calling XNextEvent.
This could potentially be mitigated by assigning the cookie when the event
is returned to the client instead of when it arrives locally. Whether this
is a scenario that we need to worry about is questionable though.

[skipping hunks]

> > diff --git a/man/XGetEventData.man b/man/XGetEventData.man
> > new file mode 100644
> > index 0000000..e1d9c74
> > --- /dev/null
> > +++ b/man/XGetEventData.man
[skipping bits of man page]
> > +.SH NOTES
> > +A cookie is defined as unclaimed if it has been returned to the client
> > +through
> > +.ZN XNextEvent
> > +but its data has not been retrieved via
> > +.ZN XGetEventData .
> > +Subsequent calls to
> > +.ZN XNextEvent
> > +may free memory associated with unclaimed cookies.
> > +Multi-threaded X clients must ensure that
> > +.ZN XGetEventData
> > +is called before the next call to
> > +.ZN XNextEvent .
> > +
> > +.SH "SEE ALSO"
> > +XNextEvent(3X11),
> 
> __libmansuffix__

no, actually. all man pages have the direct reference in the SEE ALSO
section. I'm happy to provide a follow-up patch to switch the others over in
one go though.

> > +.br
> > +\fI\*(xL\fP
> > +

[skipping]

> > diff --git a/src/XlibInt.c b/src/XlibInt.c
> > index 320e808..57d04dd 100644
> > --- a/src/XlibInt.c
> > +++ b/src/XlibInt.c
[skipping]
> > @@ -2258,6 +2258,157 @@ void _XEatData(
> > +/**
> > + * Add an event to the display's event list. This event must be freed on the
> > + * next call to XNextEvent().
> > + */
> > +void
> > +_XStoreEventCookie(Display *dpy, XEvent *event)
> > +{
> > +    Bool found = False;
> > +    XGenericEventCookie* cookie = (XGenericEventCookie*)event;
> > +    struct stored_event **head, *add;
> > +
> > +    if (!_XIsEventCookie(dpy, event))
> > +        return;
> > +
> > +    head = (struct stored_event**)(&dpy->cookiejar);
> > +
> > +    /* protect against duplicates (multiple XPeekEvents) */
> > +    DL_FOREACH(*head, add) {
> > +        if (add->ev.cookie == cookie->cookie &&
> > +            add->ev.extension == cookie->extension &&
> > +            add->ev.evtype == cookie->evtype) {
> > +            found = True;
> > +            return;
> 
> should this be break instead of return?  otherwise 'found' seems
> useless.

come to think of it, this check was pointless with v3 anyway so it didn't
matter. We don't need to protect against duplicates since events are copied
for XPeekEvent and in XPutBackEvent. The same goes for the data = NULL
assignment in _XFetchEventCookie, it's not necessary anymore. both removed.

Thanks again for the comments, the updated patch is below.

Changes to previous version:
- unneeded typecast in debug printf removed. (_XUnknownCopyEventCookie)
- XGenericEvent and XGenericEventCookie added to XEvent union and follow-up
  removal of typecasts.
- unnecessary duplicate check removed from _XStoreEventCookie.
- unecessary data = NULL assigment removed from _XFetchEventCookie.
  
Cheers,
  Peter