1
\$\begingroup\$

All this does is safely and reliably opens, writes to, and closes the file. Yet there are so many gotos and the code is quite verbose. I am not even sure that all of what I am doing is necessary.

int WriteConfig(void) {
    FILE *ConfigFile;
RetryOpen:
    if (!(ConfigFile = fopen(Config, "w"))) {
        if (errno == EINTR)
            goto RetryOpen;
        else
            goto Err;
    }
RetryWrite:
    if (fwrite(&Conf, sizeof(struct Config), 1, ConfigFile) < 1) {
        if (errno == EINTR)
            goto RetryWrite;
        if (fclose(ConfigFile))
            goto Error;
        goto Err;
    }
    if (fclose(ConfigFile))
        goto Error;
    return 0;
Err:
    AlertFuncErrno;
    return errno;
Error:
    AlertFuncErrno;
    exit(errno);
}

AlertFuncErrno is a macro that expands to a function call that notifies the user of the error (including strerror(errno)). Config is a global char * that contains a path to the config file. Conf is a global variable of type struct Config. Am I doing too much here? What action would be appropriate when library functions fail? Should I fflush prior to close, so that if there is a write error it will be caught there? Keep in mind I am attempting to create portable code.

\$\endgroup\$
0
2
\$\begingroup\$

The identifiers are highly confusing - we have Conf, Config, struct Config and ConfigFile. I can't tell without re-reading the code what each one means. We also have two labels whose difference is hard to deduce from their names.

Why are we passing Conf and Config in globals? That makes the function very inflexible. If we pass them as arguments, that lets us write any configuration to any file. Even better would be to pass an open FILE*, which would enable testing without using filesystem (by passing the write end of a pipe).

We test whether writing failed completely (fwrite() < 1), but seem not to care whether the data were partially written. So we should also test fwrite() < sizeof Conf, and in that case write again, to output the remaining part. Perhaps

const char *start = (const char*)&Conf;
size_t len = sizeof Conf;
while (len > 0) {
    errno = 0;
    size_t written = fwrite(start, 1, len, ConfigFile);
    if (!written && errno != EINTR) {
        goto Error;
    }
    start += written;
    len -= written;
}

Note that we're now using while to form a loop, rather than goto. It's much better to use the C loop structures than goto, which is easy to get wrong in many ways. Similarly, we can re-write the fopen() loop with while rather than goto.

This handling looks strange:

Err:
    AlertFuncErrno;
    return errno;
Error:
    AlertFuncErrno;
    exit(errno);

In the first case, we both emit a user message (convention says that the macro should be called ALERT_FUNC_ERRNO so we can immediately see it's a macro) and return the error indication to the caller. The caller is in a better position to know whether the error message is required or it can take alternative action without bothering the user.

The second case is very disturbing; not only has the user's config not been saved, but now we exit, losing the only copy of the config (in the process's memory). That's exactly the situation where we would want to fall back to writing to some other file so that the data are safe!

Neither of the error cases make any attempt to close the file, so the function leaks resources if the file is successfully opened but returns without saving.


Having looked at the code in some detail, there's some more fundamental issues:

  • We seem to be overwriting a single file starting from the beginning. If something goes wrong, not only have we failed to save from memory to file, but we've also lost the original contents of the file! We really need to ensure that the original contents are backed up, and only remove the backup when our data are safely stashed.

  • It's very difficult to change struct Config without causing problems when reading and writing with different versions of the program. And there's no evident way for us to know whether we have read a different version than we expected, in which case the contents may well be meaningless (perhaps there's a version field at the beginning of the struct, but since you've not shown that, I'm assuming the worst).

\$\endgroup\$
9
  • 1
    \$\begingroup\$ fwrite() is not specified to to set errno so testing it in if (!written && errno != EINTR) in unclear. If some implementation does set errno, then errno = 0 prior to the loop makes sense to not exit due to a prior errno. \$\endgroup\$ Jun 23 at 18:26
  • \$\begingroup\$ Oops, yeah, I just copied that logic from the original without questioning. \$\endgroup\$ Jun 23 at 19:55
  • \$\begingroup\$ @chux-ReinstateMonica Sure about that? \$\endgroup\$
    – user244080
    Jun 23 at 23:59
  • 1
    \$\begingroup\$ @user244080, the man page describes the implementation you are using, rather than the C standard. If you target only the one platform, then you can rely on that, but for a portable program, you can depend only on what the standard requires of implementations (modulo platform bugs, which should be reported, ideally with a fix). \$\endgroup\$ Jun 24 at 6:44
  • 1
    \$\begingroup\$ That document applies only to POSIX platforms, and says "⌦and errno shall be set to indicate the error. ⌫" - note that those advisory information markers, ⌦ and ⌫, indicate that "The functionality described is optional. The functionality described is also an extension to the ISO C standard." So even on POSIX systems, you can't depend on errno being set. And other implementations of C exist (notably DOS and Windows if you have sufficient misfortune to find you have users in the Microsoft world). \$\endgroup\$ Jun 24 at 7:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.