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

[Daniel P . Berrangé]

On Fri, Jun 11, 2021 at 09:15:52AM -0500, Eric Blake wrote:
> On Wed, Jun 09, 2021 at 01:04:56PM +0300, Andrew Melnychenko wrote:
> > 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 ".";
> > +}
> 
> Seems like this function is duplicating what glib should already be
> able to do.
> 
> > +
> > +static char *find_helper(const char *name)
> > +{
> > +    char qemu_exec[PATH_MAX];
> 
> Stack-allocating a PATH_MAX array for readlink() is poor practice.
> Better is to use g_file_read_link().
> 
> > +    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);
> 
> Could we use a compile-time determination of where we were (supposed)
> to be installed, and therefore where our helper should be installed,
> rather than the dynamic /proc/self/exe munging?

Yeah I think avoiding /proc/self/exe is desirable, because I can
imagine scenarios where this can lead to picking the wrong helper.
Better to always use the compile time install directory.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|