12:09 p.m.
Add two API calls to virCommand for generating the parameters necessary
for passing to the QEMU -add-fd option, for example "set=10,fd=20",
and the file descriptor set for the path= option, such as for example
"/dev/fdset/10"
Regards,
Stefan
---
src/libvirt_private.syms | 2 +
src/util/vircommand.c | 65 +++++++++++++++++++++++++++++++++++++++++++----
src/util/vircommand.h | 6 ++++
3 files changed, 68 insertions(+), 5 deletions(-)
Index: libvirt/src/libvirt_private.syms
===================================================================
--- libvirt.orig/src/libvirt_private.syms
+++ libvirt/src/libvirt_private.syms
@@ -165,6 +165,8 @@ virCommandSetPreExecHook;
virCommandSetWorkingDirectory;
virCommandToString;
virCommandTransferFD;
+virCommandTransferFDGetDevSet;
+virCommandTransferFDGetFDSet;
virCommandWait;
virCommandWriteArgLog;
virFork;
Index: libvirt/src/util/vircommand.c
===================================================================
--- libvirt.orig/src/util/vircommand.c
+++ libvirt/src/util/vircommand.c
@@ -109,7 +109,7 @@ struct _virCommand {
* Returns true if @set contains @fd,
* false otherwise.
*/
-static bool
+static int
virCommandFDIsSet(int fd,
const int *set,
int set_size)
@@ -118,9 +118,9 @@ virCommandFDIsSet(int fd,
while (i < set_size)
if (set[i++] == fd)
- return true;
+ return i--;
- return false;
+ return -1;
}
/*
@@ -145,7 +145,7 @@ virCommandFDSet(int fd,
if (fd < 0 || !set || !set_size)
return -1;
- if (virCommandFDIsSet(fd, *set, *set_size))
+ if (virCommandFDIsSet(fd, *set, *set_size) >= 0)
return 0;
if (VIR_REALLOC_N(*set, *set_size + 1) < 0) {
@@ -523,7 +523,7 @@ virExecWithHook(const char *const*argv,
for (i = 3; i < openmax; i++) {
if (i == infd || i == childout || i == childerr)
continue;
- if (!keepfd || !virCommandFDIsSet(i, keepfd, keepfd_size)) {
+ if (!keepfd || virCommandFDIsSet(i, keepfd, keepfd_size) < 0) {
tmpfd = i;
VIR_MASS_CLOSE(tmpfd);
} else if (virSetInherit(i, true) < 0) {
@@ -896,6 +896,61 @@ virCommandTransferFD(virCommandPtr cmd,
VIR_FORCE_CLOSE(fd);
}
+/**
+ * virCommandTransferFDGetFDSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the the QEMU -add-fd command line option
+ * for the given file descriptor. The file descriptor must previously
+ * have been 'transferred' in a virCommandTransfer() call.
+ * This function for example returns "set=10,fd=20" for file descriptor 20.
+ */
+char *
+virCommandTransferFDGetFDSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandFDIsSet(fd, cmd->transfer, cmd->transfer_size);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "set=%d,fd=%d", idx, fd) < 0) {
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+
+ return result;
+}
+
+/**
+ * virCommandTransferFDGetDevSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
+ *
+ * Get the parameters for the 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 virCommandTransfer() call.
+ */
+char *
+virCommandTransferFDGetDevSet(virCommandPtr cmd, int fd)
+{
+ char *result = NULL;
+ int idx = virCommandFDIsSet(fd, cmd->transfer, cmd->transfer_size);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "/dev/fdset/%d", idx) < 0) {
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d has not been transferred"), fd);
+ }
+ return result;
+}
+
/**
* virCommandSetPidFile:
Index: libvirt/src/util/vircommand.h
===================================================================
--- libvirt.orig/src/util/vircommand.h
+++ libvirt/src/util/vircommand.h
@@ -58,6 +58,12 @@ void virCommandPreserveFD(virCommandPtr
void virCommandTransferFD(virCommandPtr cmd,
int fd);
+char *virCommandTransferFDGetFDSet(virCommandPtr cmd,
+ int fd);
+
+char *virCommandTransferFDGetDevSet(virCommandPtr cmd,
+ int fd);
+
void virCommandSetPidFile(virCommandPtr cmd,
const char *pidfile) ATTRIBUTE_NONNULL(2);
12:29 p.m.
On Tue, Jan 22, 2013 at 01:09:50PM -0500, Stefan Berger wrote:
Add two API calls to virCommand for generating the parameters
necessary
for passing to the QEMU -add-fd option, for example "set=10,fd=20",
and the file descriptor set for the path= option, such as for example
"/dev/fdset/10"
Regards,
Stefan
---
src/libvirt_private.syms | 2 +
src/util/vircommand.c | 65 +++++++++++++++++++++++++++++++++++++++++++----
src/util/vircommand.h | 6 ++++
3 files changed, 68 insertions(+), 5 deletions(-)
This is the wrong place to be adding QEMU specific APIs - it should
be in qemu_command.{c,h}
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
1:07 p.m.
On 01/22/2013 01:29 PM, Daniel P. Berrange wrote:
On Tue, Jan 22, 2013 at 01:09:50PM -0500, Stefan Berger wrote:
> Add two API calls to virCommand for generating the parameters necessary
> for passing to the QEMU -add-fd option, for example "set=10,fd=20",
> and the file descriptor set for the path= option, such as for example
> "/dev/fdset/10"
>
> Regards,
> Stefan
>
> ---
> src/libvirt_private.syms | 2 +
> src/util/vircommand.c | 65 +++++++++++++++++++++++++++++++++++++++++++----
> src/util/vircommand.h | 6 ++++
> 3 files changed, 68 insertions(+), 5 deletions(-)
This is the wrong place to be adding QEMU specific APIs - it should
be in qemu_command.{c,h}
Assuming I move the code into qemu_command.c, are the function calls ok?
Stefan
5:35 p.m.
On 01/22/2013 12:07 PM, Stefan Berger wrote:
On 01/22/2013 01:29 PM, Daniel P. Berrange wrote:
> On Tue, Jan 22, 2013 at 01:09:50PM -0500, Stefan Berger wrote:
>> Add two API calls to virCommand for generating the parameters necessary
>> for passing to the QEMU -add-fd option, for example "set=10,fd=20",
>> and the file descriptor set for the path= option, such as for example
>> "/dev/fdset/10"
>>
>> Regards,
>> Stefan
>>
>> ---
>> src/libvirt_private.syms | 2 +
>> src/util/vircommand.c | 65
>> +++++++++++++++++++++++++++++++++++++++++++----
>> src/util/vircommand.h | 6 ++++
>> 3 files changed, 68 insertions(+), 5 deletions(-)
> This is the wrong place to be adding QEMU specific APIs - it should
> be in qemu_command.{c,h}
Assuming I move the code into qemu_command.c, are the function calls ok?
Thanks for starting on this; it is something I have also been planning
to work on.
First, we need more than just moving it into qemu_command.c; we also
need a qemu_capabilities change to add a capability to know when the
feature is available. Or even two: 'add-fd' monitor command was added
in 1.2, but -add-fd command line option wasn't until 1.3. Then again,
maybe it's just simpler to state that if you are targeting qemu 1.2,
there is nothing worth using the new add-fd QMP command for that cannot
already be done with older fallbacks, and it is only 1.3 and later where
the new fd passing becomes essential.
Then, based on the capability being present or absent, any code in
qemu_command that wants to pass an fd to qemu needs to call into a
helper routine that will either use the new -add-fd argument, fall back
to older ad-hoc processing, or error out if there is no fallback for
that particular usage.
Then we also need to use the QMP commands to update the fdsets after
qemu is started; depending on what the fd is used for, we can close the
fd once qemu has dup'd it into internal use, so that qemu doesn't hang
on to the fd that libvirt passed in forever even when it's internal use
is hot-unplugged.
We also need to figure out how to use the QMP commands during hotplug
operations, instead of the command line additions at qemu startup.
And there's still the nagging issue that even as of qemu 1.4, there is
still no way to specify block device backing chains via fds; so even if
we can pass in an fd for some use cases (like TPM), we are still waiting
for qemu to give us a way to do it in all use cases (like NFS security
for qcow2 backing chains across snapshots, block copy, and block
commit). But one thing at a time.
How much of this work are you planning on attempting, and how much do
you need me to do?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:44 p.m.
On 01/25/2013 06:35 PM, Eric Blake wrote:
On 01/22/2013 12:07 PM, Stefan Berger wrote:
> On 01/22/2013 01:29 PM, Daniel P. Berrange wrote:
>> On Tue, Jan 22, 2013 at 01:09:50PM -0500, Stefan Berger wrote:
>> Thanks for starting on this; it is something I have also been
>> planning to work on.
Glad to be able to help. When doing this my main concern was getting my
stack of TPM patches for libvirt going again to be able to test, so the
approach may have been a lot more 'naive' than your goal of supporting
the new functionality as a replacement for all existing file
descriptors. Considering that TPM would appear after -add-fd appeared,
things are a bit easier...
First, we need more than just moving it into qemu_command.c; we also
I did this in the meantime.
need a qemu_capabilities change to add a capability to know when the
feature is available. Or even two: 'add-fd' monitor command was added
A new capability for the command line support should be fairly easy.
in 1.2, but -add-fd command line option wasn't until 1.3. Then
again,
maybe it's just simpler to state that if you are targeting qemu 1.2,
there is nothing worth using the new add-fd QMP command for that cannot
already be done with older fallbacks, and it is only 1.3 and later where
the new fd passing becomes essential.
Then, based on the capability being present or absent, any code in
qemu_command that wants to pass an fd to qemu needs to call into a
helper routine that will either use the new -add-fd argument, fall back
to older ad-hoc processing, or error out if there is no fallback for
that particular usage.
Let's take this command line parameter here as an example.
tap,fd=21,id=hostnet1,vhost=on,vhostfd=23
We could write a function where we pass "fd" and the '21' as parameters
and it creates "fd=21", which we then append. We could do this as one
patch with all of them, so also with "vhostfd" and '23'. Not many
changes here, just one more VIR_FREE() call needed for each file
descriptor string.
In a subsequent patch we now test inside that function whether the new
command line parameter is available using the capability (from another
patch) and create fd=/dev/set/10 and vhostfd=/dev/set/20 respectively
*and* already add "-add-fd set=10,fd=21" and "-add-fd set=20,fd=23" to
virCommand using the functions I already posted (as the lowest layer).
Would it be that simple, or do we need to add more parameters to -add-fd
set=10,fd=21,xyz under certain circumstances?
Then we also need to use the QMP commands to update the fdsets after
qemu is started; depending on what the fd is used for, we can close the
fd once qemu has dup'd it into internal use, so that qemu doesn't hang
on to the fd that libvirt passed in forever even when it's internal use
is hot-unplugged.
Hm, isn't that already solved by the virCommandTransferFD command plus
subsequent closing of the fd post forking? I thought the -add-fd way of
passing fd's was a straight forward replacement for example for the
command line above, but maybe there's more to it... I quickly checked
the qemu_command.c sources and it looks like all fd's used (searched by
'fd=') are virCommandTransferFD'ed except for 'ioeventfd=%s' which
looks
like it's Type-to-string conversion.
We also need to figure out how to use the QMP commands during hotplug
operations, instead of the command line additions at qemu startup.
And there's still the nagging issue that even as of qemu 1.4, there is
still no way to specify block device backing chains via fds; so even if
we can pass in an fd for some use cases (like TPM), we are still waiting
for qemu to give us a way to do it in all use cases (like NFS security
for qcow2 backing chains across snapshots, block copy, and block
commit). But one thing at a time.
How much of this work are you planning on attempting, and how much do
you need me to do?
Initially my plan was to only support TPM (which doesn't hotplug for
example) ... :-/
Stefan
10:17 p.m.
On 01/26/2013 08:44 PM, Stefan Berger wrote:
On 01/25/2013 06:35 PM, Eric Blake wrote:
> On 01/22/2013 12:07 PM, Stefan Berger wrote:
>> On 01/22/2013 01:29 PM, Daniel P. Berrange wrote:
>>> On Tue, Jan 22, 2013 at 01:09:50PM -0500, Stefan Berger wrote:
>>> Thanks for starting on this; it is something I have also been
>>> planning to work on.
Glad to be able to help. When doing this my main concern was getting
my stack of TPM patches for libvirt going again to be able to test, so
the approach may have been a lot more 'naive' than your goal of
supporting the new functionality as a replacement for all existing
file descriptors. Considering that TPM would appear after -add-fd
appeared, things are a bit easier...
>
> First, we need more than just moving it into qemu_command.c; we also
I did this in the meantime.
> need a qemu_capabilities change to add a capability to know when the
> feature is available. Or even two: 'add-fd' monitor command was added
A new capability for the command line support should be fairly easy.
> in 1.2, but -add-fd command line option wasn't until 1.3. Then again,
> maybe it's just simpler to state that if you are targeting qemu 1.2,
> there is nothing worth using the new add-fd QMP command for that cannot
> already be done with older fallbacks, and it is only 1.3 and later where
> the new fd passing becomes essential.
>
> Then, based on the capability being present or absent, any code in
> qemu_command that wants to pass an fd to qemu needs to call into a
> helper routine that will either use the new -add-fd argument, fall back
> to older ad-hoc processing, or error out if there is no fallback for
> that particular usage.
Let's take this command line parameter here as an example.
tap,fd=21,id=hostnet1,vhost=on,vhostfd=23
We could write a function where we pass "fd" and the '21' as
parameters and it creates "fd=21", which we then append. We could do
this as one patch with all of them, so also with "vhostfd" and '23'.
Not many changes here, just one more VIR_FREE() call needed for each
file descriptor string.
In a subsequent patch we now test inside that function whether the new
command line parameter is available using the capability (from another
patch) and create fd=/dev/set/10 and vhostfd=/dev/set/20 respectively
*and* already add "-add-fd set=10,fd=21" and "-add-fd set=20,fd=23"
to
virCommand using the functions I already posted (as the lowest layer).
Would it be that simple, or do we need to add more parameters to
-add-fd set=10,fd=21,xyz under certain circumstances?
Ok, so likely this was a bad example and fd= does not need to be
replaced with the fd sets. There are five occurrences of path= in the
code, which could be handled similarly as described above with an
additional open() and virCommandTransferFD(). My guess is that you want
to eliminate the syscall open() usage within QEMU. Is that right ?
Regards,
Stefan
3:54 p.m.
On 01/26/2013 09:17 PM, Stefan Berger wrote:
> In a subsequent patch we now test inside that function whether
the new
> command line parameter is available using the capability (from another
> patch) and create fd=/dev/set/10 and vhostfd=/dev/set/20 respectively
> *and* already add "-add-fd set=10,fd=21" and "-add-fd
set=20,fd=23" to
> virCommand using the functions I already posted (as the lowest layer).
>
> Would it be that simple, or do we need to add more parameters to
> -add-fd set=10,fd=21,xyz under certain circumstances?
Ideally, we'd also want to use the opaque parameter of each -add-fd
call, so that when we later use QMP commands to query what fdsets exist,
the opaque parameter can tell us what filenames we passed in (it makes
bookkeeping easier if you know that fd 5 was tied to /dev/tpm, compared
to just knowing that it is an open fd but not what it came from).
Then, if I remember right, qemu's add-fd commands are set up so that
every time code in qemu calls qemu_open(), it dup's the original fd; so
even if we use -add-fd on the command line, qemu doesn't directly read()
and write() the fd we passed, but its own copy. Depending on the type
of fd (such as passing the write end of a pipe, where libvirt keeps the
read end, but the read end won't see an EOF until _all_ copies of the
write end have been closed), that means we need to tell qemu to close
the original once all copies are in place. So even if we don't need to
support hotplug right away, we _do_ need to support at least the
remove-fd QMP command after 'qemu -S -add-fd...' has exec'd, but before
issuing the 'cont' QMP command to run the guest.
Ok, so likely this was a bad example and fd= does not need to be
replaced with the fd sets. There are five occurrences of path= in the
code, which could be handled similarly as described above with an
additional open() and virCommandTransferFD(). My guess is that you want
to eliminate the syscall open() usage within QEMU. Is that right ?
Yes, at least one end goal of using -add-fd on the command line (and
'add-fd' over the monitor during hotplug) is that we can use SELinux to
prevent qemu from calling any open(). This in turn means that qemu can
use _only_ file descriptors that have been handed in by the management
app, and are therefore securely labeled. This comes into play when
using NFS: there is no way to apply SELinux labels to NFS files (well,
NFS 4 might eventually have a solution, but it's not there yet; and lots
of people are still on NFS 3). Therefore, if you use NFS storage (which
is surprisingly common when setting up shared storage for migration
purposes), you have to enable the 'virt_use_nfs' SELinux bool, but this
means that SELinux no longer prevents qemu from opening _any_ file
residing on NFS, and thus a compromised guest could read or even write
disk images associated with unrelated guests. But if qemu is forced to
use only the fds that it inherits from management, then we won't need
the virt_use_nfs sledgehammer, or its loss of security when enabled.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:23 a.m.
On Mon, Jan 28, 2013 at 02:54:28PM -0700, Eric Blake wrote:
On 01/26/2013 09:17 PM, Stefan Berger wrote:
>> In a subsequent patch we now test inside that function whether the new
>> command line parameter is available using the capability (from another
>> patch) and create fd=/dev/set/10 and vhostfd=/dev/set/20 respectively
>> *and* already add "-add-fd set=10,fd=21" and "-add-fd
set=20,fd=23" to
>> virCommand using the functions I already posted (as the lowest layer).
>>
>> Would it be that simple, or do we need to add more parameters to
>> -add-fd set=10,fd=21,xyz under certain circumstances?
Ideally, we'd also want to use the opaque parameter of each -add-fd
call, so that when we later use QMP commands to query what fdsets exist,
the opaque parameter can tell us what filenames we passed in (it makes
bookkeeping easier if you know that fd 5 was tied to /dev/tpm, compared
to just knowing that it is an open fd but not what it came from).
That implies that you trust the output from QEMU monitor commands
to be telling you the truth. We can't do that, so IMHO we can't
rely on the 'opaque' parameter.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
11:30 a.m.
On 01/29/2013 03:23 AM, Daniel P. Berrange wrote:
On Mon, Jan 28, 2013 at 02:54:28PM -0700, Eric Blake wrote:
> On 01/26/2013 09:17 PM, Stefan Berger wrote:
>>> In a subsequent patch we now test inside that function whether the new
>>> command line parameter is available using the capability (from another
>>> patch) and create fd=/dev/set/10 and vhostfd=/dev/set/20 respectively
>>> *and* already add "-add-fd set=10,fd=21" and "-add-fd
set=20,fd=23" to
>>> virCommand using the functions I already posted (as the lowest layer).
>>>
>>> Would it be that simple, or do we need to add more parameters to
>>> -add-fd set=10,fd=21,xyz under certain circumstances?
>
> Ideally, we'd also want to use the opaque parameter of each -add-fd
> call, so that when we later use QMP commands to query what fdsets exist,
> the opaque parameter can tell us what filenames we passed in (it makes
> bookkeeping easier if you know that fd 5 was tied to /dev/tpm, compared
> to just knowing that it is an open fd but not what it came from).
That implies that you trust the output from QEMU monitor commands
to be telling you the truth. We can't do that, so IMHO we can't
rely on the 'opaque' parameter.
Fair enough - libvirt will have to track in vm->priv anything that it
needs for reconstructing state, even across libvirtd restarts. Still,
we _ought_ to populate the opaque member with useful information, so
that debugging is easier (that is, while libvirt can't rely on
query-fdsets, a developer using 'virsh qemu-monitor-command' on a
trusted guest certainly should be able to).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4318
days inactive
4325
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger