[libvirt] [PATCH V1 0/4] [RFC] Add support for QEMU file descriptor sets (part 1)
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
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 */ };
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
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?
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
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 :|
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@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@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;
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
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
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
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
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; }
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
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger