5
\$\begingroup\$

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$
\$\endgroup\$
11
  • \$\begingroup\$ perror("open") would be better. \$\endgroup\$ May 31, 2015 at 10:50
  • \$\begingroup\$ Also writing errors on stderr would be better. \$\endgroup\$ May 31, 2015 at 10:51
  • \$\begingroup\$ Also don't hesitate to use 4096 or 8192 buffers instead of 20, greatly reducing the number of syscall (which are slow). \$\endgroup\$ May 31, 2015 at 10:52
  • \$\begingroup\$ 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. \$\endgroup\$ May 31, 2015 at 10:54
  • 1
    \$\begingroup\$ Go ahead remove them, but this make no sense to me, they are not answers, just atomic points. \$\endgroup\$ Jun 1, 2015 at 5:26

2 Answers 2

13
\$\begingroup\$

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:");
\$\endgroup\$
7
  • \$\begingroup\$ 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. \$\endgroup\$ May 30, 2015 at 19:31
  • \$\begingroup\$ Do not encourage error prone constructs such as while(numBytesRead = read(fd, buf, bufsize)) \$\endgroup\$
    – chqrlie
    May 31, 2015 at 18:45
  • \$\begingroup\$ 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. \$\endgroup\$
    – chqrlie
    May 31, 2015 at 19:35
  • \$\begingroup\$ Thanks @chqrlie, that's the kind of reasoning I was looking for. I updated my post. \$\endgroup\$
    – janos
    May 31, 2015 at 19:49
  • \$\begingroup\$ 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. \$\endgroup\$
    – chqrlie
    May 31, 2015 at 20:56
8
\$\begingroup\$

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.