[libvirt] [PATCH 1/3] utils: Implement virCommandPassFDGetFDIndex
Implement virCommandPassFDGetFDIndex to determine the index a given file descriptor will have when passed to the child process. When this function is called, a flag is set to prevent the reordering of the file descriptors. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 3 +++ 3 files changed, 37 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..aadb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1213,6 +1213,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassFDGetFDIndex; virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6527d85..2616446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -67,6 +67,7 @@ enum { VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), VIR_EXEC_LISTEN_FDS = (1 << 5), + VIR_EXEC_FIXED_FDS = (1 << 6), }; typedef struct _virCommandFD virCommandFD; @@ -214,6 +215,12 @@ virCommandReorderFDs(virCommandPtr cmd) if (!cmd || cmd->has_error || !cmd->npassfd) return; + if ((cmd->flags & VIR_EXEC_FIXED_FDS)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The fds are fixed and cannot be reordered")); + goto error; + } + for (i = 0; i < cmd->npassfd; i++) maxfd = MAX(cmd->passfd[i].fd, maxfd); @@ -1019,6 +1026,32 @@ virCommandPassListenFDs(virCommandPtr cmd) cmd->flags |= VIR_EXEC_LISTEN_FDS; } +/* + * virCommandPassFDGetFDIndex: + * @cmd: pointer to virCommand + * @fd: FD to get index of + * + * Determine the index of the FD in the transfer set. + * + * Returns index >= 0 if @set contains @fd, + * -1 otherwise. + */ +int +virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) +{ + size_t i = 0; + + while (i < cmd->npassfd) { + if (cmd->passfd[i].fd == fd) { + cmd->flags |= VIR_EXEC_FIXED_FDS; + return i; + } + i++; + } + + return -1; +} + /** * virCommandSetPidFile: * @cmd: the command to modify diff --git a/src/util/vircommand.h b/src/util/vircommand.h index bf65de4..198da2f 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -62,6 +62,9 @@ void virCommandPassFD(virCommandPtr cmd, void virCommandPassListenFDs(virCommandPtr cmd); +int virCommandPassFDGetFDIndex(virCommandPtr cmd, + int fd); + void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); -- 1.9.3
Move the TPM command line build code into its own function. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7853125..539c956 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8148,6 +8148,30 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, return ret; } +static int +qemuBuildTPMCommandLine(virDomainDefPtr def, + virCommandPtr cmd, + virQEMUCapsPtr qemuCaps, + const char *emulator) +{ + char *optstr; + + if (!(optstr = qemuBuildTPMBackendStr(def, qemuCaps, emulator))) + return -1; + + virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); + VIR_FREE(optstr); + + if (!(optstr = qemuBuildTPMDevStr(def, qemuCaps, emulator))) + return -1; + + virCommandAddArgList(cmd, "-device", optstr, NULL); + VIR_FREE(optstr); + + return 0; +} + + qemuBuildCommandLineCallbacks buildCommandLineCallbacks = { .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName, }; @@ -9579,19 +9603,8 @@ qemuBuildCommandLine(virConnectPtr conn, } if (def->tpm) { - char *optstr; - - if (!(optstr = qemuBuildTPMBackendStr(def, qemuCaps, emulator))) + if (qemuBuildTPMCommandLine(def, cmd, qemuCaps, emulator) < 0) goto error; - - virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); - VIR_FREE(optstr); - - if (!(optstr = qemuBuildTPMDevStr(def, qemuCaps, emulator))) - goto error; - - virCommandAddArgList(cmd, "-device", optstr, NULL); - VIR_FREE(optstr); } for (i = 0; i < def->ninputs; i++) { -- 1.9.3
On Mon, Feb 23, 2015 at 06:50:47AM -0500, Stefan Berger wrote:
Move the TPM command line build code into its own function.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
ACK
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7853125..539c956 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8148,6 +8148,30 @@ qemuBuildDomainLoaderCommandLine(virCommandPtr cmd, return ret; }
+static int +qemuBuildTPMCommandLine(virDomainDefPtr def, + virCommandPtr cmd, + virQEMUCapsPtr qemuCaps, + const char *emulator) +{ + char *optstr; + + if (!(optstr = qemuBuildTPMBackendStr(def, qemuCaps, emulator))) + return -1; + + virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); + VIR_FREE(optstr); + + if (!(optstr = qemuBuildTPMDevStr(def, qemuCaps, emulator))) + return -1; + + virCommandAddArgList(cmd, "-device", optstr, NULL); + VIR_FREE(optstr); + + return 0; +} + + qemuBuildCommandLineCallbacks buildCommandLineCallbacks = { .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName, }; @@ -9579,19 +9603,8 @@ qemuBuildCommandLine(virConnectPtr conn, }
if (def->tpm) { - char *optstr; - - if (!(optstr = qemuBuildTPMBackendStr(def, qemuCaps, emulator))) + if (qemuBuildTPMCommandLine(def, cmd, qemuCaps, emulator) < 0) goto error; - - virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); - VIR_FREE(optstr); - - if (!(optstr = qemuBuildTPMDevStr(def, qemuCaps, emulator))) - goto error; - - virCommandAddArgList(cmd, "-device", optstr, NULL); - VIR_FREE(optstr); }
for (i = 0; i < def->ninputs; i++) { -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Pass the TPM file descriptor to QEMU via command line. Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional parameters -add-fd set=10,fd=20. This addresses the use case when QEMU is started with non-root privileges and QEMU cannot open /dev/tpm0 for example. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 539c956..12e2e2f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -161,6 +161,58 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave"); /** + * qemuVirCommandGetFDSet: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * + * Get the parameters for the QEMU -add-fd command line option + * for the given file descriptor. The file descriptor must previously + * have been 'transferred' in a virCommandPassFD() call. + * This function for example returns "set=10,fd=20". + */ +static char * +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd) +{ + char *result = NULL; + int idx = virCommandPassFDGetFDIndex(cmd, fd); + + if (idx >= 0) { + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("file descriptor %d has not been transferred"), fd); + } + + return result; +} + +/** + * qemuVirCommandGetDevSet: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * + * Get the parameters for the QEMU path= parameter where a file + * descriptor is accessed via a file descriptor set, for example + * /dev/fdset/10. The file descriptor must previously have been + * 'transferred' in a virCommandPassFD() call. + */ +static char * +qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) +{ + char *result = NULL; + int idx = virCommandPassFDGetFDIndex(cmd, fd); + + if (idx >= 0) { + ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx) < 0); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("file descriptor %d has not been transferred"), fd); + } + return result; +} + + +/** * qemuPhysIfaceConnect: * @def: the definition of the VM (needed by 802.1Qbh and audit) * @driver: pointer to the driver instance @@ -6320,15 +6372,20 @@ qemuBuildRNGDevStr(virDomainDefPtr def, static char *qemuBuildTPMBackendStr(const virDomainDef *def, + virCommandPtr cmd, virQEMUCapsPtr qemuCaps, - const char *emulator) + const char *emulator, + int *tpmfd, int *cancelfd) { const virDomainTPMDef *tpm = def->tpm; virBuffer buf = VIR_BUFFER_INITIALIZER; const char *type = virDomainTPMBackendTypeToString(tpm->type); - char *cancel_path; + char *cancel_path = NULL, *devset = NULL; const char *tpmdev; + *tpmfd = -1; + *cancelfd = -1; + virBufferAsprintf(&buf, "%s,id=tpm-%s", type, tpm->info.alias); switch (tpm->type) { @@ -6340,11 +6397,42 @@ static char *qemuBuildTPMBackendStr(const virDomainDef *def, if (!(cancel_path = virTPMCreateCancelPath(tpmdev))) goto error; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ADD_FD)) { + *tpmfd = open(tpmdev, O_RDWR); + if (*tpmfd < 0) { + virReportSystemError(errno, _("Could not open TPM device %s"), + tpmdev); + goto error; + } + + virCommandPassFD(cmd, *tpmfd, + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + devset = qemuVirCommandGetDevSet(cmd, *tpmfd); + if (devset == NULL) + goto error; + + *cancelfd = open(cancel_path, O_WRONLY); + if (*cancelfd < 0) { + virReportSystemError(errno, + _("Could not open TPM device's cancel " + "path %s"), cancel_path); + goto error; + } + VIR_FREE(cancel_path); + + virCommandPassFD(cmd, *cancelfd, + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + cancel_path = qemuVirCommandGetDevSet(cmd, *cancelfd); + if (cancel_path == NULL) + goto error; + } virBufferAddLit(&buf, ",path="); - virBufferEscape(&buf, ',', ",", "%s", tpmdev); + virBufferEscape(&buf, ',', ",", "%s", devset ? devset : tpmdev); virBufferAddLit(&buf, ",cancel-path="); virBufferEscape(&buf, ',', ",", "%s", cancel_path); + + VIR_FREE(devset); VIR_FREE(cancel_path); break; @@ -6364,6 +6452,9 @@ static char *qemuBuildTPMBackendStr(const virDomainDef *def, emulator, type); error: + VIR_FREE(devset); + VIR_FREE(cancel_path); + virBufferFreeAndReset(&buf); return NULL; } @@ -8155,13 +8246,35 @@ qemuBuildTPMCommandLine(virDomainDefPtr def, const char *emulator) { char *optstr; + int tpmfd = -1; + int cancelfd = -1; + char *fdset; - if (!(optstr = qemuBuildTPMBackendStr(def, qemuCaps, emulator))) + if (!(optstr = qemuBuildTPMBackendStr(def, cmd, qemuCaps, emulator, + &tpmfd, &cancelfd))) return -1; virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); VIR_FREE(optstr); + if (tpmfd >= 0) { + fdset = qemuVirCommandGetFDSet(cmd, tpmfd); + if (!fdset) + return -1; + + virCommandAddArgList(cmd, "-add-fd", fdset, NULL); + VIR_FREE(fdset); + } + + if (cancelfd >= 0) { + fdset = qemuVirCommandGetFDSet(cmd, cancelfd); + if (!fdset) + return -1; + + virCommandAddArgList(cmd, "-add-fd", fdset, NULL); + VIR_FREE(fdset); + } + if (!(optstr = qemuBuildTPMDevStr(def, qemuCaps, emulator))) return -1; -- 1.9.3
On Mon, Feb 23, 2015 at 06:50:48AM -0500, Stefan Berger wrote:
Pass the TPM file descriptor to QEMU via command line. Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional parameters -add-fd set=10,fd=20.
This addresses the use case when QEMU is started with non-root privileges and QEMU cannot open /dev/tpm0 for example.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 539c956..12e2e2f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -161,6 +161,58 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave");
/** + * qemuVirCommandGetFDSet: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * + * Get the parameters for the QEMU -add-fd command line option + * for the given file descriptor. The file descriptor must previously + * have been 'transferred' in a virCommandPassFD() call. + * This function for example returns "set=10,fd=20". + */ +static char * +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd) +{ + char *result = NULL; + int idx = virCommandPassFDGetFDIndex(cmd, fd); + + if (idx >= 0) { + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0); ^ This line doesn't make much sense, I guess you just wanted to do it | without the comparison to zero here ---------------------------------+
Anyway, is there a reason for passing "set=X,fd=Y" instead of just doing fd=Y and being done with it? I must admit I don't know the details of /dev/fdset, so that might've just missed me.
On 02/24/2015 09:08 AM, Martin Kletzander wrote:
On Mon, Feb 23, 2015 at 06:50:48AM -0500, Stefan Berger wrote:
Pass the TPM file descriptor to QEMU via command line. Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional parameters -add-fd set=10,fd=20.
This addresses the use case when QEMU is started with non-root privileges and QEMU cannot open /dev/tpm0 for example.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 539c956..12e2e2f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -161,6 +161,58 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave");
/** + * qemuVirCommandGetFDSet: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * + * Get the parameters for the QEMU -add-fd command line option + * for the given file descriptor. The file descriptor must previously + * have been 'transferred' in a virCommandPassFD() call. + * This function for example returns "set=10,fd=20". + */ +static char * +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd) +{ + char *result = NULL; + int idx = virCommandPassFDGetFDIndex(cmd, fd); + + if (idx >= 0) { + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0); ^ This line doesn't make much sense, I guess you just wanted to do it | without the comparison to zero here ---------------------------------+
Anyway, is there a reason for passing "set=X,fd=Y" instead of just doing fd=Y and being done with it? I must admit I don't know the details of /dev/fdset, so that might've just missed me.
Passing fd by itself is not supported with this device. I guess back then when I implemented I knew I could pass the fd via this /dev/fdset and so fd= doesn't need to be implemented on top of that.
On Tue, Feb 24, 2015 at 09:15:40AM -0500, Stefan Berger wrote:
On 02/24/2015 09:08 AM, Martin Kletzander wrote:
On Mon, Feb 23, 2015 at 06:50:48AM -0500, Stefan Berger wrote:
Pass the TPM file descriptor to QEMU via command line. Instead of passing /dev/tpm0 we now pass /dev/fdset/10 and the additional parameters -add-fd set=10,fd=20.
This addresses the use case when QEMU is started with non-root privileges and QEMU cannot open /dev/tpm0 for example.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 539c956..12e2e2f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -161,6 +161,58 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave");
/** + * qemuVirCommandGetFDSet: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * + * Get the parameters for the QEMU -add-fd command line option + * for the given file descriptor. The file descriptor must previously + * have been 'transferred' in a virCommandPassFD() call. + * This function for example returns "set=10,fd=20". + */ +static char * +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd) +{ + char *result = NULL; + int idx = virCommandPassFDGetFDIndex(cmd, fd); + + if (idx >= 0) { + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0); ^ This line doesn't make much sense, I guess you just wanted to do it | without the comparison to zero here ---------------------------------+
Anyway, is there a reason for passing "set=X,fd=Y" instead of just doing fd=Y and being done with it? I must admit I don't know the details of /dev/fdset, so that might've just missed me.
Passing fd by itself is not supported with this device. I guess back then when I implemented I knew I could pass the fd via this /dev/fdset and so fd= doesn't need to be implemented on top of that.
OK, I didn't know that and that's a valid reason. One more thing about this patch though. I just noticed that this is not the first version and there were some previous ones before, so I went and skimmed some of the responses and one is still not solved/decided. Why would we number the set according to our array index instead of just the first FD in that set (for example)? The thing I'm worried about is that when we're passing FDs later on, we might do that and those numbers might clash mixing different FDs in the same set. Having said that I also looked at the code and even though we know how to pass FDs to qemu later on with QMP and add-fd, we do that only when probing for that capability; so it's not decided how we're going to name the sets yet, I guess. That also shows when looking for a local "database" of fdsets per each QEMU that's running => there's none; if we want to use that info after some time (virCommand is cleaned, libvirtd restarted, etc.), then there is no way of telling which ones were used for what. The only place where we send FDs is without FD sets and the FD number is reflected in an alias (e.g. vhost35 or something like that). Anyway, that shouldn't be your concern at all, I just think it would be nice to have fd=x,set=x and then assign an alias to the live domain for the tpm device with that x used. Looking forward to v2, Martin
On Mon, Feb 23, 2015 at 06:50:46AM -0500, Stefan Berger wrote:
Implement virCommandPassFDGetFDIndex to determine the index a given file descriptor will have when passed to the child process. When this function is called, a flag is set to prevent the reordering of the file descriptors.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 3 +++ 3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..aadb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1213,6 +1213,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassFDGetFDIndex; virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6527d85..2616446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -67,6 +67,7 @@ enum { VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), VIR_EXEC_LISTEN_FDS = (1 << 5), + VIR_EXEC_FIXED_FDS = (1 << 6),
The descriptors shouldn't get reordered *unless* there is VIR_EXEC_LISTEN_FDS flag added, so FIXED_FDS shouldn't be necessary. Or is there a bug in that approach that needs fixing?
};
typedef struct _virCommandFD virCommandFD; @@ -214,6 +215,12 @@ virCommandReorderFDs(virCommandPtr cmd) if (!cmd || cmd->has_error || !cmd->npassfd) return;
+ if ((cmd->flags & VIR_EXEC_FIXED_FDS)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The fds are fixed and cannot be reordered")); + goto error; + } + for (i = 0; i < cmd->npassfd; i++) maxfd = MAX(cmd->passfd[i].fd, maxfd);
@@ -1019,6 +1026,32 @@ virCommandPassListenFDs(virCommandPtr cmd) cmd->flags |= VIR_EXEC_LISTEN_FDS; }
+/* + * virCommandPassFDGetFDIndex: + * @cmd: pointer to virCommand + * @fd: FD to get index of + * + * Determine the index of the FD in the transfer set. + * + * Returns index >= 0 if @set contains @fd, + * -1 otherwise. + */ +int +virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd) +{ + size_t i = 0; + + while (i < cmd->npassfd) { + if (cmd->passfd[i].fd == fd) { + cmd->flags |= VIR_EXEC_FIXED_FDS; + return i; + } + i++; + } + + return -1; +} + /** * virCommandSetPidFile: * @cmd: the command to modify diff --git a/src/util/vircommand.h b/src/util/vircommand.h index bf65de4..198da2f 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -62,6 +62,9 @@ void virCommandPassFD(virCommandPtr cmd,
void virCommandPassListenFDs(virCommandPtr cmd);
+int virCommandPassFDGetFDIndex(virCommandPtr cmd, + int fd); + void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2);
-- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/24/2015 08:34 AM, Martin Kletzander wrote:
On Mon, Feb 23, 2015 at 06:50:46AM -0500, Stefan Berger wrote:
Implement virCommandPassFDGetFDIndex to determine the index a given file descriptor will have when passed to the child process. When this function is called, a flag is set to prevent the reordering of the file descriptors.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 3 +++ 3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..aadb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1213,6 +1213,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassFDGetFDIndex; virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6527d85..2616446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -67,6 +67,7 @@ enum { VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), VIR_EXEC_LISTEN_FDS = (1 << 5), + VIR_EXEC_FIXED_FDS = (1 << 6),
The descriptors shouldn't get reordered *unless* there is VIR_EXEC_LISTEN_FDS flag added, so FIXED_FDS shouldn't be necessary. Or is there a bug in that approach that needs fixing?
Hm, it depends on the order in which APIs are called where we either need to protect against re-ordering (if we use indices) or we cannot allow indices to be used if we need to reorder them. So, actually the way the patch is, it isn't correct, either. The problem of having to prevent the reordering of file descriptors when we are building the QEMU command line with file descriptors already exists today. So maybe I could just remove this flag and the related check knowing that reordering of file descriptors and building the QEMU command line that contains file descriptor (from before the order) would not be allowed today without any of the patches I am trying to add. Stefan
On Tue, Feb 24, 2015 at 09:24:51AM -0500, Stefan Berger wrote:
On 02/24/2015 08:34 AM, Martin Kletzander wrote:
On Mon, Feb 23, 2015 at 06:50:46AM -0500, Stefan Berger wrote:
Implement virCommandPassFDGetFDIndex to determine the index a given file descriptor will have when passed to the child process. When this function is called, a flag is set to prevent the reordering of the file descriptors.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 3 +++ 3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..aadb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1213,6 +1213,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassFDGetFDIndex; virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6527d85..2616446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -67,6 +67,7 @@ enum { VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), VIR_EXEC_LISTEN_FDS = (1 << 5), + VIR_EXEC_FIXED_FDS = (1 << 6),
The descriptors shouldn't get reordered *unless* there is VIR_EXEC_LISTEN_FDS flag added, so FIXED_FDS shouldn't be necessary. Or is there a bug in that approach that needs fixing?
Hm, it depends on the order in which APIs are called where we either need to protect against re-ordering (if we use indices) or we cannot allow indices to be used if we need to reorder them. So, actually the way the patch is, it isn't correct, either.
The problem of having to prevent the reordering of file descriptors when we are building the QEMU command line with file descriptors already exists today. So maybe I could just remove this flag and the related check knowing that reordering of file descriptors and building the QEMU command line that contains file descriptor (from before the order) would not be allowed today without any of the patches I am trying to add.
You surely and safely can. The point behind reordering FDs was that when you're using FD passing as done by systemd, those FDs passed must start from number 3. However, because we are already passing a bunch of file descriptors to QEMU and other commands, we *must not* reorder any file descriptors because it wouldn't work, of course. Martin
On 02/24/2015 09:52 PM, Martin Kletzander wrote:
The problem of having to prevent the reordering of file descriptors when we are building the QEMU command line with file descriptors already exists today. So maybe I could just remove this flag and the related check knowing that reordering of file descriptors and building the QEMU command line that contains file descriptor (from before the order) would not be allowed today without any of the patches I am trying to add.
You surely and safely can. The point behind reordering FDs was that when you're using FD passing as done by systemd, those FDs passed must start from number 3. However, because we are already passing a bunch of file descriptors to QEMU and other commands, we *must not* reorder any file descriptors because it wouldn't work, of course.
Or we fix the code to ALWAYS reorder file descriptors, including updating the qemu code to use the reordered numbers instead of assuming unchanged passthrough. I'm still in the middle of an audit in my local code base of what that would entail. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Feb 25, 2015 at 06:03:40AM -0700, Eric Blake wrote:
On 02/24/2015 09:52 PM, Martin Kletzander wrote:
The problem of having to prevent the reordering of file descriptors when we are building the QEMU command line with file descriptors already exists today. So maybe I could just remove this flag and the related check knowing that reordering of file descriptors and building the QEMU command line that contains file descriptor (from before the order) would not be allowed today without any of the patches I am trying to add.
You surely and safely can. The point behind reordering FDs was that when you're using FD passing as done by systemd, those FDs passed must start from number 3. However, because we are already passing a bunch of file descriptors to QEMU and other commands, we *must not* reorder any file descriptors because it wouldn't work, of course.
Or we fix the code to ALWAYS reorder file descriptors, including updating the qemu code to use the reordered numbers instead of assuming unchanged passthrough. I'm still in the middle of an audit in my local code base of what that would entail.
I'd be really afraid of that. Not because I wrote the reorder code but mainly because the added one would just add a lot of renumbering that seems pointless to me. Also the reordering is just a hack to make LISTEN_FDS work and I see no other usable case for it.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/24/2015 11:52 PM, Martin Kletzander wrote:
On Tue, Feb 24, 2015 at 09:24:51AM -0500, Stefan Berger wrote:
On 02/24/2015 08:34 AM, Martin Kletzander wrote:
On Mon, Feb 23, 2015 at 06:50:46AM -0500, Stefan Berger wrote:
Implement virCommandPassFDGetFDIndex to determine the index a given file descriptor will have when passed to the child process. When this function is called, a flag is set to prevent the reordering of the file descriptors.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/util/vircommand.c | 33 +++++++++++++++++++++++++++++++++ src/util/vircommand.h | 3 +++ 3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..aadb833 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1213,6 +1213,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandPassFDGetFDIndex; virCommandPassListenFDs; virCommandRawStatus; virCommandRequireHandshake; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6527d85..2616446 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -67,6 +67,7 @@ enum { VIR_EXEC_RUN_SYNC = (1 << 3), VIR_EXEC_ASYNC_IO = (1 << 4), VIR_EXEC_LISTEN_FDS = (1 << 5), + VIR_EXEC_FIXED_FDS = (1 << 6),
The descriptors shouldn't get reordered *unless* there is VIR_EXEC_LISTEN_FDS flag added, so FIXED_FDS shouldn't be necessary. Or is there a bug in that approach that needs fixing?
Hm, it depends on the order in which APIs are called where we either need to protect against re-ordering (if we use indices) or we cannot allow indices to be used if we need to reorder them. So, actually the way the patch is, it isn't correct, either.
The problem of having to prevent the reordering of file descriptors when we are building the QEMU command line with file descriptors already exists today. So maybe I could just remove this flag and the related check knowing that reordering of file descriptors and building the QEMU command line that contains file descriptor (from before the order) would not be allowed today without any of the patches I am trying to add.
You surely and safely can. The point behind reordering FDs was that when you're using FD passing as done by systemd, those FDs passed must start from number 3. However, because we are already passing a bunch of file descriptors to QEMU and other commands, we *must not* reorder any file descriptors because it wouldn't work, of course.
Follow this I would no remove that flag and all checks around it and leave only the new function. Stefan
Martin
participants (3)
-
Eric Blake -
Martin Kletzander -
Stefan Berger