@bbockelm commented on this pull request.

> @@ -2252,10 +2278,29 @@ int XrdOfs::rename(const char             *old_name,  // In
        evsObject->Notify(XrdOfsEvs::Mv, evInfo);
       }
 
+// If we cannot overwrite, we must open-exclusive first.  This will test whether
+// we will destroy data in the rename (without actually destroying data).
+//
+   std::unique_ptr<XrdSfsFile> tmp_fp = nullptr;
+   if (cannot_overwrite)
+      {tmp_fp.reset(newFile(einfo));
+       if (!tmp_fp)
+          {return fsError(einfo, ENOMEM);}
+       if (SFS_OK != tmp_fp->open(new_name, SFS_O_CREAT, 0700, client, infoN))

Yup - I was basically trying to balance between "introduces race condition" and "potentially leaves files dangling" (in addition to the crash, consider the fact that if the rename fails that the client may not be authorized to delete the empty file it just created).

I chose "potentially leaves files dangling" because that approach guarantees no data is destroyed, even in weird race cases.

I don't feel particularly strongly here and would be OK with switching to an existence check if you prefer that direction... just wanted to point out the downsides of the alternate approach.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/xrootd/xrootd/pull/1697#discussion_r867362263", "url": "https://github.com/xrootd/xrootd/pull/1697#discussion_r867362263", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Use REPLY-ALL to reply to list

To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1