12
\$\begingroup\$
size_t readline_tostring(char * restrict dest, size_t size, FILE * restrict stream)

fgets() is OK yet it has some short-comings that the following readline_tostring() addresses for reading a line:

  1. When the buffer is insufficient, the rest of the line is consumed (and lost). An error is indicated.

  2. In C, a line of input is up to and including the '\n' C11 7.21.2 2. When the streams ends with something other than a new-line, how that is handled is implementation defined behavior. J.3.12. This code treats a '\n' and end-of-file as the same. In both cases, a '\n' is not include in the saved buffer.

  3. If code reads a '\0', that is not practical to discern with fgets(). This code returns the size of the space used in dest, which includes an appended null character.

  4. Lesser issues include fgets() handling of NULL augments, small buffer size, undefined buffer state on ferror() and use of int vs size_t. The below code also clearly - I hope - handles that.

  5. An alternative allocate memory: Allocating memory per external input can lead to abuse. This allows external forces to overwhelm memory allocation. The following does not use memory allocation like getline substitute that will enforce 'n' as limit of characters read or getline. Another alternative could use limited allocation, but that is not done here.

Primary review requests (of the non-test code)

Portability concerns: Might a common or rare case fail on some select systems?

Handling of exceptional/error cases: Any suggested alternates?

Performance concerns are appreciated when they are backed with real measurements.

General comments (on any code).


The code below is listed as one file for code review convenience, yet would usually would be is separate .h, .c files.

/////////////////////////////////////////////////////////////////
// Header info, usually in some *.h file

/*
 * Read a _line_ of text. Save characters up to a limit, and form a _string_.
 * The string saved in `dest` never contains a '\n'.
 * A null character is always appended ***1.
 * Reading only attempted in non-pathological cases.  
 * Otherwise the end-of-file flag and error flags are cleared before reading.
 *
 * Normal: The return value is greater than  0 and represents the _size_ of `dest` used.
 *     This includes all non-'\n' characters read and an appended null character. ***2
 *     Reading text "abc\n" forms string "abc" and return 4
 *
 * Exceptional cases:
 *   In these cases, the return value is 0 and `dest[0] = '\0'` except as noted.
 *   1: Pathological: Buffer invalid for string.
 *     `dest == NULL` or `size == 0` (No data is written into `dest`)
 *   2: Pathological: Stream invalid.
 *     `stream == NULL`
 *   3: End-of-file occurs and no data read.
 *     Typical end-of-file: `feof(stream)` will return true.
 *   4: Input error.
 *     `ferror(stream)` will return true.
 *     strlen(dest) is number of characters successfully read. ***3
 *   5: Buffer is too small.
 *     First `size-1` non-'\n' characters are saved in `dest[]`.
 *     Additional characters are read up to and including '\n'.  These are not saved.
 *     The end-of-file flag and error flags are cleared again.
 *     strlen(dest) is number of characters successfully save. ***3
 *
 * ***1 Except when `dest == NULL` or `size == 0`
 * ***2 If code reads a null character, it is treated like any non-'\n' character.
 * ***3 strlen(dest) does not reflect the number of characters in `dest` 
 *       if a null character was read and saved.
 *
 */

#include <stdio.h>
#include <stdlib.h>
size_t readline_tostring(char * restrict dest, size_t size,
    FILE * restrict stream);

/////////////////////////////////////////////////////////////////
// Code, usually in some *.c file

size_t readline_tostring(char * restrict dest, size_t size,
    FILE * restrict stream) {
  // Handle pathological cases
  if (dest == NULL || size == 0) {
    return 0;
  }
  if (stream == NULL) {
    dest[0] = '\0';
    return 0;
  }
  clearerr(stream);

  size_t i = 0;
  int ch;
  while ((ch = fgetc(stream)) != '\n' && ch != EOF) {
    if (i < size) {
      dest[i++] = (char) ch;
    }
  }

  // Add null character termination - always
  // If too many were read
  if (i >= size) {
    dest[size - 1] = '\0';
    clearerr(stream);
    return 0;
  }
  dest[i] = '\0';

  if ((ch == EOF) && (i == 0 || ferror(stream))) { // end-of-file or error
    return 0;
  }

  clearerr(stream);
  return i + 1;
}

