Re: [PATCH] bpftool: Add missing libgen.h for basename()

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jan 26, 2024 at 12:53:22PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 05, 2024 at 05:53:18PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Jan 05, 2024 at 04:01:37PM +0100, Jiri Olsa escreveu:
> > > On Fri, Jan 05, 2024 at 09:48:31AM +0100, Jiri Olsa wrote:
> > > > On Thu, Jan 04, 2024 at 10:01:35AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > >    9    51.66 amazonlinux:2                 : Ok   gcc (GCC) 7.3.1 20180712 (Red Hat 7.3.1-17) , clang version 11.1.0 (Amazon Linux 2 11.1.0-1.amzn2.0.2) flex 2.5.37
> > > > >   10    60.77 amazonlinux:2023              : Ok   gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2) , clang version 15.0.7 (Amazon Linux 15.0.7-3.amzn2023.0.1) flex 2.6.4
> > > > >   11    61.29 amazonlinux:devel             : Ok   gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4) , clang version 15.0.6 (Amazon Linux 15.0.6-3.amzn2023.0.2) flex 2.6.4
> > > > >   12    74.72 archlinux:base                : Ok   gcc (GCC) 13.2.1 20230801 , clang version 16.0.6 flex 2.6.4
> > > > > 
> > > > > / $ grep -B8 -A2 -w basename /usr/include/string.h
> > > > > #ifdef _GNU_SOURCE
> > > > > #define	strdupa(x)	strcpy(alloca(strlen(x)+1),x)
> > > > > int strverscmp (const char *, const char *);
> > > > > char *strchrnul(const char *, int);
> > > > > char *strcasestr(const char *, const char *);
> > > > > void *memrchr(const void *, int, size_t);
> > > > > void *mempcpy(void *, const void *, size_t);
> > > > > #ifndef __cplusplus
> > > > > char *basename();
> > > > > #endif
> > > > > #endif
> > > > > / $ cat /etc/os-release
> > > > > NAME="Alpine Linux"
> > > > > ID=alpine
> > > > > VERSION_ID=3.19.0
> > > > > PRETTY_NAME="Alpine Linux v3.19"
> > > > > HOME_URL="https://alpinelinux.org/"
> > > > > BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues"
> > > > > / $
> > > > > 
> > > > > Weird, they had it and now removed the _GNU_SOURCE bits (edge is their
> > > > > devel distro, like rawhide is for fedora, tumbleweed for opensuse, etc).
> > > > 
> > > > let's see, I asked them in here: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15643
> > > 
> > > it got removed in musl libc recently:
> > >   https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7
> > > 
> > > so perhaps switching to POSIX version of basename is the easiest way out?
> > 
> > I think so, in all of perf we use the POSIX one, strdup'ing the arg,
> > etc.
> > 
> > Something like the patch below?
> 
> Quentin, are you ok with this? Then I can send a formal patch.
> 
> Jiri, can I have your Acked-by?

yes, jirka

> 
> - Arnaldo
>  
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index ee3ce2b8000d75d2..a5cc5938c3d7951e 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -7,6 +7,7 @@
> >  #include <ctype.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <libgen.h>
> >  #include <linux/err.h>
> >  #include <stdbool.h>
> >  #include <stdio.h>
> > @@ -56,9 +57,10 @@ static bool str_has_suffix(const char *str, const char *suffix)
> >  
> >  static void get_obj_name(char *name, const char *file)
> >  {
> > -	/* Using basename() GNU version which doesn't modify arg. */
> > -	strncpy(name, basename(file), MAX_OBJ_NAME_LEN - 1);
> > -	name[MAX_OBJ_NAME_LEN - 1] = '\0';
> > +	char file_copy[PATH_MAX];
> > +	/* Using basename() POSIX version to be more portable. */
> > +	strncpy(file_copy, file, PATH_MAX - 1)[PATH_MAX - 1] = '\0';
> > +	strncpy(name, basename(file_copy), MAX_OBJ_NAME_LEN - 1)[MAX_OBJ_NAME_LEN - 1] = '\0';
> >  	if (str_has_suffix(name, ".o"))
> >  		name[strlen(name) - 2] = '\0';
> >  	sanitize_identifier(name);
> 
> -- 
> 
> - Arnaldo