Implementation of Linux cp in modern C++

Question

This is my simple implementation of the Unix cp command. It is a C++ program but makes use of some of the lower level C library I/O functions.

Any suggestions for style or performance improvements would be appreciated.

#include<iostream>
#include<fstream>
#include<string>
#include<vector>
#include<fcntl.h>
#include<unistd.h>
int main(int argc,char* argv[]) {
    // cmd : cp file1 file2

    if ( argv[1] == nullptr || argv[2] == nullptr ){
        std::cout << "missing arguments: moderncp file1 file2 ";
        return 0;
    }
    const std::string input{argv[1]};
    const std::string output{argv[2]};

    // create a vector with 1024 byte allocated 
    std::vector<char> buf(1024);
    // input file descriptor 
    int infd = open(input.c_str(),O_RDONLY);
    // output file descriptor
    int outfd = open(output.c_str(),O_WRONLY|O_CREAT|O_EXCL,0664);

    while(true){
        auto count = read(infd,buf.data(),buf.size());
        if (count == 0) break;

        auto num_written = 0;
        auto num_to_write = count;
        while(num_written < num_to_write){
            num_written += write(outfd,buf.data()+num_written,count-num_written);

        }
    }
    close(infd);
    close(outfd);

    std::cout << input << '\t' << output ;
}

Answer 1

I see some things that may help you improve your code.

Fix the bug

The program loops infinitely (actually, as @CrisLuengo correctly points out in a comment, it's not actually infinite. However, it loops \$2^{54}\$ times for each 1024 bytes in the input file on a 64-bit machine, so it will certainly feel infinite for any human being waiting for the copy to complete!) if the output file already exists or can't be created. That's a bug that should be fixed. Generally, when a call can fail (as with open) it is important to test the result for failure before acting on the result.

Use std::fstream for portability

This could be a completely portable program using only standard C++ libraries. To do that, you'd omit <fcntl.h> and <unistd.h> and just use fstream. I'm guessing you may have chosen to use the low-level functions for speed, but it's important to measure rather than assume. The results may surprise you!

Delete file before writing for speed

On several Linux platforms, overwriting an existing program is slower than creating one. For that reason, one way to make sure that the program is as fast as possible is to call std::remove before doing the copy.

Return valid error codes

A value of 0 as an error indicates no error on Linux and many other platforms. If you execute cp with no arguments, it returns with a value of 1. If your program is intended to be an exact replacement, I'd recommend returning 1 instead of 0 if there are missing arguments.

Check for arguments using argc

We can determine if there are the correct number of arguments by examining argc. The way you're doing it has a problem:

if ( argv[1] == nullptr || argv[2] == nullptr ){

The problem is that if there are zero arguments, argv[1] and argv[2] might not be pointing to anything, so dereferencing them is undefined behavior and therefore not good programming practice. In particular, you should be aware that argc might be zero which would make testing argv[1] undefined behavior.

Don't use std::string if what you need is a char *

In this case, both of the command line arguments are turned into std::strings but then used exclusively with c_str() to turn them back int char *. Better would be to simply use char * without conversions to and from std::string.

Prefer std::array to std::vector

If the bounds of your std::vector are known at compile-time (as in this case), then it's generally better to use std::array rather than std::vector. The std::vector has provisions for resizing that this program doesn't use and doesn't need. In fact, for this program, there's actually not much difference between a plain C-style array and a std::array.

Name your constant

The buffer size is 1024 bytes, which is probably a bit on the small size. I think it would be a slight improvement to increase the size and also to name that constant.

Think about non-file files

Under Linux, we have not just files but also devices and named pipes. Think about whether your program does what you intend if the input is a named pipe. What happens if we try to copy to /dev/null ?

An alternative version

Using most of those suggestions, here's an alternative:

#include <iostream>
#include <fstream>
#include <cstdio>

int main(int argc, char *argv[]) {
    if (argc != 3) {
        std::cout << "Usage: moderncp srcfile destfile\n";
        return 1;
    }
    std::ifstream in{argv[1]};
    std::remove(argv[2]);
    std::ofstream out{argv[2]};
    if (!out) {
        std::cerr << "Could not create output file " << argv[2] << '\n';
        return 1;
    }
    static constexpr std::size_t buffsize{1024};
    char buffer[buffsize];
    while (in.read(buffer, buffsize)) {
        out.write(buffer, buffsize);
    }
    out.write(buffer, in.gcount());
}

Note that because we're using fstreams for input and output, we don't have to explicitly close the files because the destructor will do so automatically as well as flushing the output buffer.

Results

On my 64-bit Linux machine, copying a 5.3GiB file took 1:29 with the original program, 1:25 with this rewritten version and 1:21 with cp. Changing the rewritten version to use an 8K buffer allowed it to complete in 0:59, so that's clearly an improvement.

Answer 2

I am going to focus on these two lines of code:

int infd = open(input.c_str(),O_RDONLY);
int outfd = open(output.c_str(),O_WRONLY|O_CREAT|O_EXCL,0664);

Since you are making a point of using modern C++ to reimplement cp, I would expect you to use the modern C++ way of writing files. Instead, you use POSIX functions that don't even have a namespace...

If you don't want to use the streams, at least consider using std::fopen. The only thing that you cannot do using functions from the Standard Library is to set file permissions, but you don't want to do this anyway. The user will have set the umask according to personal preferences, you don't need to deviate from that.

What happens if input doesn't exist? What happens in output cannot be created? To the user, the result is identical to a successful file copy. There is no way to know if the cp call succeeded. You should check the values of infd and outfd, which will be -1 in case of failure, and return an error code. You can even check errno and give the user some more information about why the file couldn't be opened or created.

Actually, if output cannot be created, you end up in a very long loop that ends with UB:

 auto num_written = 0; // This is an `int`.
 while(num_written < num_to_write){
     num_written += write(outfd,buf.data()+num_written,count-num_written);
 }

write returns -1 on failure (outfd being not valid is cause for failure of course), so num_written will be decremented by 1 until it overflows. Overflow is UB for signed integers. Hopefully your implementation will lead to num_written to be very large at this point, breaking the loop.

Answer 3

The command

// cmd : cp file1 file2

You then say:

    std::cout << "missing arguments: moderncp file1 file2 ";

Is it cp file1 file2 or moderncp file1 file2?

In addition, I would provide more extensive documentation. What does cp do? The comment: // cmd : cp file1 file2 should be comprehensive enough for me to understand what cp is and I should not have to use Google to figure it out. Using something like Doxygen to provide more incentive to document may be helpful.

Handling Failure

    return 0;

The program failed. I have used EXIT_FAILURE in the past (in C), however, this would require cstdlib which I am not sure is a good idea. Nonetheless, 0 is reserved for a successful run, even return 1; would be better. (Maybe someone can comment on this?)

Comments

// create a vector with 1024 byte allocated 

Ok, I understand you created a 1024 byte vector, but you have not told me why. It's named buf so I know it is a buffer, but for what?

Style

C++ does not have an equivalent to PEP 8, so technically as long as your style is consistent it is "correct". However, I usually see a blank line (or two) between the includes and functions, so:

#include<unistd.h>
int main(int argc,char* argv[]) {

becomes:

#include<unistd.h>

int main(int argc,char* argv[]) {

I have also seen spaces between arguments more often than not, so:

int infd = open(input.c_str(),O_RDONLY);

becomes:

int infd = open(input.c_str(), O_RDONLY);