Showing the contents of a file

Question

This program reads a filename from standard input and then prints its content. Please review:

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>

int main()

{
    char fileName[20];

    // Get the filename from the user:
    int fileNameLength = read(0,fileName,19);
    // We need to get rid of the new line character caused by terminal
    // Replace the new line character with \0 
    fileName[fileNameLength-1] = fileName[fileNameLength];

    printf("You want to see the contents of: %s\n", fileName);

    // open the file:
    int fd = 0;
    fd = open(fileName,O_RDONLY);

    if(fd == -1) {
        // Something went wrong, perhaps no such file:
        perror(NULL);
        printf("%s\n", fileName);
    } else {
        // Read until the read method returns 0 bytes. 
        char buf[20];
        int numBytesRead = read(fd,buf,20);
        while(numBytesRead) {
            write(1,buf,numBytesRead);
            numBytesRead = read(fd,buf,20);
        }
        close(fd);
    }
    puts("");
}

And in action:

Korays-MacBook-Pro:~ koraytugay$ gcc koray.c
Korays-MacBook-Pro:~ koraytugay$ ./a.out 
k.txt
You want to see the contents of: k.txt
Hello Code Review!
This is the contents of k.txt.
Have a good day!
Korays-MacBook-Pro:~ koraytugay$

Answer 1

Magic numbers 20 and 19?

Instead of this:

char fileName[20];

// Get the filename from the user:
int fileNameLength = read(0,fileName,19);

Define MAX_FILENAME_LENGTH somewhere, and use that as the buffer size parameter, and MAX_FILENAME_LENGTH + 1 as the array size containing the buffer, to account for the terminating null character.

Write in code your intention exactly

Write in code more explicitly what you want. For example here you say in comment that you want to write a \0 to the final position of the char[], but the code doesn't do exactly that:

// We need to get rid of the new line character caused by terminal
// Replace the new line character with \0 
fileName[fileNameLength-1] = fileName[fileNameLength];

Don't assume what might be at an uninitialized memory location. If you want to set the terminating character to \0, then do exactly that:

fileName[fileNameLength-1] = '\0';

More magic number 20

All those 20 everywhere:

char buf[20];
int numBytesRead = read(fd,buf,20);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,20);
}

Why not introduce a variable so that you can change it later if you want to:

int bufsize = 20;
char buf[bufsize];
int numBytesRead = read(fd,buf,bufsize);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,bufsize);
}

And why use a buffer of size 20? Why read a file 20 byte at a time? It would be faster to read in larger chunks. Surely you have enough memory to load 4 kbyte at a time. Luckily, now it's easy to change that:

int bufsize = 4096;

Avoid code duplication

In the previous code snippet, read(fd,buf,bufsize); appears twice, which is not pretty. It can be rewritten without such duplication:

int numBytesRead;
while ((numBytesRead = read(fd, buf, bufsize)) > 0) {
    write(1, buf, numBytesRead);
}
close(fd);

On the other hand, some people find this writing style potentially error prone or confusing. In my opinion code duplication is the bigger evil, so I still prefer this writing style, which is also shorter.

Comments stating the obvious

// open the file:
int fd = 0;
fd = open(fileName,O_RDONLY);

Is that comment really necessary there? Or is it just noise?

Pointless variable initialization

int fd = 0;
fd = open(fileName,O_RDONLY);

If you're going to set fd to something else, why set it to 0?

Usability

When you run the program, it prints nothing, it's just waiting for user input. It would be better to print a prompt, for example:

puts("Enter file name:");

Answer 2

Running the posted code through gcc, with parameters:

-Wall -Wextra -pedantic

causes the compiler to raise three warnings.

1. unused variable: 'fileFound'
2. unused parameter: 'argc'
3. unused parameter: 'argv'

Please, where posting code that 'works' be sure that it cleanly compiles.

I suggest using int main( void) rather than int main(int argc, char const *argv[]), and removing the lineint fileFound = 0;`.

Regarding the call to read() to get the file name from the user:

1. Always check the returned value from read() to assure the operation was successful. (in the future, I suggest using getline()).
2. Regarding this line fileName[...-1] = fileName[...], which is copying a trash character over the last input character from the user, I suggest: fileName[fileNameLength-1] = '\0'; // eliminate newline to properly terminate the fileName string

When wanting the user to input something, (in this case, a file name), the code should always output a prompt that tells the user what they are expected to input. Otherwise the user is left looking at a blinking cursor and no indication of what they (the user) is expected to do.

Regarding the line while(numBytesRead) {, if there is any kind of read() error, then the returned value will be < 0, which will not cause the while loop to exit. I suggest while( 0 < numBytesRead ) { instead.

The latest C standard allows not placing a return(0); statement at the end of the main() function. However older versions do not make the return(0); assumption; so I suggest inserting a final line containing the return(0); statement.