3

Say I had the following code:

int calcTotalLinesFromFile(FILE *source_file) {
    /* Calculate number of lines in source_file and return */
}

int addToFile(char *name_of_file, char *file_content, int line_num) {
    FILE *temporary_file;
    FILE *source_file;
    int temp = 1;
    int ch;

    if (access(file_name, F_OK) != 0) {
        fprintf(stderr, "Cannot add content to file %s: File does not exist.\n", name_of_file);
        return errno;
    }

    source_file = fopen(name_of_file, "r");
    if (source_file == NULL) {
        perror("Cannot add content to file");
        return errno;
    }

    int total_lines = calcTotalLinesFromFile(source_file);
    if (line_num > total_lines) {
        fprintf(stderr, "Entered line number exceeds maximum file line.\n");
        return -1;
    } else if (line_num < 1) {
        fprintf(stderr, "Entered line number must be greater than zero.\n");
        return -1;
    }

    temporary_file = fopen("tempfile.tmp", "w");
    if (temporary_file == NULL) {
        perror("Could not write content to file");
        return errno;
    }
    
    /* Add content to file */
    /*       ...           */

    fclose(source_file);
    fclose(temporary_file);

    int successful_delete = remove(source_file_name);
    if (successful_delete != 0) {
        perror("Could not delete source file");
        remove("tempfile.tmp");  /* Clean up after failure */
        return errno;
    }

    int successful_rename = rename("tempfile.tmp", source_file_name);
    if (successful_rename != 0) {
        perror("Could not rename new file");
        remove("tempfile.tmp") /* Clean up after failure */
        return errno
    }

    return 0;
}

Here I perform error checks on nearly all functions that could fail so that I can keep the user informed about what exactly is going wrong and to prevent them from inputting data that would throw more errors in the program. My only problem is that it makes my method much much longer than normal, and the method definitely cannot fit on my monitor without having to scroll down. My questions are as follows:

  1. Are there other options for error handling in C?
  2. Am I possibly checking for errors too often?
  3. Is it also common to have the caller validate the parameters before passing them to the method?

Edit: fixed syntax errors.

Phoenix
  • 412
  • 2
  • 12
  • 1
    Maybe this is related: [try-catch in c](https://stackoverflow.com/questions/10586003/try-catch-statements-in-c) – Lazar Đorđević Feb 09 '21 at 19:16
  • `access` can fail for reasons other than the file not existing. If the file exists and you write an error message that says it does not, you are confusing the user. Don't make up an error message; use the string provided by the system via `errno`. eg `perror(file_name)` – William Pursell Feb 09 '21 at 19:19
  • 2
    "Too often" depends on the use-case for this code. Just you hacking around? Probably. If it's for users and long-term deployment probably not enough. Having specific error codes listed helps with a diagnosis, like `"E2930 Could not write to file"` allowing you to quickly identify the failing line. – tadman Feb 09 '21 at 19:20
  • @WilliamPursell But if I'm passing the `F_OK` flag then how else could that fail other than the file not existing? – Phoenix Feb 09 '21 at 19:21
  • @Phoenix ENOTDIR, ENAMETOOLONG, ELOOP, etc ... – William Pursell Feb 09 '21 at 19:23
  • I see. Thanks for pointing that out! – Phoenix Feb 09 '21 at 19:24
  • You can greatly reduce the length of your function with wrappers that produce an error message. If they can `exit`, that's useful. If not, you can short circuit with `if( wfopen(...) || wfrite (...) || ... ) return 1` – William Pursell Feb 09 '21 at 19:26
  • @WilliamPursell In this case, if I need to return a value from these functions, would I just pass the variable I want to store the result in by reference and assign it in the wrapper instead? – Phoenix Feb 09 '21 at 19:31
  • For the wrappers to work with a short-circuit like I describe, you really want them to just return zero on success and non-zero for failure. They can always assign `errno` if you want. It's typically not worth trying to keep a particular error value. – William Pursell Feb 09 '21 at 19:33
  • You can do things like `if( (fp = wfopen()) != NULL && ( ( c = wrap()) != 0) && ...`, but it gets a bit convoluted. Probably cleaner to do `if(wrapper()) return 1; if (wrapper2) return 1` ... – William Pursell Feb 09 '21 at 19:35
  • I don't mean returning a particular error value. For example, I would need to store the result of `wfopen()` in a variable to be able to interact with the file stream. If I use the wrapper with a short-circuit, then how would I do that? – Phoenix Feb 09 '21 at 19:37
  • Does this answer your question? [Any good idioms for error handling in straight C programs?](https://stackoverflow.com/questions/2789987/any-good-idioms-for-error-handling-in-straight-c-programs) – moooeeeep Feb 09 '21 at 19:42
  • Also related: https://stackoverflow.com/q/385975/1025391 – moooeeeep Feb 09 '21 at 19:43
  • @moooeeeep It helps with part of the question, but not necessarily the whole issue. – Phoenix Feb 09 '21 at 19:46

1 Answers1

4

First, it is not common practice to return errno. Typically functions return -1 and ask the caller to check errno. You especially shouldn't mix the two practices (you return -1 when fprintf fails but you return errno everywhere else). As for other options, one common idiom is using goto for cleanup. (See Valid use of goto for error management in C?)

You can never check errors too often. Nearly every call to the OS can fail somehow, so you should always check. Even fclose can fail.

cry0genic
  • 544
  • 3
  • 15