Messages in this thread | | | Subject | Re: [PATCH v2] perf build: Build error in libbpf with EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" | From | Daniel Borkmann <> | Date | Fri, 27 Jul 2018 20:29:42 +0200 |
| |
On 07/27/2018 07:57 PM, Jakub Kicinski wrote: > On Fri, 27 Jul 2018 09:22:03 +0200, Thomas-Mich Richter wrote: >> On 07/27/2018 04:16 AM, Daniel Borkmann wrote: >>> On 07/26/2018 03:48 AM, Jakub Kicinski wrote: >>>> On Wed, 25 Jul 2018 09:21:26 +0200, Thomas Richter wrote: >>>>> commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their own sections") >>>> >>>> Hmm.. are you sure it's not 531b014e7a2f ("tools: bpf: make use of >>>> reallocarray") that caused the issue? That commit made us switch from >>>> XSI-compliant to GNU-specific strerror_r() implementation.. >>>> >>>> /me checks >>>> >>>> Yes it looks like 531b014e7a2f~ builds just fine. >>>> >>>> Daniel, did you try to apply v1 to the bpf tree? Perhaps there is a >>>> confusion about the trees here, if this is caused by my recent change >>>> it's a bpf-next material. strerror() works, but strerror_r() seems >>>> nicer, so perhaps we could keep it if the patch worked in bpf-next? >>> >>> Yeah indeed, the issue is only in bpf-next. When I compile libbpf from >>> bpf tree with the below flags then it's all good> >>> Agree that we should rather use strerror_r() given this is a library. >> >> Are you sure to stick with strerror_r? I ask because it is the only >> occurence of strerror_r in this file. All other error messages use strerror >> as in: >> pr_warning("failed to create map (name: '%s'): %s\n", >> map->name, >> strerror(errno)); >> >> $ fgrep strerror tools/lib/bpf/libbpf.c >> strerror(errno)); >> issue I try to solve---> strerror_r(-err, errmsg, sizeof(errmsg)); >> map->name, strerror(errno), errno); >> strerror(errno)); >> pr_warning("load bpf program failed: %s\n", strerror(errno)); >> pr_warning("failed to statfs %s: %s\n", dir, strerror(errno)); >> pr_warning("failed to pin program: %s\n", strerror(errno)); >> pr_warning("failed to mkdir %s: %s\n", path, strerror(-err)); >> pr_warning("failed to pin map: %s\n", strerror(errno)); >> $ >> >> The next issue with strerror_r is to assign the return value to a variable. >> Then we end up with variable set but not used: >> libbpf.c: In function ‘bpf_object__elf_collect’: >> libbpf.c:809:35: error: variable ‘cp’ set but not used [-Werror=unused-but-set-variable] >> char errmsg[STRERR_BUFSIZE], *cp; >> ^ >> cc1: all warnings being treated as errors > > The GNU-specific strerror_r() returns a pointer to a string containing > the error message. This may be either a pointer to a string that the > function stores in buf, or a pointer to some (immutable) static string > (in which case buf is unused). If the function stores a string in buf, > then at most buflen bytes are stored (the string may be truncated if > buflen is too small and errnum is unknown). The string always includes > a terminating null byte ('\0'). > > IOW you gotta use the return value. > >> Here is the source: >> if (err) { >> char errmsg[STRERR_BUFSIZE], *cp; >> >> cp = strerror_r(-err, errmsg, sizeof(errmsg)); >> pr_warning("failed to alloc program %s (%s): %s", >> name, obj->path, errmsg); >> } >> >> To fix this requires something like: >> pr_warning("failed to alloc program %s (%s): %s", >> name, obj->path, cp); > > This looks correct. > >> And after pr_warning() is done, the local array errmsg is deleted. >> >> A lot of overkill in my opinion, unless I miss something. > > IMO using potentially mutli-thread unsafe functions in a library is not > optimal, we should strive to convert the other instances of strerror() > rather than move the other way.
Yeah, fully agree. We should convert the other ones as well over to use strerror_r().
Thanks, Daniel
| |