(resend 2)[2/5]hidclass.sys: Implement a report ring buffer
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 = (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.
More information about the wine-devel