[PATCH 2/4] X event queue mutex
Peter Hutterer
peter.hutterer at who-t.net
Mon Nov 3 21:06:49 PST 2008
Sorry for the late reply, I was tied up and missed the mail.
On Thu, Oct 23, 2008 at 02:47:30PM -0700, Jeremy Huddleston wrote:
> And here's a stab at setting up mieqProcessInputEvents in master to be
> more friendly towards this locking. master doesn't work for us on OSX,
> so I can't really verify that it works... I may have missed an e-> to e.
> or e->events[i]->event to event[i] somewhere...
>
> This also fixes what I think is a bug in the custom event handler...
> right now we have:
>
> handler(DequeueScreen(e->pDev)->myNum, e->events->event,
> e->pDev, e->nevents);
> if (!e->pDev->isMaster && e->pDev->u.master) {
> handler(DequeueScreen(e->pDev->u.master)->myNum,
> e->events->event, e->pDev->u.master, e->nevents);
> }
>
> But that fails if e->nevents > 1 ... granted all current custom event
> handlers have nevents=1, but it's worth noting.
with the move of the handler procesing (dac9e918) this should now be.
handler(.., event, ...);
if (!e->pDev->isMaster && ...)
handler(..., master_event, ...)
otherwise we're not protected against event manipulation in the handler.
> Also, why do we call the handler twice here?
Only one event is in the queue, but we need to process it twice, once as
coming from the slave device, once as coming from the master device. These two
are independent.
- e = &miEventQueue.events[miEventQueue.head];
+ memcpy(&e, &miEventQueue.events[miEventQueue.head], sizeof(EventRec));
if we're not using the EventRec as such anyway, it might just be better to
extract the things we need and improve readability.
+ event = xcalloc(e.nevents, sizeof(xEvent));
that doesn't work. GenericEvents may be larger than xEvent, hence the slightly
weird copying code. What you are doing here corrupts memory after the first
device switch. How about this one here, quick test shows it works:
>From b783944ac784c42ba84b3dfe77d60107b9581a30 Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter.hutterer at redhat.com>
Date: Tue, 4 Nov 2008 15:27:30 +1030
Subject: [PATCH] mi: clean up mieqProcessInputEvents, copy all events before processing.
Copy the EventRec's information into local variables before processing them,
this should make it safer for upcoming threading and also makes it easier to
read.
Simplify the event allocation code from the abyss it was before.
This also fixes a potential bug where a custom handler could scramble the
event before the same -now scrambled- event was then passed through the
master's custom event handler.
---
mi/mieq.c | 95 +++++++++++++++++++++++++++---------------------------------
1 files changed, 43 insertions(+), 52 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index 986e3a1..ec11e00 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -307,8 +307,12 @@ mieqProcessInputEvents(void)
mieqHandler handler;
EventRec *e = NULL;
int x = 0, y = 0;
- xEvent* event,
- *master_event = NULL;
+ int type, nevents, evlen, i;
+ ScreenPtr screen;
+ xEvent *event,
+ *master_event = NULL;
+ DeviceIntPtr dev = NULL,
+ master = NULL;
while (miEventQueue.head != miEventQueue.tail) {
if (screenIsSaved == SCREEN_SAVER_ON)
@@ -324,77 +328,64 @@ mieqProcessInputEvents(void)
e = &miEventQueue.events[miEventQueue.head];
miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
+ dev = e->pDev;
+ master = (!dev->isMaster && dev->u.master) ? dev->u.master : NULL;
+ type = e->events->event->u.u.type;
+ nevents = e->nevents;
+ screen = e->pScreen;
+
+ /* GenericEvents always have nevents == 1 */
+ evlen = (nevents > 1) ? sizeof(xEvent) : e->events->evlen;
+ event = xcalloc(nevents, evlen);
+
+ if (!event)
+ FatalError("[mi] No memory left for event processing.\n");
+
+ for (i = 0; i < nevents; i++)
+ memcpy(&event[i], e->events[i].event, evlen);
+
/* Custom event handler */
- handler = miEventQueue.handlers[e->events->event->u.u.type];
+ handler = miEventQueue.handlers[type];
- if (e->pScreen != DequeueScreen(e->pDev) && !handler) {
+ if (screen != DequeueScreen(dev) && !handler) {
/* Assumption - screen switching can only occur on motion events. */
- DequeueScreen(e->pDev) = e->pScreen;
- x = e->events[0].event->u.keyButtonPointer.rootX;
- y = e->events[0].event->u.keyButtonPointer.rootY;
- NewCurrentScreen (e->pDev, DequeueScreen(e->pDev), x, y);
+ DequeueScreen(dev) = screen;
+ x = event->u.keyButtonPointer.rootX;
+ y = event->u.keyButtonPointer.rootY;
+ NewCurrentScreen (dev, DequeueScreen(dev), x, y);
}
else {
- /* FIXME: Bad hack. The only event where we actually get multiple
- * events at once is a DeviceMotionNotify followed by
- * DeviceValuators. For now it's safe enough to just take the
- * event directly or copy the bunch of events and pass in the
- * copy. Eventually the interface for the processInputProc needs
- * to be changed. (whot)
- */
- if (e->nevents > 1)
- {
- int i;
- event = xcalloc(e->nevents, sizeof(xEvent));
- if (!event)
- FatalError("[mi] No memory left for event processing.\n");
- for (i = 0; i < e->nevents; i++)
- {
- memcpy(&event[i], e->events[i].event, sizeof(xEvent));
- }
- } else
- event = e->events->event;
- if (!e->pDev->isMaster && e->pDev->u.master)
- {
- CopyGetMasterEvent(e->pDev->u.master, event,
- &master_event, e->nevents);
- } else
+ if (master)
+ CopyGetMasterEvent(master, event,
+ &master_event, nevents);
+ else
master_event = NULL;
/* If someone's registered a custom event handler, let them
* steal it. */
if (handler)
{
- handler(DequeueScreen(e->pDev)->myNum, e->events->event,
- e->pDev, e->nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
- {
- handler(DequeueScreen(e->pDev->u.master)->myNum,
- e->events->event, e->pDev->u.master, e->nevents);
- }
+ handler(DequeueScreen(dev)->myNum, event, dev, nevents);
+ if (master)
+ handler(DequeueScreen(master)->myNum,
+ master_event, master, nevents);
} else
{
/* process slave first, then master */
- e->pDev->public.processInputProc(event, e->pDev, e->nevents);
+ dev->public.processInputProc(event, dev, nevents);
- if (!e->pDev->isMaster && e->pDev->u.master)
- {
- e->pDev->u.master->public.processInputProc(master_event,
- e->pDev->u.master, e->nevents);
- }
+ if (master)
+ master->public.processInputProc(master_event, master,
+ nevents);
}
- if (e->nevents > 1)
- xfree(event);
+ xfree(event);
xfree(master_event);
}
/* Update the sprite now. Next event may be from different device. */
- if (e->events->event[0].u.u.type == DeviceMotionNotify
- && e->pDev->coreEvents)
- {
- miPointerUpdateSprite(e->pDev);
- }
+ if (type == DeviceMotionNotify && dev->coreEvents)
+ miPointerUpdateSprite(dev);
}
}
--
1.6.0.3
More information about the xorg
mailing list