Re: (resend 2)[2/5]hidclass.sys: Implement a report ring buffer
On 10 July 2015 at 21:39, Aric Stewart <aric(a)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. 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.
+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.
+VOID RingBuffer_Destroy(struct __ReportRingBuffer *ring) Why use "VOID"? Are you concerned "void" may not be available?
+ 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?
+ 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?
On 7/12/15 2:50 PM, Henri Verbeet wrote:
On 10 July 2015 at 21:39, Aric Stewart <aric(a)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
On 13 July 2015 at 14:13, Aric Stewart <aric(a)codeweavers.com> wrote:
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.
Yeah, it can be a bit tricky to split things like that in a useful way. At the same time, it's important to be able to see how things are called by other code.
5 was picked arbitrarily. I guess 2 or any other number would be just fine.
It mostly depends on how many readers/writers you expect in the common case, but typically you'd use some power of two for these kinds of dynamically growing arrays. Of course if you already know in advance that there will only ever be a fixed number you can simplify things.
+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.
Right, you'll probably to add a #define or something for those then instead of just having magic constants. It's hard to judge without the code using this, but it may be the case that this is better enforced in the caller. Depending on how RingBuffer_SetSize() is going to be used, it may also be the case that you could just as well just destroy the entire ringbuffer and recreate it.
participants (2)
-
Aric Stewart -
Henri Verbeet