[RFC,v2] linux: use __fd_to_filename helper function instead of snprintf.
Commit Message
Change made to fchmodat and fexecve. There are tests using xasprintf
instead of this helper as well, but this commit doesn't touch them.
---
Differences from v1:
- actually builds
- doesn't use an intermediary buf variable
sysdeps/unix/sysv/linux/fchmodat.c | 13 +++----------
sysdeps/unix/sysv/linux/fexecve.c | 10 ++++------
2 files changed, 7 insertions(+), 16 deletions(-)
Comments
On 27/04/2021 10:09, Érico Nogueira via Libc-alpha wrote:
> Change made to fchmodat and fexecve. There are tests using xasprintf
> instead of this helper as well, but this commit doesn't touch them.
To adapt the tests it would require either to build it as internal/static
or add fd_to_filename to libsupport. I am not this is really an improvement.
LGTM, thanks. I will commit this for you.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
>
> Differences from v1:
> - actually builds
> - doesn't use an intermediary buf variable
>
> sysdeps/unix/sysv/linux/fchmodat.c | 13 +++----------
> sysdeps/unix/sysv/linux/fexecve.c | 10 ++++------
> 2 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> index f264f0c09d..5bd1eb96a5 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,16 +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;
> - }
> -
> - int ret = __chmod (buf, mode);
> + struct fd_to_filename filename;
> + int ret = __chmod (__fd_to_filename (pathfd, &filename), mode);
> if (ret != 0)
> {
> if (errno == ENOENT)
> diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
> index f37c245396..df25c2acb8 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>
> @@ -49,12 +50,9 @@ 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);
> -
> - /* We do not need the return value. */
> - __execve (buf, argv, envp);
> + mounted we fail. We do not need the return value. */
> + struct fd_to_filename filename;
> + __execve (__fd_to_filename (fd, &filename), argv, envp);
>
> int save = errno;
>
>
On Mon May 3, 2021 at 4:26 PM -03, Adhemerval Zanella wrote:
>
>
> On 27/04/2021 10:09, Érico Nogueira via Libc-alpha wrote:
> > Change made to fchmodat and fexecve. There are tests using xasprintf
> > instead of this helper as well, but this commit doesn't touch them.
>
> To adapt the tests it would require either to build it as
> internal/static
> or add fd_to_filename to libsupport. I am not this is really an
> improvement.
That might explain some of the build failures I got when I tried
changing the tests.
>
> LGTM, thanks. I will commit this for you.
Thanks!
I found a few more places where __fd_to_filename could be used, they are
currently using _fitoa_word together with stpcpy. Would you be
interested in making __fd_to_filename use _fitoa_word or _itoa_word? Or
should it be left as is?
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> > ---
> >
> > Differences from v1:
> > - actually builds
> > - doesn't use an intermediary buf variable
> >
> > sysdeps/unix/sysv/linux/fchmodat.c | 13 +++----------
> > sysdeps/unix/sysv/linux/fexecve.c | 10 ++++------
> > 2 files changed, 7 insertions(+), 16 deletions(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c
> > index f264f0c09d..5bd1eb96a5 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,16 +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;
> > - }
> > -
> > - int ret = __chmod (buf, mode);
> > + struct fd_to_filename filename;
> > + int ret = __chmod (__fd_to_filename (pathfd, &filename), mode);
> > if (ret != 0)
> > {
> > if (errno == ENOENT)
> > diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
> > index f37c245396..df25c2acb8 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>
> > @@ -49,12 +50,9 @@ 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);
> > -
> > - /* We do not need the return value. */
> > - __execve (buf, argv, envp);
> > + mounted we fail. We do not need the return value. */
> > + struct fd_to_filename filename;
> > + __execve (__fd_to_filename (fd, &filename), argv, envp);
> >
> > int save = errno;
> >
> >
On 03/05/2021 17:26, Érico Nogueira wrote:
> On Mon May 3, 2021 at 4:26 PM -03, Adhemerval Zanella wrote:
>>
>>
>> On 27/04/2021 10:09, Érico Nogueira via Libc-alpha wrote:
>>> Change made to fchmodat and fexecve. There are tests using xasprintf
>>> instead of this helper as well, but this commit doesn't touch them.
>>
>> To adapt the tests it would require either to build it as
>> internal/static
>> or add fd_to_filename to libsupport. I am not this is really an
>> improvement.
>
> That might explain some of the build failures I got when I tried
> changing the tests.
>
>>
>> LGTM, thanks. I will commit this for you.
>
> Thanks!
>
> I found a few more places where __fd_to_filename could be used, they are
> currently using _fitoa_word together with stpcpy. Would you be
> interested in making __fd_to_filename use _fitoa_word or _itoa_word? Or
> should it be left as is?
I think it might worth to check whether making __fd_to_filename use
_itoa_word yield any code size gain, but in general it seems a good
refactor.
@@ -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,16 +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;
- }
-
- int ret = __chmod (buf, mode);
+ struct fd_to_filename filename;
+ int ret = __chmod (__fd_to_filename (pathfd, &filename), mode);
if (ret != 0)
{
if (errno == ENOENT)
@@ -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>
@@ -49,12 +50,9 @@ 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);
-
- /* We do not need the return value. */
- __execve (buf, argv, envp);
+ mounted we fail. We do not need the return value. */
+ struct fd_to_filename filename;
+ __execve (__fd_to_filename (fd, &filename), argv, envp);
int save = errno;