Thread: Problem with C function to copy file

Hello everyone. First time poster here.

I'm writing 2 functions that will copy a file. The first version of my function seems to work without any problem. I say seems to as I haven't yet run any kind of diff command to test to see if the files are actually different. I'm doing this on DOS 6.22, believe it or not, so I don't have access to a diff command.

The first version is based on some code I found online that uses fgets to copy the file line by line. I think this will only work for text files, is that correct?

Code:

int copyFile(char sFile[256], char tFile[256]) {
    char line[128];


    FILE *sourceFile;
    FILE *targetFile;


    if (fileExists(sFile) && fileExists(tFile)) {
        sourceFile = fopen(sFile,"r");
        targetFile = fopen(tFile,"w");
        while (fgets(line,sizeof line,sourceFile) != NULL) {
            fputs(line,targetFile);
        }
        fclose(sourceFile);
        fclose(targetFile);
    } else {
        return(1);
    }
    return(0);
}

I'm able to call this function as follows:

Code:

copyFile("FILEA.TXT","FILEB.TXT")

...and it works. And near as I can tell, there are no differences in the file. However, I'm wanting to figure out this other version of the function which copies 1 character at a time. If I understand this correctly, copying 1 character at a time is better?

My second version of the function works, except that it always puts an extra character at the end of the file. At first I didn't even notice the extra character as it seems to be invisible. However, when I used my function to copy the copy program I was working on, the copied version wouldn't compile because the compiler was catching the extra character.

The second version of the function is based on the code I found here C program to copy files | Programming Simplified and here C program to copy contents of one file to another file - GeeksforGeeks.

Code:

int copyFile(char sFile[256], char tFile[256]) {

    FILE *sourceFile;
    FILE *targetFile;


    char c;


    sourceFile = fopen(sFile, "r");
    if (sourceFile == NULL) {
        printf("Source file doesn't exist.\n");
        return(1);
    }


    targetFile = fopen(tFile, "r");
    if (targetFile != NULL) {
        printf("Target already exists. ABORT!\n");
        return(1);
    }
    fclose(targetFile);


    targetFile = fopen(tFile, "w");
    if (targetFile == NULL) {
        printf("Can't open file for writing! Permission error?\n");
        return(1);
    }


    do {
        c = fgetc(sourceFile);
        fputc(c, targetFile);
    } while (c != EOF);


    fclose(sourceFile);
    fclose(targetFile);


    return(0);
}

To summarize, my questions are:

1. Is the line by line copy program limited to text files only?
2. Is the character copy version better? If so, how?
3. Why is the character copy implementation including extra characters at the end of the file?

Thanks everyone.

Originally Posted by Salem

Originally Posted by blixel

> Is the line by line copy program limited to text files only?

Pretty much. fgets() only works on files that don't contain any \0 characters.
Well it probably does work, but there's no way for you to tell whether the \0's you see are from the file, or those put in by fgets() to mark the end of the string.

Originally Posted by Salem

Originally Posted by blixel

> Is the character copy version better? If so, how?

No.
Copying binary files is best done with fread() and fwrite().

OK, I'll have to look into fread() and fwrite(). Do you have a link, or a code snippet that shows how to use those instead of fgets()?

Originally Posted by Salem

Originally Posted by blixel

> Why is the character copy implementation including extra characters at the end of the file?

Because
1. You declared c as a char rather than an int. If you had a char 0xFF in your file, it would have truncated the file, rather than appending 1 byte to it.
2. You test for EOF at the end of the loop, after you have written something to the file.

Code:

int c;
while ( (c = fgetc(sourceFile)) != EOF ) {
  fputc(c, targetFile);
}

After I posted this message, I did end up figuring out that I had to declare c as int and not char, and found the problem with my original do while loop. I don't have the link where I found that info readily available. Though I did find it odd that the original sources I was using for the code examples both showed c as being char and not int. I guess they just had it wrong and/or never tested it before posting their articles.

Regarding the loop, it wasn't completely clear to me why it wasn't working, but it was obvious that it in fact wasn't working. I ended up replacing the loop with this code:

Code:

    c = fgetc(sourceFile);
    while (c != EOF) {
        fputc(c, targetFile);
        c = fgetc(sourceFile);
    }

In my searching, I ran into the version of the code that you demonstrated above, but this version I'm using is slightly easier for me to wrap my head around.

Originally Posted by Salem

Further notes:
1. When copying binary files, you need to use the modes "rb" and "wb".
2. Ideally, char line[128] should be char line[BUFSIZ]. BUFSIZ is generally set to be compatible with the buffer sizes used throughout the rest of the system, and the size of sectors/blocks on the file system itself.
3. Your various error returns don't close already opened files.

1. Yeah, that was one of the other aspects that I found I had to change.
2. OK. Is BUFSIZ established by the compiler automatically? Or do I have to #define it?
3. Hmm... it's interesting you mention that point. Here is a copy/paste of the code I posted originally for the error handling.

Code:

    sourceFile = fopen(sFile, "r");
    if (sourceFile == NULL) {
        printf("Source file doesn't exist.\n");
        return(1);
    }

    targetFile = fopen(tFile, "r");
    if (targetFile != NULL) {
        printf("Target already exists. ABORT!\n");
        return(1);
    }
    fclose(targetFile);

    targetFile = fopen(tFile, "w");
    if (targetFile == NULL) {
        printf("Can't open file for writing! Permission error?\n");
        return(1);
    }

