Split a file into individually allocated lines This is part of a larger project that is to run on POSIX systems. My concerns with it are the lack of comments, best practices with the filesystem, and naming things. Of course all feedback is great.

fileio.h:

#ifndef FILEIO_H
#define FILEIO_H

char **readlines(const char *path);
void freelines(char **lines);

#endif


fileio.c:

#include &lt;unistd.h&gt;
#include &lt;fcntl.h&gt;
#include &lt;sys/file.h&gt;
#include &lt;sys/stat.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;string.h&gt;
#include "fileio.h"

static off_t getsize(int fd);
static char *getfile(int fd, off_t *size);
static long countlines(char *buf, off_t size);
static char *allocline(char *buf, off_t size, char **next);
static char **getlines(char *buf, off_t size);

char **readlines(const char *path)
{
    int fd, ret;
    off_t size;
    char *buf;
    char **tmp, **lns = NULL;

    fd = open(path, O_RDONLY);
    if (fd == -1)
        goto out;

    ret = flock(fd, LOCK_NB | LOCK_EX);
    if (ret == -1)
        goto out;

    size = getsize(fd);
    if (size == -1)
        goto close;

    buf = getfile(fd, &amp;size);
    if (buf == NULL)
        goto close;

    (void)flock(fd, LOCK_UN);

    tmp = getlines(buf, size);
    if (tmp == NULL)
        goto free;

    lns = tmp;

free:
    free(buf);
close:
    close(fd);
out:
    return lns;
}

void freelines(char **lines)
{
    char **ln = lines;
    while (*ln != NULL)
        free(*ln++);
    free(lines);
}

static off_t getsize(int fd)
{
    int ret;
    struct stat sb;

    ret = fstat(fd, &amp;sb);
    if (ret == -1)
        return -1;
    else
        return sb.st_size;
}

static char *getfile(int fd, off_t *size)
{
    ssize_t ret;
    char *tmp, *buf;

    buf = malloc(*size);
    if (buf == NULL)
        return NULL;

    ret = read(fd, buf, *size);
    if (ret != *size)
        goto fail;

    if (buf[*size - 1] != '\n') {
        tmp = realloc(buf, ++*size);
        if (tmp == NULL)
            goto fail;
        buf = tmp;
    }

    return buf;

fail:
    free(buf);
    return NULL;
}

static long countlines(char *buf, off_t size)
{
    long n = 0;
    char *cur = buf;

    #define REMAINING ((buf) + (size) - (cur))

    do {
        cur = memchr(cur, '\n', REMAINING);
        n++; cur++;
    } while (REMAINING &gt; 0);

    #undef REMAINING

    return n;
}

static char *allocline(char *buf, off_t size, char **next)
{
    char *ln, *end;

    end = memchr(buf, '\n', size);
    if (end == NULL)
        return NULL; /* programmer error */

    #define LENGTH ((end) - (buf) + 1)

    ln = malloc(LENGTH + 1);
    if (ln == NULL)
        return NULL;
    ln[LENGTH] = 0;
    (void)memcpy(ln, buf, LENGTH);

    #undef LENGTH

    if (next != NULL)
        *next = end + 1;
    return ln;
}

static char **getlines(char *buf, off_t size)
{
    long i, n;
    char **lns;
    char *cur;

    n = countlines(buf, size);

    lns = calloc(n + 1, sizeof(char *));
    if (lns == NULL)
        return NULL;
    lns[n] = NULL;

    for (i = 0, cur = buf; i &lt; n; i++) {
        lns[i] = allocline(cur, buf + size - cur, &amp;cur);
        if (lns[i] == NULL)
            goto fail;
    }

    return lns;

fail:
    freelines(lns);
    return NULL;
}
Bugs


If flock() fails, you don't close the file descriptor.  Instead of goto out it should be goto close.
If the file does not end in a newline, you allocate extra space for it but you don't actually fill it in.  This means that you leave your buffer with an uninitialized last character and no null terminator.  Later, in countlines(), your call to memchr() will return NULL if it doesn't find a newline on the last line, and your program will crash.


Simplification #1

This code:


tmp = getlines(buf, size);
if (tmp == NULL)
    goto free;

lns = tmp;



could be simplified to:

lns = getlines(buf, size);


Simplification #2

Instead of allocating *size bytes and then reallocating one extra byte if you need to add an extra newline, just allocate the one extra byte to start with.

Simplification #3

Instead of defining and undefining a macro like this:


#define LENGTH ((end) - (buf) + 1)

ln = malloc(LENGTH + 1);
if (ln == NULL)
    return NULL;
ln[LENGTH] = 0;
(void)memcpy(ln, buf, LENGTH);

#undef LENGTH



just use a variable:

size_t length = end - buf + 1;

ln = malloc(length + 1);
if (ln == NULL)
    return NULL;
ln[length] = 0;
memcpy(ln, buf, length);