Re: [RFC PATCH 4/5] qmp: Added qemu-ebpf-rss-path command.

[Markus Armbruster]

Andrew Melnychenko <andrew@daynix.com> writes:

> New qmp command to query ebpf helper.
> It's crucial that qemu and helper are in sync and in touch.
> Technically helper should pass eBPF fds that qemu may accept.
> And different qemu's builds may have different eBPF programs and helpers.
> Qemu returns helper that should "fit" to virtio-net.
>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>  monitor/qmp-cmds.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json     | 29 +++++++++++++++++
>  2 files changed, 107 insertions(+)
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index f7d64a6457..5dd2a58ea2 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -351,3 +351,81 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error 
> **errp)
>          abort();
>      }
>  }
> +
> +#ifdef CONFIG_LINUX
> +
> +static const char *get_dirname(char *path)
> +{
> +    char *sep;
> +
> +    sep = strrchr(path, '/');
> +    if (sep == path) {
> +        return "/";
> +    } else if (sep) {
> +        *sep = 0;
> +        return path;
> +    }
> +    return ".";
> +}
> +
> +static char *find_helper(const char *name)
> +{
> +    char qemu_exec[PATH_MAX];
> +    const char *qemu_dir = NULL;
> +    char *helper = NULL;
> +
> +    if (name == NULL) {
> +        return NULL;
> +    }
> +
> +    if (readlink("/proc/self/exe", qemu_exec, PATH_MAX) > 0) {
> +        qemu_dir = get_dirname(qemu_exec);
> +
> +        helper = g_strdup_printf("%s/%s", qemu_dir, name);
> +        if (access(helper, F_OK) == 0) {
> +            return helper;
> +        }
> +        g_free(helper);
> +    }
> +
> +    helper = g_strdup_printf("%s/%s", CONFIG_QEMU_HELPERDIR, name);
> +    if (access(helper, F_OK) == 0) {
> +        return helper;
> +    }
> +    g_free(helper);
> +
> +    return NULL;
> +}

This returns the helper in the same directory as the running executable,
or as a fallback the helper in CONFIG_QEMU_HELPERDIR.

Checking F_OK (existence) instea of X_OK is odd.

It uses /proc/self/exe to find the running executable's directory.  This
is specific to Linux[*].  You get different behavior on Linux vs. other
systems.

CONFIG_QEMU_HELPERDIR is $prefix/libexec/.

If $prefix is /usr, then qemu-system-FOO is normally installed in
/usr/bin/, and the helper in /usr/libexec/.  We look for the helper in
the wrong place first, and the right one only when it isn't in the wrong
place.  Feels overcomplicated and fragile.

Consider the following scenario:

* The system has a binary package's /usr/bin/qemu-system-x86_64 and
  /usr/libexec/qemu-ebpf-rss-helper installed

* Alice builds her own QEMU with prefix /usr (and no intention to
  install), resulting in bld/qemu-system-x86_64, bld/qemu-ebpf-rss-path,
  and a symlink bld/x86_64-softmmu/qemu-system-x86_64.

Now:

* If Alice runs bld/qemu-system-x86_64, and the host is Linux,
  find_helper() returns bld/qemu-ebpf-rss-path.  Good.

* If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
  Not good.

* If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
  /usr/libexec/qemu-ebpf-rss-helper.  Not good.

> +
> +HelperPathList *qmp_query_helper_paths(Error **errp)
> +{
> +    HelperPathList *ret = NULL;
> +    const char *helpers_list[] = {
> +#ifdef CONFIG_EBPF
> +        "qemu-ebpf-rss-helper",
> +#endif
> +        NULL
> +    };
> +    const char **helper_iter = helpers_list;
> +
> +    for (; *helper_iter != NULL; ++helper_iter) {
> +        char *path = find_helper(*helper_iter);
> +        if (path) {
> +            HelperPath *helper = g_new0(HelperPath, 1);
> +            helper->name = g_strdup(*helper_iter);
> +            helper->path = path;
> +
> +            QAPI_LIST_PREPEND(ret, helper);
> +        }
> +    }
> +
> +    return ret;
> +}
> +#else
> +
> +HelperPathList *qmp_query_helper_paths(Error **errp)
> +{
> +    return NULL;
> +}
> +
> +#endif
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 156f98203e..023bd2120d 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -519,3 +519,32 @@
>   'data': { '*option': 'str' },
>   'returns': ['CommandLineOptionInfo'],
>   'allow-preconfig': true }
> +
> +##
> +# @HelperPath:
> +#
> +# Name of the helper and binary location.
> +##
> +{ 'struct': 'HelperPath',
> +  'data': {'name': 'str', 'path': 'str'} }
> +
> +##
> +# @query-helper-paths:
> +#
> +# Query specific eBPF RSS helper for current qemu binary.
> +#
> +# Returns: list of object that contains name and path for helper.
> +#
> +# Example:
> +#
> +# -> { "execute": "query-helper-paths" }
> +# <- { "return": [
> +#        {
> +#          "name": "qemu-ebpf-rss-helper",
> +#          "path": "/usr/local/libexec/qemu-ebpf-rss-helper"
> +#        }
> +#      ]
> +#    }
> +#
> +##
> +{ 'command': 'query-helper-paths', 'returns': ['HelperPath'] }

The name query-helper-paths is generic, the documented purpose "Query
specific eBPF RSS helper" is specific.

qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
in sync with QEMU.

I doubt a query command is a good way to help with using the right one.
qemu-system-FOO doesn't really know where the right one is.  Only the
person or program that put them where they are does.

If we want to ensure the right helper runs, we should use a build
identifier compiled into the programs, like we do for modules.

For modules, the program loading a module checks the module's build
identifier matches its own.

For programs talking to each other, the peers together check their build
identifiers match.

For programs where that isn't practical, the management application can
check.

This should be a lot more reliable.

Helpers QEMU code runs itself should be run as
CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override.  This is
how qemu-bridge-helper works.

Helpers some other program runs are that other program's problem.
They'll probably work the same: built-in default that can be overridden
with configuration.


[*] For detailed advice, see
https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe