qmail-local: close race window when creating file in tmp/

[DerDakon]

The code checked if a file with the same name already exists and then tries to create it. It properly checks if the latter fails and retries later, but this is totally unnecessary: it could just try to create it, and handle it at this point if the file already exists. This also saves one stat() syscall per local delivery.

Depends on #113.

[josuah]

The string handling for path creation and file descriptor are interleaved, and with the changes the order has moved, but reading multiple times, I cannot see any change of behavior.

[DerDakon]

Would be nice to hear the opinion of @xenotrope and @schmonz given their work on #90.

[xenotrope]

Would be nice to hear the opinion of @xenotrope and @schmonz given their work on #90.

I read this code about six or seven times to see what it's doing. Classic qmail uses good, not great, uniqueness in picking Maildir filenames; The "stat(fntmptph,&st)" call loops 3 times (loops 0, 1, and 2) trying to pick a time-and-pid-combination filename that does not exist, then called open_excl on it, which is wrapper around "open(fn,O_WRONLY | O_EXCL | O_CREAT,0644)". O_EXCL will "Error if O_CREAT is set and file exists."

In short, the open_excl() will fail and call "_exit(1)" if fntmptph exists, same as in the stat() loop. Removing the stat() doesn't save a syscall so much as it forces another fork() if the file already exists...

...but, this PR keeps the sleep(2) and moves the open_exl() into the loop, so I really can't see how this behavior is dramatically different. I haven't perf tested existing file failure reporting speeds between stat() and open(). That's the only real difference here with respect to catching and failing when fntmptph is already there.

(Depending on the qmail server itself, you could possibly have multiple maildir_child() children spawning in the same directory within one second; PID recycling is not something that many OSes guarantee won't happen. Increasing Maildir uniqueness with other sources of randomness is a good way prevent this logic from ever needing to actually be useful.)

[DerDakon]

The "saves one stat()" is the good case. The good case previously is: check if the file exists with stat(), then open(). The stat() is now avoided. The bad case is previously: stat() fails, succeeds in next loop, then open() fails because of the race and things cost an extra fork() and everything on retry. Now we do open(), which fails in the first loop, then succeeds and now we own that file and there is no race.

[schmonz]

I’m not smart enough to read this and be sure about it. Automated tests of some sort would be awesome here, if you’re up for it.

[schmonz]

If you're still confident in this, let's merge it.