On 9/20/21 7:43 PM, Andrii Nakryiko wrote:
> Refactor ELF section handler definitions table to use a set of flags and
> unified SEC_DEF() macro. This allows for more succinct and table-like
> set of definitions, and allows to more easily extend the logic without
> adding more verbosity (this is utilized in later patches in the series).
>
> This approach is also making libbpf-internal program pre-load callback
> not rely on bpf_sec_def definition, which demonstrates that future
> pluggable ELF section handlers will be able to achieve similar level of
> integration without libbpf having to expose extra types and APIs.
>
> For starters, update SEC_DEF() definitions and make them more succinct.
> Also convert BPF_PROG_SEC() and BPF_APROG_COMPAT() definitions to
> a common SEC_DEF() use.
>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 183 ++++++++++++++++-------------------------
> 1 file changed, 73 insertions(+), 110 deletions(-)
To summarize VC convo we had about this patch, you don't expect custom sec_def
writers to necessarily follow your sec_def_flags approach, but it's a good
demonstration that a long's worth of flags is plenty for enabling custom
functionality. And custom sec_def writers can treat the cookie as a ptr to a
config struct if they need something more complicated, without imposing the
struct format on all other sec_defs.
[...]
> @@ -7955,15 +7965,14 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> .sec = string, \
> .prog_type = ptype, \
> .expected_attach_type = eatype, \
> - .is_exp_attach_type_optional = eatype_optional, \
> - .is_attachable = attachable, \
> - .is_attach_btf = attach_btf, \
> + .cookie = (long) ( \
> + (eatype_optional ? SEC_EXP_ATTACH_OPT : 0) | \
> + (attachable ? SEC_ATTACHABLE : 0) | \
> + (attach_btf ? SEC_ATTACH_BTF : 0) \
> + ), \
> .preload_fn = libbpf_preload_prog, \
> }
>
> -/* Programs that can NOT be attached. */
I found this comment and APROG_COMPAT comment useful. Not as clear to me what
SEC_NONE implies without some comment explaining or giving example. The other
flags are more obvious to me but might be worth being explicit there as well.
> -#define BPF_PROG_SEC(string, ptype) BPF_PROG_SEC_IMPL(string, ptype, 0, 0, 0, 0)
> -
> /* Programs that can be attached. */
> #define BPF_APROG_SEC(string, ptype, atype) \
> BPF_PROG_SEC_IMPL(string, ptype, atype, true, 1, 0)
> @@ -7976,14 +7985,11 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog,
> #define BPF_PROG_BTF(string, ptype, eatype) \
> BPF_PROG_SEC_IMPL(string, ptype, eatype, false, 0, 1)
>
> -/* Programs that can be attached but attach type can't be identified by section
> - * name. Kept for backward compatibility.
> - */
> -#define BPF_APROG_COMPAT(string, ptype) BPF_PROG_SEC(string, ptype)
> -
> -#define SEC_DEF(sec_pfx, ptype, ...) { \
> +#define SEC_DEF(sec_pfx, ptype, atype, flags, ...) { \
> .sec = sec_pfx, \
> .prog_type = BPF_PROG_TYPE_##ptype, \
> + .expected_attach_type = atype, \
> + .cookie = (long)(flags), \
> .preload_fn = libbpf_preload_prog, \
> __VA_ARGS__ \
> }
> @@ -7996,92 +8002,49 @@ static struct bpf_link *attach_lsm(const struct bpf_program *prog, long cookie);
> static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie);
>
> static const struct bpf_sec_def section_defs[] = {
> - BPF_PROG_SEC("socket", BPF_PROG_TYPE_SOCKET_FILTER),
> + SEC_DEF("socket", SOCKET_FILTER, 0, SEC_NONE),
Didn't know how strictly you felt about checkpatch line-length complaints,
won't comment on them further since you mentioned 100 chars being the new
standard. But would complain about the alignment here and elsewhere in
changes to section_defs even if checkpatch didn't exist :)
> BPF_EAPROG_SEC("sk_reuseport/migrate", BPF_PROG_TYPE_SK_REUSEPORT,
> BPF_SK_REUSEPORT_SELECT_OR_MIGRATE),
> BPF_EAPROG_SEC("sk_reuseport", BPF_PROG_TYPE_SK_REUSEPORT,
> BPF_SK_REUSEPORT_SELECT),
[...]
