Copy a directory from source to destination

Question

I wrote a program that copies a directory from a source to a destination, similar to the cp utility.

For this, I used the essential functions:

open(), write(), mkdir(), readdir().

This is the source code (ignore the main() function, this is not the final form):

#include <dirent.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>

int isDirectory(char*);
int copyFile(char*, char*);
int copyDirectory(char*, char*);

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

  copyDirectory(*(argv + 1), *(argv + 2));

  return 0;
}

int isDirectory(char *path) {
   struct stat statbuf;

   if (stat(path, &statbuf) == -1) {
      return -1;
   }
   else {
      return S_ISDIR(statbuf.st_mode);
   }
}

int copyFile(char *source, char *destination) {
  if((source == NULL) || (destination == NULL)) {
    return -1;
  }

  struct stat statbuf;
  int fdr, fdw, rv;
  char buff[256];

  stat(source, &statbuf);
  if(!S_ISREG(statbuf.st_mode)) {
    return 1;
  }

  fdr = open(source, O_RDONLY);
  fdw = open(destination, O_CREAT|O_WRONLY, S_IREAD|S_IWRITE);

  while((rv = read(fdr, &buff, 128))) {
    write(fdw, &buff, rv);
  }

  return 0;
}

int copyDirectory(char *source, char *destination) {
  if((source == NULL) || (destination == NULL)) {
    return -1;
  }

  struct dirent *entry;
  DIR *dr;
  char sourcePatch[512], destinationPatch[512];

  strcpy(sourcePatch, source);
  strcpy(destinationPatch, destination);
  strcat(destinationPatch, "/");
  strcat(sourcePatch, "/");

  if(!isDirectory(source)) {
    return -1;
  }

  if(opendir(destination) == NULL) {
    mkdir(destination, 0700);
  }

  dr = opendir(source);
  while((entry = readdir(dr))) {
    strcat(sourcePatch, entry->d_name);
    strcat(destinationPatch, entry->d_name); //printf("%s\n", sourcePatch);

    if(isDirectory(sourcePatch) && strcmp(entry->d_name, ".") && strcmp(entry->d_name, "..")) {
      copyDirectory(sourcePatch, destinationPatch);
    }
    else {
      copyFile(sourcePatch, destinationPatch);
    }

    *sourcePatch = '\0'; *destinationPatch = '\0';
    strcpy(sourcePatch, source);
    strcpy(destinationPatch, destination);
    strcat(destinationPatch, "/");
    strcat(sourcePatch, "/");
  }

  return 0;
}

Is there a better way for doing this?

Comparing my program, to the GNU cp program, I get this results:

[marius@arch labs]$ gcc -Wall cp.c -g -O3 -o cp
[marius@arch labs]$ time ./cp ~/Desktop/management ~/Desktop/testtest

real    0m0.139s
user    0m0.003s
sys     0m0.133s
[marius@arch labs]$ time cp -r ~/Desktop/management ~/Desktop/testtest2

real    0m0.018s
user    0m0.000s
sys     0m0.017s

And what about this code, can I make some improvements?

Answer 1

Overall, it is good, but I would apply the following changes:

• Use const for read only function parameters. All the char* pointers you are passing to the functions are actually only read from, so you should use const char* to document that.

• Personally, I'm not a big fan of returning -1, 0, 1, etc. This kind of error codes are popular with UNIX system calls, but I find them rather error prone. It is easy to make a mistake with the return value of a function if you don't carefully read the documentation. If the function has only two possible outcomes, success and failure, then include <stdbool.h> and return true or false. There is no chance of misunderstanding it this way.

• A few magic numbers that could be named constants. For example, why did you choose 512 and 256 for buffer sizes? That is not obvious. Named constants would be better for these cases. Also, even though the 0700 constant on mkdir is well known for UNIX users, it might not be so for others less familiar with the architecture. I would also make that a named constant.

• I suggest keeping the parameter names on function prototypes. In int copyFile(char*, char*); which one is the source file and which is the dest? Can't tell for sure just by reading the function prototype. Parameter names would make that clear without having to look at the implementation.

• I suspect you have too many #includes for such a tiny program. <time.h> is certainly not used here. Make sure to only import the dependencies that your program actually requires.

• A few system calls are not being checked for failure, so your program is not as robust as it could be.

Answer 2

• A general note: the code is very optimistic. Always test the return value of the system call, or you may end up in an inconsistent state.

• Performance wise, buf[256] is quite small. The most expensive part of the code are system calls, and you want to minimize a number of times you call read and write. Larger buffer (I mean, really large, 100M perhaps) will speed up the execution significantly.

• You may try to take a full advantage of sendfile call (where available).

• You may want to inspect directory entries for being hard links to the same file. Depending on the use case, it might be beneficial to preserve the hard link structure.

Answer 3

• use PATH_MAX, pathconf or dynamic size for the filename buffers
• for files your buffer is 256 bytes but you copy in chunks of 128 bytes, only using half the buffer
• stat() values include a prefered blocksize for IO. You should use at least that much for buffering.
• don't you care about directory and file permissions?
• don't you care about extended attributes? (well, maybe not)