[PATCH 1/2 v4] mieq: Provide better adaptability and diagnostics during mieq overflow
Jeremy Huddleston
jeremyhu at apple.com
Sun Oct 16 21:56:43 PDT 2011
On Oct 16, 2011, at 9:44 PM, Keith Packard wrote:
> 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.
Yeah, that's essentially what I meant. Perhaps staring at code too long has made my English suffer.
> 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.
n_events_at_tail ? I'm not really sure what to call it.
>
>> +
>> + /* 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.
Yeah...
>
>> - 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.
It's not really any different. I'm just doing an optimization there... Rather than just setting everything to 0 individually, just do one big memset.
> Oh. One big problem -- mieqEnqueue can be called from a signal handler,
> in which case calling 'malloc' is a big no-no.
malloc isn't being called from within mieqEnqueue. The resize is being done in mieqProcessInputEvents.
> I suspect you'll need to
> make this whole mess conditional for systems which have credible
> threaded input.
No, we just try to resize in mieqProcessInputEvents *before* it fills up.
More information about the xorg-devel
mailing list