Re: [PATCH bpf-next 11/15] bpftool: add skeleton codegen command

[Stanislav Fomichev]

On 12/12, Alexei Starovoitov wrote:
> On Thu, Dec 12, 2019 at 10:43:34AM -0800, Jakub Kicinski wrote:
> One more point from Stan's email:
> 
> > You can replace "our build system" with some other project you care about,
> > like systemd. They'd have the same problem with vendoring in recent enough
> 
> we've been working with systemd folks for ~8 month to integrate libbpf into
> their build that is using meson build system and their CI that is github based.
> So we're well aware about systemd requirements for libbpf and friends.
Just curious (searching on systemd github for bpftool/libbpf doesn't
show up any code/issues): are you saying that there will be another ~8 months
to bring in bpftool or that it's already being worked on as part of
libbpf integration?

> > bpftool or waiting for every distro to do it. And all this work is
> > because you think that doing:
> >
> >        my_obj->rodata->my_var = 123;
> >
> > Is easier / more type safe than doing:
> >        int *my_var = bpf_object__rodata_lookup(obj, "my_var");
> >        *my_var = 123;
> 
> Stan, you conveniently skipped error checking. It should have been:
>     int *my_var = bpf_object__rodata_lookup(obj, "my_var");
>     if (IS_ERROR_NULL(my_var))
>         goto out_cleanup;
>      *my_var = 123;
Yeah, but you have a choice, right? You can choose to check the error
and support old programs that don't export some global var and a new
program that has it. Or you can skip the error checks and rely on null
deref crash which is sometimes an option.

(might be not relevant with the introduction of EMBED_FILE which you
seem to be using more and more; ideally, we still would like to be able to
distribute bpf.o and userspace binary separately).

> Take a look at Andrii's patch 13/15:
> 5 files changed, 149 insertions(+), 249 deletions(-)
> Those are simple selftests, yet code removal is huge. Bigger project benefits
> even more.
Excluding fentry/fexit tests (where new find_program_by_title+attach
helper and mmap might help), it looks like the majority of those gains come
from the fact that the patch in question doesn't do any error checking.
You can drop all the CHECK() stuff for existing
find_map_by_name/find_prog_by_name instead and get the same gains.

[as usual, feel free to ignore me, I don't want to keep flaming about
it, but it's hard not to reply]