Re: [PATCH bpf-next 1/8] tools: bpftool: add basic probe capability, probe syscall and kversion

[Quentin Monnet <quentin.monnet@netronome.com>]

2018-12-13 18:50 UTC-0800 ~ Stanislav Fomichev <sdf@fomichev.me>
> On 12/13, Quentin Monnet wrote:
>> Add a new component and command for bpftool, in order to probe the
>> system to dump a set of eBPF-related parameters so that users can know
>> what features are available on the system.
>>
>> Parameters are dumped in plain or JSON output (with -j/-p options).
>> Additionally, a specific keyword can be used to provide a third possible
>> output so that the parameters are dumped as #define-d macros, ready to
>> be saved to a header file and included in an eBPF-based project.
>>
>> The current patch introduces probing of two simple parameters:
>> availability of the bpf() system call, and kernel version. Later commits
>> will add other probes.
>>
>> Sample output:
>>
>>     # bpftool feature probe kernel
>>     Scanning system call and kernel version...
>>     Kernel release is 4.19.0
>>     bpf() syscall is available
>>
>>     # bpftool --json --pretty feature probe kernel
>>     {
>>         "syscall_config": {
>>             "kernel_version_code": 267008,
>>             "have_bpf_syscall": true
>>         }
>>     }
>>
>>     # bpftool feature probe kernel macros prefix BPFTOOL_
>>     /*** System call and kernel version ***/
>>     #define BPFTOOL_LINUX_VERSION_CODE 267008
>>     #define BPFTOOL_BPF_SYSCALL
>>
>> The optional "kernel" keyword enforces probing of the current system,
>> which is the only possible behaviour at this stage. It can be safely
>> omitted.
>>
>> The feature comes with the relevant man page, but bash completion will
>> come in a dedicated commit.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---

> 
> [..]
> 
>> +		printf("#define %s%s%s\n", define_prefix,
>> +		       res ? "" : "NO_", define_name);
> 
> Should we keep it autoconf style and do:
> #define XYZ 1 - in case of supported feature
> /* #undef XYZ */ - in case of unsupported feature
> 
> ?

But then if you include this as a header, you have no way to distinguish
the case when the feature is not supported from when bpftool did not
attempt to run the probe at all?

>> +	else
>> +		printf("%s is %savailable\n", plain_name, res ? "" : "NOT ");
> 
> Why not do printf("%s %s\n", feat_name, res ? "yes" : "no") instead?
> And not complicate (drop) the output with human readability. One
> possible (dis)advantage - scripts can use this.

I've been pondering about the interest of keeping human-readable output.
I think it helps users understand the output, especially for the procfs
parameters for example.

As for scripts, they can and should stick to JSON. Plain output from
bpftool is not meant to be reliable for scripting.