HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Readline function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionreadlinestackoverflow

Problem

I have developed a readline function in C and I want to know your opinion on it:

// read a the whole line, stop if encouter eof
static char *readline(FILE *in, sid_error *error) 
{
    #define MAX_SIZE    64  
    size_t size = MAX_SIZE;
    char *str;

    if((str = malloc(sizeof *str * size)) == NULL) {
        *error = ERROR_MALLOC; 
        return NULL;
    }

    char *pos;
    char read = 0;

    while(fgets(str + size - MAX_SIZE, MAX_SIZE, in)) {
        read = 1;
        if(pos = strchr(str, '\n')) { 
            *pos = '\0';
            break;
        }

        size = size + MAX_SIZE - 1;
        str = realloc(str, sizeof *str * size); 
    }

    if(!read) {
        free(str);
        return NULL;
    }

    return str;
}

Solution

-
Inconsistent memory allocation. Code checks for a NULL return on malloc(), yet no checks on realloc(). Recommend adding a NULL check on realloc().

-
Reliance on no embedded '\0'. The following code searches up to '\0' for a '\n' to determine if reading the line is done. In unusual situations, a '\0' will occur before the final '\n' + '\0' pair. So if MAX_SIZE was a larger value, the same file input (with an embedded '\0') could read more of the line than this code. This is a weakness of fgets() and this readline() inherits it. To get around this, some function other that fgets() needs to be used for primary input.

if(pos = strchr(str, '\n'))


-
Minor performance issue. In searching for the final '\n', trimming fgets() is usually faster with strlen() than strchr(). But more importantly, this code re-searches the same buffer unnecessarily. Consider instead the following.

//  if(pos = strchr(str, '\n')) { 
if(pos = strchr(str + size - MAX_SIZE, '\n')) {


-
Reallocation increments the buffer size in a linear fashion. Increasing exponentially (example: 2x) has advantages with unusually long lines. Yet for typically applications without insanely long lines, either approach is OK.

-
Nice correct use of size_t for buffer size.

[Edit]

  • If there is the unusual embedded '\0' in input such as "abc\0xyx\n\0", then the while loop will unfortunately read the rest of the file. The while loop will not exit with if(pos = strchr(str, '\n')) and will continue reading until fgets() returns NULL. As in point #3, subsequent searches for \n should begin with the new data read and not at &str[0]. Further, looping should only occur if '\n' was not found and buffer is full.

Code Snippets

if(pos = strchr(str, '\n'))
//  if(pos = strchr(str, '\n')) { 
if(pos = strchr(str + size - MAX_SIZE, '\n')) {

Context

StackExchange Code Review Q#78571, answer score: 6

Revisions (0)

No revisions yet.