[libvirt] [PATCH V9 0/3] Add support for QEMU file descriptor sets

The following patch series adds initial support for QEMU file descriptor sets implementing support for creating the proper command line. Some devices are using the sets now. Regards, Stefan

Implement a class for file descriptor sets. This class remembers the mappings of aliases to file descriptor sets and maintains the number of the next-to-use file descriptor set. The class also writes and reads its own XML. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- v6->v7: - followed Eric's & Corey's comments on v6 v5->v6: - following to changeset 2e5d7798df v4->v5: - followed Daniel Berrange's comments - converted to virObject - provide virFdSetNew --- src/Makefile.am | 1 src/libvirt_private.syms | 8 ++ src/util/virfdset.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfdset.h | 94 ++++++++++++++++++++++++ 4 files changed, 282 insertions(+) Index: libvirt/src/util/virfdset.c =================================================================== --- /dev/null +++ libvirt/src/util/virfdset.c @@ -0,0 +1,179 @@ +/* + * virfdset.c: File descriptor set support + * + * 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/>. + * + * Author: Stefan Berger <stefanb@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "virfdset.h" +#include "virobject.h" +#include "viralloc.h" +#include "virutil.h" +#include "virerror.h" +#include "virbuffer.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + + +struct _virFdSet { + virObject object; + virHashTablePtr aliasToFdSet; + unsigned int nextfdset; +}; + +static virClassPtr virFdSetClass; +static void virFdSetDispose(void *obj); + +static int virFdSetOnceInit(void) +{ + if (!(virFdSetClass = virClassNew(virClassForObject(), + "virFdSet", + sizeof(virFdSet), + virFdSetDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virFdSet) + +static void virFdSetDispose(void *obj) +{ + virFdSetPtr fdset = obj; + + virHashFree(fdset->aliasToFdSet); +} + +virFdSetPtr virFdSetNew(void) +{ + virFdSetPtr fdset; + + if (virFdSetInitialize() < 0) + return NULL; + + if (!(fdset = virObjectNew(virFdSetClass))) + return NULL; + + if (!(fdset->aliasToFdSet = virHashCreate(10, NULL))) { + virObjectUnref(fdset); + return NULL; + } + fdset->nextfdset = 1; + + return fdset; +} + +static void virFdSetReset(virFdSetPtr fdset) +{ + virHashRemoveAll(fdset->aliasToFdSet); + fdset->nextfdset = 1; +} + +void virFdSetRemoveAlias(virFdSetPtr fdset, const char *alias) +{ + virHashRemoveEntry(fdset->aliasToFdSet, alias); +} + +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) +{ + if (virHashAddEntry(fdset->aliasToFdSet, alias, + (void *)(intptr_t)fdset->nextfdset) < 0) + return -1; + + *fdsetnum = fdset->nextfdset++; + + return 0; +} + +static void virFdSetPrintAliasToFdSet(void *payload, + const void *name, + void *data) +{ + virBufferPtr buf = data; + + virBufferAsprintf(buf, " <entry alias='%s' fdset='%u'/>\n", + (char *)name, + (unsigned int)(intptr_t)payload); +} + +void virFdSetFormatXML(virFdSetPtr fdset, virBufferPtr buf) +{ + virBufferAsprintf(buf, "<fdsets>\n"); + virHashForEach(fdset->aliasToFdSet, virFdSetPrintAliasToFdSet, buf); + virBufferAsprintf(buf, "</fdsets>\n"); +} + +int virFdSetParseXML(virFdSetPtr fdset, const char *xPath, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + int n, i; + char *key = NULL; + char *val = NULL; + unsigned int fdsetnum; + int ret = -1; + + virFdSetReset(fdset); + + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + /* nothing to parse is not an error */ + ret = 0; + goto cleanup; + } + + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (virStrToLong_ui(val, NULL, 10, &fdsetnum) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("incorrect fdset '%s', expected positive" + "integer"), val); + goto cleanup; + } + + if (fdset->nextfdset <= fdsetnum) + fdset->nextfdset = fdsetnum + 1; + + if (virHashAddEntry(fdset->aliasToFdSet, key, + (void *)(intptr_t)&fdsetnum) < 0) { + virReportOOMError(); + goto cleanup; + } + } + VIR_FREE(key); + VIR_FREE(val); + } + } + + ret = 0; + +cleanup: + if (ret < 0) + virFdSetReset(fdset); + + VIR_FREE(nodes); + VIR_FREE(key); + VIR_FREE(val); + + return ret; +} Index: libvirt/src/util/virfdset.h =================================================================== --- /dev/null +++ libvirt/src/util/virfdset.h @@ -0,0 +1,94 @@ +/* + * virfdset.h: File descriptor set support + * + * 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/>. + * + * Author: Stefan Berger <stefanb@linux.vnet.ibm.com> + */ +#ifndef __VIR_FDSET_H__ +# define __VIR_FDSET_H__ + +# include "internal.h" +# include "virbuffer.h" +# include "virxml.h" +# include "virhash.h" + +typedef struct _virFdSet virFdSet; +typedef virFdSet *virFdSetPtr; + + +/** + * virFdSetNew + * + * Create a new FdSet, + * Returns pointer to new FdSet on success, NULL if no memory was available. + */ +virFdSetPtr virFdSetNew(void) ATTRIBUTE_RETURN_CHECK; + +/** + * virFdSetRemoveAlias + * @fdset: the fdset + * @alias: the alias to remove + * + * Remove the given alias' mapping from @fdset + */ +void virFdSetRemoveAlias(virFdSetPtr fdset, const char *alias) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + + +/** + * virFdSetNextSet + * @fdset: fdset + * @alias: device alias + * @fdsetnum: pointer to unsigned int for storing the file descriptor set id + * + * Get the next file descriptor set number and store it with the given + * @alias. If successful, return the file descriptor set id in @fdsetnum. + * + * Returns 0 on success, -1 on failure. + */ +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + +/** + * virFdSetFormatXML + * @fdset: fdset + * @buf: virBufferPtr for writing into + * + * Write XML representation of the @fdset into the buffer @buf. + */ +void virFdSetFormatXML(virFdSetPtr fdset, virBufferPtr buf) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +/** + * virFdSetParseXML + * @fdset: fdset + * @xPath: xpath expression to find the @fdset's XML nodes + * @ctxt: xpath context + * + * Parse the fdset XML representation and collect the data into @fdset. + * + * Returns 0 on success, -1 on failure. + */ +int virFdSetParseXML(virFdSetPtr fdset, const char *xPath, + xmlXPathContextPtr ctxt) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + +#endif /* __VIR_FDSET_H__ */ Index: libvirt/src/Makefile.am =================================================================== --- libvirt.orig/src/Makefile.am +++ libvirt/src/Makefile.am @@ -79,6 +79,7 @@ UTIL_SOURCES = \ util/virerror.c util/virerror.h \ util/virevent.c util/virevent.h \ util/vireventpoll.c util/vireventpoll.h \ + util/virfdset.c util/virfdset.h \ util/virfile.c util/virfile.h \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -1268,6 +1268,14 @@ virEventPollUpdateHandle; virEventPollUpdateTimeout; +# util/virfdset.h +virFdSetFormatXML; +virFdSetNew; +virFdSetNextSet; +virFdSetParseXML; +virFdSetRemoveAlias; + + # util/virfile.h virFileClose; virFileDirectFdFlag;

