0

I have a simple function - its purpose is to copy a file to a .old before overwriting it. Because i'm lazy (and an answer on here suggested it) I fork and use cp to do the work.

Then i call waitpid and check the return codes.

The code calling this calls my copy function, then immediately opens the file for reading. Somehow the calling code seems to run before the cp call - the new file is what gets copied. The best example is if neither file nor backup exist. Both are created and contain what my save call outputs.

I am struggling to see where I have gone wrong, help would be appreciated.

copy_old();
std::ofstream savefile (SETTINGS_LOCATION);
if (savefile.is_open())
{
    savefile << ...


void settings::copy_old()
{
    int childExitStatus;
    pid_t pid;

    pid = fork();

    if (pid == 0) { /* child */
        execl("/bin/cp", "/bin/cp", "-f", SETTINGS_LOCATION, SETTINGS_LOCATION_B, (char *)0);
    }
    else if (pid < 0) {
        ERR("Could not Backup Previous Settings");
    }
    else {
        pid_t ws = waitpid( pid, &childExitStatus, WNOHANG);
        if (ws == -1)
        {
            ERR("Could not Backup Previous Settings1");
        }

        if( !WIFEXITED(childExitStatus) || WEXITSTATUS(childExitStatus)) /* exit code in childExitStatus */
        {
            ERR("Settings backup may have been unsuccessful");
        }
    }
}
Hector
  • 1,170
  • 2
  • 10
  • 27
  • Beyond the accepted answer on `waitpid`, everything in that else clause is pretty shaky coding. – Duck Feb 10 '14 at 23:06
  • Due to the nature of the application settings read/writes will be rare and unlikely to change from defaults. As such in the extremely unlikely issue the code fails its not a major issue. Either it works, or it doesn't, no big deal, stick it in the logs. As far as im aware the above covers pretty much any case? Maybe I should store something more specific but it seems a little overkill here. – Hector Feb 11 '14 at 01:03
  • What I was getting at is that (1) the childExitStatus is irrelevant if the `waitpid` fails; and (2) the `if WIFEXITED` statement is flawed. If WIFEXITED evaluates to true WEXITSTATUS may not be tested due to short circuit evaluation. IF WIFEXITED evaluates to false WEXITSTATUS will be evaluated but it is meaningless when WIFEXITED is false. To wit, from man (2) wait`: This macro should be employed only if WIFEXITED returned true. – Duck Feb 11 '14 at 01:28
  • 2
    @Duck: The `WIFEXITED` / `WEXITSTATUS` logic is correct (after fixing the problem with `waitpid` returning error status, that is). An `ERR` is raised if `WIFEXITED` is false (in which case `WEXITSTATUS` is not invoked) or if `WIFEXITED` is true and `WEXITSTATUS` is nonzero. – David Hammen Feb 11 '14 at 02:41
  • 1
    @David Hammen, you are absolutely right. Sorry. Apologies all around. Apparently a bad day to stop sniffing glue. – Duck Feb 11 '14 at 03:29

1 Answers1

2

Of course waitpid isn't waiting. You told it not to:

pid_t ws = waitpid( pid, &childExitStatus, WNOHANG);

WNOHANG means "don't wait". Change that WNOHANG to 0 if you want waitpid to wait.

David Hammen
  • 32,454
  • 9
  • 60
  • 108
  • Made the mistake of blindly copying code from another accepted answer. I've spent 40 mins staring at it scratching my head. Feeling pretty stupid right now. Thanks – Hector Feb 10 '14 at 22:14