Hi Érico,

On Sun, 2020-11-01 at 22:49 -0300, Érico Nogueira via Elfutils-devel
wrote:
> From: Érico Rolim <erico.erc@gmail.com>
> 
> Use defined constants for permission values. Also add fallback
> definitions for them in system.h, to allow for compatibility with
> systems that don't provide these macros.
> 
> Include system.h in all tests/ files that required it.

That is a nice cleanup.

> Signed-off-by: Érico Rolim <erico.erc@gmail.com>
> ---
> 
> I'm a bit unsure about the ChangeLog entry, since these changes were
> tree-wide.

I added them for you since I was reviewing the changes anyway.
I also made a few small tweaks, see below.

> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
> client.c
> index 0e5177bc..ce1d819b 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -212,13 +212,13 @@ debuginfod_init_cache (char *cache_path, char *interval_path, char *maxage_path)
>      return 0;
>  
>    /* Create the cache and config files as necessary.  */
> -  if (stat(cache_path, &st) != 0 && mkdir(cache_path, 0777) < 0)
> +  if (stat(cache_path, &st) != 0 && mkdir(cache_path, ACCESSPERMS) < 0)
>      return -errno;

OK for mkdir.
 
>    int fd = -1;
>  
>    /* init cleaning interval config file.  */
> -  fd = open(interval_path, O_CREAT | O_RDWR, 0666);
> +  fd = open(interval_path, O_CREAT | O_RDWR, DEFFILEMODE);
>    if (fd < 0)
>      return -errno;

OK for open with O_CREAT.

> @@ -227,7 +227,7 @@ debuginfod_init_cache (char *cache_path, char *interval_path, char *maxage_path)
>  
>    /* init max age config file.  */
>    if (stat(maxage_path, &st) != 0
> -      && (fd = open(maxage_path, O_CREAT | O_RDWR, 0666)) < 0)
> +      && (fd = open(maxage_path, O_CREAT | O_RDWR, DEFFILEMODE)) < 0)
>      return -errno;

OK.

>    if (dprintf(fd, "%ld", cache_default_max_unused_age_s) < 0)
> diff --git a/lib/system.h b/lib/system.h
> index 292082bd..7b650f11 100644
> --- a/lib/system.h
> +++ b/lib/system.h
> @@ -85,6 +85,18 @@
>       __res; })
>  #endif
>  
> +#ifndef ACCESSPERMS
> +#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */
> +#endif
> +
> +#ifndef ALLPERMS
> +#define ALLPERMS (S_ISUID|S_ISGID|S_ISVTX|S_IRWXU|S_IRWXG|S_IRWXO) /* 07777 */
> +#endif
> +
> +#ifndef DEFFILEMODE
> +#define DEFFILEMODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)/* 0666 */
> +#endif

Definitions look correct.

