[RFC] linux: use __fd_to_filename helper function instead of snprintf.

[RFC] linux: use __fd_to_filename helper function instead of snprintf.

[Érico Nogueira]

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

Re: [RFC] linux: use __fd_to_filename helper function instead of snprintf.

[Florian Weimer]

* Érico Nogueira via Libc-alpha:

> 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);

Sorry, this patch fails to build if applied to current master:

../sysdeps/unix/sysv/linux/fexecve.c: In function ‘fexecve’:
../sysdeps/unix/sysv/linux/fexecve.c:55:15: error: implicit declaration of function ‘fd_to_filename’ [-Werror=implicit-function-declaration]
   55 |   char *buf = fd_to_filename(fd, &filename);
      |               ^~~~~~~~~~~~~~

Please also add a space before the opening '(', to match GNU style, and
consider if the code becomes clearer if you avoid the buf variable.

Thanks,
Florian

Re: [RFC] linux: use __fd_to_filename helper function instead of snprintf.

[Érico Nogueira]

Em 27/04/2021 03:46, Florian Weimer escreveu:
> * Érico Nogueira via Libc-alpha:
> 
>> 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);
> 
> Sorry, this patch fails to build if applied to current master:

I hadn't commited the fix from my work tree, sorry.

> 
> ../sysdeps/unix/sysv/linux/fexecve.c: In function ‘fexecve’:
> ../sysdeps/unix/sysv/linux/fexecve.c:55:15: error: implicit declaration of function ‘fd_to_filename’ [-Werror=implicit-function-declaration]
>     55 |   char *buf = fd_to_filename(fd, &filename);
>        |               ^~~~~~~~~~~~~~
> 
> Please also add a space before the opening '(', to match GNU style, and
> consider if the code becomes clearer if you avoid the buf variable.

I think it's basically as clear as, but a bit more compact, so I will 
use that instead. Thanks!

> 
> Thanks,
> Florian
>