On Fri, 6 Jul 2018 09:16:24 +0200, Daniel Borkmann wrote:
> On 07/06/2018 12:57 AM, Jakub Kicinski wrote:
> > On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote:
> >> On 07/04/2018 04:54 AM, Jakub Kicinski wrote:
> >>> Add map parameter to prog load which will allow reuse of existing
> >>> maps instead of creating new ones.
> >>>
> >>> Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> >>> Reviewed-by: Quentin Monnet <quentin.mon...@netronome.com>
> >> [...]
> >>> +
> >>> + fd = map_parse_fd(&argc, &argv);
> >>> + if (fd < 0)
> >>> + goto err_free_reuse_maps;
> >>> +
> >>> + map_replace = reallocarray(map_replace, old_map_fds + 1,
> >>> + sizeof(*map_replace));
> >>> + if (!map_replace) {
> >>> + p_err("mem alloc failed");
> >>> + goto err_free_reuse_maps;
> >>
> >> Series in general looks good to me. However, above reallocarray() doesn't
> >> exist and hence build fails, please see below. Is that from newest glibc?
> >>
> >> You probably need some fallback implementation or in general have something
> >> bpftool internal that doesn't make a bet on its availability.
> >>
> >> # make
> >>
> >> Auto-detecting system features:
> >> ... libbfd: [ on ]
> >> ... disassembler-four-args: [ OFF ]
> >>
> >> CC bpf_jit_disasm.o
> >> LINK bpf_jit_disasm
> >> CC bpf_dbg.o
> >> LINK bpf_dbg
> >> CC bpf_asm.o
> >> BISON bpf_exp.yacc.c
> >> CC bpf_exp.yacc.o
> >> FLEX bpf_exp.lex.c
> >> CC bpf_exp.lex.o
> >> LINK bpf_asm
> >> DESCEND bpftool
> >>
> >> Auto-detecting system features:
> y>> ... libbfd: [ on ]
> >> ... disassembler-four-args: [ OFF ]
> >>
> >> CC map_perf_ring.o
> >> CC xlated_dumper.o
> >> CC perf.o
> >> CC prog.o
> >> prog.c: In function ‘do_load’:
> >> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’
> >> [-Wimplicit-function-declaration]
> >> map_replace = reallocarray(map_replace, old_map_fds + 1,
> >> ^~~~~~~~~~~~
> >> prog.c:785:16: warning: assignment makes pointer from integer without a
> >> cast [-Wint-conversion]
> >> map_replace = reallocarray(map_replace, old_map_fds + 1,
> >> ^
> >> CC common.o
> >> CC cgroup.o
> >> CC main.o
> >> CC json_writer.o
> >> CC cfg.o
> >> CC map.o
> >> CC jit_disasm.o
> >> CC disasm.o
> >>
> >> Auto-detecting system features:
> >> ... libelf: [ on ]
> >> ... bpf: [ on ]
> >>
> >> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs
> >> from latest version at 'include/uapi/linux/bpf.h'
> >> CC libbpf.o
> >> CC bpf.o
> >> CC nlattr.o
> >> CC btf.o
> >> LD libbpf-in.o
> >> LINK libbpf.a
> >> LINK bpftool
> >> prog.o: In function `do_load':
> >> prog.c:(.text+0x23d): undefined reference to `reallocarray'
> >> collect2: error: ld returned 1 exit status
> >> Makefile:89: recipe for target 'bpftool' failed
> >> make[1]: *** [bpftool] Error 1
> >> Makefile:99: recipe for target 'bpftool' failed
> >> make: *** [bpftool] Error 2
> >
> > Oh no.. Sorry & thanks for catching this. It would be nice to not
> > depend on Glibc version defines, in case someone is using a different
> > library. Jiong suggested we can just use the feature detection, so I
> > have something like this:
>
> Yeah, that would be okay to do if you want to go that route, sure. Other
> option
> I had in mind would have been to import include/linux/overflow.h into the
> tools/include/linux/ and have a minor wrapper similar to kmalloc_array() in a
> utils.h in bpftool to get to the same for all users. But I think feature test
> is totally fine too, and in general some form of reallocarray() would be good
> to have rather than plain realloc().
>
> So, below complies for me. Although don't we need to define a CFLAG based on
> the outcome of the test similar as in feature-disassembler-four-args?
> Otherwise
> with the below diff the test doesn't really do much, no? Meaning, adding a ...
>
> ifeq ($(feature-reallocarray), 1)
> CFLAGS += -DHAVE_REALLOCARRAY
> endif
>
> ... to the Makefile and in compat.h having it enabled through:
>
> #ifndef HAVE_REALLOCARRAY
> static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
> {
> return realloc(ptr, nmemb * size);
> }
> #endif
Ugh, you're very right, reallocarray is a weak alias in glibc, which is
probably why I didn't see any errors.
> In any case, we could use reallocarray() also in couple of other places in
> bpftool and libbpf.
I'll look into it, too.
