> On Aug 4, 2020, at 10:32 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Aug 4, 2020 at 8:59 PM Song Liu <songliubraving@xxxxxx> wrote:
>>
>>
>>
>>> On Aug 4, 2020, at 6:38 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>>>
>>> On Mon, Aug 3, 2020 at 6:18 PM Song Liu <songliubraving@xxxxxx> wrote:
>>>>
>>>>
>>>>
>>>>> On Aug 2, 2020, at 6:40 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>>>>>
>>>>> On Sat, Aug 1, 2020 at 1:50 AM Song Liu <songliubraving@xxxxxx> wrote:
>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>
>>>>>> };
>>>>>>
>>>>>> LIBBPF_API int bpf_prog_test_run_xattr(struct bpf_prog_test_run_attr *test_attr);
>>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>>> index b9f11f854985b..9ce175a486214 100644
>>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>>> @@ -6922,6 +6922,7 @@ static const struct bpf_sec_def section_defs[] = {
>>>>>> BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
>>>>>> BPF_PROG_SEC("lwt_xmit", BPF_PROG_TYPE_LWT_XMIT),
>>>>>> BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
>>>>>> + BPF_PROG_SEC("user", BPF_PROG_TYPE_USER),
>>>>>
>>>>> let's do "user/" for consistency with most other prog types (and nice
>>>>> separation between prog type and custom user name)
>>>>
>>>> About "user" vs. "user/", I still think "user" is better.
>>>>
>>>> Unlike kprobe and tracepoint, user prog doesn't use the part after "/".
>>>> This is similar to "perf_event" for BPF_PROG_TYPE_PERF_EVENT, "xdl" for
>>>> BPF_PROG_TYPE_XDP, etc. If we specify "user" here, "user/" and "user/xxx"
>>>> would also work. However, if we specify "user/" here, programs that used
>>>> "user" by accident will fail to load, with a message like:
>>>>
>>>> libbpf: failed to load program 'user'
>>>>
>>>> which is confusing.
>>>
>>> xdp, perf_event and a bunch of others don't enforce it, that's true,
>>> they are a bit of a legacy,
>>
>> I don't see w/o "/" is a legacy thing. BPF_PROG_TYPE_STRUCT_OPS just uses
>> "struct_ops".
>>
>>> unfortunately. But all the recent ones do,
>>> and we explicitly did that for xdp_dev/xdp_cpu, for instance.
>>> Specifying just "user" in the spec would allow something nonsensical
>>> like "userargh", for instance, due to this being treated as a prefix.
>>> There is no harm to require users to do "user/my_prog", though.
>>
>> I don't see why allowing "userargh" is a problem. Failing "user" is
>> more confusing. We can probably improve that by a hint like:
>>
>> libbpf: failed to load program 'user', do you mean "user/"?
>>
>> But it is pretty silly. "user/something_never_used" also looks weird.
>
> "userargh" is terrible, IMO. It's a different identifier that just
> happens to have the first 4 letters matching "user" program type.
> There must be either a standardized separator (which happens to be
> '/') or none. See the suggestion below.
We have no problem deal with "a different identifier that just happens
to have the first letters matching", like xdp vs. xdp_devmap and
xdp_cpumap, right?
>>
>>> Alternatively, we could introduce a new convention in the spec,
>>> something like "user?", which would accept either "user" or
>>> "user/something", but not "user/" nor "userblah". We can try that as
>>> well.
>>
>> Again, I don't really understand why allowing "userblah" is a problem.
>> We already have "xdp", "xdp_devmap/", and "xdp_cpumap/", they all work
>> fine so far.
>
> Right, we have "xdp_devmap/" and "xdp_cpumap/", as you say. I haven't
> seen so much pushback against trailing forward slash with those ;)
I haven't seen any issue with old "perf_event", "xdp" and new "struct_ops"
either.
>
> But anyways, as part of deprecating APIs and preparing libbpf for 1.0
> release over this half, I think I'm going to emit warnings for names
> like "prog_type_whatever" or "prog_typeevenworse", etc. And asking
> users to normalize section names to either "prog_type" or
> "prog_type/something/here", whichever makes sense for a specific
> program type.
Exactly, "user" makes sense here; while "kprobe/__set_task_comm" makes
sense for kprobe.
> Right now libbpf doesn't allow two separate BPF programs
> with the same section name, so enforcing strict "user" is limiting to
> users. We are going to lift that restriction pretty soon, though. But
> for now, please stick with what we've been doing lately and mark it as
> "user/", later we'll allow just "user" as well.
Since we would allow "user" later, why we have to reject it for now?
Imagine the user just compiled and booted into a new kernel with user
program support; and then got the following message:
libbpf: failed to load program 'user'
If I were the user, I would definitely question whether the kernel was
correct...
