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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

If this brings us closer to GNU behaviour, that's fine, but I do find it weird that a build system would be doing anything involving sudo to run llvm-objcopy/llvm-strip.



================
Comment at: llvm/include/llvm/Support/FileSystem.h:1163-1166
+/// @brief Change ownership of a file
+///
+/// @param Owner The owner of the file to change to
+/// @param Group The group of the file to change to
----------------



================
Comment at: llvm/lib/Support/Unix/Path.inc:1216
 
+std::error_code changeFileOwnership(const Twine &Name, uint32_t Owner, uint32_t Group) {
+  int FD;
----------------
This function doesn't look like it's been clang-formatted properly.


================
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());
----------------
I'm not particularly familiar with these lower-level fucntions. What's the `RetryAfterSignal` about? Should it be commented?


================
Comment at: llvm/test/tools/llvm-objcopy/preserve-file-ownership.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: sudo llvm-objcopy %t
----------------
This test won't work on Windows, I expect. Also, please add a top-level comment to the test explaining what the behaviour is that this test is testing.


================
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
----------------
Won't this require root access to run? Pretty sure that not everyone who runs the test is going to have it...


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:332
+#if !defined(_WIN32) || defined(__CYGWIN32__)
+  // Preserve the ownership if the input file is overwritten
+  if (Config.InputFilename != "-" &&
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881