On 03/07/2013 11:14 AM, Stefan Berger wrote:
Implement a class for file descriptor sets. This class remembers the mappings of aliases to file descriptor sets and maintains the number of the next-to-use file descriptor set. The class also writes and reads its own XML.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
'make syntax-check' failed: po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -149,6 +149,7 @@ src/util/virerror.c src/util/virerror.h src/util/vireventpoll.c +src/util/virfdset.c src/util/virfile.c src/util/virhash.c src/util/virhook.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch But that fix is trivial, so I've made it locally. Depending on how the rest of the series looks, I might just push without waiting for you to respin a v10.
+int virFdSetParseXML(virFdSetPtr fdset, const char *xPath, + xmlXPathContextPtr ctxt) +{
+ + if (n > 0) { + for (i = 0 ; i < n ; i++) {
No need for the 'if' or the subsequent indentation of the for loop. Again, trivial fix. ACK. If I don't have any major findings on the rest of the series, then I'll go ahead and push the corrected patch without needing to see a v10. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/07/2013 05:00 PM, Eric Blake wrote:
On 03/07/2013 11:14 AM, Stefan Berger wrote:
Implement a class for file descriptor sets. This class remembers the mappings of aliases to file descriptor sets and maintains the number of the next-to-use file descriptor set. The class also writes and reads its own XML.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
'make syntax-check' failed:
po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -149,6 +149,7 @@ src/util/virerror.c src/util/virerror.h src/util/vireventpoll.c +src/util/virfdset.c src/util/virfile.c src/util/virhash.c src/util/virhook.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch
Also: require_whitespace_in_translation src/util/virfdset.c:148: _("incorrect fdset '%s', expected positive""integer"), val); maint.mk: missing whitespace at line split
ACK. If I don't have any major findings on the rest of the series, then I'll go ahead and push the corrected patch without needing to see a v10.
Still applies. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/07/2013 07:03 PM, Eric Blake wrote:
On 03/07/2013 05:00 PM, Eric Blake wrote:
On 03/07/2013 11:14 AM, Stefan Berger wrote:
Implement a class for file descriptor sets. This class remembers the mappings of aliases to file descriptor sets and maintains the number of the next-to-use file descriptor set. The class also writes and reads its own XML.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- 'make syntax-check' failed:
po_check --- ./po/POTFILES.in +++ ./po/POTFILES.in @@ -149,6 +149,7 @@ src/util/virerror.c src/util/virerror.h src/util/vireventpoll.c +src/util/virfdset.c src/util/virfile.c src/util/virhash.c src/util/virhook.c maint.mk: you have changed the set of files with translatable diagnostics; apply the above patch Also:
require_whitespace_in_translation src/util/virfdset.c:148: _("incorrect fdset '%s', expected positive""integer"), val); maint.mk: missing whitespace at line split
I didn't see those because I didn't have the patches maintained by git. Stefan

Extend the QEMU private domain structure with virFdSet. Persist the virFdSet using XML and parse its XML. Free the FdSet upon domain stop. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- v6->v7: - followed Eric's comments on v6 v5->v6: - change found in patch 3 moved to this patch --- src/qemu/qemu_domain.c | 11 +++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 3 +++ 3 files changed, 17 insertions(+) Index: libvirt/src/qemu/qemu_domain.c =================================================================== --- libvirt.orig/src/qemu/qemu_domain.c +++ libvirt/src/qemu/qemu_domain.c @@ -252,6 +252,7 @@ static void qemuDomainObjPrivateFree(voi qemuAgentClose(priv->agent); } VIR_FREE(priv->cleanupCallbacks); + virObjectUnref(priv->fdset); VIR_FREE(priv); } @@ -326,6 +327,10 @@ static int qemuDomainObjPrivateXMLFormat if (priv->fakeReboot) virBufferAsprintf(buf, " <fakereboot/>\n"); + virBufferAdjustIndent(buf, 2); + virFdSetFormatXML(priv->fdset, buf); + virBufferAdjustIndent(buf, -2); + return 0; } @@ -471,6 +476,10 @@ static int qemuDomainObjPrivateXMLParse( priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; + if (!(priv->fdset = virFdSetNew()) || + virFdSetParseXML(priv->fdset, "./fdset/entry", ctxt) < 0) + goto error; + return 0; error: @@ -478,6 +487,8 @@ error: priv->monConfig = NULL; VIR_FREE(nodes); virObjectUnref(qemuCaps); + virObjectUnref(priv->fdset); + priv->fdset = NULL; return -1; } 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 "virfdset.h" # define QEMU_EXPECTED_VIRT_TYPES \ ((1 << VIR_DOMAIN_VIRT_QEMU) | \ @@ -162,6 +163,8 @@ struct _qemuDomainObjPrivate { qemuDomainCleanupCallback *cleanupCallbacks; size_t ncleanupCallbacks; size_t ncleanupCallbacks_max; + + virFdSetPtr fdset; }; struct qemuDomainWatchdogEvent Index: libvirt/src/qemu/qemu_process.c =================================================================== --- libvirt.orig/src/qemu/qemu_process.c +++ libvirt/src/qemu/qemu_process.c @@ -4224,6 +4224,9 @@ void qemuProcessStop(virQEMUDriverPtr dr priv->monConfig = NULL; } + virObjectUnref(priv->fdset); + priv->fdset = NULL; + /* shut it off for sure */ ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE|

On 03/07/2013 11:14 AM, Stefan Berger wrote:
Extend the QEMU private domain structure with virFdSet. Persist the virFdSet using XML and parse its XML. Free the FdSet upon domain stop.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
Index: libvirt/src/qemu/qemu_domain.c =================================================================== --- libvirt.orig/src/qemu/qemu_domain.c +++ libvirt/src/qemu/qemu_domain.c @@ -252,6 +252,7 @@ static void qemuDomainObjPrivateFree(voi qemuAgentClose(priv->agent); } VIR_FREE(priv->cleanupCallbacks); + virObjectUnref(priv->fdset); VIR_FREE(priv); }
@@ -326,6 +327,10 @@ static int qemuDomainObjPrivateXMLFormat if (priv->fakeReboot) virBufferAsprintf(buf, " <fakereboot/>\n");
+ virBufferAdjustIndent(buf, 2); + virFdSetFormatXML(priv->fdset, buf);
Oops - potential NULL deref - virFdSetParseXML requires a non-null arg, but you haven't yet added in the code that guarantees that priv->fdset exists on a running domain (and for an offline domain, you are intentionally leaving priv->fdset NULL). Still, I can fix it, so ACK, and no need to send a v10, depending on how 3/3 fares: diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index cfd4b8e..22c7ade 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -327,9 +327,11 @@ static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) if (priv->fakeReboot) virBufferAsprintf(buf, " <fakereboot/>\n"); - virBufferAdjustIndent(buf, 2); - virFdSetFormatXML(priv->fdset, buf); - virBufferAdjustIndent(buf, -2); + if (priv->fdset) { + virBufferAdjustIndent(buf, 2); + virFdSetFormatXML(priv->fdset, buf); + virBufferAdjustIndent(buf, -2); + } return 0; } -- 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> libvirt now creates the following parts for the QEMU command line: old: -drive file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw new: -add-fd set=1,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img -add-fd set=1,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img -drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0,format=raw old: -chardev tty,id=charserial0,path=/dev/ttyS0 new: -add-fd set=2,fd=30,opaque=/dev/ttyS0 -chardev tty,id=charserial0,path=/dev/fdset/1 old: -chardev pipe,id=charserial1,path=/tmp/testpipe new: -add-fd set=3,fd=32,opaque=/tmp/testpipe -chardev pipe,id=charserial1,path=/dev/fdset/2 Test cases are part of this patch now. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- v8->v9: - adapted to recent changes in qemu_command.c v7->v8: - address some of the comments v6->v7: - followed Corey's comments on v6 v4->v5: - removed one test case testing for 'old' command line v3->v4: - Adapt to changes in patch 1 - better handling of error case on the hotplug code v2->v3: - Use an unsigned int for remembering the next-to-use fdset v1->v2: - Addressed many of Eric's comment; many changes, though - virBitmap holds used file descriptor sets; it's attached to QEMU private domain structure - persisting and parsing the fdset in the virDomainDeviceInfo XML - rebuilding the fdset bitmap upon libvirt start and after parsing the virDomainDeviceInfo XML --- src/qemu/qemu_command.c | 269 +++++++++++++++++++----- src/qemu/qemu_command.h | 14 - src/qemu/qemu_driver.c | 5 src/qemu/qemu_driver.h | 4 src/qemu/qemu_hotplug.c | 15 + src/qemu/qemu_process.c | 6 tests/qemuxml2argvdata/qemuxml2argv-add-fd.args | 23 ++ tests/qemuxml2argvdata/qemuxml2argv-add-fd.xml | 36 +++ tests/qemuxml2argvtest.c | 11 tests/qemuxmlnstest.c | 8 10 files changed, 331 insertions(+), 60 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 "qemu_driver.h" #include <sys/stat.h> #include <fcntl.h> @@ -133,6 +134,70 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO /** + * qemuCommandPrintAddFd: + * @fdset: the number of the file descriptor set to which @fd belongs + * @fd: fd that will be assigned to QEMU + * @open_flags: the flags used for opening the fd; of interest are only + * O_RDONLY, O_WRONLY, O_RDWR + * @name: optional name of the file + * + * Write the parameters for the QEMU -add-fd command line option + * for the given file descriptor and return the string. + * This function for example returns "set=10,fd=20,opaque=RDWR:/foo/bar". + */ +static char * +qemuCommandPrintAddFd(int fdset, int fd, int open_flags, const char *name) +{ + const char *mode = ""; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (name) { + switch ((open_flags & O_ACCMODE)) { + case O_RDONLY: + mode = "RDONLY:"; + break; + case O_WRONLY: + mode = "WRONLY:"; + break; + case O_RDWR: + mode = "RDWR:"; + break; + } + } + + virBufferAsprintf(&buf, "set=%d,fd=%d", fdset, fd); + if (name) { + virBufferAsprintf(&buf, ",opaque=%s", mode); + virBufferEscape(&buf, ',', ",", "%s", name); + } + + if (virBufferError(&buf)) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +/** + * qemuCommandPrintDevFdSet: + * @buf: buffer to write the file descriptor set string + * @fdset: the file descriptor set + * + * Get the parameters for the QEMU path= parameter where a file + * descriptor is accessed via a file descriptor set, for example + * /dev/fdset/10. The file descriptor must previously have been + * 'transferred' in a virCommandTransfer() call. + */ +static void +qemuCommandPrintDevFdSet(virBufferPtr buf, int fdset) +{ + virBufferAsprintf(buf, "/dev/fdset/%d", fdset); +} + + +/** * qemuPhysIfaceConnect: * @def: the definition of the VM (needed by 802.1Qbh and audit) * @driver: pointer to the driver instance @@ -2200,11 +2265,70 @@ no_memory: goto cleanup; } +static int +qemuCreatePathForFilePath(virQEMUDriverPtr driver, virBufferPtr buf, + const char *prefix, const char *path, + const int *open_flags, + virCommandPtr cmd, virFdSetPtr fdset, + const virDomainDeviceInfoPtr info, + virQEMUCapsPtr qemuCaps) +{ + char *addfdstr = NULL; + int i; + + virBufferAdd(buf, prefix, -1); + /* + * cmd == NULL hints at hotplugging; use old way of doing things + * fdset == NULL hints at a call path where we should not open files + * In this case we fall back to the old command line + * (at least for now) + */ + if (!cmd || !fdset || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ADD_FD)) { + virBufferEscape(buf, ',', ",", "%s", path); + } else { + unsigned int fdsetnum; + + if (virFdSetNextSet(fdset, info->alias, &fdsetnum) < 0) + goto error; + + qemuCommandPrintDevFdSet(buf, fdsetnum); + + i = 0; + while (open_flags[i] != -1) { + int fd = qemuOpenFile(driver, path, open_flags[i], NULL, NULL); + if (fd < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not open %s"), path); + goto error; + } + virCommandTransferFD(cmd, fd); + + addfdstr = qemuCommandPrintAddFd(fdsetnum, fd, open_flags[i], + path); + if (!addfdstr) + goto error; + virCommandAddArgList(cmd, "-add-fd", addfdstr, NULL); + VIR_FREE(addfdstr); + + i++; + } + } + + return 0; + +error: + virFdSetRemoveAlias(fdset, info->alias); + return -1; +} + char * qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk, bool bootable, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd, + virQEMUDriverPtr driver, + virFdSetPtr fdset) { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); @@ -2366,7 +2490,13 @@ qemuBuildDriveStr(virConnectPtr conn ATT "block type disk")); goto error; } - virBufferEscape(&opt, ',', ",", "file=%s,", disk->src); + if (qemuCreatePathForFilePath(driver, &opt, + "file=", disk->src, + (int[]){O_RDONLY, O_RDWR, -1}, + cmd, fdset, &disk->info, + qemuCaps) < 0) + goto error; + virBufferAddChar(&opt, ','); } } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) @@ -2863,7 +2993,10 @@ error: char *qemuBuildFSStr(virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED) + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, + virCommandPtr cmd, + virQEMUDriverPtr qemu_driver, + virFdSetPtr fdset) { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); @@ -2912,7 +3045,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr f } virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); - virBufferAsprintf(&opt, ",path=%s", fs->src); + if (qemuCreatePathForFilePath(qemu_driver, &opt, + ",path=", fs->src, (int[]){O_RDWR, -1}, + cmd, fdset, &fs->info, qemuCaps) < 0) + goto error; if (fs->readonly) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_READONLY)) { @@ -3870,7 +4006,10 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHo * host side of the character device */ static char * qemuBuildChrChardevStr(virDomainChrSourceDefPtr dev, const char *alias, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, virCommandPtr cmd, + virQEMUDriverPtr driver, + virFdSetPtr fdset, + const virDomainDeviceInfoPtr info) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; @@ -3889,19 +4028,32 @@ qemuBuildChrChardevStr(virDomainChrSourc break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferAsprintf(&buf, "%s,id=char%s,path=%s", + virBufferAsprintf(&buf, "%s,id=char%s", STRPREFIX(alias, "parallel") ? "parport" : "tty", - alias, dev->data.file.path); + alias); + if (qemuCreatePathForFilePath(driver, &buf, + ",path=", dev->data.file.path, + (int[]){O_RDWR, -1}, cmd, fdset, + info, qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CHR_TYPE_FILE: - virBufferAsprintf(&buf, "file,id=char%s,path=%s", alias, - dev->data.file.path); + virBufferAsprintf(&buf, "file,id=char%s", alias); + if (qemuCreatePathForFilePath(driver, &buf, + ",path=", dev->data.file.path, + (int[]){O_RDWR, -1}, cmd, fdset, + info, qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferAsprintf(&buf, "pipe,id=char%s,path=%s", alias, - dev->data.file.path); + virBufferAsprintf(&buf, "pipe,id=char%s",alias); + if (qemuCreatePathForFilePath(driver, &buf, + ",path=", dev->data.file.path, + (int[]){O_RDWR, -1}, cmd, + fdset, info, qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CHR_TYPE_STDIO: @@ -4181,7 +4333,9 @@ error: static int qemuBuildRNGBackendArgs(virCommandPtr cmd, virDomainRNGDefPtr dev, - virQEMUCapsPtr qemuCaps) + virQEMUCapsPtr qemuCaps, + virQEMUDriverPtr driver, + virFdSetPtr fdset) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *backend = NULL; @@ -4197,8 +4351,21 @@ qemuBuildRNGBackendArgs(virCommandPtr cm } virBufferAsprintf(&buf, "rng-random,id=%s", dev->info.alias); - if (dev->source.file) - virBufferAsprintf(&buf, ",filename=%s", dev->source.file); + if (dev->source.file) { + /* + * QEMU 1.4 does not use qemu_open() for accessing the rng file, + * so we will only later be able to use fdsets for this + * device. For now use a NULL pointer for fdset to avoid + * having an fdset used. + */ + virFdSetPtr tmp_fdset = NULL; + if (qemuCreatePathForFilePath(driver, &buf, + ",filename=", dev->source.file, + (int[]){O_RDONLY|O_NONBLOCK, -1}, + cmd, tmp_fdset, &dev->info, + qemuCaps) < 0) + goto cleanup; + } virCommandAddArg(cmd, "-object"); virCommandAddArgBuffer(cmd, &buf); @@ -4214,7 +4381,8 @@ qemuBuildRNGBackendArgs(virCommandPtr cm } if (!(backend = qemuBuildChrChardevStr(dev->source.chardev, - dev->info.alias, qemuCaps))) + dev->info.alias, qemuCaps, + cmd, driver, fdset, NULL))) goto cleanup; virCommandAddArgList(cmd, "-chardev", backend, NULL); @@ -5150,7 +5318,8 @@ qemuBuildCommandLine(virConnectPtr conn, const char *migrateFrom, int migrateFd, virDomainSnapshotObjPtr snapshot, - enum virNetDevVPortProfileOp vmop) + enum virNetDevVPortProfileOp vmop, + virFdSetPtr fdset) { int i, j; int disableKQEMU = 0; @@ -5445,11 +5614,11 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev if it's available */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV)) { - virCommandAddArg(cmd, "-chardev"); if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor", - qemuCaps))) + qemuCaps, cmd, driver, fdset, + NULL))) goto error; - virCommandAddArg(cmd, chrdev); + virCommandAddArgList(cmd, "-chardev", chrdev, NULL); VIR_FREE(chrdev); virCommandAddArg(cmd, "-mon"); @@ -5896,8 +6065,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 @@ -5913,12 +6080,12 @@ qemuBuildCommandLine(virConnectPtr conn, } optstr = qemuBuildDriveStr(conn, disk, emitBootindex ? false : !!bootindex, - qemuCaps); + qemuCaps, cmd, driver, fdset); if (deviceFlagMasked) virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE); if (!optstr) goto error; - virCommandAddArg(cmd, optstr); + virCommandAddArgList(cmd, "-drive", optstr, NULL); VIR_FREE(optstr); if (!emitBootindex) @@ -6094,7 +6261,7 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainFSDefPtr fs = def->fss[i]; virCommandAddArg(cmd, "-fsdev"); - if (!(optstr = qemuBuildFSStr(fs, qemuCaps))) + if (!(optstr = qemuBuildFSStr(fs, qemuCaps, cmd, driver, fdset))) goto error; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); @@ -6375,14 +6542,14 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&smartcard->data.passthru, smartcard->info.alias, - qemuCaps))) { + qemuCaps, cmd, driver, fdset, + &smartcard->info))) { virBufferFreeAndReset(&opt); goto error; } - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); virBufferAsprintf(&opt, "ccid-card-passthru,chardev=char%s", @@ -6413,12 +6580,13 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&serial->source, serial->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, + fdset, + &serial->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); @@ -6450,12 +6618,13 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(¶llel->source, parallel->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, + fdset, + ¶llel->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); @@ -6487,12 +6656,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&channel->source, channel->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, fdset, + &channel->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); addr = virSocketAddrFormat(channel->target.addr); @@ -6522,12 +6691,13 @@ qemuBuildCommandLine(virConnectPtr conn, * the newer -chardev interface. */ ; } else { - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&channel->source, channel->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, + fdset, + &channel->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); } @@ -6560,12 +6730,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&console->source, console->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, fdset, + &console->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); @@ -6582,12 +6752,12 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&console->source, console->info.alias, - qemuCaps))) + qemuCaps, cmd, driver, fdset, + &console->info))) goto error; - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); @@ -6924,14 +7094,14 @@ qemuBuildCommandLine(virConnectPtr conn, virDomainRedirdevDefPtr redirdev = def->redirdevs[i]; char *devstr; - virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&redirdev->source.chr, redirdev->info.alias, - qemuCaps))) { + qemuCaps, cmd, driver, fdset, + &redirdev->info))) { goto error; } - virCommandAddArg(cmd, devstr); + virCommandAddArgList(cmd, "-chardev", devstr, NULL); VIR_FREE(devstr); if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) @@ -7127,7 +7297,8 @@ qemuBuildCommandLine(virConnectPtr conn, if (def->rng) { /* add the RNG source backend */ - if (qemuBuildRNGBackendArgs(cmd, def->rng, qemuCaps) < 0) + if (qemuBuildRNGBackendArgs(cmd, def->rng, qemuCaps, driver, + fdset) < 0) goto error; /* add the device */ Index: libvirt/src/qemu/qemu_command.h =================================================================== --- libvirt.orig/src/qemu/qemu_command.h +++ libvirt/src/qemu/qemu_command.h @@ -58,7 +58,8 @@ virCommandPtr qemuBuildCommandLine(virCo const char *migrateFrom, int migrateFd, virDomainSnapshotObjPtr current_snapshot, - enum virNetDevVPortProfileOp vmop) + enum virNetDevVPortProfileOp vmop, + virFdSetPtr fdset) ATTRIBUTE_NONNULL(1); /* Generate string for arch-specific '-device' parameter */ @@ -95,9 +96,16 @@ char *qemuDeviceDriveHostAlias(virDomain char *qemuBuildDriveStr(virConnectPtr conn, virDomainDiskDefPtr disk, bool bootable, - virQEMUCapsPtr qemuCaps); + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd, + virQEMUDriverPtr driver, + virFdSetPtr fdset); + char *qemuBuildFSStr(virDomainFSDefPtr fs, - virQEMUCapsPtr qemuCaps); + virQEMUCapsPtr qemuCaps, + virCommandPtr cmd, + virQEMUDriverPtr driver, + virFdSetPtr 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 @@ -2540,7 +2540,7 @@ qemuCompressGetCommand(virQEMUSaveFormat /* Internal function to properly create or open existing files, with * ownership affected by qemu driver setup. */ -static int +int qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver) { @@ -5359,7 +5359,8 @@ static char *qemuDomainXMLToNative(virCo if (!(cmd = qemuBuildCommandLine(conn, driver, def, &monConfig, monitor_json, qemuCaps, - NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP))) + NULL, -1, NULL, VIR_NETDEV_VPORT_PROFILE_OP_NO_OP, + NULL))) goto cleanup; ret = virCommandToString(cmd); Index: libvirt/src/qemu/qemu_hotplug.c =================================================================== --- libvirt.orig/src/qemu/qemu_hotplug.c +++ libvirt/src/qemu/qemu_hotplug.c @@ -247,7 +247,8 @@ int qemuDomainAttachPciDiskDevice(virCon if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps, + NULL, driver, priv->fdset))) goto error; if (!(devstr = qemuBuildDriveDevStr(NULL, disk, 0, priv->qemuCaps))) @@ -304,6 +305,8 @@ cleanup: return ret; error: + virFdSetRemoveAlias(priv->fdset, disk->info.alias); + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) && (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && releaseaddr && @@ -487,7 +490,8 @@ int qemuDomainAttachSCSIDisk(virConnectP goto error; } - if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps, + NULL, driver, priv->fdset))) goto error; for (i = 0 ; i <= disk->info.addr.drive.controller ; i++) { @@ -554,6 +558,8 @@ cleanup: return ret; error: + virFdSetRemoveAlias(priv->fdset, disk->info.alias); + if (virSecurityManagerRestoreImageLabel(driver->securityManager, vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", disk->src); @@ -605,7 +611,8 @@ int qemuDomainAttachUsbMassstorageDevice if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; - if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) + if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps, + NULL, driver, priv->fdset))) goto error; if (!(devstr = qemuBuildDriveDevStr(NULL, disk, 0, priv->qemuCaps))) goto error; @@ -647,6 +654,8 @@ cleanup: return ret; error: + virFdSetRemoveAlias(priv->fdset, disk->info.alias); + if (virSecurityManagerRestoreImageLabel(driver->securityManager, vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", disk->src); Index: libvirt/src/qemu/qemu_process.c =================================================================== --- libvirt.orig/src/qemu/qemu_process.c +++ libvirt/src/qemu/qemu_process.c @@ -3742,6 +3742,9 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } + if (!(priv->fdset = virFdSetNew())) + goto cleanup; + VIR_DEBUG("Preparing monitor state"); if (qemuProcessPrepareMonitorChr(cfg, priv->monConfig, vm->def->name) < 0) goto cleanup; @@ -3786,7 +3789,8 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, - 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/src/qemu/qemu_driver.h =================================================================== --- libvirt.orig/src/qemu/qemu_driver.h +++ libvirt/src/qemu/qemu_driver.h @@ -24,6 +24,10 @@ #ifndef __QEMU_DRIVER_H__ # define __QEMU_DRIVER_H__ +# include "qemu_conf.h" + int qemuRegister(void); +int qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver); #endif /* __QEMU_DRIVER_H__ */ Index: libvirt/tests/qemuxml2argvtest.c =================================================================== --- libvirt.orig/tests/qemuxml2argvtest.c +++ libvirt/tests/qemuxml2argvtest.c @@ -13,6 +13,7 @@ # include "internal.h" # include "testutils.h" # include "viralloc.h" +# include "virfdset.h" # include "qemu/qemu_capabilities.h" # include "qemu/qemu_command.h" # include "qemu/qemu_domain.h" @@ -95,9 +96,12 @@ static int testCompareXMLToArgvFiles(con virConnectPtr conn; char *log = NULL; virCommandPtr cmd = NULL; + virFdSetPtr fdset; if (!(conn = virGetConnect())) goto out; + if (!(fdset = virFdSetNew())) + goto out; conn->secretDriver = &fakeSecretDriver; if (!(vmdef = virDomainDefParseFile(driver.caps, xml, @@ -154,7 +158,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(); @@ -201,6 +206,7 @@ out: virCommandFree(cmd); virDomainDefFree(vmdef); virObjectUnref(conn); + virObjectUnref(fdset); return ret; } @@ -889,6 +895,9 @@ mymain(void) DO_TEST("virtio-rng-egd", QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_EGD); + DO_TEST("add-fd", QEMU_CAPS_ADD_FD, + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE); + virObjectUnref(driver.config); virObjectUnref(driver.caps); VIR_FREE(map); Index: libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.args =================================================================== --- /dev/null +++ libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.args @@ -0,0 +1,23 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +\ +-add-fd set=1,fd=3,opaque=RDONLY:/dev/null \ +-add-fd set=1,fd=4,opaque=RDWR:/dev/null \ +-drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +\ +-add-fd set=2,fd=5,opaque=RDWR:/dev/null \ +-chardev tty,id=charserial0,path=/dev/fdset/2 \ +-device isa-serial,chardev=charserial0,id=serial0 \ +\ +-add-fd set=3,fd=6,opaque=RDWR:/dev/null \ +-chardev pipe,id=charserial1,path=/dev/fdset/3 \ +-device isa-serial,chardev=charserial1,id=serial1 \ +\ +-add-fd set=4,fd=7,opaque=RDWR:/dev/null \ +-chardev parport,id=charparallel0,path=/dev/fdset/4 \ +-device isa-parallel,chardev=charparallel0,id=parallel0 \ +\ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 Index: libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.xml =================================================================== --- /dev/null +++ libvirt/tests/qemuxml2argvdata/qemuxml2argv-add-fd.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/dev/null'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <serial type='dev'> + <source path='/dev/null'/> + <target port='0'/> + </serial> + <serial type='pipe'> + <source path='/dev/null'/> + <target port='1'/> + </serial> + <parallel type='dev'> + <source path='/dev/null'/> + <target port='0'/> + </parallel> + <memballoon model='virtio'/> + </devices> +</domain> Index: libvirt/tests/qemuxmlnstest.c =================================================================== --- libvirt.orig/tests/qemuxmlnstest.c +++ libvirt/tests/qemuxmlnstest.c @@ -41,10 +41,14 @@ static int testCompareXMLToArgvFiles(con char *log = NULL; char *emulator = NULL; virCommandPtr cmd = NULL; + virFdSetPtr fdset; if (!(conn = virGetConnect())) goto fail; + if (!(fdset = virFdSetNew())) + goto fail; + len = virtTestLoadFile(cmdline, &expectargv); if (len < 0) goto fail; @@ -110,7 +114,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) { @@ -150,6 +155,7 @@ static int testCompareXMLToArgvFiles(con VIR_FREE(actualargv); virCommandFree(cmd); virDomainDefFree(vmdef); + virObjectUnref(fdset); virObjectUnref(conn); return ret; }

