[libvirt] [PATCH V6 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
Rather than passing the next-to-use file descriptor set Id and the hash table for rembering the mappings of aliases to file descriptor sets around, encapsulate the two in a class. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- 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 | 10 ++ src/util/virfdset.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfdset.h | 112 +++++++++++++++++++++++++ 4 files changed, 328 insertions(+) Index: libvirt/src/util/virfdset.c =================================================================== --- /dev/null +++ libvirt/src/util/virfdset.c @@ -0,0 +1,205 @@ +/* + * 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); +} + +static void virFdSetFreeIntPtr(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(payload); +} + +virFdSetPtr virFdSetNew(void) +{ + virFdSetPtr fdset; + + if (virFdSetInitialize() < 0) + return NULL; + + if (!(fdset = virObjectNew(virFdSetClass))) + return NULL; + + if (!(fdset->aliasToFdSet = virHashCreate(10, virFdSetFreeIntPtr))) { + virObjectUnref(fdset); + return NULL; + } + fdset->nextfdset = 1; + + return fdset; +} + +void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +} + +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) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + } + + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) { + VIR_FREE(num); + 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 *)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 = NULL; + int ret = 0; + + virFdSetReset(fdset); + + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (VIR_ALLOC(fdsetnum) < 0) { + virReportOOMError(); + ret = -1; + goto error; + } + if (virStrToLong_ui(val, NULL, 10, fdsetnum) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("incorrect fdset '%s', expected positive" + "integer"), val); + VIR_FREE(fdsetnum); + ret = -1; + goto error; + } + + if (fdset->nextfdset <= *fdsetnum) + fdset->nextfdset = *fdsetnum + 1; + + if (virHashAddEntry(fdset->aliasToFdSet, key, fdsetnum) < 0) { + virReportOOMError(); + VIR_FREE(fdset); + ret = -1; + goto error; + } + } + VIR_FREE(key); + VIR_FREE(val); + } + } + +error: + 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,112 @@ +/* + * 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 __FDSET_H__ +# define __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; + +/** + * virFdSetFree + * @fdset : the FdSet to free + * + * Free the FdSet + */ +void virFdSetFree(virFdSetPtr fdset); + +/** + * virFdSetReset + * @fdset: fdset + * + * Reset the @fdset and forget about all mappings + * of aliases to file descriptor set data + */ +void virFdSetReset(virFdSetPtr fdset) + ATTRIBUTE_NONNULL(1); + +/** + * 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 + * @fdset: 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 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 @@ -650,6 +650,16 @@ virFDStreamOpen; virFDStreamOpenFile; +# virfdset.h +virFdSetFormatXML; +virFdSetFree; +virFdSetNew; +virFdSetNextSet; +virFdSetParseXML; +virFdSetRemoveAlias; +virFdSetReset; + + # hash.h virHashAddEntry; virHashCreate;
On 02/14/2013 07:00 AM, Stefan Berger wrote:
Rather than passing the next-to-use file descriptor set Id and the hash table for rembering the mappings of aliases to
Remembering is missing an e
file descriptor sets around, encapsulate the two in a class.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- 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 | 10 ++ src/util/virfdset.c | 205 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfdset.h | 112 +++++++++++++++++++++++++ 4 files changed, 328 insertions(+)
Index: libvirt/src/util/virfdset.c =================================================================== --- /dev/null +++ libvirt/src/util/virfdset.c @@ -0,0 +1,205 @@ +/* + * 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); +} + +static void virFdSetFreeIntPtr(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + VIR_FREE(payload); +} + +virFdSetPtr virFdSetNew(void) +{ + virFdSetPtr fdset; + + if (virFdSetInitialize() < 0) + return NULL; + + if (!(fdset = virObjectNew(virFdSetClass))) + return NULL; + + if (!(fdset->aliasToFdSet = virHashCreate(10, virFdSetFreeIntPtr))) { + virObjectUnref(fdset); + return NULL; + } + fdset->nextfdset = 1; + + return fdset; +} + +void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +} + +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) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + } + + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) { + VIR_FREE(num); + 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 *)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 = NULL; + int ret = 0; + + virFdSetReset(fdset); + + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (VIR_ALLOC(fdsetnum) < 0) { + virReportOOMError(); + ret = -1; + goto error; + } + if (virStrToLong_ui(val, NULL, 10, fdsetnum) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("incorrect fdset '%s', expected positive" + "integer"), val); + VIR_FREE(fdsetnum); + ret = -1; + goto error; + } + + if (fdset->nextfdset <= *fdsetnum) + fdset->nextfdset = *fdsetnum + 1; + + if (virHashAddEntry(fdset->aliasToFdSet, key, fdsetnum) < 0) { + virReportOOMError(); + VIR_FREE(fdset); + ret = -1; + goto error; + } + } + VIR_FREE(key); + VIR_FREE(val); + } + } + +error: + 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,112 @@ +/* + * 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 __FDSET_H__ +# define __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; + +/** + * virFdSetFree + * @fdset : the FdSet to free + * + * Free the FdSet + */ +void virFdSetFree(virFdSetPtr fdset); + +/** + * virFdSetReset + * @fdset: fdset + * + * Reset the @fdset and forget about all mappings + * of aliases to file descriptor set data + */ +void virFdSetReset(virFdSetPtr fdset) + ATTRIBUTE_NONNULL(1); + +/** + * 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 + * @fdset: pointer to unsigned int for storing the file descriptor set id
This should be @fdsetnum -- Regards, Corey Bryant
+int virFdSetParseXML(virFdSetPtr fdset, const char *xPath, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr *nodes = NULL; + int n, i; + char *key = NULL; + char *val = NULL; + unsigned int *fdsetnum = NULL; + int ret = 0; + + virFdSetReset(fdset); + + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (VIR_ALLOC(fdsetnum) < 0) { + virReportOOMError(); + ret = -1; + goto error; + } + if (virStrToLong_ui(val, NULL, 10, fdsetnum) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("incorrect fdset '%s', expected positive" + "integer"), val); + VIR_FREE(fdsetnum); + ret = -1; + goto error; + } + + if (fdset->nextfdset <= *fdsetnum) + fdset->nextfdset = *fdsetnum + 1; + + if (virHashAddEntry(fdset->aliasToFdSet, key, fdsetnum) < 0) { + virReportOOMError(); + VIR_FREE(fdset);
Is this supposed to be freeing fdsetnum? -- Regards, Corey Bryant
On 02/14/2013 05:00 AM, Stefan Berger wrote:
Rather than passing the next-to-use file descriptor set Id and the hash table for rembering the mappings of aliases to file descriptor sets around, encapsulate the two in a class.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
In addition to Corey's review,
Index: libvirt/src/util/virfdset.c ===================================================================
+ +void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +}
This function is not necessary; callers can directly use virObjectUnref().
+ +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + }
Is it necessary to allocate an integer? Rather, I would just:
+ + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) {
virHashAddEntry(fdset->aliasToFdSet, alias, (void*)fdset->nextfdset);
+ VIR_FREE(num); + return -1; + } + + *fdsetnum = fdset->nextfdset++;
That would also avoid the need for a hash cleanup function.
+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 *)payload);
Minor type mismatch - the hash stores ints but here you are printing unsigned ints (won't matter in practice). Then again, if you store 'int' instead of 'int*' in the hashtable, this would just be '(int)payload' instead of '*(unsigned int*)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 = NULL; + int ret = 0; + + virFdSetReset(fdset);
Is anyone outside of this file ever going to call virFdSetReset, or can you mark it static?
+ + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } + if (n > 0) { + for (i = 0 ; i < n ; i++) { + key = virXMLPropString(nodes[i], "alias"); + val = virXMLPropString(nodes[i], "fdset"); + if (key && val) { + if (VIR_ALLOC(fdsetnum) < 0) { + virReportOOMError(); + ret = -1; + goto error;
Again, if you just store the int directly (since int is smaller than void*), this avoids wasteful allocations of an int*.
+++ libvirt/src/util/virfdset.h
+ +/** + * virFdSetNew + * + * Create a new FdSet, + * Returns pointer to new FdSet on success, NULL if no memory was available. + */ +virFdSetPtr virFdSetNew(void) ATTRIBUTE_RETURN_CHECK; + +/** + * virFdSetFree + * @fdset : the FdSet to free
No space before the colon.
Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -650,6 +650,16 @@ virFDStreamOpen; virFDStreamOpenFile;
+# virfdset.h +virFdSetFormatXML; +virFdSetFree; +virFdSetNew; +virFdSetNextSet; +virFdSetParseXML; +virFdSetRemoveAlias; +virFdSetReset; + + # hash.h virHashAddEntry; virHashCreate;
Hmm, this file is a bit out of order; it's easier to find exports if we were to sort sections by file name. I'll submit that patch independently. Overall looks pretty nice - I'm glad Dan provided a better design than what I had been thinking of. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/15/2013 04:49 PM, Eric Blake wrote:
On 02/14/2013 05:00 AM, Stefan Berger wrote:
Rather than passing the next-to-use file descriptor set Id and the hash table for rembering the mappings of aliases to file descriptor sets around, encapsulate the two in a class.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- In addition to Corey's review,
Index: libvirt/src/util/virfdset.c =================================================================== + +void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +} This function is not necessary; callers can directly use virObjectUnref().
I personally find it 'nicer' to have a virXYZNew() and a virXYZFree() . I think Daniel had suggested something like that.
+ +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + } Is it necessary to allocate an integer? Rather, I would just:
Yes, it's necessary since the hash table does not make a copy of the void * since it doesn't know the size of the value. However, it makes a copy of the key.
+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 *)payload); Minor type mismatch - the hash stores ints but here you are printing unsigned ints (won't matter in practice).
Fixed. + + virFdSetReset(fdset);
Is anyone outside of this file ever going to call virFdSetReset, or can you mark it static?
We have callers.
On 02/15/2013 03:34 PM, Stefan Berger wrote:
+void virFdSetFree(virFdSetPtr fdset) +{ + virObjectUnref(fdset); +} This function is not necessary; callers can directly use virObjectUnref().
I personally find it 'nicer' to have a virXYZNew() and a virXYZFree() . I think Daniel had suggested something like that.
The virXYZNew() is nice. But the virXYZFree() is spurious - if we know that XYZ is a virObject, then virObjectUnref() is sufficient. Besides, if XYZ is a virObject, it is misleading to have a function named *Free() that doesn't always free data (it frees data on the last unref, but if other references are still held is it just decrementing the refcount). For comparison, we have virDomainObjNew(), but no virDomainObjFree(), because all clients of virDomainObjPtr use virObjectUnref().
+ +int virFdSetNextSet(virFdSetPtr fdset, const char *alias, + unsigned int *fdsetnum) +{ + int *num; + + if (VIR_ALLOC(num) < 0) { + virReportOOMError(); + return -1; + } Is it necessary to allocate an integer? Rather, I would just:
Yes, it's necessary since the hash table does not make a copy of the void * since it doesn't know the size of the value. However, it makes a copy of the key.
Not true - the hash table makes a copy of sizeof(void*) bytes, and since sizeof(void*) >= sizeof(int), casting int to void* lets you avoid an allocation. We do it all the time: src/nwfilter/nwfilter_dhcpsnoop.c: if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0) { src/qemu/qemu_conf.c: if (virHashAddEntry(driver->sharedDisks, key, (void *)0x1)) ... just that here, you would be using a value that is not necessarily 0x1.
+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 *)payload); Minor type mismatch - the hash stores ints but here you are printing unsigned ints (won't matter in practice).
Fixed.
+ + virFdSetReset(fdset);
Is anyone outside of this file ever going to call virFdSetReset, or can you mark it static?
We have callers.
Why does anyone outside this file ever have to do a reset? The set is per-vm, and short of reloading (which the parseXML handles), I don't see any reason why there need to be outside callers. But maybe I'll change my mind when I get through the other patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/15/2013 04:49 PM, Eric Blake wrote:
On 02/14/2013 05:00 AM, Stefan Berger wrote:
+ + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) { virHashAddEntry(fdset->aliasToFdSet, alias, (void*)fdset->nextfdset);
May have to cast this to a long on 64bit machines first. util/virfdset.c: In function 'virFdSetNextSet': util/virfdset.c:98:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] util/virfdset.c:95:35: error: unused parameter 'fdsetnum' [-Werror=unused-parameter] Not nice.
On 02/15/2013 05:43 PM, Stefan Berger wrote:
On 02/15/2013 04:49 PM, Eric Blake wrote:
On 02/14/2013 05:00 AM, Stefan Berger wrote:
+ + *num = fdset->nextfdset; + + if (virHashAddEntry(fdset->aliasToFdSet, alias, num) < 0) { virHashAddEntry(fdset->aliasToFdSet, alias, (void*)fdset->nextfdset);
May have to cast this to a long on 64bit machines first.
Wonder why existing uses didn't warn - oh, because they used a constant instead of a variable. Write it as (void*)(intptr_t)fdset->nextfdset on input, and extract it with (int)(intptr_t)payload when printing.
util/virfdset.c: In function 'virFdSetNextSet': util/virfdset.c:98:25: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] util/virfdset.c:95:35: error: unused parameter 'fdsetnum' [-Werror=unused-parameter]
Not nice.
But not the end of the world either - we've got other places where an intermediate cast through intptr_t is necessary to shut up compilers. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Extend the QEMU private domain structure with virFdSet. Persist the virFdSet using XML and parse its XML. Reset the FdSet upon domain stop. Stefan Berger <stefanb@linux.vnet.ibm.com> --- v5->v6: - change found in patch 3 moved to this patch --- src/qemu/qemu_domain.c | 13 +++++++++++++ src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 2 ++ 3 files changed, 18 insertions(+) Index: libvirt/src/qemu/qemu_domain.c =================================================================== --- libvirt.orig/src/qemu/qemu_domain.c +++ libvirt/src/qemu/qemu_domain.c @@ -211,6 +211,9 @@ static void *qemuDomainObjPrivateAlloc(v if (VIR_ALLOC(priv) < 0) return NULL; + if (!(priv->fdset = virFdSetNew())) + goto error; + if (qemuDomainObjInitJob(priv) < 0) goto error; @@ -222,6 +225,7 @@ static void *qemuDomainObjPrivateAlloc(v return priv; error: + virFdSetFree(priv->fdset); VIR_FREE(priv); return NULL; } @@ -251,6 +255,7 @@ static void qemuDomainObjPrivateFree(voi qemuAgentClose(priv->agent); } VIR_FREE(priv->cleanupCallbacks); + virFdSetFree(priv->fdset); VIR_FREE(priv); } @@ -325,9 +330,14 @@ static int qemuDomainObjPrivateXMLFormat if (priv->fakeReboot) virBufferAsprintf(buf, " <fakereboot/>\n"); + virBufferAdjustIndent(buf, 2); + virFdSetFormatXML(priv->fdset, buf); + virBufferAdjustIndent(buf, -2); + return 0; } + static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) { qemuDomainObjPrivatePtr priv = data; @@ -470,6 +480,9 @@ static int qemuDomainObjPrivateXMLParse( priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; + if (virFdSetParseXML(priv->fdset, "./fdset/entry", ctxt) < 0) + goto error; + return 0; error: 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) | \ @@ -160,6 +161,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 @@ -4255,6 +4255,8 @@ void qemuProcessStop(virQEMUDriverPtr dr priv->monConfig = NULL; } + virFdSetReset(priv->fdset); + /* shut it off for sure */ ignore_value(qemuProcessKill(vm, VIR_QEMU_PROCESS_KILL_FORCE|
On 02/14/2013 05:00 AM, Stefan Berger wrote:
Extend the QEMU private domain structure with virFdSet. Persist the virFdSet using XML and parse its XML. Reset the FdSet upon domain stop.
Stefan Berger <stefanb@linux.vnet.ibm.com>
--- v5->v6: - change found in patch 3 moved to this patch
@@ -470,6 +480,9 @@ static int qemuDomainObjPrivateXMLParse(
priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;
+ if (virFdSetParseXML(priv->fdset, "./fdset/entry", ctxt) < 0) + goto error;
Does this work on the upgrade path? If older libvirt did not use fdsets, then the XML element will not be present, but in patch 1, you had: + if ((n = virXPathNodeSet(xPath, ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to parse qemu file descriptor sets")); + goto error; + } which implies an error if there was nothing to parse. But in reality, nothing to parse should be treated the same as success - there's no set to reinstate, because it was from an older libvirt that didn't use a set. /me rereads patch 1... Ewww - this works, but only by accident. In patch 1, you initialized int ret = 0, therefore, the 'error:' label is reached with ret still at 0, and the function returns success. Typically, we name a label 'cleanup:' rather than 'error:' when the label can be used on the success path; also, we tend to initialize ret to -1 and then flip it to 0 just before cleanup, instead of your approach of initializing to 0 and flipping to -1 on all call sites that goto error because of a real problem.
+++ libvirt/src/qemu/qemu_process.c @@ -4255,6 +4255,8 @@ void qemuProcessStop(virQEMUDriverPtr dr priv->monConfig = NULL; }
+ virFdSetReset(priv->fdset);
An offline domain doesn't need an fdset. I think that rather than pre-allocate the set when creating the vm, then resetting it, that instead we create it just before starting a domain, and free the set when stopping the domain: virObjectUnref(priv->fdset); priv->fdset = NULL; so that we aren't wasting memory for a set with offline domains, since offline domains also have no aliases that would live in an fdset. And if we do that, then my complaint on 1/3 about not needing a public virFdSetReset() is valid, as this appears to be the only place you do a reset. -- 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=1,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=2,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> --- 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 | 243 +++++++++++++++++++----- 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 | 4 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, 308 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 "qemu_driver.h" #include <sys/stat.h> #include <fcntl.h> @@ -133,6 +134,70 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO /** + * qemuCommandPrintFDSet: + * @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 * +qemuCommandPrintFDSet(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%s%s", fdset, fd, + (name) ? ",opaque=" : "", + mode); + if (name) + virBufferEscape(&buf, ',', ",", "%s", name); + + if (virBufferError(&buf)) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +/** + * qemuCommandPrintDevSet: + * @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 +qemuCommandPrintDevSet(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 @@ -2229,11 +2294,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 *fdsetstr = 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; + + qemuCommandPrintDevSet(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); + + fdsetstr = qemuCommandPrintFDSet(fdsetnum, fd, open_flags[i], + path); + if (!fdsetstr) + goto error; + virCommandAddArgList(cmd, "-add-fd", fdsetstr, NULL); + VIR_FREE(fdsetstr); + + 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); @@ -2395,7 +2519,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)) @@ -2892,7 +3022,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); @@ -2941,7 +3074,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)) { @@ -3899,7 +4035,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; @@ -3918,19 +4057,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: @@ -5072,7 +5224,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; @@ -5367,11 +5520,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"); @@ -5818,8 +5971,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 @@ -5835,12 +5986,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) @@ -6016,7 +6167,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); @@ -6297,14 +6448,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", @@ -6335,12 +6486,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"); @@ -6372,12 +6524,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"); @@ -6409,12 +6562,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); @@ -6444,12 +6597,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); } @@ -6482,12 +6636,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"); @@ -6504,12 +6658,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"); @@ -6846,14 +7000,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)) @@ -7079,6 +7233,7 @@ qemuBuildCommandLine(virConnectPtr conn, no_memory: virReportOOMError(); error: + virFdSetReset(fdset); virObjectUnref(cfg); /* free up any resources in the network driver */ for (i = 0; i <= last_good_net; i++) 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 @@ -2505,7 +2505,7 @@ qemuCompressProgramName(int compress) /* 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) { @@ -5334,7 +5334,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 @@ -264,7 +264,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))) @@ -321,6 +322,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 && @@ -504,7 +507,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++) { @@ -571,6 +575,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); @@ -622,7 +628,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; @@ -664,6 +671,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 @@ -3816,7 +3816,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 */ @@ -4118,6 +4119,7 @@ cleanup: virBitmapFree(nodemask); virCommandFree(cmd); VIR_FORCE_CLOSE(logfile); + virFdSetReset(priv->fdset); qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags); virObjectUnref(cfg); virObjectUnref(caps); 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); + virFdSetFree(fdset); return ret; } @@ -882,6 +888,9 @@ mymain(void) QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_QXL_VGA); + 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); + virFdSetFree(fdset); virObjectUnref(conn); return ret; }
On 02/14/2013 07:00 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>
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=1,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=2,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>
--- 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 | 243 +++++++++++++++++++----- 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 | 4 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, 308 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 "qemu_driver.h"
#include <sys/stat.h> #include <fcntl.h> @@ -133,6 +134,70 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO
/** + * qemuCommandPrintFDSet: + * @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 * +qemuCommandPrintFDSet(int fdset, int fd, int open_flags, const char *name)
It would be easier to read the code if this were called something like qemuCommandPrintAddFd().
+{ + 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%s%s", fdset, fd, + (name) ? ",opaque=" : "", + mode); + if (name) + virBufferEscape(&buf, ',', ",", "%s", name); + + if (virBufferError(&buf)) { + virReportOOMError(); + virBufferFreeAndReset(&buf); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +/** + * qemuCommandPrintDevSet: + * @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 +qemuCommandPrintDevSet(virBufferPtr buf, int fdset)
And this could be called qemuCommandPrintDevFdSet().
+{ + 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 @@ -2229,11 +2294,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 *fdsetstr = NULL; + int i; + + virBufferAdd(buf, prefix, -1); + /* + * cmd == NULL hints at hotplugging; use old way of doing things
I assume hotplug will be implemented at a later time by someone.
+ * 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; + + qemuCommandPrintDevSet(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); + + fdsetstr = qemuCommandPrintFDSet(fdsetnum, fd, open_flags[i], + path);
And fdsetstr could be called addfdstr?
+ if (!fdsetstr) + goto error; + virCommandAddArgList(cmd, "-add-fd", fdsetstr, NULL); + VIR_FREE(fdsetstr); + + i++; + } + } + + return 0; + +error:
Do fd's need to get cleaned up? Are we leaking them here?
+ virFdSetRemoveAlias(fdset, info->alias); + return -1; +} +
-- Regards, Corey Bryant
On 02/15/2013 03:51 PM, Corey Bryant wrote:
+ char *fdsetstr = NULL; + int i; + + virBufferAdd(buf, prefix, -1); + /* + * cmd == NULL hints at hotplugging; use old way of doing things
I assume hotplug will be implemented at a later time by someone.
Yes, this can be done independently.
+ if (!fdsetstr) + goto error; + virCommandAddArgList(cmd, "-add-fd", fdsetstr, NULL); + VIR_FREE(fdsetstr); + + i++; + } + } + + return 0; + +error:
Do fd's need to get cleaned up? Are we leaking them here?
No, the virCommandTransferFD has put the fd into the virCommand and when that gets aborted due to the error propagation it will close the fd as well. I will wait for further reviews also regarding the renaming of functions.
On 02/14/2013 05:00 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>
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=1,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=2,fd=32,opaque=/tmp/testpipe -chardev pipe,id=charserial1,path=/dev/fdset/2
The example might be a little clearer if you use sets 1, 2, 3 instead of 1, 1, 2.
Test cases are part of this patch now.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Hmm, I started reviewing this before I saw that you sent a v7. I'll go ahead and send anyways, in case it helps.
/** + * qemuCommandPrintFDSet: + * @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 * +qemuCommandPrintFDSet(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;
Is it worth a default case when the mode is unrecognized? Then again, unless the Linux kernel ever gains O_SEARCH/O_EXEC support, there probably won't ever be any code hitting the default case.
+ } + } + + virBufferAsprintf(&buf, "set=%d,fd=%d%s%s", fdset, fd, + (name) ? ",opaque=" : "", + mode); + if (name) + virBufferEscape(&buf, ',', ",", "%s", name);
Slightly easier to read as: virBufferAsprintf(&buf, "set=%d,fd=%d", fdset, fd); if (name) virBufferEscape(&buf, ',', ",", ",opaque=%s", name);
+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 *fdsetstr = 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; + + qemuCommandPrintDevSet(buf, fdsetnum); + + i = 0; + while (open_flags[i] != -1) {
Laine was mentioning in another review that we aren't very consistent between open_flags and openFlags for local variable names, but that the studlyCaps version seems to be more popular. Rather than having the user supply a sentinel, would it be better to have the user provide nopenFlags? That is, when opening a single fd, passing '&mode, 1' is easier than passing 'int[] { mode, -1}', especially if we don't want to use C99 initializer syntax. For that matter, would it be any easier to pass a flags instead of a mode, where we have the bitwise-or of: QEMU_USE_RDONLY = 1 << 0, // O_RDONLY QEMU_USE_RDWR = 1 << 1, // O_RDWR QEMU_USE WRONLY = 1 << 2, // O_WRONLY on the grounds that writing 'QEMU_USE_RDONLY | QEMU_USE_RDWR' looks a little cleaner than writing '(int[]){O_RDONLY, O_RDWR, -1}' (no temporary arrays required).
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 \ +\
Interesting way to make it more legible - I like it. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/15/2013 10:01 PM, Eric Blake wrote:
On 02/14/2013 05:00 AM, Stefan Berger wrote:
+static char * +qemuCommandPrintFDSet(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;
Is it worth a default case when the mode is unrecognized? Then again, unless the Linux kernel ever gains O_SEARCH/O_EXEC support, there probably won't ever be any code hitting the default case.
Then we can leave it as-is I suppose. We can always treat other flags separately in this case statement if needed in the future.
+ } + } + + virBufferAsprintf(&buf, "set=%d,fd=%d%s%s", fdset, fd, + (name) ? ",opaque=" : "", + mode); + if (name) + virBufferEscape(&buf, ',', ",", "%s", name); Slightly easier to read as:
virBufferAsprintf(&buf, "set=%d,fd=%d", fdset, fd); if (name) virBufferEscape(&buf, ',', ",", ",opaque=%s", name);
Something like this, yes. 'mode' still needs to be printed in the if(name) part but cannot be part of virBufferEscape.
Rather than having the user supply a sentinel, would it be better to have the user provide nopenFlags? That is, when opening a single fd, passing '&mode, 1' is easier than passing 'int[] { mode, -1}', especially if we don't want to use C99 initializer syntax. For that matter, would it be any easier to pass a flags instead of a mode, where we have the bitwise-or of:
QEMU_USE_RDONLY = 1 << 0, // O_RDONLY QEMU_USE_RDWR = 1 << 1, // O_RDWR QEMU_USE WRONLY = 1 << 2, // O_WRONLY
on the grounds that writing 'QEMU_USE_RDONLY | QEMU_USE_RDWR' looks a little cleaner than writing '(int[]){O_RDONLY, O_RDWR, -1}' (no temporary arrays required).
For that we would need additional code everywhere where we need to convert these QEMU_USE_* to the POSIX flags by bitwise sampling the flags in a loop,which is practically everywhere where the POSIX flags are understood today, e.g., qemuOpenFile(). I am not sure it will make things easier. Stefan
participants (3)
-
Corey Bryant -
Eric Blake -
Stefan Berger