/////////////////////////////////////////////////////////////////
// Test code

#include <string.h>
#include <ctype.h>

// Sample usage showing how to discern the results.
void sample(char * restrict dest, size_t size, FILE * restrict stream) {
  size_t sz;
  while ((sz = readline_tostring(dest, size, stream)) > 0) {
    printf("Size:%zu string:\"%s\"\n", sz, dest);
  }

  // Well formed code need not perform this 1st test
  if (dest == NULL || size == 0 || stream == NULL) {
    puts("Pathological case");
  } else if (feof(stream)) {
    puts("End of file");
  } else if (ferror(stream)) {
    puts("Input error");
  } else {
    printf("Line too long: begins with <%s>\n", dest);
  }
  puts("");
}

void test4(const char *s) {
  FILE *stream = fopen("tmp.bin", "wb");
  size_t len = strlen(s);
  fwrite(s, 1, len, stream);
  fclose(stream);
  for (size_t i = 0; i < len; i++) {
    printf(isprint((unsigned char)s[i]) ? "%c" : "<%d>", s[i]);
  }
  puts("");

  stream = fopen("tmp.bin", "r");
  char buf[4];
  sample(buf, sizeof buf, stream);
  fclose(stream);
  fflush(stdout);
}

int main(void) {
  test4("12\nAB\n");
  test4("123\nABC\n");
  test4("1234\nABCD\n");
  test4("");
  test4("1");
  test4("12");
  test4("123");
  test4("1234");
  return 0;
}

Output

12<10>AB<10>
Size:3 string:"12"
Size:3 string:"AB"
End of file

123<10>ABC<10>
Size:4 string:"123"
Size:4 string:"ABC"
End of file

1234<10>ABCD<10>
Line too long: begins with <123>


End of file

1
Size:2 string:"1"
End of file

12
Size:3 string:"12"
End of file

123
Size:4 string:"123"
End of file

1234
Line too long: begins with <123>
\$\endgroup\$
  • 1
    \$\begingroup\$ When the line contains \0 and the function returns 0 due to line-too-long or EOF or error, you cannot tell whether any of the characters beyond the first \0 are valid. That's in accordance with your specification, I guess, but it's hard to see how that could be desirable. \$\endgroup\$ – Nate Eldredge Oct 31 '17 at 5:40
  • \$\begingroup\$ @NateEldredge When the function returns 0, and feof(), ferror() are false, the result in those cases is a " line-too-long" and size-1 read characters are in the buffer - some of which may be '\0'. When 0 is returned and feof() is true, there are no characters read and the the initial '\0' is the end. Your comment best applies to the rare case 0 is returned and ferror() is true. The valid characters read extend to before the first null character and maybe more. \$\endgroup\$ – chux Oct 31 '17 at 14:06
3
+100
\$\begingroup\$

First impressions

The code is clear and easy to follow. You had two choices to ignore overlength lines (the other being a second loop to absorb the overflow). This choice seems reasonable.

I'd consider making the parameters const when defining the function, just to help avoid accidents and to be extra clear:

