3
\$\begingroup\$

I wrote a program that implements the same function as the mv command.

I know this is not the best implementation, so I would like some suggestions for improvement, possible problems. What you should change?

code:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <sys/stat.h>
int main(int argc, char **argv)
{
  char *source, *destination, *new_source, *new_destination;
  char *current_directory;
  if (argc != 3) {
    printf("usage- %s source destination\n", *argv);
    exit(EXIT_FAILURE);
  }
  // work on copy
  source = (char*)malloc(strlen(argv[1]) + 1);
  strcpy(source, argv[1]);
  destination = (char*)malloc(strlen(argv[2]) + 1);
  strcpy(destination, argv[2]);
            
            current_directory = getenv("PWD");
            new_source = (char*)malloc(strlen(source) + 1 + strlen(current_directory) + 1);
            strcpy(new_source,current_directory);
            strcat(new_source,"/");
            strcat(new_source,source);
  new_destination = (char*)malloc(strlen(destination) + 1 + strlen(current_directory) + 1 + strlen(source) + 1);
  strcpy(new_destination,current_directory);
  strcat(new_destination,"/");
  strcat(new_destination,destination);
  strcat(new_destination,"/");
  strcat(new_destination,source);
  
  /*execute systemcall*/
  if(rename(new_source,new_destination) != 0){
    fprintf(stderr,"eroare: %s\n",strerror(errno));
  }
  free(new_source);
  free(new_destination);
  free(source);
  free(destination);
  exit(EXIT_SUCCESS);
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Nice to see the use of the standard EXIT_SUCCESS and EXIT_FAILURE.

The return value from the rename() system call can actually be a check against EXIT_SUCCESS because that system call is returning either EXIT_SUCCESS or EXIT_FAILURE.

From main() Use Return Rather Than Exit

Since all of this is happening in the main() function there is no reason to use the exit() function. Since the operating system entry point is main() return and exit are equivalent.

Too Much Work

It isn't clear why it was necessary to copy the strings from argv to strings, it should be easy enough to use the argv strings directly, this removes the need to call malloc() and free() which also use system calls and therefore affect the performance of the program.

Error Prevention

The code should check that the source file named in argv[1] exists and can be opened.

\$\endgroup\$
3
\$\begingroup\$
  • Never cast what malloc returns. It is not necessary, and may lead to hard-to-find bugs.

  • strcpy(), strcat(), strcat() sequence is anti-idiomatic. Prefer

     sprintf(new_source, "%s/%s", current_directory, source);
    
  • Prefixing the source and destination with PWD seriously limits usability of the program: it can't deal with absolute paths at all.

    Along the same line, the destination is always treated like a directory name. It is not how mv is supposed to wrok.

\$\endgroup\$

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.