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

2018-12-14 10:45 UTC-0800 ~ Stanislav Fomichev [off-list ref]

On 12/14, Quentin Monnet wrote:

quoted

2018-12-13 18:50 UTC-0800 ~ Stanislav Fomichev [off-list ref]

quoted

On 12/13, Quentin Monnet wrote:

quoted

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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---

quoted

[..]

quoted

+		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).

quoted

quoted

quoted

+	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.