On 03/07/2013 11:14 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>
Discussion on the qemu list has made it obvious that we want this for NFS-mounted images, but maybe not for local images or on other file systems that actually support SELinux labeling (after all, the point of fd passing is not to move DAC checking out of the kernel and into user-space libvirtd, but to make up for lack of SELinux labeling on NFS). Still, we are waiting for a qemu solution on how to do fd passing for backing files (the so-called -blockdev design), which means that for now, the best we could do with the selinux bool virt_use_nfs disabled is support only flat images (no backing file, no creation of snapshots).
<serial type='dev'> <source path='/dev/ttyS0'/> <target port='0'/> </serial> <serial type='pipe'> <source path='/tmp/testpipe'/> <target port='1'/> </serial>
And here, these files support SELinux labeling, so maybe fd passing is overkill, other than proof of concept that we are doing fd passing correctly. So, I'm debating on how much of this patch needs to be applied, or whether we should split it into smaller chunks to ease backporting of some portions to older libvirt without dragging in everything.
libvirt now creates the following parts for the QEMU command line:
old: -drive file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw new: -add-fd set=1,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img -add-fd set=1,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img -drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0,format=raw
old: -chardev tty,id=charserial0,path=/dev/ttyS0 new: -add-fd set=2,fd=30,opaque=/dev/ttyS0 -chardev tty,id=charserial0,path=/dev/fdset/1
old: -chardev pipe,id=charserial1,path=/tmp/testpipe new: -add-fd set=3,fd=32,opaque=/tmp/testpipe -chardev pipe,id=charserial1,path=/dev/fdset/2
Still, this looks interesting, and the framework it sets up looks like it will be reusable as we start thinking about fd passing and implications on hotplug.
Test cases are part of this patch now.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- v8->v9: - adapted to recent changes in qemu_command.c
I'm still working on my reply to the actual contents... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/11/2013 05:36 PM, Eric Blake wrote:
On 03/07/2013 11:14 AM, Stefan Berger wrote:
<serial type='dev'> <source path='/dev/ttyS0'/> <target port='0'/> </serial> <serial type='pipe'> <source path='/tmp/testpipe'/> <target port='1'/> </serial>
And here, these files support SELinux labeling, so maybe fd passing is overkill, other than proof of concept that we are doing fd passing correctly. So, I'm debating on how much of this patch needs to be applied, or whether we should split it into smaller chunks to ease backporting of some portions to older libvirt without dragging in everything.
I misinterpreted your fd-passing related comments on TPM support for QEMU and thought that this is where you wanted to move in general also thinking that seccomp support for eliminating open() must be one goal. Actually, while I wrote this patch I also had a part that passed the monitor via fd to QEMU, but obviously there is no support for this. This could possibly eliminate the socket() call from QEMU. Knocking out open and socket syscalls would then become dependent on which devices are used by QEMU ( I suppose some devices still require open to be called in the path somewhere ), thus making this configuration-dependent and likely difficult to test. I guess the use-case where no SELinux support is available is weak or non-existent so that seccomp would need to be used. Stefan

