get_line(): An alternative implementation of standard C library's getline() function

Question

get_line(): An alternative implementation of standard C library's getline() function.

Syntax: char *get_line(int fd, int *error_num);

I know that I can declare/define variables anywhere in a function but I don't do that. I use K & R style of declaring/defining all variables at the top of the function. So, please don't give review comments on this.

The code has been compiled using the following gcc flags:

-Wall -Werror -Wextra -Wundef -Wunreachable-code -Winit-self -Wparentheses -Wconversion -Wsign-conversion -Wsign-compare -Werror-implicit-function-declaration -Wmissing-prototypes -Wmissing-declarations -Wformat-security

The code is below:

---

get_lines_from_file.c

#include "get_lines_from_file.h"

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

/*
 * const char *get_error_string(int error):
 *
 * Function get_error_string() returns the error string corresponding to the
 * value in 'error' argument.
 *
 */
const char *get_error_string(int error)
{

    switch (error) {

        case NO_ERROR:
            return "No error happened.";
        case INVALID_FD:
            return "Invalid fd. fd value is less than 0.";
        case NO_MEMORY:
            return "No memory available.";
        case NO_MORE_DATA:
            return "The file doesn't have any more data.";
        default:
            return "Invalid error number given.";

    }

} // end of print_error_string

/*
 * char *get_line(int fd, int *error_num):
 * 
 * Function get_line() returns a line from the file represented by fd.
 *
 * This function supports regular files only (and not pipes, sockets, etc).
 *
 * If fd is less than 0 then NULL is returned and if error_num is not NULL, then
 * *error_num is assigned the appropriate error value (INVALID_FD in this case).
 *
 * In case of any error, if error_num is not NULL then *error_num is assigned
 * the appropriate error value.
 *
 * The high level algorithm of this function is:
 *
 *      The function get_line() reads some bytes in a buffer from the file and
 *      tries to find newline in the buffer. If a newline is not found then it
 *      reads more bytes from the file in the buffer. When a newline is found in
 *      the buffer, then the newline is replaced with null byte and the buffer
 *      is reallocated to correct size. Then the file offset for reading is set
 *      to the start of the next line. And then the buffer is returned to the user.
 *
 * Please note that the returned line/buffer doesn't contain any newlines.
 *
 * The line/buffer returned by this function is allocated using realloc(), so it is
 * user's responsibility to free the line (free memory).
 *
 */
