2018-12-14 10:45 UTC-0800 ~ Stanislav Fomichev <s...@fomichev.me>
> On 12/14, Quentin Monnet wrote:
>> 2018-12-13 18:50 UTC-0800 ~ Stanislav Fomichev <s...@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.mon...@netronome.com>
>>>> Reviewed-by: Jakub Kicinski <jakub.kicin...@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?
> How do you expect to exercise that knowledge? Something like the following?
>
> #ifdef FEAT_X
> /* we know X is present, use it */
> #else
> # ifdef NO_FEAT_X
> /* we know X is not there, fall back to something else or let the user
> * know we depend on it
> */
> # else
> /* we don't know whether the feature is there or not,
> * what are we supposed to do?
> *
> * isn't it essentially the same as 'ifdef FEAT_X'?
> * we try to use the feature anyway here, I suppose?
> */
> # endif
> #endif
>
> My thinking of using that was something like the following (in a simple
> autoconf like fashion):
> #ifndef FEAT_X
> /* error or fallback to something else */
> #endif
> /* use feature (or whatever fallback we've set up in the previous ifdef)
> */
But then with autoconf you're supposed to probe everything that you need
to know to compile your program, right? I don't believe there would be a
case where some random features are not tested at all by the script...
In your above example, the "#ifndef FEAT_X /* error */ #endif", you do
return an error if the feature has not been probed instead of trying
anyway to see if it's there, as you mentioned above that... Did I miss
something? Did you mean something like "#ifdef FEAT_NO_X /* error */
#endif"? I believe the fact we reliably found that the feature is not
present _is_ an important information.
>
> My worry is that we just export too much and it's hard to use.
I understand and I'm not opposed to changing my output. I'd like to
avoid loosing information (and keep the same amount of info in JSON and
#define outputs).
>
>>
>>>> + 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.
> Makes sense, if you think that it provides more info than just rephrased
> json field name, then go for it :-)
Currently, a bit more descriptive info for the procfs parameters. And
the kernel version is in human-readable form (ok we don't care much).
But it's true it does not bring much. Mostly I thought of it as a nicer
output for someone who wants to probe the system just to have a quick
look at what's supported, it feels easier to interpret than JSON or
#defines.
