[RFC] [PATCH] Use numeric permissions directly.

[RFC] [PATCH] Use numeric permissions directly.

[Érico Nogueira]

musl doesn't define ALLPERMS and ACESSPERMS macros, and the code already
uses literals for these same values in some place. This commit applies
the same style to the whole project.

Signed-off-by: Érico Rolim <erico.erc@gmail.com>
---

This commit can go the other way as well, by adding blocks like

 /* musl libc doesn't define these constants */
 #ifndef ALLPERMS
 #define ALPPERMS 07777
 #endif
 [...]

and then replacing all ocurrences of literal 0777 or 07777 with the
respective macros.

 src/ar.c            | 8 ++++----
 src/elfcompress.c   | 4 ++--
 src/ranlib.c        | 2 +-
 src/strip.c         | 2 +-
 tests/elfstrmerge.c | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/ar.c b/src/ar.c
index 7d33d814..b1606d2c 100644
--- a/src/ar.c
+++ b/src/ar.c
@@ -806,7 +806,7 @@ cannot rename temporary file to %.*s"),
 	      if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
 	      /* Set the mode of the new file to the same values the
 		 original file has.  */
-	      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+	      if (fchmod (newfd, st.st_mode & 07777) != 0
 		  || close (newfd) != 0)
 		goto nonew_unlink;
 	      newfd = -1;
@@ -1060,7 +1060,7 @@ do_oper_delete (const char *arfname, char **argv, int argc,
      setting the mode (which might be reset/ignored if the owner is
      wrong.  */
   if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
-  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+  if (fchmod (newfd, st.st_mode & 07777) != 0
       || close (newfd) != 0)
     goto nonew_unlink;
   newfd = -1;
@@ -1400,7 +1400,7 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc,
     newfd = mkstemp (tmpfname);
   else
     {
-      newfd = open (arfname, O_RDWR | O_CREAT | O_EXCL, DEFFILEMODE);
+      newfd = open (arfname, O_RDWR | O_CREAT | O_EXCL, 0666);
       if (newfd == -1 && errno == EEXIST)
 	/* Bah, first the file did not exist, now it does.  Restart.  */
 	return do_oper_insert (oper, arfname, argv, argc, member);
@@ -1547,7 +1547,7 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc,
 	 setting the modes, or they might be reset/ignored if the
 	 owner is wrong.  */
       if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
-      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+      if (fchmod (newfd, st.st_mode & 07777) != 0
 	  || close (newfd) != 0)
         goto nonew_unlink;
       newfd = -1;
diff --git a/src/elfcompress.c b/src/elfcompress.c
index 6ba6af41..95b0511f 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -581,7 +581,7 @@ process_file (const char *fname)
   else
     {
       fnew = xstrdup (foutput);
-      fdnew = open (fnew, O_WRONLY | O_CREAT, st.st_mode & ALLPERMS);
+      fdnew = open (fnew, O_WRONLY | O_CREAT, st.st_mode & 07777);
     }
 
   if (fdnew < 0)
@@ -1281,7 +1281,7 @@ process_file (const char *fname)
   if (fchown (fdnew, st.st_uid, st.st_gid) != 0)
     if (verbose >= 0)
       error (0, errno, "Couldn't fchown %s", fnew);
-  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
+  if (fchmod (fdnew, st.st_mode & 07777) != 0)
     if (verbose >= 0)
       error (0, errno, "Couldn't fchmod %s", fnew);
 
diff --git a/src/ranlib.c b/src/ranlib.c
index 483a1b65..fa8b2e33 100644
--- a/src/ranlib.c
+++ b/src/ranlib.c
@@ -265,7 +265,7 @@ handle_file (const char *fname)
 	  if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
 	  /* Set the mode of the new file to the same values the
 	     original file has.  */
-	  if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+	  if (fchmod (newfd, st.st_mode & 07777) != 0
 	      || close (newfd) != 0)
 	    goto nonew_unlink;
 	  newfd = -1;
diff --git a/src/strip.c b/src/strip.c
index 48792a70..e6306cbc 100644
--- a/src/strip.c
+++ b/src/strip.c
@@ -766,7 +766,7 @@ process_file (const char *fname)
   switch (elf_kind (elf))
     {
     case ELF_K_ELF:
-      result = handle_elf (fd, elf, NULL, fname, st.st_mode & ACCESSPERMS,
+      result = handle_elf (fd, elf, NULL, fname, st.st_mode & 0777,
 			   preserve_dates ? tv : NULL);
       break;
 
diff --git a/tests/elfstrmerge.c b/tests/elfstrmerge.c
index ba0d68df..40c17900 100644
--- a/tests/elfstrmerge.c
+++ b/tests/elfstrmerge.c
@@ -367,7 +367,7 @@ main (int argc, char **argv)
   else
     {
       fnew = argv[2];
-      fdnew = open (fnew, O_WRONLY | O_CREAT, st.st_mode & ALLPERMS);
+      fdnew = open (fnew, O_WRONLY | O_CREAT, st.st_mode & 07777);
     }
 
   if (fdnew < 0)
@@ -652,7 +652,7 @@ main (int argc, char **argv)
   elfnew = NULL;
 
   /* Try to match mode and owner.group of the original file.  */
-  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
+  if (fchmod (fdnew, st.st_mode & 07777) != 0)
     error (0, errno, "Couldn't fchmod %s", fnew);
   if (fchown (fdnew, st.st_uid, st.st_gid) != 0)
     error (0, errno, "Couldn't fchown %s", fnew);
-- 
2.29.0

Re: [RFC] [PATCH] Use numeric permissions directly.

[Mark Wielaard]

Hi Érico,

On Mon, 2020-10-26 at 01:29 -0300, Érico Nogueira via Elfutils-devel wrote:
> musl doesn't define ALLPERMS and ACESSPERMS macros, and the code already
> uses literals for these same values in some place. This commit applies
> the same style to the whole project.
> 
> Signed-off-by: Érico Rolim <erico.erc@gmail.com>
> ---
> 
> This commit can go the other way as well, by adding blocks like
> 
>  /* musl libc doesn't define these constants */
>  #ifndef ALLPERMS
>  #define ALPPERMS 07777
>  #endif
>  [...]
> 
> and then replacing all ocurrences of literal 0777 or 07777 with the
> respective macros.

I prefer this alternative (although I admit we haven't be very
consistent with using the macro names). If you add the above
check/define for the constants to lib/system.h I think it will
automatically be picked up in all places that you had to change.

>  src/ar.c            | 8 ++++----
>  src/elfcompress.c   | 4 ++--
>  src/ranlib.c        | 2 +-
>  src/strip.c         | 2 +-
>  tests/elfstrmerge.c | 4 ++--

Otherwise it should be a simple addition of #include "system.h" at the
top of those files.

Cheers,

Mark