(resend 2)[2/5]hidclass.sys: Implement a report ring buffer

Aric Stewart aric at codeweavers.com
Mon Jul 13 07:13:25 CDT 2015



On 7/12/15 2:50 PM, Henri Verbeet wrote:
> On 10 July 2015 at 21:39, Aric Stewart <aric at codeweavers.com> wrote:
>> +typedef struct __ReportRingBuffer
> Did you borrow this from somewhere? Identifiers starting with double
> underscore are reserved. (And using a single underscore instead isn't
> much better.) The naming is also inconsistent with the existing code.

No, I wrote it all. I did not write it directly integrated. I wrote it with an eye just to its own internal constancy.  The renaming of the struct like that was recommended to me by another developer. I can change it.

> And I have to say that goes for much of the rest of the series as
> well. There seems to be no consistent style from one patch to the
> next, and sometimes within the same structure. (E.g. __WINE_ELEMENT,
> __WINE_HID_REPORT, etc.) And of course most of this patch is unused
> code until patch 5/5 when RingBuffer_Create() is actually called.
> 

Yeah, the trick partly is there is a lot of ground work to do. Yes, most of these patches are not used until later patches. But I need to build that ground work so that the later patches work and it is not all introduced in one mammoth patch. 


Clearly I need to install a style checker for myself.

>> +struct __ReportRingBuffer* RingBuffer_Create(UINT buffer_size)
>> +{
>> +    ReportRingBuffer *ring;
>> +    ring = HeapAlloc(GetProcessHeap(), 0, sizeof(*ring));
>> +    if (!ring)
>> +        return NULL;
>> +    ring->start = ring->end = 0;
>> +    ring->size = 32;
>> +    ring->buffer_size = buffer_size;
>> +    ring->pointer_alloc = 5;
>> +    ring->pointers = HeapAlloc(GetProcessHeap(), 0, sizeof(int) * 5);
>> +    memset(ring->pointers, 0xff, sizeof(int) * 5);
>> +    ring->buffer = HeapAlloc(GetProcessHeap(), 0, buffer_size * ring->size);
>> +    InitializeCriticalSection(&ring->lock);
>> +    ring->lock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": RingBuffer.lock");
>> +    return ring;
>> +}
> Why handle allocation failure for "ring", but not for "ring->pointers"
> and "ring->buffer"? Why did you pick "5" for the initial size of
> "ring->pointers"? Seems like an odd choice.
> 

Clearly this code needs this review. Thank you for helping.

5 was picked arbitrarily.  I guess 2 or any other number would be just fine. 

>> +VOID RingBuffer_Destroy(struct __ReportRingBuffer *ring)
> Why use "VOID"? Are you concerned "void" may not be available?
> 

No, this is mostly because I did try to standardize my code a bit to use the VOID identifier instead of the void identifier, so clearly made the wrong choice there.

>> +    if (size < 2 || size > 256 || size == ring->size)
>> +        return;
> What's special about 2 and 256? And if this does nothing, why would
> you ever call the function like that?
> 

Those are limits on the ring buffer stated in MSDN for HID ring buffers. 
IOCTL_SET_NUM_DEVICE_INPUT_BUFFERS sets that value. 

>> +    new_buffer = HeapAlloc(GetProcessHeap(), 0, ring->buffer_size * size);
>> +    HeapFree(GetProcessHeap(), 0, ring->buffer);
>> +    ring->buffer = new_buffer;
> Why bother with "new_buffer" if you're not going to handle failures?

Because I should be handling failures and clearly I missed that.

-aric



More information about the wine-devel mailing list