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)
*/
My worry is that we just export too much and it's hard to use.
>
> >> + 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 :-)
