On Thu, Aug 23, 2018 at 12:08:19AM -0500, Derek Martin wrote: > What I was actually suggesting is to not call out the EEXIST error > case differently from other error cases. Making that change has no > other effect on the retry logic in that function. But it does > eliminate the illogical retry forever if the message file already > exists for some reason.
The calling function (e.g. _maildir_commit_message()) is not trying to safe_rename() to the same filename over and over though. It is trying to find an available target filename, and keeps modifying the target filename using an incremented Counter and time(). So it makes sense for it to retry if the target filename already existed. Or am I misunderstanding your point? > > Steffen's cautions apply to dotlock code, which is a different case and > > is not affected by this change. > > It's fundamentally the same thing though. The mechanism for dotlock > works like this: > [...] > If the link succeeds, we have the lock, but the rc from link is > unreliable, so... This is the important question. Under what circumstances is the rc from the link() unreliable? Are you saying a "0" success return value should not be trusted? Because the crux of my justification for the change is that the link() returned success and there is no need for a double check. The man page indicates a __failure__ code from link() can't be trusted for NFS (if the server happened to die but created the link), but nothing is mentioned about a success return code being unreliable. I haven't been working on this kind of stuff for years though, so I rely on you all to tell me when I make stupid changes. I really need to know if this is a stupid change. > - The stat comparison is supposed to catch the case where the return > value from link() incorrectly indicates failure, but it does not do > that because it only happens when link() succeeds. That defeats the > purpose of doing the check at all. No, the stat comparison was a double check for if the link __succeeds__. There is already failure handling code - reverting to trying a rename() for certain errno values. It was merely Vincent's hypothesis that the check was in the wrong place, but I don't believe that is correct. Your point about stat vs lstat sounds important, but let's first resolve whether the double-check needs to be restored or not. -- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
