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

[Stanislav Fomichev <sdf@fomichev.me>]

On 12/14, Quentin Monnet wrote:
> 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?
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 :-)