0
\$\begingroup\$

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 <unistd.h>
#include <fcntl.h>
#include <sys/file.h>
#include <sys/stat.h>
#include <stdlib.h>
#include <string.h>
#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, &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, &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 > 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 < n; i++) {
        lns[i] = allocline(cur, buf + size - cur, &cur);
        if (lns[i] == NULL)
            goto fail;
    }

    return lns;

fail:
    freelines(lns);
    return NULL;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ uhm, you do use parts lf the standard c library -- any reason why not to use <stdio.h> too? \$\endgroup\$ – ljrk Aug 13 '16 at 7:34
  • \$\begingroup\$ @larkey I don't need stdio buffering overhead here; I'm just reading the whole file. \$\endgroup\$ – nebuch Aug 13 '16 at 18:19
  • \$\begingroup\$ okay, well I just think it'd make the code even more portable and more clean with better error-handling. Usually speed doesn't matter that much that stdio would be a problem, but that's ofcourse up to you to decide. Just stroke me curious ^^ \$\endgroup\$ – ljrk Aug 13 '16 at 18:23
1
\$\begingroup\$

Bugs

  1. If flock() fails, you don't close the file descriptor. Instead of goto out it should be goto close.
  2. 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);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks! How about the (void) before memcpy; do you suggest I omit it? \$\endgroup\$ – nebuch Aug 13 '16 at 18:17
  • \$\begingroup\$ @nebuch Well it isn't needed. If you have your compilation settings such that it gives you a warning for unused return values, then you would leave it in to avoid the warning. Other than that, I wouldn't recommend it. \$\endgroup\$ – JS1 Aug 14 '16 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.