On Tue, Oct 1, 2019 at 4:31 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Tue, Oct 1, 2019 at 3:45 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > > > On Mon, Sep 30, 2019 at 11:58:51AM -0700, Andrii Nakryiko wrote: > > > Make bpf_helpers.h and bpf_endian.h official part of libbpf. Ensure they > > > are installed along the other libbpf headers. > > > > > > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx> > > > > [...] [...] > > > > As mentioned earlier, the whole helper function description below > > should get a big cleanup in here when moved into libbpf API. > > Right, I just recalled that today, sorry I didn't do it for this version. > > There were two things you mentioned on that thread that you wanted to clean up: > 1. using __u32 instead int and stuff like that > 2. using macro to hide some of the ugliness of (void *) = BPF_FUNC_xxx > > So with 1) I'm concerned that we can't just assume that __u32 is > always going to be defined. Also we need bpf_helpers.h to be usable > both with including system headers, as well as auto-generated > vmlinux.h. In first case, I don't think we can assume that typedef is > always defined, in latter case we can't really define it on our own. > So I think we should just keep it as int, unsigned long long, etc. > Thoughts? > > For 2), I'm doing that right now, but it's not that much cleaner, let's see. > > Am I missing something else? Ok, so this doesn't work with just #define BPF_FUNC(NAME, ...) (*NAME)(__VA_ARGS__) __attribute__((unused)) = (void *) BPF_FUNC_##NAME because helper is called bpf_map_update_elem(), but enum value is BPF_FUNC_map_update_elem (without bpf_ prefix). So one way to do this would be to have bpf_ prepended to function name by macro: static int BPF_FUNC(map_update_elem, ....); But this is super confusing, because this definition becomes basically un-greppable. I think we should just keep it as is, or go all the way in for super-verbose static int BPF_FUNC(BPF_FUNC_map_update_elem, bpf_map_elem, ...); I hate both, honestly. > > > > > > +static void *(*bpf_map_lookup_elem)(void *map, const void *key) = > > > + (void *) BPF_FUNC_map_lookup_elem; > > > +static int (*bpf_map_update_elem)(void *map, const void *key, const void *value, > > > + unsigned long long flags) = > > [...] > > > + [...] > > > -- > > > 2.17.1 > > >
