4
\$\begingroup\$

The goal of this function is to print the contents of a file. Pretty much what the simple invocation of cat does.

Since C is famously hard, I am looking forward to your comments.

void showlog(char const * const filename) {
    FILE *f = fopen(filename, "r");
    if(f == NULL) {
        printf("ERROR: failed to open file!\n");
        exit(1);
    }

    const int bufsz = 4000;
    char buffer[bufsz];
    while(1) {
        char* b = fgets(buffer, bufsz, f);
        if(b != buffer) {
            break;
        }
        printf("%s", buffer);
    }

    fclose(f);
}
\$\endgroup\$
  • \$\begingroup\$ Is the file always expected to be text or potentially binary data? \$\endgroup\$ – chux Jun 11 '18 at 13:45
  • \$\begingroup\$ regarding: printf("ERROR: failed to open file!\n"); Error messages should be output to stderr, not stdout and when the error is indicated by a C library function, should also output (to stderr) the reason the system thinks the error occurred. The easiest way to do this is call perror( "your error message" ); \$\endgroup\$ – user3629249 Jun 11 '18 at 14:13
  • 1
    \$\begingroup\$ BTW: C is not hard. Now, prolog or modula or ada is hard \$\endgroup\$ – user3629249 Jun 11 '18 at 14:20
  • \$\begingroup\$ @user3629249 Ada is just annoying \$\endgroup\$ – an earwig Jun 11 '18 at 18:05
  • 3
    \$\begingroup\$ I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you. \$\endgroup\$ – Martin R Jun 12 '18 at 8:36
6
\$\begingroup\$

From top to bottom without any specific order.

Validate your inputs, if they do not come from user or configuration and you're reasonably sure that they're valid then assert about them. The minimum needed to prevent random unexpected behaviours and access violations:

assert(filename);

To declare preconditions will help future readers to quickly understand them and it's an invaluable help during debugging (also, and especially, together with unit testing when running them in the local machine compiling with DEBUG).

