3
\$\begingroup\$

This is a function that reads until EOF and reallocs as it goes.

char *
read_to_end(int fd, int *bread)
{
    ssize_t size, nread, rc;
    char *buf, *tmp;

    nread = 0;
    size = MINLEN;
    buf = malloc(size);

    while ((rc = read(fd, buf + nread, size - nread))) {
        if (rc < 0) {
            if (errno == EINTR)
                continue;

            goto error;
        }

        nread += rc;

        /* See if we need to expand. */
        if (size - nread < MINLEN) {
            size *= 2;

            /* Make sure realloc doesn't fail. */
            if (!(tmp = realloc(buf, size)))
                goto error;

            buf = tmp;
        }
    }

    /* Shrink if necessary. */
    if (size != nread && !(tmp = realloc(buf, nread)))
        goto error;

    buf = tmp;
    *bread = nread;
    return buf;

error:
    free(buf);
    return NULL;
}

Do you have any improvement suggestions?

\$\endgroup\$
2
  • \$\begingroup\$ You can use fseek() and ftell() to get the size if this is a file. \$\endgroup\$ Commented Feb 25, 2012 at 21:33
  • \$\begingroup\$ Without knowing more about which goal you want to reach is very difficult to give you a direction. But IMO it is never a good practice to bufferize a whole copy of a file into memory. \$\endgroup\$ Commented Mar 13, 2012 at 21:23

2 Answers 2

Reset to default
3
\$\begingroup\$

Seeing this is 'unanswered' here's my tuppence worth:

If you are reading a file I suggest you get the file size first, eg using fstat(), allocate a buffer of the correct size and read into that.

Alternatively, if the file is big and you have OS support, consider mapping the file into memory using mmap(). This maps the file into virtual memory but does not read it immediately. As you access the memory into which the file is mapped the file is read into that address space for you (in the background) by the OS.

On your code, I have a few comments:

  • If you have a stderr, perhaps add a call to perror() on error.

  • It looks as if you always re-alloc, even if the initial buffer is big enough.

  • If zero bytes are read, you will realloc(buf, 0). realloc man page says this is ok.

  • Add a bracket around size != nread - I always have to check operator precedence in statements like this and I guess I am not alone. Brackets save the reader the trouble.

  • I would prefer *bread to be set to 0 on failure. Should bread be size_t ?

\$\endgroup\$
1
\$\begingroup\$

I would refrain of using GOTO in pure C code. It's not very common...

Also, when you return the buf pointer to the calling function, that function is going to need to actively manage that piece of memory (It becomes in charge of the "free"-ing process).

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Using goto to jump to a common cleanup block is actually quite common, and a good idea. \$\endgroup\$ Commented Mar 12, 2012 at 15:02
  • \$\begingroup\$ I wasn't aware of that. Either way, isn't goto a poor man's exception handling? \$\endgroup\$ Commented Mar 12, 2012 at 15:26
  • \$\begingroup\$ Generally speaking we try to avoid using GOTOs because it can make code less readable and hard to maintain. But there is some cases (like error handling) where it is really convenient to use GOTOs instead of levels of conditional statement. \$\endgroup\$ Commented Mar 13, 2012 at 21:31
  • \$\begingroup\$ @Evert Poor man’s exception handling is exactly what it is here. And aptly so, since C doesn’t provide exceptions. This usage of goto is about the only one I’d condone. \$\endgroup\$ Commented Mar 31, 2012 at 21:44

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.