remove() in c doesn't work from function

Question

I tried to delete a file and then rename the temporary file in the name of the deleted file from a function that did not work. Please help me

boolean delete_user(char user_name[256])   //only for Admin
{
    boolean status = FALSE;//what does the function return
    User_Data *ud = NULL;
    boolean found_user = FALSE;
    FILE *new_file = NULL;
    FILE *fp = NULL;
    char user_name_to_copy[256];
    char password_to_copy[256];
    char old_file_name[256];

    ud = find_user(user_name);
    if (ud == NULL) {
        printf("The username wasn't found!\n");
        return FALSE;
    }
    if (!strcmp(ud->permission_type, "Admin")) {
        printf("Cant delete an admin.");
        return FALSE;
    } else {
        // the user to delete was found 
        new_file = fopen("duplicate.txt", "wt");
        strcpy(old_file_name, ud->permission_type);
        strcat(old_file_name, "s.txt"); //the name of the file is in plural and ends with .txt
        fp = fopen(old_file_name, "rt");
        while (!feof(fp)) {
            //copy all the users except the user to delete the new file 
            fscanf(fp, "%s %s\n", user_name_to_copy, password_to_copy);
            if (strcmp(user_name_to_copy, user_name)) {
                fprintf(new_file, "%s %s\n", user_name_to_copy, password_to_copy);
            }
        }
        fclose(fp);
        fclose(new_file);
        printf(" %d ", remove(old_file_name));
        rename("duplicate.txt", old_file_name);
        remove("duplicate.txt");
        return TRUE;
    }
}

This function does not work when I call it from another function, but from the main the function works just fine.

Answer 1

There are multiple problems in your code:

• You do not check if fopen() successfully opened the files.

• You do not prevent potential buffer overflow when concatenating strings to compute the permissions file.

• You should pass the array sizes to fscanf to prevent potential buffer overflows.

• You should output meaningful and informative error messages, with the cause of failure, using strerror(errno).

• The loop to copy the permission file is incorrect: Why is “while ( !feof (file) )” always wrong?

• You remove the duplicate file even if you failed to rename it but managed to remove the original file: both files may be lost. Conversely, if the rename operation succeeded, removing the duplicate file is redundant.

Here is how you could improve the code:

#include <errno.h>
#include <string.h>

boolean delete_user(char user_name[256]) {  //only for Admin
    boolean status = FALSE; // the function return value
    User_Data *ud = NULL;
    boolean found_user = FALSE;
    FILE *new_file = NULL;
    FILE *fp = NULL;
    char user_name_to_copy[256];
    char password_to_copy[256];
    char old_file_name[256];

    ud = find_user(user_name);
    if (ud == NULL) {
        printf("User '%s' was not found!\n", user_name);
        return FALSE;
    }
    if (!strcmp(ud->permission_type, "Admin")) {
        printf("Cannot delete user '%s', user has admin status.\n", user_name);
        return FALSE;
    }
    // the user to delete was found 
    new_file = fopen("duplicate.txt", "wt");
    if (new_file == NULL) {
        printf("Cannot open file 'duplicate.txt': %s\n"
               strerror(errno));
        return FALSE;
    }
    // the name of the file is in plural and ends with .txt
    snprintf(old_file_name, sizeof old_file_name, "%ss.txt", ud->permission_type);
    fp = fopen(old_file_name, "rt");
    if (fp == NULL) {
        printf("Cannot open user file '%s': %s\n"
               old_file_name, strerror(errno));
        return FALSE;
    }
    // copy all the users except the user to delete the new file 
    while (fscanf(fp, "%255s %255s\n", user_name_to_copy, password_to_copy) == 2) {
        if (strcmp(user_name_to_copy, user_name)) {
            fprintf(new_file, "%s %s\n", user_name_to_copy, password_to_copy);
        }
    }
    fclose(fp);
    fclose(new_file);
    if (remove(old_file_name)) {
        printf("Error removing file '%s': %s\n",
               old_file_name, strerror(errno));
        remove("duplicate.txt");
        return FALSE;
    }
    if (rename("duplicate.txt", old_file_name)) {
        printf("Error renaming file 'duplicate.txt' to '%s': %s\n",
                old_file_name, strerror(errno));
        // keep duplicate.txt
        return FALSE;
    }
    // duplicates.txt was successfully renamed, no need to remove it.
    return TRUE;
}

Notes:

• You create the temporary file in the current directory, which might reside on a different drive from the permissions file. rename() may fail to move the duplicate file from the current directory to that directory. Running the code with proper diagnostics should if this is the reason for failure.

Answer 2

    printf(" %d ", remove(old_file_name));
    rename("duplicate.txt", old_file_name);
    remove("duplicate.txt");

This code is mostly redundant. There's no need to remove old_file_name, rename will blow over it. (Turns out this is a POSIX thing, the C standard does not guarantee it). There's no need to remove duplicate.txt, it's been renamed.

    if( remove(old_file_name) != 0 ) {
        fprintf( stderr, "Could not remove %s: %s", old_file_name, strerror(errno) );
    }

    if( rename("duplicate.txt", old_file_name) != 0 ) {
        fprintf( stderr, "Could not rename %s to %s: %s", "duplicate.txt", old_file_name, strerror(errno) );
    }

You need to do this checking every file operation. That includes fopen and fscanf.

Also, you might have the files backwards. It's rename( old, new ) but it really looks like you wrote rename( new, old ). If you don't have them reversed, consider variable names that make the code easier to understand.

---

    fp = fopen(old_file_name, "rt");
    while (!feof(fp)) {
        //copy all the users except the user to delete the new file 
        fscanf(fp, "%s %s\n", user_name_to_copy, password_to_copy);
        if (strcmp(user_name_to_copy, user_name)) {
            fprintf(new_file, "%s %s\n", user_name_to_copy, password_to_copy);
        }
    }

This code is problematic. As mentioned, it fails to check whether fopen and fscanf succeed. Using fscanf is a problem, if it fails to match it doesn't advance the file pointer. You can read the same line over and over again, failing each time. In general, avoid fscanf and scanf.

Instead, read the whole line with fgets and parse it with sscanf. Be sure to check whether sscanf succeeded, else you'll print gibberish. Note that sscanf has bounds on how big a string it will read to protect against buffer overflow.

    FILE *fp = open_file(old_file_name, "rt");
    char line[1024];
    while (fgets(line, 1024, fp)) {
        //copy all the users except the user to delete the new file 
        if( sscanf(fp, "%255s %255s\n", user_name_to_copy, password_to_copy) != 2 ) {
            fprintf(stderr, "Couldn't understand line '%s'\n", line);
            continue;
        }
        if (strcmp(user_name_to_copy, user_name)) {
            fprintf(new_file, "%s %s\n", user_name_to_copy, password_to_copy);
        }
    }
    fclose(fp);

Since checking whether a file opened or not gets redundant, I recommend writing a little function to do that.

FILE *open_file(char *filename, char *mode) {
    FILE *fp = fopen(filename, mode);
    if( fp == NULL ) {
        fprintf(
            stderr, "Could not open '%s' for '%s': %s\n",
            filename, mode, strerror(errno)
        );
        exit(1);
    }

    return fp;
}