Re: [PATCH net-next 1/5] libbpf: add ability to guess program type based on section name

[Roman Gushchin]

On Fri, Dec 01, 2017 at 02:46:06PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
> > Thanks Roman!
> > One comment in-line.
> > 
> > 2017-11-30 13:42 UTC+0000 ~ Roman Gushchin <guro@xxxxxx>
> > > The bpf_prog_load() function will guess program type if it's not
> > > specified explicitly. This functionality will be used to implement
> > > loading of different programs without asking a user to specify
> > > the program type. In first order it will be used by bpftool.
> > > 
> > > Signed-off-by: Roman Gushchin <guro@xxxxxx>
> > > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > > Cc: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 5aa45f89da93..9f2410beaa18 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -1721,6 +1721,41 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
> > >  BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
> > >  BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
> > >  
> > > +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> > > +{
> > > +	if (!prog->section_name)
> > > +		goto err;
> > > +
> > > +	if (strncmp(prog->section_name, "socket", 6) == 0)
> > > +		return BPF_PROG_TYPE_SOCKET_FILTER;
> > > +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> > > +		return BPF_PROG_TYPE_KPROBE;
> > > +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> > > +		return BPF_PROG_TYPE_KPROBE;
> > > +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> > > +		return BPF_PROG_TYPE_TRACEPOINT;
> > > +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> > > +		return BPF_PROG_TYPE_XDP;
> > > +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> > > +		return BPF_PROG_TYPE_PERF_EVENT;
> > > +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_SKB;
> > > +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_SOCK;
> > > +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> > > +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> > > +		return BPF_PROG_TYPE_SOCK_OPS;
> > > +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> > > +		return BPF_PROG_TYPE_SK_SKB;  
> > 
> > I do not really like these hard-coded lengths, maybe we could work out
> > something nicer with a bit of pre-processing work? Perhaps something like:
> > 
> > #define SOCKET_FILTER_SEC_PREFIX "socket"
> > #define KPROBE_SEC_PREFIX "kprobe/"
> > [â]
> > 
> > #define TRY_TYPE(string, __TYPE)				\
> > 	do {							\
> > 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
> > 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
> > 			return BPF_PROG_TYPE_ ## __TYPE;	\
> > 	} while(0);
> 
> I like the suggestion, but I think return and goto statements hiding
> inside macros are slightly frowned upon in the netdev.  Perhaps just 
> a macro that wraps the strncmp() with sizeof would be enough?  Without
> the return inside?

Hm, I'm not sure that using macroses here makes the code much easier to read.
Maybe we can use just strcmp() instead?
As we compare with hardcoded strings, there is no real difference.
Something like this:

if (!strcmp("socket", prog->section_name))
	return BPF_PROG_TYPE_SOCKET_FILTER;
if (!strcmp("kprobe/", prog->section_name))
	return BPF_PROG_TYPE_KPROBE;
if (!strcmp("kretprobe/", prog->section_name))
	return BPF_PROG_TYPE_KPROBE;
if (!strcmp("tracepoint/", prog->section_name))
	return BPF_PROG_TYPE_TRACEPOINT;
if (!strcmp("xdp", prog->section_name))
	return BPF_PROG_TYPE_XDP;
if (!strcmp("perf_event", prog->section_name))
	return BPF_PROG_TYPE_PERF_EVENT;
if (!strcmp("cgroup/skb", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_SKB;
if (!strcmp("cgroup/sock", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_SOCK;
if (!strcmp("cgroup/dev", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_DEVICE;
if (!strcmp("sockops", prog->section_name))
	return BPF_PROG_TYPE_SOCK_OPS;
if (!strcmp("sk_skb", prog->section_name))
	return BPF_PROG_TYPE_SK_SKB;

Thanks!