Do not print errors to stdout, callers may want to redirect output to something else and you do not want to mixup errors with output (especially because errors won't then visible to the caller...) You can use perror() for this:

perror("Cannot open file.");

However when dealing with shell scripts or a list/batch of inputs it may also be helpful to know which file caused an error (and why):

fprintf(stderr, "Error opening file '%s': %s", filename, strerror(errno));

Note that fopen() sets errno to an error code to understand why it failed and strerror() converts that numeric error code to a human-readable error message.

Assuming that to read 4000 bytes each time is the optimum is somehow a simplification but you may like to use a whole (power of 2) number like 4 KiB. Do not hardcode that magic number inside your function, move outside with a meaningful name (like DEFAULT_INPUT_BUFFER_SIZE, just picking one). There are chances you'll need it elsewhere (for command line help or in another I/O function, for example). Simplify your code: fgets(buffer, sizeof(buffer), f): if you rename bufsz (do it) you'll do it only in one place (even better if you decide to make the size dynamic and it's calculated by a function, in this case switch to malloc()).

It's not required but it might be helpful during debugging to initialise this buffer to \0, if you want to then simply add = "" to the declaration. From now on I will assume that you're reading a text file (because of fgets() and "r"), if it's not true then you should switch to fread().

Expression b != buffer is slightly misleading. fgets() returns NULL when it can't read anything or str (its input) otherwise. You can do it this way but it's...unusual. while (fgets(...)) is much more common.

Technically you know you reached EOF before attempting a new read, you may check feof() but I think it's a nano-optimisation you can just ignore. You should, however, check for errors: fgets() returns NULL also in that case:

while (fgets(buffer, bufsz, f)) {
}

if (ferror(f)) {
    perror("Error reading input file...");
}

Do not forget that also fclose() may fail (for example because buffer is flushed or media is not accessible). Check its return value as we did for fopen().

Note: in real world please do not use cat to display a text file content: there are specific commands (which do even more) like less, more, tee and many others.


I leave few notes as exercises and tips to explore different solutions.

What will happen if your program does not terminate immediately and the input file does not ends with a newline?

Assuming to process each chunk (for example to remove blank lines) is fgets() the best solution? Consider getline() for such cases.

\$\endgroup\$
  • \$\begingroup\$ Do not puts. It adds a newline, and the output is not verbatim anymore. \$\endgroup\$ – vnp Jun 11 '18 at 19:17
  • \$\begingroup\$ @vnp right, fixed. Tnx! \$\endgroup\$ – Adriano Repetti Jun 11 '18 at 20:46
  • \$\begingroup\$ Do you have any suggestions about how to test the cases that you fixed? For example, when might fgets fail? \$\endgroup\$ – Gregor P. Student Jun 12 '18 at 8:09
  • \$\begingroup\$ Also, considering that there is nothing else I am allowed to do according to the man page, how am I supposed to treat errors when calling fclose? \$\endgroup\$ – Gregor P. Student Jun 12 '18 at 8:11
  • \$\begingroup\$ Inform user and return proper error code! If something failed then operation has not been completed. Imagine if you cp something to your backup disk and it copied only a part of it but you don't know... \$\endgroup\$ – Adriano Repetti Jun 12 '18 at 8:48
1
\$\begingroup\$

this:

while(1) {
    char* b = fgets(buffer, bufsz, f);
    if(b != buffer) {
        break;
    }
}

does not quite do what you want. Suggest:

while( fgets( buffer, bufsz, f) )
{
    printf("%s", buffer);
}
\$\endgroup\$
  • \$\begingroup\$ I understand that the second is more idiomatic but it does the same thing, right? \$\endgroup\$ – Gregor P. Student Jun 12 '18 at 8:08
  • \$\begingroup\$ Code should always be written for ease of readability and understanding. Code should be written to be neither 'obscure' nor 'convoluted'. Employers are strongly against anything that is not crystal clear. This is mostly because many others, besides your self, will be maintaining the code. If you really want to write 'obscure', etc code, there are several groups, contests, etc for that purpose \$\endgroup\$ – user3629249 Jun 13 '18 at 13:05
  • \$\begingroup\$ @GregorP.Student It is the same function, yet less ideomatic. \$\endgroup\$ – chux Jun 13 '18 at 22:29
1
\$\begingroup\$

The goal of this function is to print the contents of a file

What does it take to print everything and nothing else?

Do not open in text mode

If the program might be applied to a non-regular text file, OP's use of fopen(filename, "r"), fgets() and printf("%s"...) rely on the reading/writing translation of end-of-line characters to be non-lossy. While this is not a common concern, on a machine that expects a "\r\n" at the end of each line, input like "123\n456\r\n789\r" may not output "123\n456\r\n789\r". Recommend to use binary mode.

// FILE *f = fopen(filename, "r");
FILE *f = fopen(filename, "rb");

Text files that do not end in a end-of-line can be troubling too. By going to binary, these text translation issues are moot.

If this approach is taken, code also needs to switch stdout to binary mode.

FILE *out_file = freopen(stdout, "wb");

Do not use fgets()

There is no requirement to read a line at a time. fgets() is a troublesome function to use if the input has null characters '\0' in it. On reading with fgets(), a null characters is not special. Finding the true length of a buffer that has, say "abc\0def" just read into it, is problematic. As printf("%s", buffer); prints only up to that first null character, the "\0def" is lost. Used fread()/fwrite(). This simpler approach likely results in faster I/O too.

unsigned char buffer[4096];
size_t in;
while ((in = fread(buffer, 1, sizeof buffer, f) > 0) {
  size_t out = fwrite(buffer, 1, in, stdout);
  if (in != out) {
    Handle_Trouble();
  }
}

Null characters are common in UTF-16 text files.

Use unsigned char

On exceeding rare machines, char contains a trap representation. Use unsigned char for data of uncertain providence.


Of course reading a file with random data may be print non-sensible output, but it will be a true echo of the file. Often when wanting to print text, code will examine the input for non-printable characters and print some alternative text: some sort of data dump, but that is beyond this post.

\$\endgroup\$
  • \$\begingroup\$ I suppose writing in this way does not actually do anything useful if the encoding of the terminal is different from the encoding in the file, right? How would I handle that properly? \$\endgroup\$ – Gregor P. Student Jun 14 '18 at 12:11
  • \$\begingroup\$ @GregorP.Student Files do not need an encoding to "print the contents of a file" via cat as code can simply read a character and then print it - and the output is whatever. Yet to print using a certain encoding on output, your code requirement needs to change from "what the simple invocation of cat does" to "print ASCII" or "print to any discernible UTF encoding." In such cases, the input stream needs to be characterized (example, see BOM and then qualified as it is read. The output encoding poses limitations, can it print "العربية한국어中文"? \$\endgroup\$ – chux Jun 14 '18 at 12:22
1
\$\begingroup\$

In addition to Adriano's answer:

You can replace

printf("%s", buffer);

with

fputs(buffer, stdout);

This avoids all of the excess backend code used for parsing in printf(). You could also use write(), given your context.

write(1, buffer, strlen(b));

Or, even better, don't use strlen() at all and instead use fread() and fwrite(). Here is my take on this function:

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

#define BUFF_SIZE 4096

void showlog(char const *const filename) {
    FILE *f = fopen(filename, "r");
    size_t nmemb = 0;
    char buffer[BUFF_SIZE];

    if(!f)
        error(EXIT_FAILURE, 0, "Could not open file");

    while((nmemb = fread(buffer, 1, sizeof buffer, f)) > 0)
        fwrite(buffer, 1, nmemb, stdout);

    if(ferror(f))
        error(EXIT_FAILURE, 0, "Error reading file");

    if(fclose(f) == EOF)
        error(EXIT_FAILURE, 0, "Could not close file");
}

You could also incorporate chux's suggestion on opening the file in binary mode.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is definitely better given the question that I asked but of course eventually I want to do some linewise processing. ;) \$\endgroup\$ – Gregor P. Student Jun 14 '18 at 12:07
  • \$\begingroup\$ @GregorP.Student Good point, the particular solution you choose to use will depend on how you plan to use this function. \$\endgroup\$ – esote Jun 14 '18 at 12:22

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.