4
\$\begingroup\$

I've always used fgets to read a file. However, I want to read a file that may have embedded \0. I thought of using ftell to query the size, but it doesn't seem to work on all files.

I've got a test file,

31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61

Here is my fgets.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <string.h> /* strlen */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    int is_done = 0;

    for( ; ; ) {
        /* Fail when contents bigger than the size;
         would be a good spot to use realloc. */
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        if(!fgets(f, granularity, stdin))
            { if(ferror(stdin)) break; is_done = 1; break; }
        f += strlen(f);
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this, (I'm on a UNIX-like machine,)

$ bin/fgets < test 
31 32 33 E2 82 AC 0D 0A 61 

Here is my fread.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    size_t read;
    int is_done = 0;

    for( ; ; ) {
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        read = fread(f, 1, granularity, stdin);
        if(ferror(stdin)) break;
        assert(read >= 0 && read <= granularity);
        f += read;
        if(read != granularity) { is_done = 1; break; }
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this,

$ bin/fread < test
31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61 

I would like to know if this is pedantically correct and how I improve.

\$\endgroup\$
  • 2
    \$\begingroup\$ If a file contains NULs, then it would usually be called a binary file rather than a text file. \$\endgroup\$ – 200_success Apr 27 at 1:53
  • \$\begingroup\$ I thought that depended on what encoding it's in. Would UTF-8 be a potentially binary file, then? en.wikipedia.org/wiki/UTF-8#Modified_UTF-8 Is there an unambiguous definition of text/binary? \$\endgroup\$ – Neil Edelman Apr 27 at 18:18
  • \$\begingroup\$ UTF-8 does not have NUL bytes. UTF-16 would. \$\endgroup\$ – 200_success Apr 27 at 18:20
  • 2
    \$\begingroup\$ What is the point of granularity vs. just using fread(fine, 1, sizeof file, stdin)? \$\endgroup\$ – chux Apr 27 at 20:09
  • \$\begingroup\$ @200_success Although uncommon to have a null character in a text file, C's read_file does not exclude them and robust code does have have UB when reading them. Neil's goal is a good one. \$\endgroup\$ – chux Apr 27 at 20:21
6
\$\begingroup\$

I find your code hard to read, for the following reasons:

  • the variable file is not really a file but a buffer
  • the variable f (which usually stands for file) is a pointer into that buffer
  • the variable a has a name which does not convey any meaning at all
  • if the first if statement you indent the brace in the next line, and in the very last if statement the body is in the same line
  • there are spaces missing in the code at places where I expect them, such as after an if or for
  • the forever loop is usually written as for (;;), not for( ; ; )
  • the main block of the code is inside the for loop, and there's not a single empty line in that part. This suggests that the whole block is doing a single thing with no possible interruption or logical break
  • the comma operator is generally frowned upon
  • is_done is not really about the work being done, it's more about being successful

Because of all the above reasons, I would write the code differently:

#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

int main(void) {
    char buf[1000];
    size_t buflen = 0;
    const size_t granularity = 80;

    while (true) {
        if (granularity > sizeof buf - buflen) {
            errno = ERANGE;
            break;
        }

        size_t nread = fread(buf + buflen, 1, granularity, stdin);
        if (ferror(stdin))
            break;

        buflen += nread;
    }

    for (size_t i = 0; i < buflen; i++)
        printf("%02hhX ", buf[i]);
    fputc('\n', stdout);

    if (ferror(stdin)) {
        perror("stdin");
        return EXIT_FAILURE;
    }
}

And here is what I changed:

  • I renamed all variables to match their purpose
  • I replaced the various pointers into the buffer with indexes
  • I removed the is_done variable since the program should not print an error just because the file is a multiple of 80 bytes
  • I moved the variable a to a smaller scope by declaring it inside the for loop where it is used; I also renamed it to i, since it is now an index instead of a pointer
  • I replaced the comma operator with an if statement, since that is the form that is commonly used
  • I included <stdbool.h> to have a boolean type and the constants true and false
  • I replaced for (;;) with while (true), which is less magic
  • I sorted the included headers alphabetically since for headers from the C standard library, the order doesn't matter
  • I renamed the variable read to nread, to avoid possible conflicts with the POSIX function of the same name
\$\endgroup\$
  • \$\begingroup\$ This is an excellent review. ssize_t and bool are C99, but agree. I'm more interested in is how your control flow differs from mine; pubs.opengroup.org/onlinepubs/007904875/functions/fread.html says nothing about returning a negative value, though in practice that's what it does. Could you elaborate? \$\endgroup\$ – Neil Edelman Apr 27 at 17:23
  • \$\begingroup\$ Oops, I must have remembered something wrong then. Sorry. I fixed my review. Thanks for notifying me. \$\endgroup\$ – Roland Illig Apr 27 at 17:28
  • \$\begingroup\$ No, I thought so, too; in practice a negative value indicates error, but upon reading the spec carefully, I see no indication, unless it's in one of the links. Usually I use fgets. If I'm understanding this, you could have a partial read and errno would be set, it would return a positive value. \$\endgroup\$ – Neil Edelman Apr 27 at 18:10
  • 3
    \$\begingroup\$ @NeilEdelman "ssize_t and bool are C99" --> ssize_t is not C99. it is not part of any C standard, yet common *nix. \$\endgroup\$ – chux Apr 27 at 19:58
  • 2
    \$\begingroup\$ The only part I would disagree with is replacing for(;;) (read: forever), as that's very idiomatic. \$\endgroup\$ – Deduplicator Apr 28 at 15:22
4
\$\begingroup\$

Roland Illig provided an excellent review; there are a couple of points I'd like to add:

The standard header file <stdio.h> defines the macro/constant BUFSIZ. This macro was developed primarily for input and output buffers. In the original C it was defined as 1024, but now it varies from system to system, probably based on the file system blocking size.

It might have been better to define to define the character array using BUFSIZ.

The assert() macro is generally useful as a debugging tool and not included in production code. It will be optimized out of the code if the macro NDEBUG is defined.

The first time I read through the code, I missed all of the break; statements; it might be better if each statement is on a separate line.

\$\endgroup\$
  • \$\begingroup\$ In your opinion, is not ferror an absolute guarantee that the return value is between [0, read]? \$\endgroup\$ – Neil Edelman Apr 28 at 21:24
3
\$\begingroup\$

Reading a text file that may have embedded nulls

fgets() is simply not the best tool to to do this.

Code that well handles text files with null characters employs *nix getline() or similar functions.

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

int main(void) {
  size_t sz = 0;
  char *buf = NULL;
  unsigned long long line = 0;
  ssize_t count;
  char ch = '\n';
  while ((count = getline(&buf, &sz, stdin)) > 0) {
    printf("%llu", ++line);
    for (ssize_t i = 0; i < count; i++) {
      ch = buf[i];
      printf(" %02hhX:", ch);
    }
  }
  if (ch != '\n') {
    printf("\n");
  }
  free(buf);
  if (ferror(stdin)) {
    perror("stdin");
    return EXIT_FAILURE;
  }
  return EXIT_SUCCESS;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If you're using GNU C99, I think this is the way to go. \$\endgroup\$ – Neil Edelman Apr 27 at 21:42
2
\$\begingroup\$

I would normally use ftell and allocate the buffer size needed, but I assume that you have some non-standard Unix Distro with specific environment or compiler settings, if you say that ftell doesnt work sometimes.

I provide an example with fix buffer size, just for demonstration. Buffer size reallocation and more error checking would probably be needed for production code.

Regarding code, generally, I always preferr staying generic and grouping code in functions. It's not only making code easier to read, but also makes a thinking process easier. And there's a reusabillity point too, of course.

So this would be my refactoring - suggestion (I didn't pay much attention to your error codes. And it could be that you have to put declarations on top, if you are using some older C compiler like C89 etc.):

#include <stdio.h>

#define GRANULARITY 80
#define MIN(a, b) (((a) < (b)) ? (a) : (b))

size_t read_file(FILE* fd,      // file descriptor
                 char *buf, // buffer
                 size_t size); // buffer size

int main(int argc, char* argv[])
{
    char buf[1000];
    size_t num_read = read_file(stdin, buf, sizeof(buf));

    if (num_read > 0)
    {
        for (int i = 0; i < sizeof(buf); ++i)
            printf("%02hhX ", buf[i]);
    }
    else if (errno == ERANGE)
    {
        // error - there are more bytes to read, handle it
    }   
    else if (ferror(fd))    
    {
         // stream error - handle it
    }

    return 0;
}

size_t read_file(FILE* fd,
                 char *buf,
                 size_t size)
{
    size_t num_read = 0;
    char *pos = buf;
    size_t n = 0;

    while ( (n = fread(pos, 1, MIN(GRANULARITY, buf+size-pos), fd)) > 0 )
    {
        num_read += n;
        pos += n;
    }

    if (!feof(fd))
        errno = ERANGE;   // buf too small - there are more bytes to read

    return num_read;

Edit: After reading some comments: changed the interface return type from ssize_t to size_t and moved the error-code checking responsibility to the caller.

\$\endgroup\$
  • \$\begingroup\$ That's a good generalisation. If you set errno to eg, ERANGE, and always returned -1, then the code would have one less level of errors to check for. My intention was to write it to work on any compliant C90 system, so I've been careful what assumptions I'm making, viz c-faq.com/osdep/filesize.html and c-faq.com/stdio/textvsbinary.html. \$\endgroup\$ – Neil Edelman Apr 27 at 17:58
  • 1
    \$\begingroup\$ When reading large files that can fit in size_t, num_read += n; can readily exceed ssize_t range. Recommend staying with size_t and not convolute with ssize_t. \$\endgroup\$ – chux Apr 27 at 20:13
  • 1
    \$\begingroup\$ Pedantically if (ferror(fd)) can be true even with no read errors here, as ferror() can be true before read_file() is called. \$\endgroup\$ – chux Apr 27 at 20:15
  • 1
    \$\begingroup\$ @chux: yes, that's good point. have considered it in the edit. also regarding ferror(fd) checking - it's the responsibility of the caller now - at my opinion, that would be more correct regarding interface semantics. the caller should check ferror(fd) before and after calling the read_file funciton. this also very similar with the standard library fread(). \$\endgroup\$ – StPiere Apr 28 at 8:14

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.