[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/FileOutputBuffer.h:38
+
+    /// Preserve ownership if the file already exists
+    F_keep_ownership = 4,
----------------
Nit: comments should end with a `.` (same above).


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:131
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
----------------
Is there a way we could improve this API to create the temporary file with the right UID and GID in the first place? That seems like a better idea to me than having to do something post creation.

A thought to consider (I don't know what the desirable behaviour is here): what happens if a user were to Ctrl-C (or the program otherwise terminated unexpectedly) after the TempFile creation, before the ownership was updated? Would the temp file be in a desirable state?


================
Comment at: llvm/lib/Support/Unix/Path.inc:1216
+  auto Chown = [&]() { return ::fchown(FD, Owner, Group); };
+  // Retry if fchown call fails due to interruption
+  if ((sys::RetryAfterSignal(-1, Chown)) < 0)
----------------
Nit: missing trailing full stop at end of comment. It would be helpful to explain the why behind the retry in this comment. It isn't obvious to those less experienced with this sort of code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93881/new/

https://reviews.llvm.org/D93881