size_t readline_tostring(char *const restrict dest, size_t const size,
                         FILE *const restrict stream)
{

Clearing stream errors

I'm not convinced that it's right to call clearerr() in this code, as that can potentially hide useful information from the caller. There's no comment to justify this, and I think there should be.

Returning partial line

It might be better to discourage use of the partial line returned when EOF is reached, by setting dest[0] = '\0' in that case. Arguably, client code might want to spot that there's potentially-usable results if return value is 0 but dest[0] is non-null (e.g. if the discarded part is within a line-terminated comment).

It's certainly better to make dest an empty string when we get an error.

Tests

The test code looks a little rushed. How do we know it's safe to overwrite tmp.bin in the current working directory? We should be using tmpfile() or tmpnam() instead, and actually testing that we successfully write our temporary data. And unlink the file afterwards (if not using tmpfile(), which creates an unlinked file already).

Writing the file in binary mode and reading in text mode seems like a poor choice, and I'm not convinced it's even legal on record-oriented file systems.

None of the tests are self-checking, and none include embedded NUL character or consecutive newlines.

Here's some self-checking tests. I'm not completely happy with this, but hope it could be a start:

#include <stdarg.h>
#include <string.h>
#include <ctype.h>

// Sample usage showing how to discern the results.
size_t sample(char *const restrict dest, size_t const size,
              FILE *const restrict stream)
{
    size_t sz = readline_tostring(dest, size, stream);
    printf("Size:%zu string:\"%s\"\n", sz, dest);

    // Well formed code need not perform this 1st test
    if (dest == NULL || size == 0 || stream == NULL) {
        puts("Pathological case");
    } else if (feof(stream)) {
        puts("End of file");
    } else if (ferror(stream)) {
        puts("Input error");
    } else {
        printf("Line too long: begins with <%s>\n", dest);
    }

    return sz;
}

/* return a count of errors (0 for success) */
/* Varargs are (size, string) pairs of expected result */
/* Terminate with -1 */
int test4(const char *s, size_t len, ...)
{
    for (size_t i = 0; i < len; i++) {
        printf(isprint((unsigned char)s[i]) ? "%c" : "<%d>", s[i]);
    }
    puts("");

    FILE *const stream = tmpfile();
    if (!stream || fwrite(s, 1, len, stream) != len) {
        perror("");
        return 1;
    }
    rewind(stream);


    va_list args;
    va_start(args, len);

    int errors = 0;
    int expected;
    while ((expected = va_arg(args, int)) >= 0) {
        const char *s = va_arg(args, const char*);
        char buf[4];
        size_t actual = sample(buf, sizeof buf, stream);
        if (actual != (size_t)expected) {
            printf("FAIL (%d): Expected %d, got %zd\n",
                   ++errors, expected, actual);
        } else if (memcmp(buf, s, actual) != 0) {
            printf("FAIL (%d): Expected %s, got %s\n",
                   ++errors, s, buf);
        }
    }
    va_end(args);

    if (errors) {
        puts("FAILED");
    }
    puts("");

    fclose(stream);
    fflush(stdout);

    return errors;
}

int main(void) {
    /* two lines each of 4 chars and a newline */
    static const char *test_string = "1234\n"  "A\0BC\n";

    return
        + test4(test_string, 5, 0, "123", -1)
        + test4(test_string+1, 5, 4, "234", -1)
        + test4(test_string+3, 5, 2, "4", 4, "A\0B", -1)
        + test4(test_string+4, 5, 1, "",  0, "A\0BC", -1);
}

There are no tests for handling invalid arguments (null or zero-length buffer, null stream), nor for pathological cases (buffer of one byte). These should be added.

\$\endgroup\$
  • \$\begingroup\$ Toby, I truly appreciate your review. I strongly agree with clearerr() lack of comment is a short coming. clearerr() is warranted yet subtle and deserves an explanation. The last line lacking a '\n' is more forgiving to accept as a line than to discard it as many text files lack a final '\n'. See also 7.21.2 2 const in parameters that are stable is perhaps a good idea or not in a function implementation (I'm on the fence here), but is clutter in a .h file declaration. Clean-up of tmp.bin is a good idea. Writing in binary mode was done for exactness in creating test cases. \$\endgroup\$ – chux Mar 1 '18 at 16:22
  • \$\begingroup\$ I definitely wasn't suggesting a useless const in the header file - sorry if I gave that impression! Definitely only in the definition and not in the header file's declaration. I think I see your point about writing in binary mode - unfortunately, I don't think that using tmpfile will work if we need to do that, because we can't rewind and change the text/binary mode together. \$\endgroup\$ – Toby Speight Mar 1 '18 at 21:24
  • \$\begingroup\$ clearerr(stream); rational: When readline_tostring() returns 0 due to input error that just occurred or line too long, with embedded clearerr(stream);, calling ferror() absolutely distinguishes between those 2. Without clearerr(stream);, calling ferror() is less certain since that may return true for a prior input error. Code could also examine dest[0], yet that could be a read null character. I suspected the line-too-long indication mechanism would the most troubling aspect of my code. It would be more fgets()-like to not clearerr(stream); Hmmmm. \$\endgroup\$ – chux Mar 1 '18 at 23:25
  • \$\begingroup\$ BTW, I thought I had selected this answer to receive the bounty. Yet somehow that failed and the answer was award the bounty by the community. \$\endgroup\$ – chux Mar 3 '18 at 18:28

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.