Re: [PATCH 3/7] Add buffer pool library

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Ram,

Ramkumar Ramachandra wrote:

> line_buffer creates a couple of static buffers and expose an API for
> using them.

So this provides a thread-unsafe fgets() and fread() where the caller
does not have to supply a buffer.  Sounds convenient.

> Taken directly
> from David Michael Barr's svn-dump-fast-export repository.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Missing From: line and sign-off.

[...]
> +char *buffer_read_line(void)
> +{
> +    char *end;

style nitpick: use tabs to indent.

> +    uint32_t n_read;
> +
> +    if (line_len) {
> +        memmove(line_buffer, &line_buffer[line_len],
> +                line_buffer_len - line_len);
> +        line_buffer_len -= line_len;
> +        line_len = 0;
> +    }
> +
> +    end = memchr(line_buffer, '\n', line_buffer_len);
> +    while (line_buffer_len < LINE_BUFFER_LEN - 1 &&
> +           !feof(stdin) && NULL == end) {
> +        n_read =
> +            fread(&line_buffer[line_buffer_len], 1,
> +                  LINE_BUFFER_LEN - 1 - line_buffer_len,
> +                  stdin);
> +        end = memchr(&line_buffer[line_buffer_len], '\n', n_read);
> +        line_buffer_len += n_read;
> +    }

Why not fgets()?

[...]
> +char *buffer_read_string(uint32_t len)
> +{
> +    char *s = malloc(len + 1);
> +    uint32_t offset = 0;
> +    if (line_buffer_len > line_len) {
> +        offset = line_buffer_len - line_len;
> +        if (offset > len)
> +            offset = len;
> +        memcpy(s, &line_buffer[line_len], offset);

So if this buffer library is in use, all input needs to pass through
it?  I would prefer to avoid that if possible.

> +        line_len += offset;
> +    }
> +    while (offset < len && !feof(stdin)) {
> +        offset += fread(&s[offset], 1, len - offset, stdin);
> +    }

On error, wouldn’t this be an infinite loop?  Maybe:

  offset += fread(&s[offset], 1, len - offset, stdin);
  if (ferror(stdin)) {
	free(s);
	return NULL;
 }

One iteration should be sufficient, since fread loops internally.

[...]
> +void buffer_copy_bytes(uint32_t len)
> +{
> +    uint32_t in, out;
> +    if (line_buffer_len > line_len) {
> +        in = line_buffer_len - line_len;
> +        if (in > len)
> +            in = len;
> +        out = 0;
> +        while (out < in && !ferror(stdout)) {
> +            out +=
> +                fwrite(&line_buffer[line_len + out], 1, in - out, stdout);
> +        }

Likewise.

> +        len -= in;
> +        line_len += in;
> +    }
> +    while (len > 0 && !feof(stdin)) {
> +        in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
> +        in = fread(byte_buffer, 1, in, stdin);
> +        len -= in;
> +        out = 0;
> +        while (out < in && !ferror(stdout)) {
> +            out += fwrite(&byte_buffer[out], 1, in - out, stdout);

Likewise.

Why isn’t line_buffer used here?

[...]
> +void buffer_skip_bytes(uint32_t len)
> +{
[...]
> +    while (len > 0 && !feof(stdin)) {
> +        in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
> +        in = fread(byte_buffer, 1, in, stdin);
> +        len -= in;
> +    }

Likewise.

Too bad stdio does not supply a function for this (fseek is the
closest I can find, and it does not work on unseekable files).

Thanks,
Jonathan