char *get_line(int fd, int *error_num)
{

#define NEW_LINE '\n'
#define BUF_SIZE_INCREMENT 256 // normally lines are not greater than 256 bytes.

    char *buf = NULL;
    char *buf_addr_holder = NULL;
    long index_to_load_data = 0;
    long curr_buf_size = 0;
    long num_chars_read = 0;
    ssize_t ret_val = -1;
    int end_of_file_reached_or_error = 0;
    int new_line_found = 0;
    long i = 0;

    if (error_num)
        *error_num = NO_ERROR;

    if (fd < 0) {
        if (error_num)
            *error_num = INVALID_FD;
        return NULL;
    }

    while (1) {

#ifdef DEBUG_ON
        printf("%s: curr_buf_size before realloc = %ld.\n", __FILE__, curr_buf_size);
#endif

        buf_addr_holder = realloc(buf, (size_t)(curr_buf_size + BUF_SIZE_INCREMENT));
        if(!buf_addr_holder) {
            free(buf);
#ifdef DEBUG_ON
        printf("%s:%d: buffer has been freed.\n", __FILE__, __LINE__);
#endif
            if (error_num)
                *error_num = NO_MEMORY;
            return NULL;
        }
        curr_buf_size = curr_buf_size + BUF_SIZE_INCREMENT;
        buf = buf_addr_holder;
        index_to_load_data = num_chars_read;

#ifdef DEBUG_ON
        printf("%s: curr_buf_size after realloc = %ld.\n", __FILE__, curr_buf_size);
#endif

        // read data from file
        while (1) {

            ret_val = read(fd, buf + index_to_load_data, (size_t)(curr_buf_size - num_chars_read));

            if (ret_val <= 0) {
                if (num_chars_read == 0) {
                    if (error_num)
                        *error_num = NO_MORE_DATA;
                    if (buf) {
                        free(buf);
#ifdef DEBUG_ON
        printf("%s:%d: buffer has been freed.\n", __FILE__, __LINE__);
#endif
                    }
                    return NULL;
                } else {
                    end_of_file_reached_or_error = 1;
                    break;
                }
            }

            num_chars_read = num_chars_read + ret_val;

            if (num_chars_read == curr_buf_size)
                break;

        } // end of inner while (1)

        if (num_chars_read > curr_buf_size) {
            printf("%s: Some bug in program. num_chars_read (%ld) is greater than"
                   " curr_buf_size (%ld). Exiting..\n", __FILE__, num_chars_read, curr_buf_size);
            exit(1);
        }

#ifdef DEBUG_ON
        if (num_chars_read < curr_buf_size) {
            printf("%s: num_chars_read (%ld) is less than curr_buf_size (%ld).\n",
                   __FILE__, num_chars_read, curr_buf_size);
        } else if (num_chars_read == curr_buf_size) {
            printf("%s: num_chars_read (%ld) is equal to curr_buf_size (%ld).\n",
                   __FILE__, num_chars_read, curr_buf_size);
        }
#endif

        new_line_found = 0;

        for (i = index_to_load_data; i < num_chars_read; i++) {
            if (buf[i] == NEW_LINE) {
                buf[i] = 0;
                new_line_found = 1;
                break;
            }
        } // end of for loop

        if (new_line_found) {

            // realloc and seek
            buf_addr_holder = realloc(buf, (size_t)(i + 1));
#ifdef DEBUG_ON
        printf("%s: buffer reallocated from %ld bytes to %ld bytes.\n", __FILE__, curr_buf_size, i + 1);
#endif
            if(!buf_addr_holder) { // strange, this should not happen
                free(buf);
#ifdef DEBUG_ON
        printf("%s:%d: buffer has been freed.\n", __FILE__, __LINE__);
#endif
                if (error_num)
                    *error_num = NO_MEMORY;
                return NULL;
            }
            lseek(fd, (i + 1) - num_chars_read, SEEK_CUR);
#ifdef DEBUG_ON
            printf("%s: Seeking %ld bytes from current read offset.\n", __FILE__, (i + 1) - num_chars_read);
#endif
            return buf_addr_holder;

        } else { // new line not found

            // new line not found and end of file has been reached.
            // so, allocate one extra byte for terminating null byte and return.
            if (end_of_file_reached_or_error) {
#ifdef DEBUG_ON
        printf("%s: Newline not found in buffer but end of file reached. curr_buf_size"
               " = %ld, num_chars_read = %ld.\n", __FILE__, curr_buf_size, num_chars_read);

        if (num_chars_read == curr_buf_size) {
            printf("%s: -----THIS SHOULD NEVER HAPPEN-----. NEWLINE NOT FOUND IN BUFFER BUT"
                   " END OF FILE REACHED AND CURR_BUF_SIZE AND NUM_CHARS_READ ARE EQUAL"
                   " (%ld).\n", __FILE__, curr_buf_size);
        }
#endif
                buf_addr_holder = realloc(buf, (size_t)(num_chars_read + 1));
                if(!buf_addr_holder) {
                    free(buf);
#ifdef DEBUG_ON
        printf("%s:%d: buffer has been freed.\n", __FILE__, __LINE__);
#endif
                    if (error_num)
                        *error_num = NO_MEMORY;
                    return NULL;
                }
                buf_addr_holder[num_chars_read] = 0;
                return buf_addr_holder;
            }

        } // end of if - else (new_line_found)

#ifdef DEBUG_ON
        printf("%s: Newline not found in buffer, reading more data. curr_buf_size"
               " = %ld, num_chars_read = %ld.\n", __FILE__, curr_buf_size, num_chars_read);
#endif

    } // end of outer while (1)

} // end of get_line

---

get_lines_from_file.h

#ifndef GET_LINES_FROM_FILE_H
#define GET_LINES_FROM_FILE_H

enum {
    NO_ERROR = 0, // no error happened.
    INVALID_FD = -1, // fd is less than 0.
    NO_MEMORY = -2, // memory not available.
    NO_MORE_DATA = -3, // file has no bytes/characters.
};

/*
 * char *get_line(int fd, int *error_num):
 * 
 * Function get_line() returns a line from the file represented by fd.
 * 
 * This function supports regular files only (and not pipes, sockets, etc).
 *
 * If fd is less than 0 then NULL is returned and if error_num is not NULL, then
 * *error_num is assigned the appropriate error value (INVALID_FD in this case).
 *
 * In case of any error, if error_num is not NULL then *error_num is assigned
 * the appropriate error value.
 *
 * The high level algorithm of this function is:
 *
 *      The function get_line() reads some bytes in a buffer from the file and
 *      tries to find newline in the buffer. If a newline is not found then it
 *      reads more bytes from the file in the buffer. When a newline is found in
 *      the buffer, then the newline is replaced with null byte and the buffer
 *      is reallocated to correct size. Then the file offset for reading is set
 *      to the start of the next line. And then the buffer is returned to the user.
 *
 * Please note that the returned line/buffer doesn't contain any newlines.
 *
 * The line/buffer returned by this function is allocated using realloc(), so it is
 * user's responsibility to free the line (free memory).
 *
 */