On 03/12/2013 05:21 AM, Stefan Berger wrote:
And here, these files support SELinux labeling, so maybe fd passing is overkill, other than proof of concept that we are doing fd passing correctly. So, I'm debating on how much of this patch needs to be applied, or whether we should split it into smaller chunks to ease backporting of some portions to older libvirt without dragging in everything.
I misinterpreted your fd-passing related comments on TPM support for QEMU and thought that this is where you wanted to move in general also thinking that seccomp support for eliminating open() must be one goal.
I think my interpretation of the end goal changed in between my comments on v1 and now on v9. That is, on the qemu list, it was pointed out to me that we only NEED fd passing for NFS files; using fd passing everywhere might be easier to code, but the only place where fd passing adds security is with NFS files.
Actually, while I wrote this patch I also had a part that passed the monitor via fd to QEMU, but obviously there is no support for this. This could possibly eliminate the socket() call from QEMU. Knocking out open and socket syscalls would then become dependent on which devices are used by QEMU ( I suppose some devices still require open to be called in the path somewhere ), thus making this configuration-dependent and likely difficult to test. I guess the use-case where no SELinux support is available is weak or non-existent so that seccomp would need to be used.
I don't know if seccomp in isolation is reasonable - SELinux can block open() for NFS while allowing it for saner file systems that allow selinux labeling, and seems like it is a tractable problem to determine which files SELinux would block; whereas seccomp blindly blocking all open() might be too much hassle to ever decently support. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Mar 11, 2013 at 03:36:56PM -0600, Eric Blake wrote:
On 03/07/2013 11:14 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>
Discussion on the qemu list has made it obvious that we want this for NFS-mounted images, but maybe not for local images or on other file systems that actually support SELinux labeling (after all, the point of fd passing is not to move DAC checking out of the kernel and into user-space libvirtd, but to make up for lack of SELinux labeling on NFS). Still, we are waiting for a qemu solution on how to do fd passing for backing files (the so-called -blockdev design), which means that for now, the best we could do with the selinux bool virt_use_nfs disabled is support only flat images (no backing file, no creation of snapshots).
If the fd passing code doesn't work for backing files, then IMHO we should not apply this at all. We need a fully working solution or none at all. A half-implemented solution will just cause pain for everyone involved I also agree that we should *NOT* use FD passing, unless we absolutely need it. ie we should only be using with NFS, when virt_use_nfs is not enabled. Every place where we use FD passing makes it harder for people to take the ARGV from /var/log/libvirt/qemu/$GUEST.log and run it directly. This is quite important from a support POV. Regards, 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 :|
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Stefan Berger