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\$
3
  • 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, 2016 at 7:34
  • \$\begingroup\$ @larkey I don't need stdio buffering overhead here; I'm just reading the whole file. \$\endgroup\$
    – nebuch
    Aug 13, 2016 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, 2016 at 18:23

1 Answer 1

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);
\$\endgroup\$
2
  • \$\begingroup\$ Thanks! How about the (void) before memcpy; do you suggest I omit it? \$\endgroup\$
    – nebuch
    Aug 13, 2016 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, 2016 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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