>  static inline ssize_t __attribute__ ((unused))
>  pwrite_retry (int fd, const void *buf, size_t len, off_t off)
>  {
> diff --git a/src/unstrip.c b/src/unstrip.c
> index 0257d9cc..c99ee612 100644
> --- a/src/unstrip.c
> +++ b/src/unstrip.c
> @@ -315,7 +315,7 @@ make_directories (const char *path)
>    if (dir == NULL)
>      error(EXIT_FAILURE, errno, _("memory exhausted"));
>  
> -  while (mkdir (dir, 0777) < 0 && errno != EEXIST)
> +  while (mkdir (dir, ACCESSPERMS) < 0 && errno != EEXIST)

OK

>      {
>        if (errno == ENOENT)
>          make_directories (dir);
> @@ -2192,7 +2192,7 @@ DWARF data in '%s' not adjusted for prelinking bias; consider prelink -u"),
>  
>        /* Copy the unstripped file and then modify it.  */
>        int outfd = open (output_file, O_RDWR | O_CREAT,
> -			  stripped_ehdr->e_type == ET_REL ? 0666 : 0777);
> +			  stripped_ehdr->e_type == ET_REL ? DEFFILEMODE : ACCESSPERMS);

OK assumes executables and shared libraries should have execute perms
set. The line gets a but long though. I moved part of it to the next
line.

>        if (outfd < 0)
>  	error (EXIT_FAILURE, errno, _("cannot open '%s'"), output_file);
>        Elf *outelf = elf_begin (outfd, ELF_C_WRITE, NULL);
> diff --git a/tests/alldts.c b/tests/alldts.c
> index 28b3063c..3e9f9fe6 100644
> --- a/tests/alldts.c
> +++ b/tests/alldts.c
> @@ -32,6 +32,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  
>  int
> @@ -68,7 +69,7 @@ main (void)
>    (void) __fsetlocking (stdout, FSETLOCKING_BYCALLER);
>  
>    /* Open the file.  */
> -  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %m\n", fname);

OK.

> diff --git a/tests/arextract.c b/tests/arextract.c
> index 2c4dc758..936d7f55 100644
> --- a/tests/arextract.c
> +++ b/tests/arextract.c
> @@ -95,7 +95,7 @@ Failed to get base address for the archive element: %s\n",
>  	    }
>  
>  	  /* Open the output file.  */
> -	  outfd = open (argv[3], O_CREAT | O_TRUNC | O_RDWR, 0666);
> +	  outfd = open (argv[3], O_CREAT | O_TRUNC | O_RDWR, DEFFILEMODE);
>  	  if (outfd == -1)
>  	    {
>  	      printf ("cannot open output file: %m");

OK.

> diff --git a/tests/ecp.c b/tests/ecp.c
> index 1df40a32..44a7bda2 100644
> --- a/tests/ecp.c
> +++ b/tests/ecp.c
> @@ -43,7 +43,7 @@ main (int argc, char *argv[])
>      error (EXIT_FAILURE, 0, "problems opening '%s' as ELF file: %s",
>  	   argv[1], elf_errmsg (-1));
>  
> -  int outfd = creat (argv[2], 0666);
> +  int outfd = creat (argv[2], DEFFILEMODE);
>    if (outfd == -1)
>      error (EXIT_FAILURE, errno, "cannot open output file '%s'", argv[2]);

OK.

> diff --git a/tests/elfstrtab.c b/tests/elfstrtab.c
> index c27d6cfb..145a8aa9 100644
> --- a/tests/elfstrtab.c
> +++ b/tests/elfstrtab.c
> @@ -26,6 +26,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(elf)
>  #include <gelf.h>
> @@ -134,7 +135,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    printf ("\nfname: %s\n", fname);
>    stridx = 0;
>  
> -  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));

OK.

> @@ -280,7 +281,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    close (fd);
>  
>    /* Read the ELF from disk now.  */
> -  fd = open (fname, O_RDWR, 0666);
> +  fd = open (fname, O_RDWR, DEFFILEMODE);

This looks like an existing "bug". No mode necessary for O_RDWR without
O_CREAT. I removed it.

>    if (fd == -1)
>      {
>        printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
> @@ -349,7 +350,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    close (fd);
>  
>    // And read it in one last time.
> -  fd = open (fname, O_RDONLY, 0666);
> +  fd = open (fname, O_RDONLY, DEFFILEMODE);

Likewise.

>    if (fd == -1)
>      {
>        printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));
> diff --git a/tests/emptyfile.c b/tests/emptyfile.c
> index 6d086246..a236fba1 100644
> --- a/tests/emptyfile.c
> +++ b/tests/emptyfile.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(elf)
>  #include <gelf.h>
> @@ -67,7 +68,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    printf ("\nfname: %s\n", fname);
>    stridx = 0; // Reset strtab strings index
>  
> -  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));

OK.

> @@ -125,7 +126,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    close (fd);
>  
>    /* Reread the ELF from disk now.  */
> -  fd = open (fname, O_RDWR, 0666);
> +  fd = open (fname, O_RDWR, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot (re)open `%s': %s\n", fname, strerror (errno));
> @@ -208,7 +209,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    close (fd);
>  
>    // And read it in one last time.
> -  fd = open (fname, O_RDONLY, 0666);
> +  fd = open (fname, O_RDONLY, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));

Both don't need a mode as above.

> diff --git a/tests/fillfile.c b/tests/fillfile.c
> index 915e249d..186dcad0 100644
> --- a/tests/fillfile.c
> +++ b/tests/fillfile.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(elf)
>  #include <gelf.h>
> @@ -201,7 +202,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    printf ("\nfname: %s\n", fname);
>    stridx = 0; // Reset strtab strings index
>  
> -  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));

OK.

> @@ -266,7 +267,7 @@ check_elf (const char *fname, int class, int use_mmap)
>  
>    /* Reread the ELF from disk now.  */
>    printf ("Rereading %s\n", fname);
> -  fd = open (fname, O_RDWR, 0666);
> +  fd = open (fname, O_RDWR, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot (re)open `%s': %s\n", fname, strerror (errno));
> @@ -347,7 +348,7 @@ check_elf (const char *fname, int class, int use_mmap)
>  
>    // And read it in one last time.
>    printf ("Rereading %s again\n", fname);
> -  fd = open (fname, O_RDONLY, 0666);
> +  fd = open (fname, O_RDONLY, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));

