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.
