safe_rename() and verifying the result of link(2)

[Kevin J. McCarthy]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.mutt.org/pipermail/mutt-dev/attachments/20180823/f7702ef6/attachment.asc>