[PATCH] Switch from numerical to defined constants for permissions.
Mark Wielaard
mark@klomp.org
Tue Nov 3 15:11:15 GMT 2020
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Switch-from-numerical-to-defined-constants-for-permi.patch
Type: text/x-patch
Size: 15272 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20201103/ffb40def/attachment-0001.bin>
More information about the Elfutils-devel
mailing list