On Sun, Aug 26, 2018 at 06:48:46PM -0700, Kevin J. McCarthy wrote: > Here's a first try at a patch to check for the historically documented > "link() returning -1 but succeeding" case for NFS.
This seems fine. Though, it seems it leaves in the stat comparison if link() succeeds, which is superfluous (could cause needless performance hit on slow networked filesystems), and violates KISS and "Don't write code you don't need" principles. I personally would have avoided goto by eliminating that superfluous check, and putting the rename() bits in an else clause to the if (lstat() && lstat() && compare_stat()) conditional. It obviously also does not attempt to address the follow symlink behavior I mentioned, which is also fine. It may well be that safe_rename() is never used in contexts where that's likely to matter, so it's possible that it would be reasonable to ignore that issue. -- Derek D. Martin http://www.pizzashack.org/ GPG Key ID: 0xDFBEAD02 -=-=-=-=- This message is posted from an invalid address. Replying to it will result in undeliverable mail due to spam prevention. Sorry for the inconvenience. > From f0faf6cf205e0b96c2fb93ea12393458843aa485 Mon Sep 17 00:00:00 2001 > From: Kevin McCarthy <ke...@8t8.us> > Date: Sun, 26 Aug 2018 18:43:20 -0700 > Subject: [PATCH] Add additional error handling to safe_rename(). > > It is apparently possible for link() to return an error but the link > to still be created. Add a double check for that case. If the files > match, unlink the src and return success. > --- > lib.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/lib.c b/lib.c > index 9a5d5325..0f65a641 100644 > --- a/lib.c > +++ b/lib.c > @@ -446,12 +446,31 @@ int safe_symlink(const char *oldpath, const char > *newpath) > int safe_rename (const char *src, const char *target) > { > struct stat ssb, tsb; > + int link_errno; > > if (!src || !target) > return -1; > > if (link (src, target) != 0) > { > + link_errno = errno; > + > + /* > + * It is historically documented that link can return -1 if NFS > + * dies after creating the link. In that case, we are supposed > + * to use stat to check if the link was created. > + */ > + if ((lstat (src, &ssb) == 0) && > + (lstat (target, &tsb) == 0) && > + (compare_stat (&ssb, &tsb) == 0)) > + { > + dprint (1, (debugfile, > + "safe_rename: link (%s, %s) reported failure: %s (%d) but > actually succeded\n", > + src, target, strerror (errno), errno)); > + goto success; > + } > + > + errno = link_errno; > > /* > * Coda does not allow cross-directory links, but tells > @@ -533,11 +552,11 @@ int safe_rename (const char *src, const char *target) > } > #endif > > +success: > /* > * Unlink the original link. Should we really ignore the return > * value here? XXX > */ > - > if (unlink (src) == -1) > { > dprint (1, (debugfile, "safe_rename: unlink (%s) failed: %s (%d)\n", > -- > 2.18.0 >
pgpLgxG9IZzhS.pgp
Description: PGP signature
