public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: "Érico Nogueira" <ericonr@disroot.org>
To: libc-alpha@sourceware.org
Cc: "Érico Nogueira" <ericonr@disroot.org>
Subject: [RFC] linux: use __fd_to_filename helper function instead of snprintf.
Date: Tue, 27 Apr 2021 00:09:37 -0300	[thread overview]
Message-ID: <20210427030937.10380-1-ericonr@disroot.org> (raw)

snprintf() is a very big hammer and pulls in a lot of code that might be
unnecessary. It was also being used in an inconsistent manner between
the affected files.

Change made to fchmodat and fexecve. There are tests using xasprintf
instead of this helper as well, but this commit doesn't touch them.
---

I found these call sites using `grep -R 'printf.*proc/self/fd'`, but
there are probably other instances that weren't as simple.

support/support_descriptors.c:        char *path = xasprintf ("/proc/self/fd/%ld", fd);
sysdeps/unix/sysv/linux/tst-ttyname.c:      char *linkname = xasprintf ("/proc/self/fd/%d", slave);
sysdeps/unix/sysv/linux/tst-ttyname.c:          char *linkname = xasprintf ("/proc/self/fd/%d", slave);
sysdeps/unix/sysv/linux/tst-ttyname.c:    char *linkname = xasprintf ("/proc/self/fd/%d", slave);
sysdeps/unix/sysv/linux/tst-memfd_create.c:        char *fd_path = xasprintf ("/proc/self/fd/%d", fd);
io/tst-open-tmpfile.c:  char *proc_fd_path = xasprintf ("/proc/self/fd/%d", fd);
io/tst-open-tmpfile.c:  char *proc_fd_path = xasprintf ("/proc/self/fd/%d", fd);

Some of the tests can be moved to use __fd_to_filename, but I held off
on doing this to hear feedback on the idea first.

It seems not all files follow the same include order / grouping policy,
so I'd appreciate pointers on that as well.

 sysdeps/unix/sysv/linux/fchmodat.c | 11 +++--------
 sysdeps/unix/sysv/linux/fexecve.c  |  5 +++--
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
index f264f0c09d..4154192cc4 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -18,6 +18,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <fd_to_filename.h>
 #include <not-cancel.h>
 #include <stdio.h>
 #include <sys/stat.h>
@@ -69,14 +70,8 @@ fchmodat (int fd, const char *file, mode_t mode, int flag)
 
       /* For most file systems, fchmod does not operate on O_PATH
 	 descriptors, so go through /proc.  */
-      char buf[32];
-      if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0)
-	{
-	  /* This also may report strange error codes to the caller
-	     (although snprintf really should not fail).  */
-	  __close_nocancel (pathfd);
-	  return -1;
-	}
+      struct fd_to_filename filename;
+      char *buf = __fd_to_filename(pathfd, &filename);
 
       int ret = __chmod (buf, mode);
       if (ret != 0)
diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
index f37c245396..218337a3b5 100644
--- a/sysdeps/unix/sysv/linux/fexecve.c
+++ b/sysdeps/unix/sysv/linux/fexecve.c
@@ -22,6 +22,7 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 
+#include <fd_to_filename.h>
 #include <sysdep.h>
 #include <sys/syscall.h>
 #include <kernel-features.h>
@@ -50,8 +51,8 @@ fexecve (int fd, char *const argv[], char *const envp[])
 #ifndef __ASSUME_EXECVEAT
   /* We use the /proc filesystem to get the information.  If it is not
      mounted we fail.  */
-  char buf[sizeof "/proc/self/fd/" + sizeof (int) * 3];
-  __snprintf (buf, sizeof (buf), "/proc/self/fd/%d", fd);
+  struct fd_to_filename filename;
+  char *buf = fd_to_filename(fd, &filename);
 
   /* We do not need the return value.  */
   __execve (buf, argv, envp);
-- 
2.31.1


             reply	other threads:[~2021-04-27  3:10 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-27  3:09 Érico Nogueira [this message]
2021-04-27  6:46 ` Florian Weimer
2021-04-27 13:06   ` Érico Nogueira

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210427030937.10380-1-ericonr@disroot.org \
    --to=ericonr@disroot.org \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).