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

[Lennart Poettering]

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...

> +
> +        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...

> + * 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...

Lennart

-- 
Lennart Poettering, Red Hat