Re: [PATCH 09/27] qemu: Introduce helper functions for passing FDs to qemu
On Thu, Feb 10, 2022 at 18:35:44 +0100, Ján Tomko wrote:
On a Wednesday in 2022, Peter Krempa wrote:
> The existing helpers we have are very clumsy and there's no integration
> with the monitor.
>
> This patch introduces new helpers to bridge the gap and simplify handing
[...]
> +/**
> + * qemuFDPassNew:
> + * @prefix: prefix used for naming the passed FDs
> + * @dompriv: qemu domain private data
> + * @useFDSet: Always use FD sets (/dev/fdset/N) for this instance
> + *
> + * Create a new helper object for passing FDs to qemu. If @useFDSet is false
> + * the older approach of directly using FD number on the commandline and
'getfd'
> + * QMP command is used. Otherwise '-add-fd' and /dev/fdset/N is used to
pass the
> + * FD.
> + *
I dislike using bools in helpers, can this be turned into a flag?
Well I dislike flags because it makes the code of the function harder to
read as opposed to making the call itself easier to read, so I try to
avoid them if it's feasible in a given situation.
Alternatively, to discourage the older approach you can create a
thin
qemuFDPassNewLegacy wrapper and leave qemuFDPassNew without the comment.
I think I'll go with this one.
(Can we even convince QEMU to use fdset for sockets? Or is that not
worth pursuing?)
I actually have a qemu patch prepared which makes the code paths using
direct FD passing or the 'getfd' QMP command to accept fdsets too, but I
still have to test it.
The idea is to hide the complexity in these wrappers and then just use
fdsets.
> + * Non-test uses must pass a valid @dompriv.
> + *
> + * @prefix is used for naming the FD if needed and is later referenced when
> + * removing the FDSet via monitor.
> + */
> +qemuFDPass *
> +qemuFDPassNew(const char *prefix,
> + void *dompriv,
> + bool useFDSet)
> +{
> + qemuDomainObjPrivate *priv = dompriv;
> + qemuFDPass *fdpass = g_new0(qemuFDPass, 1);
> +
> + fdpass->prefix = g_strdup(prefix);
> + fdpass->useFDSet = useFDSet;
> +
> + if (fdpass->useFDSet && priv)
> + fdpass->fdSetID = qemuDomainFDSetIdNew(priv);
> +
> + return fdpass;
> +}
> +
> +
> +/**
> + * qemuFDPassAddFD:
> + * @fdpass: The fd passing helper struct
> + * @fd: File descriptor to pass
> + * @suffix: Name suffix for the file descriptor name
> + *
> + * Adds @fd to be passed to qemu when transferring @fdpass to qemu.
QEMU
> When @fdpass
> + * is configured to use FD set mode, multiple file descriptors can be passed by
> + * calling this function repeatedly.
> + *
> + * @suffix is used to build the name of the file descriptor by concatenating
> + * it with @prefix passed to qemuFDPassNew. @suffix may be NULL, in which case
> + * it's considered to be an empty string.
> + *
> + * Returns 0 on success, -1 on error (when attempting to pass multiple FDs) using
> + * the 'direct' method.
> + */
> +int
> +qemuFDPassAddFD(qemuFDPass *fdpass,
> + int *fd,
> + const char *suffix)
> +{
> + struct qemuFDPassFD newfd = { .fd = *fd };
> +
> + if (newfd.fd < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid file descriptor"));
> + return -1;
> + }
> +
> + if (fdpass->nfds >= 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("direct FD passing supports only 1 file
descriptor"));
This check conflicts with the comment above saying multiple FDs are
allowed with fdsets. Also, please s/1/one/
oops, the idea was to do that check only when the legacy mode is used.
Changes in this series actually don't use the multi-fd mode yet.