[libvirt PATCH 0/3] Introudce virCommandPassFDIndex
Simplify passing file descriptors via add-fd by adding a new parameter to virCommandPassFD that returns the index of the passed FD right away, removing the need to look it up later and pretend to handle errors which cannot happen - we've just put the file descriptor there, why would it not be there? This should also silence Coverity - in that regard it's an alternative to Pavel's patch: https://www.redhat.com/archives/libvir-list/2020-November/msg00885.html Ján Tomko (3): util: introduce virCommandPassFDIndex qemu: introduce qemuBuildFDSet qemu: use qemuVirCommandGetDevSet less src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 38 +++++++++++++++++++++++++------------- src/util/vircommand.c | 28 ++++++++++++++++++++++++++-- src/util/vircommand.h | 5 +++++ 4 files changed, 57 insertions(+), 15 deletions(-) -- 2.26.2
Just like virCommandPassFD, but it also returns an index of the passed FD in the FD set. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 28 ++++++++++++++++++++++++++-- src/util/vircommand.h | 5 +++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 79a23f34cb..08f16eedb4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1883,6 +1883,7 @@ virCommandNewVAList; virCommandNonblockingFDs; virCommandPassFD; virCommandPassFDGetFDIndex; +virCommandPassFDIndex; virCommandRawStatus; virCommandRequireHandshake; virCommandRun; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 9a4f3d7f32..2b9f28a918 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -999,10 +999,11 @@ virCommandNewVAList(const char *binary, va_list list) VIR_FORCE_CLOSE(fd) /** - * virCommandPassFD: + * virCommandPassFDIndex: * @cmd: the command to modify * @fd: fd to reassign to the child * @flags: extra flags; binary-OR of virCommandPassFDFlags + * @idx: pointer to fill with the index of the FD in the transfer set * * Transfer the specified file descriptor to the child, instead * of closing it on exec. @fd must not be one of the three @@ -1013,7 +1014,7 @@ virCommandNewVAList(const char *binary, va_list list) * should cease using the @fd when this call completes */ void -virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) +virCommandPassFDIndex(virCommandPtr cmd, int fd, unsigned int flags, size_t *idx) { int ret = 0; @@ -1037,6 +1038,29 @@ virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) VIR_COMMAND_MAYBE_CLOSE_FD(fd, flags); return; } + + if (idx) + *idx = cmd->npassfd - 1; +} + +/** + * virCommandPassFD: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * @flags: extra flags; binary-OR of virCommandPassFDFlags + * + * Transfer the specified file descriptor to the child, instead + * of closing it on exec. @fd must not be one of the three + * standard streams. + * + * If the flag VIR_COMMAND_PASS_FD_CLOSE_PARENT is set then fd will + * be closed in the parent no later than Run/RunAsync/Free. The parent + * should cease using the @fd when this call completes + */ +void +virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) +{ + virCommandPassFDIndex(cmd, fd, flags, NULL); } /* diff --git a/src/util/vircommand.h b/src/util/vircommand.h index e12c88bcc3..0ea6c8229f 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -57,6 +57,11 @@ void virCommandPassFD(virCommandPtr cmd, int fd, unsigned int flags) G_GNUC_NO_INLINE; +void virCommandPassFDIndex(virCommandPtr cmd, + int fd, + unsigned int flags, + size_t *idx) G_GNUC_NO_INLINE; + int virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd); -- 2.26.2
An alternative to qemuVirCommandGetFDSet that takes the index into the passed FD set as an argument and does not try to look it up. Use it as well ass virCommandPassFDIndex in qemuBuildChrChardevFileStr and qemuBuildInterfaceCommandLine. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 479bcc0b0c..d9c6c4bc43 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -224,6 +224,23 @@ qemuBuildMasterKeyCommandLine(virCommandPtr cmd, } +/** + * qemuBuildFDSet: + * @fd: fd to reassign to the child + * @idx: index in the fd set + * + * Format the parameters for the -add-fd command line option + * for the given file descriptor. The file descriptor must previously + * have been 'transferred' in a virCommandPassFDIndex() call, + * and @idx is the value returned by that call. + */ +static char * +qemuBuildFDSet(int fd, size_t idx) +{ + return g_strdup_printf("set=%zu,fd=%d", idx, fd); +} + + /** * qemuVirCommandGetFDSet: * @cmd: the command to modify @@ -4614,6 +4631,7 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, g_autofree char *fdpath = NULL; int flags = 0; int logfd; + size_t idx; if (appendval == VIR_TRISTATE_SWITCH_ABSENT || appendval == VIR_TRISTATE_SWITCH_OFF) @@ -4628,9 +4646,8 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, NULL, NULL)) < 0) return -1; - virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd))) - return -1; + virCommandPassFDIndex(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); + fdset = qemuBuildFDSet(logfd, idx); virCommandAddArg(cmd, "-add-fd"); virCommandAddArg(cmd, fdset); @@ -8183,11 +8200,10 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, if (vdpafd > 0) { g_autofree char *fdset = NULL; g_autofree char *addfdarg = NULL; + size_t idx; - virCommandPassFD(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); - fdset = qemuVirCommandGetFDSet(cmd, vdpafd); - if (!fdset) - goto cleanup; + virCommandPassFDIndex(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); + fdset = qemuBuildFDSet(vdpafd, idx); vdpafdName = qemuVirCommandGetDevSet(cmd, vdpafd); /* set opaque to the devicepath so that we can look up the fdset later * if necessary */ -- 2.26.2
Do not look up the index of the passed FD in places where we already have it. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_command.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d9c6c4bc43..33f9b96bf8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4628,7 +4628,6 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, { if (logManager) { g_autofree char *fdset = NULL; - g_autofree char *fdpath = NULL; int flags = 0; int logfd; size_t idx; @@ -4652,10 +4651,7 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, virCommandAddArg(cmd, "-add-fd"); virCommandAddArg(cmd, fdset); - if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd))) - return -1; - - virBufferAsprintf(buf, ",%s=%s,%s=on", filearg, fdpath, appendarg); + virBufferAsprintf(buf, ",%s=/dev/fdset/%zu,%s=on", filearg, idx, appendarg); } else { virBufferAsprintf(buf, ",%s=", filearg); virQEMUBuildBufferEscapeComma(buf, fileval); @@ -8204,7 +8200,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virCommandPassFDIndex(cmd, vdpafd, VIR_COMMAND_PASS_FD_CLOSE_PARENT, &idx); fdset = qemuBuildFDSet(vdpafd, idx); - vdpafdName = qemuVirCommandGetDevSet(cmd, vdpafd); + vdpafdName = g_strdup_printf("/dev/fdset/%zu", idx); /* set opaque to the devicepath so that we can look up the fdset later * if necessary */ addfdarg = g_strdup_printf("%s,opaque=%s", fdset, -- 2.26.2
On Tue, Nov 24, 2020 at 15:02:24 +0100, Ján Tomko wrote:
Simplify passing file descriptors via add-fd by adding a new parameter to virCommandPassFD that returns the index of the passed FD right away, removing the need to look it up later and pretend to handle errors which cannot happen - we've just put the file descriptor there, why would it not be there?
This should also silence Coverity - in that regard it's an alternative to Pavel's patch: https://www.redhat.com/archives/libvir-list/2020-November/msg00885.html
Ján Tomko (3): util: introduce virCommandPassFDIndex qemu: introduce qemuBuildFDSet qemu: use qemuVirCommandGetDevSet less
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Ján Tomko -
Peter Krempa