[PATCH] D62718: [llvm-objcopy] Change output file permissions to be the same as the input file

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 7 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:240
       return E;
     if (!Config.SplitDWO.empty())
       if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
----------------
rupprecht wrote:
> We should copy the same permissions to the split dwo file too
> 
> Instead of creating a separate block below as you now have, I think it makes sense to refactor this a tiny bit, e.g. change `restoreDateOnFile` to a more general `restoreStatOnFile` method (that takes a `PreserveDates` arg)
> 
> Actually... see my comment below, I think setting based on an initial stat is not actually the right way to go
@rupprecht, could you take a look to see if this is what you were suggesting? 


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:222
+      return E;
+    if (!Config.SplitDWO.empty())
+      if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
----------------
@rupprecht I was a little confused when you said we should copy the same permissions for the split dwo file. Which permissions were you referring to? 


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:230
+
+  mode_t Mask = ::umask(0);
+  (void) ::umask(Mask);
----------------
I grep'd through lib and include and couldn't find any calls to umask so I don't think LLVM currently has a wrapper around chmod(2). This clearly looks out of place though. Looking for suggestions. I didn't bother with error handling yet because I am assuming this is going to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62718