10:52 a.m.
The following patch series adds initial support for QEMU file
descriptor sets implementing support for creating the proper
command line.
What's missing is currently the monitor support removing the sets
once QEMU has started. So these patches are posted for RFC purposes...
Regards,
Stefan
10:52 a.m.
New subject: [libvirt] [PATCH V1 1/4] Add support for QEMU -add-fd support detection
Add support for QEMU -add-fd command line parameter detection
---
src/qemu/qemu_capabilities.c | 3 +++
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 4 insertions(+)
Index: libvirt/src/qemu/qemu_capabilities.c
===================================================================
--- libvirt.orig/src/qemu/qemu_capabilities.c
+++ libvirt/src/qemu/qemu_capabilities.c
@@ -203,6 +203,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
"usb-serial", /* 125 */
"usb-net",
+ "add-fd"
);
@@ -1064,6 +1065,8 @@ qemuCapsComputeCmdFlags(const char *help
qemuCapsSet(caps, QEMU_CAPS_SMBIOS_TYPE);
if (strstr(help, "-sandbox"))
qemuCapsSet(caps, QEMU_CAPS_SECCOMP_SANDBOX);
+ if (strstr(help, "-add-fd"))
+ qemuCapsSet(caps, QEMU_CAPS_ADD_FD);
if ((netdev = strstr(help, "-netdev"))) {
/* Disable -netdev on 0.12 since although it exists,
Index: libvirt/src/qemu/qemu_capabilities.h
===================================================================
--- libvirt.orig/src/qemu/qemu_capabilities.h
+++ libvirt/src/qemu/qemu_capabilities.h
@@ -165,6 +165,7 @@ enum qemuCapsFlags {
QEMU_CAPS_SCLP_S390 = 124, /* -device sclp* */
QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
+ QEMU_CAPS_ADD_FD = 127, /* -add-fd */
QEMU_CAPS_LAST, /* this must always be the last item */
};
5:24 p.m.
New subject: [libvirt] [PATCH V1 1/4] Add support for QEMU -add-fd support detection
On 01/29/2013 09:52 AM, Stefan Berger wrote:
Add support for QEMU -add-fd command line parameter detection
---
src/qemu/qemu_capabilities.c | 3 +++
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 4 insertions(+)
Index: libvirt/src/qemu/qemu_capabilities.c
===================================================================
--- libvirt.orig/src/qemu/qemu_capabilities.c
+++ libvirt/src/qemu/qemu_capabilities.c
@@ -203,6 +203,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
"usb-serial", /* 125 */
"usb-net",
+ "add-fd"
Needs a trailing comma.
);
@@ -1064,6 +1065,8 @@ qemuCapsComputeCmdFlags(const char *help
qemuCapsSet(caps, QEMU_CAPS_SMBIOS_TYPE);
if (strstr(help, "-sandbox"))
qemuCapsSet(caps, QEMU_CAPS_SECCOMP_SANDBOX);
+ if (strstr(help, "-add-fd"))
+ qemuCapsSet(caps, QEMU_CAPS_ADD_FD);
Won't work the way you want. -add-fd wasn't supported until qemu 1.3,
but for qemu 1.2 and beyond, we only use QMP probing, not -help
scraping. So we really need to determine which QMP command we can issue
to determine whether -add-fd is available on the command line; but since
Anthony hasn't yet implemented 'query-config-schema'[1] (and even if he
does, it probably won't be in time for qemu 1.3), we'll have to settle
on version checks for now :(
[1]https://www.redhat.com/archives/libvir-list/2013-January/msg01600.html
Index: libvirt/src/qemu/qemu_capabilities.h
===================================================================
--- libvirt.orig/src/qemu/qemu_capabilities.h
+++ libvirt/src/qemu/qemu_capabilities.h
@@ -165,6 +165,7 @@ enum qemuCapsFlags {
QEMU_CAPS_SCLP_S390 = 124, /* -device sclp* */
QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
+ QEMU_CAPS_ADD_FD = 127, /* -add-fd */
Okay, so you are taking the approach that we insist on command line
support (qemu 1.3), not just the QMP command existing (qemu 1.2). It's
easy to probe whether 'add-fd' QMP exists (query-command), but harder to
determine if command line support is present when using just QMP with
the current state of qemu.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:16 p.m.
New subject: [libvirt] [PATCH V1 1/4] Add support for QEMU -add-fd support detection
On 01/29/2013 06:24 PM, Eric Blake wrote:
On 01/29/2013 09:52 AM, Stefan Berger wrote:
> Index: libvirt/src/qemu/qemu_capabilities.h
> ===================================================================
> --- libvirt.orig/src/qemu/qemu_capabilities.h
> +++ libvirt/src/qemu/qemu_capabilities.h
> @@ -165,6 +165,7 @@ enum qemuCapsFlags {
> QEMU_CAPS_SCLP_S390 = 124, /* -device sclp* */
> QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
> QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
> + QEMU_CAPS_ADD_FD = 127, /* -add-fd */
Okay, so you are taking the approach that we insist on command line
support (qemu 1.3), not just the QMP command existing (qemu 1.2). It's
easy to probe whether 'add-fd' QMP exists (query-command), but harder to
determine if command line support is present when using just QMP with
the current state of qemu.
Would you then not use -add-fd on the command line but hotplug those
devices instead?
10:39 a.m.
New subject: [libvirt] [PATCH V1 1/4] Add support for QEMU -add-fd support detection
On 01/29/2013 05:16 PM, Stefan Berger wrote:
On 01/29/2013 06:24 PM, Eric Blake wrote:
> On 01/29/2013 09:52 AM, Stefan Berger wrote:
>
>> Index: libvirt/src/qemu/qemu_capabilities.h
>> ===================================================================
>> --- libvirt.orig/src/qemu/qemu_capabilities.h
>> +++ libvirt/src/qemu/qemu_capabilities.h
>> @@ -165,6 +165,7 @@ enum qemuCapsFlags {
>> QEMU_CAPS_SCLP_S390 = 124, /* -device sclp* */
>> QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
>> QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
>> + QEMU_CAPS_ADD_FD = 127, /* -add-fd */
> Okay, so you are taking the approach that we insist on command line
> support (qemu 1.3), not just the QMP command existing (qemu 1.2). It's
> easy to probe whether 'add-fd' QMP exists (query-command), but harder to
> determine if command line support is present when using just QMP with
> the current state of qemu.
>
Would you then not use -add-fd on the command line but hotplug those
devices instead?
We have two options:
1. Try to support both qemu 1.2 and qemu 1.3. For qemu 1.3, we can use
the command line (which feels simpler to me), but for qemu 1.2 we would
have to do everything through QMP. This means that we'd have to write
code that does not attach _any_ -drive arguments, and instead start
'qemu -s', hotplug all the drives, and then start the domain. But if we
can get things working for 1.2 via hotplug only, then there is no need
to use the command line for 1.3 - use a single interface for it all.
This method is easy to probe - does the 'add-fd' QMP command exist in
'query-commands'?
2. Not support fd passing unless targeting qemu 1.3 or newer. When
targeting 1.2 and older, use the command line as always, and state that
qemu will use open() and the user has to use virt_use_nfs. With 1.3,
use -add-fd on the command line, so that we don't have to worry about
hot-plugging disks after 'qemu -s' on initial start; we'd still have to
wire up QMP command support for hotplug, but at least we don't have to
refactor the command line computation into delaying things to hotplug.
Since there is no 'query-config-schema' command in 1.3, we'll have to
hack in some other capability check; for this I have two ideas: a)
straight version check (if 'query-version' reports 1.3 or newer, assume
-add-fd exists); this does not backport well. b) during the initial QMP
probes, if 'add-fd' exists, try calling 'add-fd' with a specified
fdset-id argument and /dev/null as an fd; qemu 1.2 will reject the call
(in that version, qemu insisted on managing set ids itself; you could
only pass fdset-id when adding to an existing set); qemu 1.3 and later
will accept it (the management app libvirt can choose its own allocation
pattern for set ids).
I'm leaning towards 2b, so I'll code that up as part of my QMP work.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:33 a.m.
New subject: [libvirt] [PATCH V1 1/4] Add support for QEMU -add-fd support detection
On Wed, Jan 30, 2013 at 09:39:25AM -0700, Eric Blake wrote:
On 01/29/2013 05:16 PM, Stefan Berger wrote:
> On 01/29/2013 06:24 PM, Eric Blake wrote:
>> On 01/29/2013 09:52 AM, Stefan Berger wrote:
>>
>>> Index: libvirt/src/qemu/qemu_capabilities.h
>>> ===================================================================
>>> --- libvirt.orig/src/qemu/qemu_capabilities.h
>>> +++ libvirt/src/qemu/qemu_capabilities.h
>>> @@ -165,6 +165,7 @@ enum qemuCapsFlags {
>>> QEMU_CAPS_SCLP_S390 = 124, /* -device sclp* */
>>> QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */
>>> QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
>>> + QEMU_CAPS_ADD_FD = 127, /* -add-fd */
>> Okay, so you are taking the approach that we insist on command line
>> support (qemu 1.3), not just the QMP command existing (qemu 1.2). It's
>> easy to probe whether 'add-fd' QMP exists (query-command), but harder
to
>> determine if command line support is present when using just QMP with
>> the current state of qemu.
>>
> Would you then not use -add-fd on the command line but hotplug those
> devices instead?
We have two options:
1. Try to support both qemu 1.2 and qemu 1.3. For qemu 1.3, we can use
the command line (which feels simpler to me), but for qemu 1.2 we would
have to do everything through QMP. This means that we'd have to write
code that does not attach _any_ -drive arguments, and instead start
'qemu -s', hotplug all the drives, and then start the domain. But if we
can get things working for 1.2 via hotplug only, then there is no need
to use the command line for 1.3 - use a single interface for it all.
This method is easy to probe - does the 'add-fd' QMP command exist in
'query-commands'?
2. Not support fd passing unless targeting qemu 1.3 or newer. When
targeting 1.2 and older, use the command line as always, and state that
qemu will use open() and the user has to use virt_use_nfs. With 1.3,
use -add-fd on the command line, so that we don't have to worry about
hot-plugging disks after 'qemu -s' on initial start; we'd still have to
wire up QMP command support for hotplug, but at least we don't have to
refactor the command line computation into delaying things to hotplug.
Since there is no 'query-config-schema' command in 1.3, we'll have to
hack in some other capability check; for this I have two ideas: a)
straight version check (if 'query-version' reports 1.3 or newer, assume
-add-fd exists); this does not backport well. b) during the initial QMP
probes, if 'add-fd' exists, try calling 'add-fd' with a specified
fdset-id argument and /dev/null as an fd; qemu 1.2 will reject the call
(in that version, qemu insisted on managing set ids itself; you could
only pass fdset-id when adding to an existing set); qemu 1.3 and later
will accept it (the management app libvirt can choose its own allocation
pattern for set ids).
I'm leaning towards 2b, so I'll code that up as part of my QMP work.
I vote for 2nd option - do not try to support QEMU 1.2.
Effectively QEMU 1.2 is just 50% of the solution - QEMU 1.3 is the first
place where you have 100% of the solution. While you might be able to
hack around the limitations of QEMU 1.2, this is effectively dead-end
work - it is easier just to tell people to upgrade to newer QEMU.
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 :|
10:52 a.m.
New subject: [libvirt] [PATCH V1 2/4] Implement a virIntSet class
Implementation of a virIntSet class.
---
src/Makefile.am | 1
src/libvirt_private.syms | 8 +++++
src/util/virintset.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
src/util/virintset.h | 44 +++++++++++++++++++++++++++
4 files changed, 127 insertions(+)
Index: libvirt/src/Makefile.am
===================================================================
--- libvirt.orig/src/Makefile.am
+++ libvirt/src/Makefile.am
@@ -75,6 +75,7 @@ UTIL_SOURCES = \
util/virhashcode.c util/virhashcode.h \
util/virhook.c util/virhook.h \
util/virinitctl.c util/virinitctl.h \
+ util/virintset.c util/virintset.h \
util/viriptables.c util/viriptables.h \
util/virjson.c util/virjson.h \
util/virkeycode.c util/virkeycode.h \
Index: libvirt/src/util/virintset.c
===================================================================
--- /dev/null
+++ libvirt/src/util/virintset.c
@@ -0,0 +1,74 @@
+/*
+ * virintset.c: integer set
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Authors:
+ * Stefan Berger <stefanb(a)linux.vnet.ibm.com>
+ */
+#include "virintset.h"
+#include "viralloc.h"
+
+void virIntSetInit(virIntSetPtr set)
+{
+ set->size = 0;
+ set->capacity = 0;
+ set->set = NULL;
+}
+
+void virIntSetClear(virIntSetPtr set)
+{
+ VIR_FREE(set->set);
+ virIntSetInit(set);
+}
+
+ssize_t virIntSetAdd(virIntSetPtr set, int val)
+{
+ size_t res;
+
+ if (virIntSetIndexOf(set, val) > 0)
+ return -EEXIST;
+
+ if (set->capacity <= set->size) {
+ if (VIR_REALLOC_N(set->set, set->capacity + 10) < 0) {
+ return -ENOMEM;
+ }
+ set->capacity += 10;
+ }
+
+ res = set->size;
+
+ set->set[set->size++] = val;
+
+ return res;
+}
+
+size_t virIntSetSize(const virIntSetPtr set)
+{
+ return set->size;
+}
+
+ssize_t virIntSetIndexOf(const virIntSetPtr set, int val)
+{
+ size_t i;
+
+ for (i = 0; i < set->size; i++)
+ if (set->set[i] == val)
+ return i;
+
+ return -ENOENT;
+}
Index: libvirt/src/util/virintset.h
===================================================================
--- /dev/null
+++ libvirt/src/util/virintset.h
@@ -0,0 +1,44 @@
+/*
+ * virintset.h: integer set
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Authors:
+ * Stefan Berger <stefanb(a)linux.vnet.ibm.com>
+ */
+
+#ifndef __VIR_INTSET_H__
+# define __VIR_INTSET_H__
+
+# include "internal.h"
+
+typedef struct _virIntSet virIntSet;
+typedef virIntSet *virIntSetPtr;
+
+struct _virIntSet {
+ size_t size;
+ size_t capacity;
+ int *set;
+};
+
+void virIntSetInit(virIntSetPtr set);
+void virIntSetClear(virIntSetPtr set);
+ssize_t virIntSetAdd(virIntSetPtr set, int val);
+size_t virIntSetSize(const virIntSetPtr set);
+ssize_t virIntSetIndexOf(const virIntSetPtr set, int val);
+
+#endif /* __VIR_INTSET_H__ */
Index: libvirt/src/libvirt_private.syms
===================================================================
--- libvirt.orig/src/libvirt_private.syms
+++ libvirt/src/libvirt_private.syms
@@ -1393,6 +1393,14 @@ virFileWrapperFdNew;
virInitctlSetRunLevel;
+# virintset.h
+virIntSetAdd;
+virIntSetClear;
+virIntSetIndexOf;
+virIntSetInit;
+virIntSetSize;
+
+
# virkeycode.h
virKeycodeSetTypeFromString;
virKeycodeSetTypeToString;
5:33 p.m.
New subject: [libvirt] [PATCH V1 2/4] Implement a virIntSet class
On 01/29/2013 09:52 AM, Stefan Berger wrote:
Implementation of a virIntSet class.
---
src/Makefile.am | 1
src/libvirt_private.syms | 8 +++++
src/util/virintset.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
src/util/virintset.h | 44 +++++++++++++++++++++++++++
4 files changed, 127 insertions(+)
+
+ssize_t virIntSetAdd(virIntSetPtr set, int val)
Documentation of these functions would be helpful.
+{
+ size_t res;
+
+ if (virIntSetIndexOf(set, val) > 0)
+ return -EEXIST;
+
+ if (set->capacity <= set->size) {
+ if (VIR_REALLOC_N(set->set, set->capacity + 10) < 0) {
+ return -ENOMEM;
+ }
+ set->capacity += 10;
+ }
Rather than using VIR_REALLOC_N to manage the set size by hand, I
recommend using VIR_RESIZE_N(set->set, set->capacity, set->size, 1).
+
+ res = set->size;
+
+ set->set[set->size++] = val;
+
+ return res;
+}
Do we ever expect the sets to grow large enough that storing the ints in
sorted order would be beneficial (giving us O(logN) instead of O(N)
behaviors on lookup)?
+
+#ifndef __VIR_INTSET_H__
+# define __VIR_INTSET_H__
+
+# include "internal.h"
+
+typedef struct _virIntSet virIntSet;
+typedef virIntSet *virIntSetPtr;
+
+struct _virIntSet {
+ size_t size;
+ size_t capacity;
+ int *set;
+};
This struct could be opaque (moving it into the .c file shouldn't
invalidate any callers, which should all be going through the functions
rather than peeking at the members of the struct).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:52 a.m.
New subject: [libvirt] [PATCH V1 3/4] Add a file descriptor set to QEMU private domain struct
Add a file descriptor set to the QEMU private domain structure.
---
src/qemu/qemu_domain.c | 4 ++++
src/qemu/qemu_domain.h | 3 +++
2 files changed, 7 insertions(+)
Index: libvirt/src/qemu/qemu_domain.c
===================================================================
--- libvirt.orig/src/qemu/qemu_domain.c
+++ libvirt/src/qemu/qemu_domain.c
@@ -37,6 +37,7 @@
#include "domain_event.h"
#include "virtime.h"
#include "virstoragefile.h"
+#include "virintset.h"
#include <sys/time.h>
#include <fcntl.h>
@@ -220,6 +221,8 @@ static void *qemuDomainObjPrivateAlloc(v
priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
+ virIntSetInit(&priv->fdset);
+
return priv;
error:
@@ -252,6 +255,7 @@ static void qemuDomainObjPrivateFree(voi
qemuAgentClose(priv->agent);
}
VIR_FREE(priv->cleanupCallbacks);
+ virIntSetClear(&priv->fdset);
VIR_FREE(priv);
}
Index: libvirt/src/qemu/qemu_domain.h
===================================================================
--- libvirt.orig/src/qemu/qemu_domain.h
+++ libvirt/src/qemu/qemu_domain.h
@@ -32,6 +32,7 @@
# include "qemu_conf.h"
# include "qemu_capabilities.h"
# include "virchrdev.h"
+# include "virintset.h"
# define QEMU_EXPECTED_VIRT_TYPES \
((1 << VIR_DOMAIN_VIRT_QEMU) | \
@@ -160,6 +161,8 @@ struct _qemuDomainObjPrivate {
qemuDomainCleanupCallback *cleanupCallbacks;
size_t ncleanupCallbacks;
size_t ncleanupCallbacks_max;
+
+ virIntSet fdset;
};
struct qemuDomainWatchdogEvent
5:41 p.m.
New subject: [libvirt] [PATCH V1 3/4] Add a file descriptor set to QEMU private domain struct
On 01/29/2013 09:52 AM, Stefan Berger wrote:
Add a file descriptor set to the QEMU private domain structure.
---
src/qemu/qemu_domain.c | 4 ++++
src/qemu/qemu_domain.h | 3 +++
2 files changed, 7 insertions(+)
In isolation, this patch looks okay. But the real question is what it
will be used for. I'm worried that we don't have quite the right
implementation design in place yet.
Unfortunately, I think we need much more than storing just a set of
integers - we need a full blown hash table, where the key is the fd we
passed through, and the value is more details (the file name or other
details describing the fd that we are passing, and the resulting qemu
fdset number that the fd will belong to). Furthermore, we need to make
this storage persist across libvirt reboots, which means it needs to be
part of the (internal-only) XML that we save in
/var/run/libvirt/qemu/$dom.xml, so we must also modify
qemuDomainObjPrivateXMLFormat() to output the contents of this set (or
hash table) in a reusable manner.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:13 a.m.
New subject: [libvirt] [PATCH V1 3/4] Add a file descriptor set to QEMU private domain struct
On 01/29/2013 04:41 PM, Eric Blake wrote:
On 01/29/2013 09:52 AM, Stefan Berger wrote:
> Add a file descriptor set to the QEMU private domain structure.
>
> ---
> src/qemu/qemu_domain.c | 4 ++++
> src/qemu/qemu_domain.h | 3 +++
> 2 files changed, 7 insertions(+)
In isolation, this patch looks okay. But the real question is what it
will be used for. I'm worried that we don't have quite the right
implementation design in place yet.
Unfortunately, I think we need much more than storing just a set of
integers - we need a full blown hash table, where the key is the fd we
passed through, and the value is more details (the file name or other
details describing the fd that we are passing, and the resulting qemu
fdset number that the fd will belong to). Furthermore, we need to make
this storage persist across libvirt reboots, which means it needs to be
part of the (internal-only) XML that we save in
/var/run/libvirt/qemu/$dom.xml, so we must also modify
qemuDomainObjPrivateXMLFormat() to output the contents of this set (or
hash table) in a reusable manner.
For that matter, I just realized that we already have an <alias> XMl
element in the internal-only XML of each /var/run/libvirt/qemu/dom.xml;
which is part of the virDomainDeviceInfo. It seems like this struct
would be a useful place for us to add another XML attribute stating what
qemu fdset is associated with any given device. That also means that in
patch 4/4, we don't have to pass a virIntSet around; any code like
qemuBuildChrChardevStr that is already looking up an 'alias' associated
with a device can also look up an fdset from the same struct.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:52 a.m.
New subject: [libvirt] [PATCH V1 4/4] Add support for file descriptor sets
Add support for file descriptor sets by converting some of the
command line parameters to use /dev/fdset/%d if -add-fd is found
to be supported by QEMU. For those devices libvirt now open()s
the device to obtain the file descriptor and 'transfers' the
fd using virCommand.
For the following fragments of domain XML
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/fc14-x86_64.img'/>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
<serial type='dev'>
<source path='/dev/ttyS0'/>
<target port='0'/>
</serial>
<serial type='pipe'>
<source path='/tmp/testpipe'/>
<target port='1'/>
</serial>
now create the following parts of command line:
-add-fd set=0,fd=23,opaque=/var/lib/libvirt/images/fc14-x86_64.img -drive
file=/dev/fdset/0,if=none,id=drive-ide0-0-0,format=raw
-add-fd set=1,fd=30,opaque=/dev/ttyS0 -chardev tty,id=charserial0,path=/dev/fdset/1
-add-fd set=2,fd=32,opaque=/tmp/testpipe -chardev pipe,id=charserial1,path=/dev/fdset/2
---
src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_command.h | 13 ++
src/qemu/qemu_driver.c | 6 +
src/qemu/qemu_hotplug.c | 9 +
src/qemu/qemu_process.c | 3
tests/qemuxml2argvtest.c | 8 +
tests/qemuxmlnstest.c | 7 +
7 files changed, 215 insertions(+), 55 deletions(-)
Index: libvirt/src/qemu/qemu_command.c
===================================================================
--- libvirt.orig/src/qemu/qemu_command.c
+++ libvirt/src/qemu/qemu_command.c
@@ -46,6 +46,7 @@
#include "base64.h"
#include "device_conf.h"
#include "virstoragefile.h"
+#include "virintset.h"
#include <sys/stat.h>
#include <fcntl.h>
@@ -133,6 +134,65 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO
/**
+ * qemuVirCommandGetFDSet:
+ * @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".
+ */
+static char *
+qemuVirCommandGetFDSet(virIntSetPtr fdset, int fd,
+ const char *opaque)
+{
+ char *result = NULL;
+ int idx = virIntSetIndexOf(fdset, fd);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "set=%d,fd=%d%s%s", idx, fd,
+ (opaque) ? ",opaque=" : "",
+ (opaque) ? opaque : "") < 0) {
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d not inn fdset"), fd);
+ }
+
+ return result;
+}
+
+/**
+ * qemuVirCommandGetDevSet:
+ * @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.
+ */
+static char *
+qemuVirCommandGetDevSet(virIntSetPtr fdset, int fd)
+{
+ char *result = NULL;
+ int idx = virIntSetIndexOf(fdset, fd);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "/dev/fdset/%d", idx) < 0) {
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d not in fdset"), fd);
+ }
+ return result;
+}
+
+
+/**
* qemuPhysIfaceConnect:
* @def: the definition of the VM (needed by 802.1Qbh and audit)
* @driver: pointer to the driver instance
@@ -2223,11 +2283,65 @@ no_memory:
goto cleanup;
}
+static char *
+qemuCreatePathForFilePath(const char *fmt, const char *path,
+ virCommandPtr cmd, virIntSetPtr fdset,
+ qemuCapsPtr caps)
+{
+ char *res = NULL;
+ char *devset = NULL;
+ char *fdsetstr = NULL;
+
+ /*
+ * cmd == NULL hints at hotplugging; use old way of doing things
+ */
+ if (!cmd || !qemuCapsGet(caps, QEMU_CAPS_ADD_FD)) {
+ if (virAsprintf(&res, fmt, path) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+ } else {
+ int fd = open(path, O_RDWR);
+ if (fd < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not open %s"), path);
+ return NULL;
+ }
+ virCommandTransferFD(cmd, fd);
+ if (virIntSetAdd(fdset, fd) == -ENOMEM) {
+ virReportOOMError();
+ return NULL;
+ }
+ devset = qemuVirCommandGetDevSet(fdset, fd);
+ if (!devset)
+ return NULL;
+ if (virAsprintf(&res, fmt, devset) < 0)
+ goto error;
+
+ fdsetstr = qemuVirCommandGetFDSet(fdset, fd, path);
+ if (!fdsetstr)
+ goto error;
+ virCommandAddArgList(cmd, "-add-fd", fdsetstr, NULL);
+ }
+
+ VIR_FREE(devset);
+ VIR_FREE(fdsetstr);
+
+ return res;
+
+error:
+ VIR_FREE(devset);
+ VIR_FREE(fdset);
+ return NULL;
+}
+
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskDefPtr disk,
bool bootable,
- qemuCapsPtr caps)
+ qemuCapsPtr caps,
+ virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
@@ -2235,6 +2349,7 @@ qemuBuildDriveStr(virConnectPtr conn ATT
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ char *pathfdset = NULL;
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2389,7 +2504,12 @@ qemuBuildDriveStr(virConnectPtr conn ATT
"block type disk"));
goto error;
}
- virBufferEscape(&opt, ',', ",", "file=%s,",
disk->src);
+ pathfdset = qemuCreatePathForFilePath("file=%s", disk->src,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferEscape(&opt, ',', ",", "%s,",
pathfdset);
+ VIR_FREE(pathfdset);
}
}
if (qemuCapsGet(caps, QEMU_CAPS_DEVICE))
@@ -2886,11 +3006,14 @@ error:
char *qemuBuildFSStr(virDomainFSDefPtr fs,
- qemuCapsPtr caps ATTRIBUTE_UNUSED)
+ qemuCapsPtr caps ATTRIBUTE_UNUSED,
+ virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
+ char *pathfdset = NULL;
if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -2935,7 +3058,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr f
}
virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX,
fs->info.alias);
- virBufferAsprintf(&opt, ",path=%s", fs->src);
+ pathfdset = qemuCreatePathForFilePath("path=%s", fs->src, cmd, fdset,
caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&opt, ",%s", pathfdset);
if (fs->readonly) {
if (qemuCapsGet(caps, QEMU_CAPS_FSDEV_READONLY)) {
@@ -2952,10 +3078,12 @@ char *qemuBuildFSStr(virDomainFSDefPtr f
virReportOOMError();
goto error;
}
+ VIR_FREE(pathfdset);
return virBufferContentAndReset(&opt);
error:
+ VIR_FREE(pathfdset);
virBufferFreeAndReset(&opt);
return NULL;
}
@@ -3884,15 +4012,16 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHo
}
-
/* This function outputs a -chardev command line option which describes only the
* host side of the character device */
static char *
qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias,
- qemuCapsPtr caps)
+ qemuCapsPtr caps, virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
bool telnet;
+ char *pathfdset = NULL;
switch (dev->type) {
case VIR_DOMAIN_CHR_TYPE_NULL:
@@ -3908,19 +4037,31 @@ qemuBuildChrChardevStr(virDomainChrSourc
break;
case VIR_DOMAIN_CHR_TYPE_DEV:
- virBufferAsprintf(&buf, "%s,id=char%s,path=%s",
+ pathfdset = qemuCreatePathForFilePath("path=%s",
dev->data.file.path,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&buf, "%s,id=char%s,%s",
STRPREFIX(alias, "parallel") ? "parport" :
"tty",
- alias, dev->data.file.path);
+ alias, pathfdset);
break;
case VIR_DOMAIN_CHR_TYPE_FILE:
- virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias,
- dev->data.file.path);
+ pathfdset = qemuCreatePathForFilePath("path=%s",
dev->data.file.path,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&buf, "file,id=char%s,%s", alias,
+ pathfdset);
break;
case VIR_DOMAIN_CHR_TYPE_PIPE:
- virBufferAsprintf(&buf, "pipe,id=char%s,path=%s", alias,
- dev->data.file.path);
+ pathfdset = qemuCreatePathForFilePath("path=%s",
dev->data.file.path,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&buf, "pipe,id=char%s,%s", alias,
+ pathfdset);
break;
case VIR_DOMAIN_CHR_TYPE_STDIO:
@@ -3989,10 +4130,13 @@ qemuBuildChrChardevStr(virDomainChrSourc
goto error;
}
+ VIR_FREE(pathfdset);
+
return virBufferContentAndReset(&buf);
error:
virBufferFreeAndReset(&buf);
+ VIR_FREE(pathfdset);
return NULL;
}
@@ -5056,7 +5200,8 @@ qemuBuildCommandLine(virConnectPtr conn,
const char *migrateFrom,
int migrateFd,
virDomainSnapshotObjPtr snapshot,
- enum virNetDevVPortProfileOp vmop)
+ enum virNetDevVPortProfileOp vmop,
+ virIntSetPtr fdset)
{
int i, j;
int disableKQEMU = 0;
@@ -5350,11 +5495,10 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev if it's available */
if (qemuCapsGet(caps, QEMU_CAPS_CHARDEV)) {
- virCommandAddArg(cmd, "-chardev");
if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor",
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, chrdev);
+ virCommandAddArgList(cmd, "-chardev", chrdev, NULL);
VIR_FREE(chrdev);
virCommandAddArg(cmd, "-mon");
@@ -5797,8 +5941,6 @@ qemuBuildCommandLine(virConnectPtr conn,
break;
}
- virCommandAddArg(cmd, "-drive");
-
/* Unfortunately it is not possible to use
-device for floppies, or Xen paravirt
devices. Fortunately, those don't need
@@ -5814,12 +5956,12 @@ qemuBuildCommandLine(virConnectPtr conn,
}
optstr = qemuBuildDriveStr(conn, disk,
emitBootindex ? false : !!bootindex,
- caps);
+ caps, cmd, fdset);
if (deviceFlagMasked)
qemuCapsSet(caps, QEMU_CAPS_DEVICE);
if (!optstr)
goto error;
- virCommandAddArg(cmd, optstr);
+ virCommandAddArgList(cmd, "-drive", optstr, NULL);
VIR_FREE(optstr);
if (!emitBootindex)
@@ -5995,7 +6137,7 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainFSDefPtr fs = def->fss[i];
virCommandAddArg(cmd, "-fsdev");
- if (!(optstr = qemuBuildFSStr(fs, caps)))
+ if (!(optstr = qemuBuildFSStr(fs, caps, cmd, fdset)))
goto error;
virCommandAddArg(cmd, optstr);
VIR_FREE(optstr);
@@ -6275,14 +6417,13 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru,
smartcard->info.alias,
- caps))) {
+ caps, cmd, fdset))) {
virBufferFreeAndReset(&opt);
goto error;
}
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
virBufferAsprintf(&opt, "ccid-card-passthru,chardev=char%s",
@@ -6313,12 +6454,11 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (qemuCapsGet(caps, QEMU_CAPS_CHARDEV) &&
qemuCapsGet(caps, QEMU_CAPS_DEVICE)) {
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&serial->source,
serial->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
virCommandAddArg(cmd, "-device");
@@ -6350,12 +6490,11 @@ qemuBuildCommandLine(virConnectPtr conn,
/* Use -chardev with -device if they are available */
if (qemuCapsGet(caps, QEMU_CAPS_CHARDEV) &&
qemuCapsGet(caps, QEMU_CAPS_DEVICE)) {
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(¶llel->source,
parallel->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
virCommandAddArg(cmd, "-device");
@@ -6387,12 +6526,11 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&channel->source,
channel->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
addr = virSocketAddrFormat(channel->target.addr);
@@ -6422,12 +6560,11 @@ qemuBuildCommandLine(virConnectPtr conn,
* the newer -chardev interface. */
;
} else {
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&channel->source,
channel->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
}
@@ -6460,12 +6597,11 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&console->source,
console->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
virCommandAddArg(cmd, "-device");
@@ -6482,12 +6618,11 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&console->source,
console->info.alias,
- caps)))
+ caps, cmd, fdset)))
goto error;
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
virCommandAddArg(cmd, "-device");
@@ -6824,14 +6959,13 @@ qemuBuildCommandLine(virConnectPtr conn,
virDomainRedirdevDefPtr redirdev = def->redirdevs[i];
char *devstr;
- virCommandAddArg(cmd, "-chardev");
if (!(devstr = qemuBuildChrChardevStr(&redirdev->source.chr,
redirdev->info.alias,
- caps))) {
+ caps, cmd, fdset))) {
goto error;
}
- virCommandAddArg(cmd, devstr);
+ virCommandAddArgList(cmd, "-chardev", devstr, NULL);
VIR_FREE(devstr);
if (!qemuCapsGet(caps, QEMU_CAPS_DEVICE))
Index: libvirt/src/qemu/qemu_command.h
===================================================================
--- libvirt.orig/src/qemu/qemu_command.h
+++ libvirt/src/qemu/qemu_command.h
@@ -30,6 +30,7 @@
# include "qemu_conf.h"
# include "qemu_domain.h"
# include "qemu_capabilities.h"
+# include "virintset.h"
/* Config type for XML import/export conversions */
# define QEMU_CONFIG_FORMAT_ARGV "qemu-argv"
@@ -58,7 +59,8 @@ virCommandPtr qemuBuildCommandLine(virCo
const char *migrateFrom,
int migrateFd,
virDomainSnapshotObjPtr current_snapshot,
- enum virNetDevVPortProfileOp vmop)
+ enum virNetDevVPortProfileOp vmop,
+ virIntSetPtr fdset)
ATTRIBUTE_NONNULL(1);
/* Generate string for arch-specific '-device' parameter */
@@ -95,9 +97,14 @@ char *qemuDeviceDriveHostAlias(virDomain
char *qemuBuildDriveStr(virConnectPtr conn,
virDomainDiskDefPtr disk,
bool bootable,
- qemuCapsPtr caps);
+ qemuCapsPtr caps,
+ virCommandPtr cmd,
+ virIntSetPtr fdset);
+
char *qemuBuildFSStr(virDomainFSDefPtr fs,
- qemuCapsPtr caps);
+ qemuCapsPtr caps,
+ virCommandPtr cmd,
+ virIntSetPtr fdset);
/* Current, best practice */
char * qemuBuildDriveDevStr(virDomainDefPtr def,
Index: libvirt/src/qemu/qemu_driver.c
===================================================================
--- libvirt.orig/src/qemu/qemu_driver.c
+++ libvirt/src/qemu/qemu_driver.c
@@ -5336,7 +5336,9 @@ static char *qemuDomainXMLToNative(virCo
virCommandPtr cmd = NULL;
char *ret = NULL;
int i;
+ virIntSet fdset;
+ virIntSetInit(&fdset);
virCheckFlags(0, NULL);
qemuDriverLock(driver);
@@ -5437,7 +5439,8 @@ static char *qemuDomainXMLToNative(virCo
if (!(cmd = qemuBuildCommandLine(conn, driver, def,
&monConfig, monitor_json, caps,
- NULL, -1, NULL,
VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)))
+ NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
+ &fdset)))
goto cleanup;
ret = virCommandToString(cmd);
@@ -5448,6 +5451,7 @@ cleanup:
virObjectUnref(caps);
virCommandFree(cmd);
virDomainDefFree(def);
+ virIntSetClear(&fdset);
return ret;
}
Index: libvirt/src/qemu/qemu_hotplug.c
===================================================================
--- libvirt.orig/src/qemu/qemu_hotplug.c
+++ libvirt/src/qemu/qemu_hotplug.c
@@ -262,7 +262,8 @@ int qemuDomainAttachPciDiskDevice(virCon
if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->caps) < 0)
goto error;
- if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps)))
+ if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps,
+ NULL, &priv->fdset)))
goto error;
if (!(devstr = qemuBuildDriveDevStr(NULL, disk, 0, priv->caps)))
@@ -503,7 +504,8 @@ int qemuDomainAttachSCSIDisk(virConnectP
goto error;
}
- if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps)))
+ if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps, NULL,
+ &priv->fdset)))
goto error;
for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) {
@@ -622,7 +624,8 @@ int qemuDomainAttachUsbMassstorageDevice
if (qemuCapsGet(priv->caps, QEMU_CAPS_DEVICE)) {
if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->caps) < 0)
goto error;
- if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps)))
+ if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->caps,
+ NULL, &priv->fdset)))
goto error;
if (!(devstr = qemuBuildDriveDevStr(NULL, disk, 0, priv->caps)))
goto error;
Index: libvirt/src/qemu/qemu_process.c
===================================================================
--- libvirt.orig/src/qemu/qemu_process.c
+++ libvirt/src/qemu/qemu_process.c
@@ -3779,7 +3779,8 @@ int qemuProcessStart(virConnectPtr conn,
VIR_DEBUG("Building emulator command line");
if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig,
priv->monJSON != 0, priv->caps,
- migrateFrom, stdin_fd, snapshot, vmop)))
+ migrateFrom, stdin_fd, snapshot, vmop,
+ &priv->fdset)))
goto cleanup;
/* now that we know it is about to start call the hook if present */
Index: libvirt/tests/qemuxml2argvtest.c
===================================================================
--- libvirt.orig/tests/qemuxml2argvtest.c
+++ libvirt/tests/qemuxml2argvtest.c
@@ -18,6 +18,7 @@
# include "qemu/qemu_domain.h"
# include "datatypes.h"
# include "cpu/cpu_map.h"
+# include "virintset.h"
# include "testutilsqemu.h"
@@ -92,6 +93,9 @@ static int testCompareXMLToArgvFiles(con
virConnectPtr conn;
char *log = NULL;
virCommandPtr cmd = NULL;
+ virIntSet fdset;
+
+ virIntSetInit(&fdset);
if (!(conn = virGetConnect()))
goto out;
@@ -151,7 +155,8 @@ static int testCompareXMLToArgvFiles(con
if (!(cmd = qemuBuildCommandLine(conn, &driver, vmdef, &monitor_chr,
(flags & FLAG_JSON), extraFlags,
migrateFrom, migrateFd, NULL,
- VIR_NETDEV_VPORT_PROFILE_OP_NO_OP))) {
+ VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
+ &fdset))) {
if (flags & FLAG_EXPECT_FAILURE) {
ret = 0;
virResetLastError();
@@ -198,6 +203,7 @@ out:
virCommandFree(cmd);
virDomainDefFree(vmdef);
virObjectUnref(conn);
+ virIntSetClear(&fdset);
return ret;
}
Index: libvirt/tests/qemuxmlnstest.c
===================================================================
--- libvirt.orig/tests/qemuxmlnstest.c
+++ libvirt/tests/qemuxmlnstest.c
@@ -41,6 +41,9 @@ static int testCompareXMLToArgvFiles(con
char *log = NULL;
char *emulator = NULL;
virCommandPtr cmd = NULL;
+ virIntSet fdset;
+
+ virIntSetInit(&fdset);
if (!(conn = virGetConnect()))
goto fail;
@@ -110,7 +113,8 @@ static int testCompareXMLToArgvFiles(con
if (!(cmd = qemuBuildCommandLine(conn, &driver,
vmdef, &monitor_chr, json, extraFlags,
migrateFrom, migrateFd, NULL,
- VIR_NETDEV_VPORT_PROFILE_OP_NO_OP)))
+ VIR_NETDEV_VPORT_PROFILE_OP_NO_OP,
+ &fdset)))
goto fail;
if (!!virGetLastError() != expectError) {
@@ -151,6 +155,7 @@ static int testCompareXMLToArgvFiles(con
virCommandFree(cmd);
virDomainDefFree(vmdef);
virObjectUnref(conn);
+ virIntSetClear(&fdset);
return ret;
}
9:06 p.m.
New subject: [libvirt] [PATCH V1 4/4] Add support for file descriptor sets
On 01/29/2013 09:52 AM, Stefan Berger wrote:
Add support for file descriptor sets by converting some of the
command line parameters to use /dev/fdset/%d if -add-fd is found
to be supported by QEMU. For those devices libvirt now open()s
the device to obtain the file descriptor and 'transfers' the
fd using virCommand.
For the following fragments of domain XML
<disk type='file' device='disk'>
<driver name='qemu' type='raw'/>
<source file='/var/lib/libvirt/images/fc14-x86_64.img'/>
<target dev='hda' bus='ide'/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
<serial type='dev'>
<source path='/dev/ttyS0'/>
<target port='0'/>
</serial>
<serial type='pipe'>
<source path='/tmp/testpipe'/>
<target port='1'/>
</serial>
now create the following parts of command line:
Rearranging things just a bit:
-add-fd set=1,fd=30,opaque=/dev/ttyS0 -chardev
tty,id=charserial0,path=/dev/fdset/1
-add-fd set=2,fd=32,opaque=/tmp/testpipe -chardev
pipe,id=charserial1,path=/dev/fdset/2
These two conversions looks sane; although it might help to list the old
style next to the new style in the commit message:
old: -chardev tty,id=charserial0,path=/dev/ttyS0
new: -add-fd set=1,fd=30,opaque=/dev/ttyS0 -chardev
tty,id=charserial0,path=/dev/fdset/1
-add-fd set=0,fd=23,opaque=/var/lib/libvirt/images/fc14-x86_64.img -drive
file=/dev/fdset/0,if=none,id=drive-ide0-0-0,format=raw
This conversion feels insufficient for qemu 1.3. As written, your patch
can only ever tie one fd to a set (that is, each qemu fdset contains
exactly one passed fd). But my understanding of the qemu 1.3 code is
that for -drive arguments, qemu_open() is called twice: once with
O_RDONLY to probe the file format, and again with O_RDWR to actually use
the file; furthermore, qemu_open() refuses to use an O_RDWR fd for
O_RDONLY operations (in the name of safety). Arguably, this is a flaw
in the qemu design (qemu shouldn't need to open() a file twice to use
it, and I argued at the time that qemu implemented fdset that an O_RDWR
fd should serve just fine for an O_RDONLY request); but since it exists,
we must live with it. That means that for THIS line, the proper
translation really has to be:
old: -drive
file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw
new: -add-fd
set=0,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img
-add-fd set=0,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img
-drive file=/dev/fdset/0,if=none,id=drive-ide0-0-0,format=raw
where we really are passing two fds, 23 as O_RDONLY, and 24 as O_RDWR,
within the single fdset 0.
---
src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_command.h | 13 ++
src/qemu/qemu_driver.c | 6 +
src/qemu/qemu_hotplug.c | 9 +
src/qemu/qemu_process.c | 3
tests/qemuxml2argvtest.c | 8 +
tests/qemuxmlnstest.c | 7 +
We need to add a new test: a new tests/qemuxml2argvdata .args file, with
the matching .xml file copied from an existing test, along with a line
in qemuxml2argvtest.c to run the new test using the new capability right
next to the existing test run without the capability. Doing this will
make it more obvious that we are generating appropriate command lines
with or without the capability detected. I guess it gets tricky, though
- the test cannot actually open /dev/ttyS0, but must limit itself to
either opening files under control of the testsuite, or factoring the
qemu_command.c code so that we can generate the command line that would
exist if an fd were open but without actually opening the fd.
7 files changed, 215 insertions(+), 55 deletions(-)
Index: libvirt/src/qemu/qemu_command.c
===================================================================
--- libvirt.orig/src/qemu/qemu_command.c
+++ libvirt/src/qemu/qemu_command.c
@@ -46,6 +46,7 @@
#include "base64.h"
#include "device_conf.h"
#include "virstoragefile.h"
+#include "virintset.h"
#include <sys/stat.h>
#include <fcntl.h>
@@ -133,6 +134,65 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO
/**
+ * qemuVirCommandGetFDSet:
+ * @cmd: the command to modify
+ * @fd: fd to reassign to the child
Missing documentation for @opaque; but at the level of libvirt, we know
what it is, and might as well call it 'name' instead of 'opaque' (it is
only qemu that is treating it as opaque).
+ *
+ * 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".
+ */
+static char *
+qemuVirCommandGetFDSet(virIntSetPtr fdset, int fd,
+ const char *opaque)
+{
+ char *result = NULL;
+ int idx = virIntSetIndexOf(fdset, fd);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "set=%d,fd=%d%s%s", idx, fd,
+ (opaque) ? ",opaque=" : "",
+ (opaque) ? opaque : "") < 0) {
Careful - opaque might contain commas, in which case you need to pass it
through virBufferEscape(buf, ',', ",", "%s", opaque) instead
of using it
directly.
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d not inn fdset"), fd);
s/inn/in/
+ }
+
+ return result;
+}
+
+/**
+ * qemuVirCommandGetDevSet:
+ * @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.
+ */
+static char *
+qemuVirCommandGetDevSet(virIntSetPtr fdset, int fd)
+{
+ char *result = NULL;
+ int idx = virIntSetIndexOf(fdset, fd);
+
+ if (idx >= 0) {
+ if (virAsprintf(&result, "/dev/fdset/%d", idx) < 0) {
+ virReportOOMError();
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("file descriptor %d not in fdset"), fd);
+ }
+ return result;
+}
+
Do these two functions need 'vir' in their name, or could we get by with
qemuCommandGetFDSet and qemuCommandGetDevSet?
+
+/**
* qemuPhysIfaceConnect:
* @def: the definition of the VM (needed by 802.1Qbh and audit)
* @driver: pointer to the driver instance
@@ -2223,11 +2283,65 @@ no_memory:
goto cleanup;
}
+static char *
+qemuCreatePathForFilePath(const char *fmt, const char *path,
+ virCommandPtr cmd, virIntSetPtr fdset,
+ qemuCapsPtr caps)
+{
+ char *res = NULL;
+ char *devset = NULL;
+ char *fdsetstr = NULL;
+
+ /*
+ * cmd == NULL hints at hotplugging; use old way of doing things
+ */
+ if (!cmd || !qemuCapsGet(caps, QEMU_CAPS_ADD_FD)) {
+ if (virAsprintf(&res, fmt, path) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
See my thoughts below about printing into an existing virBuffer instead
of allocating a new string with virAsprintf.
+ } else {
+ int fd = open(path, O_RDWR);
Won't work to call raw open(); you MUST go through qemuOpenFile() (which
works around situations such as libvirtd running as root but needing to
open a file on a root-squash NFS share); also, this would be the point
where we apply security manager labeling to the opened fd. In fact, the
caller probably ought to be able supply additional flags to open, such
as deciding whether O_CREAT, O_TRUNC, etc. makes sense on a per-caller
basis.
+ if (fd < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not open %s"), path);
When a system call fails, we should report it with
virReportSystemError(errno, _("..."))
+ return NULL;
+ }
+ virCommandTransferFD(cmd, fd);
+ if (virIntSetAdd(fdset, fd) == -ENOMEM) {
I would check for < 0, not just == -ENOMEM, to catch all possible errors
(although ENOMEM is the only current error).
+ virReportOOMError();
+ return NULL;
+ }
+ devset = qemuVirCommandGetDevSet(fdset, fd);
+ if (!devset)
+ return NULL;
+ if (virAsprintf(&res, fmt, devset) < 0)
+ goto error;
Missed a virReportOOMError().
+
+ fdsetstr = qemuVirCommandGetFDSet(fdset, fd, path);
+ if (!fdsetstr)
+ goto error;
+ virCommandAddArgList(cmd, "-add-fd", fdsetstr, NULL);
+ }
+
Below here...
+ VIR_FREE(devset);
+ VIR_FREE(fdsetstr);
+
+ return res;
+
+error:
+ VIR_FREE(devset);
+ VIR_FREE(fdset);
+ return NULL;
...it is simpler to write:
cleanup:
VIR_FREE(devset);
VIR_FREE(fdsetstr);
return res;
since all error paths already ensured res was NULL; then you aren't
duplicating the cleanup paths.
+}
+
char *
qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED,
virDomainDiskDefPtr disk,
bool bootable,
- qemuCapsPtr caps)
+ qemuCapsPtr caps,
+ virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus);
@@ -2235,6 +2349,7 @@ qemuBuildDriveStr(virConnectPtr conn ATT
virDomainDiskGeometryTransTypeToString(disk->geometry.trans);
int idx = virDiskNameToIndex(disk->dst);
int busid = -1, unitid = -1;
+ char *pathfdset = NULL;
if (idx < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2389,7 +2504,12 @@ qemuBuildDriveStr(virConnectPtr conn ATT
"block type disk"));
goto error;
}
- virBufferEscape(&opt, ',', ",", "file=%s,",
disk->src);
+ pathfdset = qemuCreatePathForFilePath("file=%s", disk->src,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferEscape(&opt, ',', ",", "%s,",
pathfdset);
Hmm, rather than malloc'ing pathfdset only to then copy it into the
malloc'd opt buffer, why not just print the path directly into the opt
buffer to begin with? In other words, instead of having
qemuCreatePathForFilePath return a char*, instead, have it take a
virBufferPtr argument (the existing &opt), and merely append the right
contents (path with comma escaping for old qemu, and /dev/fdset/%n for
new qemu).
+ VIR_FREE(pathfdset);
}
}
if (qemuCapsGet(caps, QEMU_CAPS_DEVICE))
@@ -2886,11 +3006,14 @@ error:
char *qemuBuildFSStr(virDomainFSDefPtr fs,
- qemuCapsPtr caps ATTRIBUTE_UNUSED)
+ qemuCapsPtr caps ATTRIBUTE_UNUSED,
+ virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer opt = VIR_BUFFER_INITIALIZER;
const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
+ char *pathfdset = NULL;
if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -2935,7 +3058,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr f
}
virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX,
fs->info.alias);
- virBufferAsprintf(&opt, ",path=%s", fs->src);
+ pathfdset = qemuCreatePathForFilePath("path=%s", fs->src, cmd, fdset,
caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&opt, ",%s", pathfdset);
Hmm - does fd passing really work for filesystem pass through? After
all, that implies that qemu will be opening multiple files beneath the
directory being passed as the source name. It may be that fdset and fs
passthrough are incompatible.
if (fs->readonly) {
if (qemuCapsGet(caps, QEMU_CAPS_FSDEV_READONLY)) {
@@ -2952,10 +3078,12 @@ char *qemuBuildFSStr(virDomainFSDefPtr f
virReportOOMError();
goto error;
}
+ VIR_FREE(pathfdset);
return virBufferContentAndReset(&opt);
error:
+ VIR_FREE(pathfdset);
virBufferFreeAndReset(&opt);
return NULL;
}
@@ -3884,15 +4012,16 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHo
}
-
/* This function outputs a -chardev command line option which describes only the
* host side of the character device */
static char *
qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias,
- qemuCapsPtr caps)
+ qemuCapsPtr caps, virCommandPtr cmd,
+ virIntSetPtr fdset)
{
virBuffer buf = VIR_BUFFER_INITIALIZER;
bool telnet;
+ char *pathfdset = NULL;
switch (dev->type) {
case VIR_DOMAIN_CHR_TYPE_NULL:
@@ -3908,19 +4037,31 @@ qemuBuildChrChardevStr(virDomainChrSourc
break;
case VIR_DOMAIN_CHR_TYPE_DEV:
- virBufferAsprintf(&buf, "%s,id=char%s,path=%s",
+ pathfdset = qemuCreatePathForFilePath("path=%s",
dev->data.file.path,
+ cmd, fdset, caps);
+ if (!pathfdset)
+ goto error;
+ virBufferAsprintf(&buf, "%s,id=char%s,%s",
STRPREFIX(alias, "parallel") ? "parport" :
"tty",
- alias, dev->data.file.path);
+ alias, pathfdset);
Again, printing directly into the existing &buf, instead of allocating a
new string only to copy pathfdset into the existing &buf, would be
nicer. Something like:
virBufferAsprintf(&buf, "%s,id=char%s,",
STRPREFIX(alias, "parallel") ? "parport" :
"tty",
alias);
if (qemuCreatePathForFile(&buf, "path=%s", dev->data.file.path,
cmd, fdset, caps) < 0)
goto error;
I'm also working on some patches to expose the QMP commands via
src/qemu/qemu_monitor.h, which will work in tandem with this series,
once we figure out in what direction we are heading.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4314
days inactive
4315
days old
12 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger