[PATCH 1/2 v4] mieq: Provide better adaptability and diagnostics during mieq overflow
Keith Packard
keithp at keithp.com
Sun Oct 16 21:44:03 PDT 2011
On Sun, 16 Oct 2011 21:16:09 -0700, Jeremy Huddleston <jeremyhu at apple.com> wrote:
> + /* We block signals, so SIGIO does trigger mieqEnqueue to write to the
> + * queue as we're modifying it.
> + */
> + OsBlockSignals();
this comment is incorrect; the goal is to avoid having SIGIO come in and
smash the queue.
> + /* First copy the existing events */
> + memcpy(new_events, &eventQueue->events[miEventQueue.head],
> + (eventQueue->nevents - eventQueue->head) * sizeof(EventRec));
> + memcpy(&new_events[eventQueue->nevents - eventQueue->head], eventQueue->events,
> + (eventQueue->head) * sizeof(EventRec));
Tricky, but I think it's right. Might be easier to read
if each argument to memcpy were on a separate line?
+ memcpy(new_events,
+ &eventQueue->events[miEventQueue.head],
+ (eventQueue->nevents - eventQueue->head) * sizeof(EventRec));
+
+ memcpy(&new_events[eventQueue->nevents - eventQueue->head],
+ eventQueue->events,
+ (eventQueue->head) * sizeof(EventRec));
You could give a name to 'nevents - head'? Not because the compiler
won't figure it out, but just because it might be clearer how the new
event queue is getting constructed.
> +
> + /* Initialize the new portion */
> + for (i = eventQueue->nevents; i < new_nevents; i++) {
> + InternalEvent* evlist = InitEventList(1);
> + if (!evlist) {
> + free(new_events);
> + OsReleaseSignals();
> + return FALSE;
> + }
You'll have to free all of the InternalEvents allocated up to the point
of failure.
> - miEventQueue.head = miEventQueue.tail = 0;
> + memset(&miEventQueue, 0, sizeof(miEventQueue));
Looks like you're also fixing potential bugs with uninitialized values
across server reset; that seems like a different change which isn't
directly related to this fix. I'd post that change first and layer your
resize stuff on top of it.
Oh. One big problem -- mieqEnqueue can be called from a signal handler,
in which case calling 'malloc' is a big no-no. I suspect you'll need to
make this whole mess conditional for systems which have credible
threaded input.
--
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111016/2bc161b4/attachment.pgp>
More information about the xorg-devel
mailing list