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

[Jian Cai via Phabricator via llvm-commits]

jcai19 added inline comments.


================
Comment at: llvm/lib/Support/Unix/Path.inc:1216
 
+std::error_code changeFileOwnership(const Twine &Name, uint32_t Owner, uint32_t Group) {
+  int FD;
----------------
jhenderson wrote:
> This function doesn't look like it's been clang-formatted properly.
Good catch! git clang-format HEAD~1 somehow did not notice it. I have updated it.


================
Comment at: llvm/lib/Support/Unix/Path.inc:1221
+  auto Chown = [&]() { return fchown(FD, Owner, Group); };
+  if ((sys::RetryAfterSignal(-1, Chown)) < 0)
+    return std::error_code(errno, std::generic_category());
----------------
jhenderson wrote:
> I'm not particularly familiar with these lower-level fucntions. What's the `RetryAfterSignal` about? Should it be commented?
> What's the RetryAfterSignal about? 
I think it basically keeps calling the same function again if EINTR (interrupted function call) happens https://llvm.org/doxygen/Errno_8h_source.html#l00033. 

> Should it be commented?
I just added some comments in the latest patch. FYI this function is called several times in the same file and has not been commented anywhere else, so let me know if you think we should just remove it to keep consistent. 


================
Comment at: llvm/test/tools/llvm-objcopy/preserve-file-ownership.test:2
+# RUN: yaml2obj %s -o %t
+# RUN: sudo llvm-objcopy %t
+# RUN: ls -l %t | FileCheck -DUSER=$(ls -ld %s | awk '{print $3}') -DGROUP=$(ls -ld %s | awk '{print $4}') %s
----------------
jhenderson wrote:
> Won't this require root access to run? Pretty sure that not everyone who runs the test is going to have it...
That's a good point. I could remove sudo from the commands but this change won't get tested. Any suggestion on how we could better test this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881