3
\$\begingroup\$

I'm reading Bruce Molay's book on Unix programming, and as an exercise I've implemented a copy of the tail command.

My approach goes over the entire file once, to count newlines, then again to store their positions in an array, and finally a third time to print out the bytes past the position of the nth newline. I know this must be inefficient, though I'm not sure of the ideal way to go about it.

Another point of uncertainty―to get around seeking restrictions on stdin, I write a copy of it to a temp file, then perform all operations on that. I think there's probably a way to do this without a temp file, though I'm not sure what it is.

tail1.c

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

#define BUFSIZE 4096
#define N_LINES 10
#define TMPFILE "/tmp/stdin_tmpf.bin"

void oops(char *s1, char *s2) {
    fprintf(stderr, "Error: %s\n(errno) ", s1);
    perror(s2);
    exit(1);
}

unsigned count_chars(const char *str, char byte, int n_chars) {
    int cnt = 0;
    for (int i = 0; i < n_chars; ++i)
        if (str[i] == byte)
            ++cnt;
    
    return cnt;
}

unsigned find_cutoff(int fd) {
    int linecnt, n_chars;
    int subpos, block;
    char cbuf[BUFSIZE];
    subpos = block = linecnt = n_chars = 0;
    
    // count lines to allocate linelocs array
    while ((n_chars = read(fd, cbuf, BUFSIZE)) > 0)
        linecnt += count_chars(cbuf, '\n', n_chars); 
    if (linecnt <= N_LINES)
        return 0;

    // array of positions of newlines
    int linelocs[linecnt];
    linelocs[0] = 0;
    int loc_index = 0;

    if (lseek(fd, 0, SEEK_SET) == -1)
        oops("couldn't seek start", "");

    while ((n_chars = read(fd, cbuf, BUFSIZE)) > 0) {
        for (int i = 0; i < n_chars; ++i)
            if (cbuf[i] == '\n') {
                loc_index++;
                subpos = i+1;
                linelocs[loc_index] = (BUFSIZE*block) + subpos;
            }

        block++;
    }

    return linelocs[linecnt - N_LINES];
}

// create temporary file holding stdin contents
int stdin_tmpf() {
    int out_fd, in_fd;
    int n_chars;
    char buf[BUFSIZE];

    if ((in_fd = fileno(stdin)) == -1)
        oops("couldn't open stdin", "");
    if ((out_fd = open(TMPFILE, O_RDWR | O_CREAT)) == -1)
        oops("failed to create tmpf", "");
    while ((n_chars = read(in_fd, buf, BUFSIZE)) > 0)
        if (write(out_fd, buf, n_chars) != n_chars)
            oops("read/write error", "stdin_tmpf");

    if (lseek(out_fd, 0, SEEK_SET) == -1)
        oops("seek failure", "stdin_tmpf");
    return out_fd;
}

int main(int argc, char **argv) {
    int in_fd, out_fd;
    bool cleanup = false;

    if (argc == 1) {
        in_fd = stdin_tmpf();
        cleanup = true;
    } else if ((in_fd = open(argv[1], O_RDONLY)) == -1)
        oops("Couldn't open file", argv[1]);
    if ((out_fd = fileno(stdout)) == -1)
        oops("Couldn't open stdout", "");

    unsigned cutoff = find_cutoff(in_fd);
    int n_chars;
    char buf[BUFSIZE];

    if (lseek(in_fd, cutoff, SEEK_SET) == -1)
        oops("couldn't seek cutoff", ""); // TODO int to str
    while ((n_chars = read(in_fd, buf, BUFSIZE)) > 0)
        if (write(out_fd, buf, n_chars) != n_chars)
            oops("couldn't write stdout", "");

    if (close(in_fd) == -1 || close(out_fd) == -1)
        oops("couldn't close files", "");
    if (cleanup && unlink(TMPFILE) == -1)
        oops("failed to cleanup", TMPFILE);

    return 0;
}

Quite new to C programming, I've been a Python programmer for many years. Any tips are greatly appreciated!

\$\endgroup\$
3
  • \$\begingroup\$ C++ might be a bit easier if you are coming from Python :) \$\endgroup\$
    – G. Sliepen
    Jun 4 at 16:28
  • 1
    \$\begingroup\$ My (very) eventual goal is contributing to the Linux kernel, so I think that would be a bit off track \$\endgroup\$
    – albert
    Jun 4 at 16:45
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a new question instead. \$\endgroup\$
    – Mast
    Jun 5 at 19:12
3
\$\begingroup\$

You only need one pass

Your solution to have three passes over the input has some big problems. Most notably, if the input is very large, you also need to create a large temporary file. But you now also have to deal with issues surrounding temporary files, like what if I run two instances of tail in parallel? What if someone created a symlink from /tmp/stdin_tmpf.bin to some other file?

You can avoid all this by doing only a single pass over the input. The trick is that you know you only need to remember the last N_LINES, so just create a buffer for N_LINES lines. Start filling the buffer, and once it holds N_LINES lines, when you read in the next line, delete the oldest line. Once you finished reading the input, just write out the contents of the buffer.

Note that this is also what the coreutils tail program does.

Use size_t for sizes, counts and indices

I see you use unsigned and int interchangably for keeping track of counts, like n_chars and cnt in count_chars(). However, the right type to use is size_t. Do this whereever appropriate.

Reporting errors

I see you check every return value and report an error both to stderr and by exitting with a non-zero exit code. That's very good! However, I don't see why you both do a fprintf(stderr, ...) and call perror(). I think one is enough. Also, prefer using EXIT_FAILURE as the exit code.

If you are targetting Linux or BSD only, you might consider calling err() instead of your own oops() function.

\$\endgroup\$
2
  • \$\begingroup\$ Best way to allocate the buffer for N_LINES? Do I have to assume a maximum line length? \$\endgroup\$
    – albert
    Jun 4 at 17:57
  • 1
    \$\begingroup\$ You should never assume a maximum line length. You could use the POSIX getline() function to read lines (it will do the memory allocation for you), or use fgets() to read into a pre-allocated buffer, but then check if it read a newline, otherwise realloc() the buffer into something larger and call fgets() again to read the remainder of the line (and possibly repeat this multiple times for a single line). \$\endgroup\$
    – G. Sliepen
    Jun 4 at 18:15

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.