I found that in the second part, the attempt at closing target file was causing a segmentation fault when I ran this under Debian Linux. After a bit of searching, I ran into this link: c - fclose() causes Segmentation Fault - Stack Overflow. The relevant part of that link had a reply to someone else who had a similar problem, and the person said:

After asserting that fp1 is NULL, you're trying to call close on the null pointer, which will cause a segmentation fault.

After looking at my code again, I decided I needed to take out that fclose() call. Once I did that, the segmentation fault that I was getting under Debian went away.

So I'm at a bit of a loss as to how to properly fclose() on those various error conditions.

The last point. In my original post I mentioned that I didn't have a way to diff the files under DOS. I forgot that DOS has the command, FC.EXE, which is a file comparison utility. After rewriting my do while loop, changing c from char to int, and adding "rb" and "wb" to the file fopens, the files were being copied with no differences according to FC.EXE.

Here's the new version of the code as it stands now, without having added any of the other changes you mentioned here. (Such as BUFSIZ or fread() and fwrite().

Code:

#include <stdio.h>

#define MAXFILENAME 256        /* Maximum file name length */


int copyFile(char sFile[MAXFILENAME], char tFile[MAXFILENAME]);


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


    int copyResult;


    if (argc != 3) {
        printf("Usage: copyfile.exe <source file> <target file>\n");
        return(1);
    } else {


        copyResult = copyFile(argv[1], argv[2]);


        if (copyResult) {
           /* It didn't work my dude */
            return (1);
        }
    }
    return (0);
}


int copyFile(char sFile[MAXFILENAME], char tFile[MAXFILENAME]) {


    FILE *sourceFile;
    FILE *targetFile;


    int c;


    sourceFile = fopen(sFile, "rb");
    if (sourceFile == NULL) {
        printf("Source file doesn't exist.\n");
        return(1);
    }


    targetFile = fopen(tFile, "r");
    if (targetFile != NULL) {
        printf("Target already exists. Aborting...\n");
        return(1);
    }
    
    targetFile = fopen(tFile, "wb");
    if (targetFile == NULL) {
        printf("Can't open file for writing! Permission error?\n");
        return(1);
    }


    c = fgetc(sourceFile);
    while (c != EOF) {
        fputc(c, targetFile);
        c = fgetc(sourceFile);
    }


    fclose(sourceFile);
    fclose(targetFile);


    return(0);
}

Thanks for the reply and I would be interested in implementing any improvements you can suggest ... such as fread() and fwrite(), and using fclose() as needed ... so long as it does't cause seg faults under Linux. For this version, I suppose BUFSIZ isn't a factor since I'm not doing the line by line copy any more. Since the line by line copy method appears to be limited to text files only, I see no reason to bother with that version of the code any more.

Instead of using such an statement:

[...]

Code:

printf("Source file doesn't exist.\n");

[...]

you better use:

Code:

fprintf(stderr, "Source file doesn't exist.\n");

Code:

sourceFile = fopen(sFile, "r");
if (sourceFile == NULL)

NULL doesn't mean that the file doesn't exist, it means it can't be opened. Reasons might be that the file is opened in an other process, it might be corrupt....
To get more meaningful error message use perror or strerror(errno);

Originally Posted by Salem

That's what google is for.

Got it.

Originally Posted by Salem

The embedded assignment and test is fairly idiomatic code that you'll see quite often, so it's worth persevering with understanding it.
The problem with your approach is that you have a repeated line of code.

I understand the efficiency of the shorter version, but computer science isn't my major, so I tend to compromise on ease of understanding. Having said that, in this particular case, I understand what the while loop is doing, even if the exact steps through the while loop aren't as clear to me with the shorter version. So I'll just go with the shorter version.

Originally Posted by Salem

The closing on error thing.

Ah... ok. I understand the placement of the file closing now. I was putting them in the wrong place.

Thanks for your assistance and patience.

Here is where the program stands now ... including ordak's tip about stderr, and OldGuy2's tip about strerror(errno). I also changed the return values so that each error had a different number to go with it. Not sure if that matters, but it seemed reasonable. I did a google search for fread() and fwrite(). I'll have to find some better examples before I can make that change.

Code:

#include <stdio.h>
#include <errno.h>


#define MAXFILENAME 256        /* Maximum file name length */


int copyFile(char sFile[MAXFILENAME], char tFile[MAXFILENAME]);


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


    int copyResult;


    if (argc != 3) {
        printf("Usage: copyfile <source file> <target file>\n");
        return(1);
    } else {
        copyResult = copyFile(argv[1], argv[2]);
    }
    return (copyResult);
}


int copyFile(char sFile[MAXFILENAME], char tFile[MAXFILENAME]) {


    FILE *sourceFile;
    FILE *targetFile;


    int c;


    sourceFile = fopen(sFile, "rb");
    if (sourceFile == NULL) {
        fprintf(stderr, "Error opening file %s: %s", sFile,strerror(errno));
        return(2);
    }


    targetFile = fopen(tFile, "rb");
    if (targetFile != NULL) {
        fclose(targetFile); // target file already exists, so
        fclose(sourceFile); // we're not going to copy it after all
        fprintf(stderr, "Target already exists. Aborting...\n");
        return(3);
    }


    targetFile = fopen(tFile, "wb");
    if (targetFile == NULL) {
        fclose(sourceFile); // unable to write, so we're aborting
        fprintf(stderr, "Can't open file for writing! Permission error?\n");
        return(4);
    }


    while ( ( c = fgetc(sourceFile)) != EOF ) {
        fputc(c, targetFile);
    }


    fclose(sourceFile);
    fclose(targetFile);


    return(0);
}