On Mon, Oct 09, 2023 at 09:16:11 +0300, Andrew Melnychenko wrote:
> Added code for monitor and monitor_json.
> The "request-ebpf" return's eBPF binary object encoded in base64.
> The function qemuMonitorGetEbpf() returns a decoded blob.
>
> QEMU provides eBPF that can be loaded and passed to it from Libvirt.
> QEMU requires exact eBPF program/maps, so it can be retrieved using QAPI.
> To load eBPF program - administrative capabilities are required, so Libvirt may load it and pass it to the QEMU instance.
> For now, there is only "RSS"(Receive Side Scaling) for virtio-net eBPF program and maps.
Please wrap the text in the commit message to the usual line length.
Since it's just explanation, the sentences can be neatly wrapped. Long
lines in commit message are acceptable only if it's impossible to split
them or splitting them would destroy the content (e.g. do not split
pasted output of commands).
> Signed-off-by: Andrew Melnychenko <andrew@xxxxxxxxxx>
> ---
> src/qemu/qemu_monitor.c | 23 +++++++++++++++++++++++
> src/qemu/qemu_monitor.h | 3 +++
> src/qemu/qemu_monitor_json.c | 21 +++++++++++++++++++++
> src/qemu/qemu_monitor_json.h | 3 +++
> 4 files changed, 50 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 320729f067..07596c78ee 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4512,3 +4512,26 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
>
> return NULL;
> }
See coding style problems mentioned in reply to cover letter.
> +
> +void *
> +qemuMonitorGetEbpf(qemuMonitor *mon, const char *ebpfName, size_t *size)
> +{
> + QEMU_CHECK_MONITOR_NULL(mon);
Fails to compile:
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
from /usr/include/glib-2.0/glib/gtypes.h:34,
from /usr/include/glib-2.0/glib/galloca.h:34,
from /usr/include/glib-2.0/glib.h:32,
from /usr/include/glib-2.0/gobject/gbinding.h:30,
from /usr/include/glib-2.0/glib-object.h:24,
from /usr/include/glib-2.0/gio/gioenums.h:30,
from /usr/include/glib-2.0/gio/giotypes.h:30,
from /usr/include/glib-2.0/gio/gio.h:28,
from ../../../libvirt/src/qemu/qemu_monitor.c:27:
../../../libvirt/src/qemu/qemu_monitor.c: In function ‘qemuMonitorGetEbpf’:
/usr/include/glib-2.0/glib/gmacros.h:1347:43: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
1347 | #define _GLIB_CLEANUP(func) __attribute__((cleanup(func)))
| ^~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmacros.h:1380:29: note: in expansion of macro ‘_GLIB_CLEANUP’
1380 | #define g_autoptr(TypeName) _GLIB_CLEANUP(_GLIB_AUTOPTR_FUNC_NAME(TypeName)) _GLIB_AUTOPTR_TYPENAME(TypeName)
| ^~~~~~~~~~~~~
../../../libvirt/src/qemu/qemu_monitor.c:4520:5: note: in expansion of macro ‘g_autoptr’
4520 | g_autoptr(virJSONValue) reply = NULL;
| ^~~~~~~~~
cc1: all warnings being treated as errors
Make sure your code compiles and passes test suite after every single
patch per our coding guidelines.
> + g_autoptr(virJSONValue) reply = NULL;
> + const char *ebpfBase64 = NULL;
> + void *ebpfObject = NULL;
> +
> + if (ebpfName == NULL || size == NULL)
You are mixing returns which do not set a libvirt error ...
> + return NULL;
> +
> + reply = qemuMonitorJSONGetEbpf(mon, ebpfName);
> +
> + if (reply == NULL)
... with those that do. This is not a good idea as the caller can't
distinguish easily whether it's an error or desired NULL value. We don't
allow that.
> + return NULL;
> +
> + ebpfBase64 = virJSONValueObjectGetString(reply, "object");
> +
> + ebpfObject = g_base64_decode(ebpfBase64, size);
I'd very strongly prefer that this is stored in the base64 form inside
libvirt and decoded just before use. Storing strings is simpler, doesn't
require you to pass around the 'size' needlessly and simplifies the XML
formatter/parser in capabilities.
Also please move all extraction of data from the monitor reply to the
JSON command. The qemu_monitor.c wrapper at this point should simply
pass through. It might eventually be completely removed as it serves no
purpose once the HMP monitor support was deleted. (And before that it'd
be semantically wrong because you pass the JSON out of the QMP
function).
> +
> + return ebpfObject;
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6c590933aa..15f32f105c 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1579,3 +1579,6 @@ qemuMonitorExtractQueryStats(virJSONValue *info);
> virJSONValue *
> qemuMonitorGetStatsByQOMPath(virJSONValue *arr,
> char *qom_path);
> +
> +void *
> +qemuMonitorGetEbpf(qemuMonitor *mon, const char *ebpfName, size_t *size);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 8152eea9a0..a7d0865ddc 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -8851,3 +8851,24 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
>
> return virJSONValueObjectStealArray(reply, "return");
> }
> +
> +
> +virJSONValue *
> +qemuMonitorJSONGetEbpf(qemuMonitor *mon, const char *ebpfName)
> +{
> + g_autoptr(virJSONValue) cmd = NULL;
> + g_autoptr(virJSONValue) reply = NULL;
> +
> + if (!(cmd = qemuMonitorJSONMakeCommand("request-ebpf",
> + "s:id", ebpfName, NULL)))
> + return NULL;
> +
> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
Once again, both of the above functions set a libvirt error on failure
...
> + return NULL;
> +
> + /* return empty hash */
> + if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
> + return NULL;
But this does not. Caller can't distinguish between a monitor failure
and command not found. Also the comment makes no sense.
> +
> + return virJSONValueObjectStealObject(reply, "return");
> +}
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 06023b98ea..6a2fc963ba 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -825,3 +825,6 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon,
> qemuMonitorQueryStatsTargetType target,
> char **vcpus,
> GPtrArray *providers);
> +
> +virJSONValue *
> +qemuMonitorJSONGetEbpf(qemuMonitor *mon, const char *ebpfName);
> --
> 2.42.0
>
