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

[Jeremy Huddleston]

On Oct 17, 2011, at 4:58 PM, Peter Hutterer wrote:

>> +    if (new_nevents <= eventQueue->nevents)
>> +        return FALSE;
>> +
>> +    if (!eventQueue) {
>> +        ErrorF("[mi] mieqGrowQueue called with a NULL eventQueue\n");
>> +        return FALSE;
>> +    }
> 
> this condition needs to be swapped with the one above it, otherwise you get
> a NULL dereference

><

>> +
>> +    if (CountBits((const uint8_t *)&new_nevents, sizeof(new_nevents) != 1)) {
> 
> this looks like a misplacement of a parens. Plus, this requires a comment as
> for the allowed values for nevents. Just add some documentation to
> mieqGrowQueue(), it would make reading the code a bit easier.

Yeah, it's not a requirement.  I'm just nuking that line.

>> +    }
>> +
>> +    if (eventQueue->nevents) {
>> +        /* % is not well-defined with negative numbers... sigh */
>> +        n_enqueued = miEventQueue.tail - miEventQueue.head + eventQueue->nevents;
> 
> shouldn't this be using eventQueue?

>< Yes.  I missed that one, thanks.

>> +           &eventQueue->events[miEventQueue.head],
> 
> shouldn't this be using eventQueue?

And that one.

> _please_ write a test. It's a self-contained function and these
> types of errors are found very easily with a few simple for loops and
> assert.

Yeah, I will.

>> +    if ((miEventQueue.tail - miEventQueue.head) % miEventQueue.nevents >= (miEventQueue.nevents >> 1) &&
> 
> Wouldn't this suffer from the same issue regarding % on negative numbers?

Yeah, that one was already fixed after Keith's comments, but I didn't send an updated patch yet.

> also, please put comments in here. I had to stare at this for too long to
> understand what you're doing here. a simple "grow queue when we have more
> than half the queue size queued up" would have made this a bit easier.

Ok, I'll spruce it up a bit.

> (I also don't know why >> 1 is preferable to /2. Not really critical path
> here anyway and /2 or * 2 would be much more obvious to read)

Habbit.