# Showing the contents of a file

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:
// 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 {
char 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\$

• perror("open") would be better. – Julien Palard May 31 '15 at 10:50
• Also writing errors on stderr would be better. – Julien Palard May 31 '15 at 10:51
• Also don't hesitate to use 4096 or 8192 buffers instead of 20, greatly reducing the number of syscall (which are slow). – Julien Palard May 31 '15 at 10:52
• For the filename buffer it exists a constant named PATH_MAX, better use a system dependent define that a hard coded and obviously too short one. – Julien Palard May 31 '15 at 10:54
• Go ahead remove them, but this make no sense to me, they are not answers, just atomic points. – Julien Palard Jun 1 '15 at 5:26

### Magic numbers 20 and 19?

char fileName[20];

// Get the filename from the user:


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];
}


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

int bufsize = 20;
char 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;
}
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.

// 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:");

• Thanks for the suggestion to not repeat the read() of the file statement. I had not placed that in my answer, as I did not want to make suggestions that might confuse the OP. – user3629249 May 30 '15 at 19:31
• Do not encourage error prone constructs such as while(numBytesRead = read(fd, buf, bufsize)) – chqrlie May 31 '15 at 18:45
• This construct is error prone because of the combined assignment/test. I recommend writing: while((numBytesRead = read(fd, buf, bufsize)) > 0). gcc -Wall -W -Werror does not let me compile your code, and also finds more subtle problems and bugs. – chqrlie May 31 '15 at 19:35
• Thanks @chqrlie, that's the kind of reasoning I was looking for. I updated my post. – janos May 31 '15 at 19:49
• The OP uses the low level system call interface instead of the standard library stream functions from <stdio.h>. This is not a good idea in general. The call  write(1, buf, numBytesRead); for instance may return a number less than numBytesRead if writing to a device or a pipe, or if it was interrupted by a signal. The programmer needs to handle these conditions. It is much simpler and not necessarily less efficient to use higher level functions. – chqrlie May 31 '15 at 20:56

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.

• Your post is a little difficult to understand. I recommend that you format your code properly. – SirPython May 30 '15 at 19:02
• well, I followed the suggestion to just copy the comments to an answer. I'll modify the formatting now. – user3629249 May 30 '15 at 19:04
• As I wrote that line, I was thinking that I was missing a detail. I have since removed that suggestion. – user3629249 May 30 '15 at 19:29
• @user3629249 I doubt anyone suggested you format prose as code. (See edit). – OJFord May 30 '15 at 19:54