Re: [PATCH v3 5/6] qmp: Added new command to retrieve eBPF blob.

[Markus Armbruster <armbru@redhat.com>]

Andrew Melnychenko <andrew@daynix.com> writes:

> Added command "request-ebpf". This command returns
> eBPF program encoded base64. The program taken from the
> skeleton and essentially is an ELF object that can be
> loaded in the future with libbpf.
>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>

In review of v2, I asked why we need to provide eBPF programs via QMP,
and not simply via the filesystem, like all the other build artifacts.
Daniel explained

    Fetching it from QEMU gives us a strong guarantee that the eBPF
    code actually matches the QEMU binary we're talking to, which is
    useful if you're dealing with RPMs which can be upgraded behind
    your back, or have multiple parallel installs of QEMU.

I asked to work this rationale into the QAPI schema doc comments and the
commit message.

Only the doc comments are updated in this version.  Please update the
commit message as well.

I remain unconvinced this justifies adding an entirely new transport.
If we want to guard against mismatch, we can do it the usual way: bake
the version into the build artifacts that must match, then check they
do.

Since I feel vetoing the change would overstep my authority as a QAPI
maintainer, I'm asking you to treat my objection as advice.

If you you elect not to follow my advice, I want you to describe the
path not chosen in the commit message.  I.e. briefly explain the need
for an exact version match, describe the ways to ensure it we
considered, and the reasons for your pick.

> ---
>  qapi/ebpf.json        | 55 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/meson.build      |  1 +
>  qapi/qapi-schema.json |  1 +
>  3 files changed, 57 insertions(+)
>  create mode 100644 qapi/ebpf.json
>
> diff --git a/qapi/ebpf.json b/qapi/ebpf.json
> new file mode 100644
> index 00000000000..6f9a1f1a0c9
> --- /dev/null
> +++ b/qapi/ebpf.json
> @@ -0,0 +1,55 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +##
> +# = Machines

Copy-pasta :)

> +##
> +
> +{ 'include': 'common.json' }
> +
> +##
> +# @EbpfObject:
> +#
> +# Structure that holds eBPF ELF object encoded in base64.
> +#
> +# Since: 8.1
> +#
> +##
> +{ 'struct': 'EbpfObject',
> +  'data': {'object': 'str'} }
> +
> +##
> +# @EbpfProgramID:
> +#
> +# An enumeration of the eBPF programs. Currently, only RSS is presented.
> +# RSS - Receive side scaling, technology that allows steering traffic
> +# between queues by calculation hash. Users may set up indirection table
> +# and hash/packet types configurations. Used with virtio-net.

Let's stick to the common way to document enum members, like this:

   # The eBPF programs that can be gotten with request-ebpf.
   #
   # @rss: ...

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'EbpfProgramID',
> +  'data': [ { 'name': 'rss', 'if': 'CONFIG_EBPF' } ] }

Uh, this makes just the member @rss conditional on CONFIG_EBPF.
Why not the entire enum?

> +
> +##
> +# @request-ebpf:
> +#
> +# Returns eBPF object that can be loaded with libbpf.
> +# Management applications (g.e. libvirt) may load it and pass file
> +# descriptors to QEMU. Which allows running QEMU without BPF capabilities.
> +# It's crucial that eBPF program/map is compatible with QEMU, so it's
> +# provided through QMP.
> +#
> +# Returns: RSS eBPF object encoded in base64.
> +#
> +# Since: 8.1
> +#
> +##
> +{ 'command': 'request-ebpf',
> +  'data': { 'id': 'EbpfProgramID' },
> +  'returns': 'EbpfObject',
> +  'if': 'CONFIG_EBPF' }
> +
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 9fd480c4d81..12a1cb1f9c2 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -52,6 +52,7 @@ qapi_all_modules = [
>    'transaction',
>    'virtio',
>    'yank',
> +  'ebpf',

Keep this list alphabetically sorted, please.

>  ]
>  if have_system
>    qapi_all_modules += [
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 6594afba312..169cfe8d386 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -79,3 +79,4 @@
>  { 'include': 'virtio.json' }
>  { 'include': 'cryptodev.json' }
>  { 'include': 'cxl.json' }
> +{ 'include': 'ebpf.json' }

This puts it at the end of the QMP reference manual.  Is this the best
spot?