[systemd-devel] [RFC 02/12] ring: add basic ring-buffer helper

[David Herrmann]

Hi

On Wed, Nov 27, 2013 at 10:53 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Wed, 27.11.13 19:48, David Herrmann (dh.herrmann at gmail.com) wrote:
>
>> +void ring_flush(Ring *r) {
>> +        r->start = 0;
>> +        r->end = 0;
>> +}
>> +
>> +void ring_clear(Ring *r) {
>> +        free(r->buf);
>> +        memset(r, 0, sizeof(*r));
>
> zero(*r) is a tiny bit nicer.
>
>> +}
>> +
>> +/*
>> + * Resize ring-buffer to size @nsize. @nsize must be a power-of-2, otherwise
>> + * ring operations will behave incorrectly.
>> + */
>> +static int ring_resize(Ring *r, size_t nsize) {
>> +        char *buf;
>
> Hmm, "char" suggests a bit that this is about text. But it's mostly raw
> bytes, right? So I'd always use uint8_t for these things, it feels so
> much "rawer"... Not that it would matter much...

Yepp, uint8_t it is.

>> +
>> +        buf = malloc(nsize);
>> +        if (!buf)
>> +                return -ENOMEM;
>> +
>> +        if (r->end == r->start) {
>> +                r->end = 0;
>> +                r->start = 0;
>
> Hmm, so if end and start match the buffer is empty? So you can never
> fill it entirely? Or am I missing something?
>
> I'd always maintain start index + fill level instead of a start
> index + end index, to avoid the confusion regarding the fill-level...

Well, start+end is how ring-buffers were implemented historically. I
think the reason is that "wrapping over" is easier to calculate for
indices than for lengths (you cannot do: r->len = (r->len + 1) &
RING_MASK). Even all examples on wikipedia use two indices
(http://en.wikipedia.org/wiki/Circular_buffer). And all kernel
ring-buffers use start/end..

Also note that the one special byte is not "unused". It cannot be
filled, in case the buffer is full, that's right. But it's used during
normal buffer-operation if the occupied space "moves along" the
buffer.

If you can find me circular/ring-buffers that use start+len, I can fix
that up. Otherwise, I'd rather keep this close to existing
implementations. Besides, a lot of "wrapping calculations" get easier
if start/end are restricted by RING_MASK, which wouldn't be the case
for "len".

>> + * Push @len bytes from @u8 into the ring buffer. The buffer is resized if it
>> + * is too small. -ENOMEM is returned on OOM, 0 on success.
>> + */
>> +int ring_push(Ring *r, const char *u8, size_t len) {
>
> So, here you call the parameter u8 suggesting unsigned bytes, but you
> use char, which indicates a text string, and is
> signed... Confusing. I'd recommend using "const void *" here by default,
> since all types implicitly downgrade to "const void*" without casting...

I usually use "u8" for "utf8".. no idea where that came from. But yes,
void* makes sense for input/output and uint8_t* for internal
operations.

Thanks
David