Again as above, removed the mode.

> diff --git a/tests/newdata.c b/tests/newdata.c
> index 9af99564..0d662f68 100644
> --- a/tests/newdata.c
> +++ b/tests/newdata.c
> @@ -26,6 +26,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(elf)
>  #include <gelf.h>
> @@ -243,7 +244,7 @@ check_elf (int class, int use_mmap)
>  
>    printf ("\ncheck_elf: %s\n", fname);
>  
> -  int fd = open (fname, O_RDWR|O_CREAT|O_TRUNC, 00666);
> +  int fd = open (fname, O_RDWR|O_CREAT|O_TRUNC, 0DEFFILEMODE);

The extra 0 prefix seems wrong. Removed.

>    if (fd == -1)
>      {
>        printf ("cannot create `%s': %s\n", fname, strerror (errno));
> diff --git a/tests/update1.c b/tests/update1.c
> index f4c14753..b7be4e5f 100644
> --- a/tests/update1.c
> +++ b/tests/update1.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  
>  int
> @@ -38,7 +39,7 @@ main (int argc, char *argv[] __attribute__ ((unused)))
>    Elf32_Ehdr *ehdr;
>    int i;
>  
> -  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));
> diff --git a/tests/update2.c b/tests/update2.c
> index 5805163d..71455633 100644
> --- a/tests/update2.c
> +++ b/tests/update2.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  
>  int
> @@ -39,7 +40,7 @@ main (int argc, char *argv[] __attribute__ ((unused)))
>    Elf32_Phdr *phdr;
>    int i;
>  
> -  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));
> diff --git a/tests/update3.c b/tests/update3.c
> index 7a4224dd..62f67f74 100644
> --- a/tests/update3.c
> +++ b/tests/update3.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(dwelf)
>  
> @@ -46,7 +47,7 @@ main (int argc, char *argv[] __attribute__ ((unused)))
>    Dwelf_Strent *shstrtabse;
>    int i;
>  
> -  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));
> diff --git a/tests/update4.c b/tests/update4.c
> index a9bd4bf9..a703b592 100644
> --- a/tests/update4.c
> +++ b/tests/update4.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(dwelf)
>  
> @@ -50,7 +51,7 @@ main (int argc, char *argv[] __attribute__ ((unused)))
>    Dwelf_Strent *shstrtabse;
>    int i;
>  
> -  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));

All these look fine.

> diff --git a/tests/vendorelf.c b/tests/vendorelf.c
> index bc13cce3..e1341c76 100644
> --- a/tests/vendorelf.c
> +++ b/tests/vendorelf.c
> @@ -27,6 +27,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "system.h"
>  
>  #include ELFUTILS_HEADER(elf)
>  #include <gelf.h>
> @@ -36,7 +37,7 @@ check_elf (const char *fname, int class, int use_mmap)
>  {
>    printf ("\nfname: %s\n", fname);
>  
> -  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, 0666);
> +  int fd = open (fname, O_RDWR | O_CREAT | O_TRUNC, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s': %s\n", fname, strerror (errno));

OK.

> @@ -124,7 +125,7 @@ check_elf (const char *fname, int class, int use_mmap)
>    close (fd);
>  
>    /* Reread the ELF from disk now.  */
> -  fd = open (fname, O_RDONLY, 0666);
> +  fd = open (fname, O_RDONLY, DEFFILEMODE);
>    if (fd == -1)
>      {
>        printf ("cannot open `%s' read-only: %s\n", fname, strerror (errno));

Mode removed.

I pushed the attached variant of your patch.

Thanks,

Mark