Read line from file issue

Question

I wrote this simple readline function, it can return each line length but it doesn't return a pointer to the allocated buffer. Another issue is the last line ignored(it doesn't return it):

FILE *passFile = NULL;
char *current = NULL;
size_t len = 0;
passFile = fopen("pass.txt", "r");
while(readline(passFile, &current, &len) != -1) {
    printf("%s\n", current); // SEGMENTAION FAULT
    printf("%d\n", len);
    free(current);
    current = NULL;
}

ssize_t
readline(FILE *file, char **bufPtr, size_t *len)
{
    char c, *buf = NULL;
    size_t n = 0;
    buf = (char*)malloc(sizeof(char));
    while((c = fgetc(file)) != '\n' && (c != EOF)) {
        buf[n] = c;
        ++n;
        buf = realloc(buf, n + 1);
    }

    buf[n] = '\0';
    *bufPtr = buf;
    *len = n;
    if(c == EOF)    // reach end of file
        return -1;

    return 0;
}

Answer 1

Now it works:

ssize_t
readline(FILE *file, char **bufPtr, size_t *len)
{
    if(feof(file))  // reach end of file
        return -1;

    char c, *buf = NULL;
    size_t n = 0, portion = CHUNK;
    buf = (char*)malloc(sizeof(char) * CHUNK);
    while((c = fgetc(file)) != '\n' && (c != EOF)) {
        buf[n] = c;
        ++n;
        if(n == portion) {
            buf = realloc(buf, CHUNK + n);
            portion += n;
        }
    }

    buf[n] = '\0';
    *bufPtr = buf;
    *len = n;

    return 0;
}

Answer 2

Your readline() function is not returning a pointer to allocated memory. In your call, current is never set, so the pointer is invalid and you get the error.

In C, functions are "call by value". Inside readline(), bufPtr is a copy of whatever was passed to readline(). Assigning to bufPtr merely overwrites the local copy and does not return a value that the calling code can see.

In pseudocode:

TYPE a;

define function foo(TYPE x)
{
    x = new_value;
}

foo(a);  // does not change a

This only changes the local copy of x and does not return a value. You change it to use a pointer... the function still gets a copy, but now it's a copy of a pointer, and it can use that pointer value to find the original variable. In pseudocode:

TYPE a;

define function foo(TYPE *px)
{
    *px = new_value;
}

foo(&a);  // does change a

Now, to change your function:

ssize_t
readline(FILE *file, char **pbufPtr, size_t *len)
{
    // ...deleted...
    buf[n] = '\0';
    *pbufPtr = buf;
    // ...deleted...
}

And you call it like so:

while(readline(passFile, &current, &len) != -1)

P.S. It is not a good idea to call realloc() the way you do here. It's potentially a very slow function, and for an input string of 65 characters you will call it 65 times. It would be better to use an internal buffer for the initial file input, then use malloc() to allocate a string that is just the right size and copy the string into the buffer. If the string is too long to fit in the internal buffer at once, use malloc() to get a big-enough place to copy out the part of the string you have in the internal buffer, then continue using the internal buffer to copy more of the string, and then call realloc() as needed. Basically I'm suggesting you have an internal buffer of size N, and copy the string in chunks of N characters at a time, thus minimizing the number of calls to realloc() while still allowing arbitrary-length input strings.

EDIT: Your last-line problem is that you return -1 when you hit end of file, even though there is a line to return.

Change your code so that you return -1 only if c == EOF and n == 0, so a final line that ends with EOF will be correctly returned.

You should also make readline() use the feof() function to check if file is at end-of-file, and if so, return -1 without calling malloc().

Basically, when you return -1, you don't want to call malloc(), and when you did call malloc() and copy data into it, you don't want to return -1! -1 should mean "you got nothing because we hit end of file". If you got something before we hit end of file, that's not -1, that is 0. Then the next call to readline() after that will return -1.

Answer 3

In your readline function you pass current by value. So if you change bufPtr inside your function, it doesn't change value of current outside. If you want to change value of current pass it by reference: &current and change readline() parameter to char **bufPTR.
You could pass current the way you did if you wanted to change something it points to, but you want to change where it points in first place.

Answer 4

replace your readlinefunction with this

char*   readline(FILE *file, size_t *len)
{
    char c, *buf = NULL;
    size_t n = 0;
    buf = (char*)malloc(sizeof(char));
    while((c = fgetc(file)) != '\n' && (c != EOF)) {
        buf[n] = c;
        ++n;
        buf = realloc(buf, n + 1);
    }

    buf[n] = '\0';
    bufPtr = buf;
    *len = n;
    if(c == EOF)    // reach end of file
        return NULL;

    return buf;
}

and then in main replace this line while(readline(passFile, current, &len) != -1) with this while((current = readline(passFile, &len) != NULL)