5:50 a.m.
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(a)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
5:50 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: Move TPM command line build code into own function
Move the TPM command line build code into its own function.
Signed-off-by: Stefan Berger <stefanb(a)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
7:36 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: Move TPM command line build code into own function
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:50 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Pass file descriptor when using TPM passthrough
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(a)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
8:08 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Pass file descriptor when using TPM passthrough
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(a)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.
8:15 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Pass file descriptor when using TPM passthrough
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(a)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.
11:57 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: Pass file descriptor when using TPM passthrough
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(a)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
7:34 a.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:24 a.m.
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(a)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
10:52 p.m.
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(a)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
7:03 a.m.
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
7:58 a.m.
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
5:08 p.m.
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(a)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
3559
days inactive
3561
days old
12 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Stefan Berger