[PATCH 1/2 v4] mieq: Provide better adaptability and diagnostics during mieq overflow

[Keith Packard]

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>