char *get_line(int fd, int *error_num);

/*
 * const char *get_error_string(int error):
 *
 * Function get_error_string() returns the error string corresponding to the
 * value in 'error' argument.
 *
 */
const char *get_error_string(int error);

#endif

---

test_get_lines_from_file.c

#include "get_lines_from_file.h"

#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <fcntl.h>

static void print_usage_and_exit(void)
{

    printf("\n");
    printf("Usage: test_get_lines_from_file file_name\n");
    printf("\n");
    exit(1);

} // print_usage_and_exit

int main(int argc, char * argv[])
{

    int fd = -1;
    char *line = NULL;
    int error_num = 0;

    if (argc != 2) {
        printf("\nOnly two arguments are required. Program name and file name.\n");
        print_usage_and_exit();
    }

    fd = open(argv[1], O_RDONLY);
    if (fd < 0) {
        printf("\n%s: File \"%s\" could not be opened. Exiting..\n", __FILE__, argv[1]);
        exit(1);
    }

    while ((line = get_line(fd, &error_num)) != NULL) {
        printf("%s\n", line);
        free(line);
    }

    if ((error_num != NO_ERROR) && (error_num != NO_MORE_DATA)) {
        printf("%s\n", get_error_string(error_num));
    }

    close(fd);

} // end of main

Answer 1

You already know you need to move your variables to smaller scope where possible, so no need to say any more about that. (K&R C never required variables to all be declared at function scope.)

The get_line interface seems to prevent writing a pure filter that preserves input that's not properly terminated with newline. You may or may not consider that a problem.

get_error_string seems like a name that's prone to collision - you probably want to make that more specific. Or just use the usual errno values such as EBADF and ENOMEM (which gets you translated error messages without extra work, and stops you having to fold down many different kinds of error into a few less informative ones).

I don't see any good reason for NEW_LINE and BUF_SIZE_INCREMENT to be macros (rather than ordinary C constant objects).

We can simplify some expressions and make them easier to read. In particular,

    curr_buf_size = curr_buf_size + BUF_SIZE_INCREMENT;
        num_chars_read = num_chars_read + ret_val;

It's much kinder on your readers to use addition assignment:

    curr_buf_size += BUF_SIZE_INCREMENT;
        num_chars_read += ret_val;

If we get an error on our first read (e.g. fd isn't a readable file descriptor) then we report "no more data" instead of a proper error.

if (buf) free(buf); has a pointless condition. free(buf); is a much simpler replacement (since free() is specified to do nothing when passed a null pointer).

It's very hard to read the DEBUG_ON sections that are outdented with respect to their enclosing scopes. Code within preprocessor conditionals normally retains its indentation level.

Tests of program logic should normally not be included when NDEBUG is defined, so that they work like assert(). And our error needs to go to the correct output stream:

#ifndef NDEBUG
        if (num_chars_read > curr_buf_size) {
            fprintf(stderr,
                    "%s:%d: Some bug in program. num_chars_read (%ld) is greater than"
                    " curr_buf_size (%ld). Exiting..\n",
                    __FILE__, __LINE__,
                    num_chars_read, curr_buf_size);
            exit(1);
        }
#endif

We really don't want a library function to exit() the whole program unilaterally, even if the library is faulty.

This loop is crazy:

    for (i = index_to_load_data; i < num_chars_read; i++) {
        if (buf[i] == NEW_LINE) {
            buf[i] = 0;
            new_line_found = 1;
            break;
        }
    } // end of for loop

There's really no reason to write our own memchr() implementation every time.

Variable i needs to be size_t, and to have a better name given the large scope. Single-letter names should only be used for variables with small scope.

When we find a newline, we attempt to unread it using lseek() but never test whether it was successful (as it won't be when reading a pipe, for example). This will result in data loss. It might be a good idea to test whether fd is seekable right at the start of the function, before we read anything from it.

The test program prints its error messages to stdout - use stderr instead. Consider using perror() (or fprintf() with strerror(errno)) to get more informative diagnostics when the file can't be opened.

As usual, it would be better if we could arrange some self-checking tests rather than having to rely on the tester's inspection of results.