[libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review. Changes since v1: * Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool XML namespace. Could be split again, but separately the patches were too small IMO. * Patch 2: (NEW) Implementation of providing XML Namespace functions. The details of the operation are in the subsequent patch. I could combine 2 and 3, but perhaps it's easier to review separated. * Patch 3: (REWORK) Rework the previous series patch1 in order to utilize the Storage Pool XML Namespace. Adjust the description of the configuration and modify the storagepoolxml2xmltest appropriately. * Patch 4: (REWORK) Utilize the XML Namespace data to generate the command line which doesn't change since v1. * Patch 5: (REWORK) Rework the virsh code in order to generate the proper syntax to utilize Storage Pool XML Namespaces. * Patch 6: (NEW) Well, new as in this series, but it's essentially a rework of what was posted in the series jtomko referenced. The code will "prove" (to a degree) that the concept can work for multiple pool types. The "downside" is that there's no (easy) way that I could find to generate output to show the commands are executed. John Ferlan (6): conf: Introduce virStoragePoolXMLNamespace nfs: Add infrastructure to manage XML namespace options docs,tests: Add schema, description, and tests for NFS namespace storage: Add NFS storage pool mount options to command line virsh: Add source-mount-opts for pool commands rbd: Utilize storage pool namespace to manage config options docs/formatstorage.html.in | 87 ++++++++++ docs/schemas/storagepool.rng | 43 +++++ src/conf/storage_conf.c | 51 +++++- src/conf/storage_conf.h | 24 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 127 ++++++++++++++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- src/storage/storage_util.c | 36 ++++ src/storage/storage_util.h | 14 ++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 4 + .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 17 ++ .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 7 + tools/virsh-pool.c | 39 ++++- tools/virsh.pod | 5 + 19 files changed, 680 insertions(+), 7 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml -- 2.20.1
Introduce the infrastructure necessary to manage a Storage Pool XML Namespace. The general concept is similar to virDomainXMLNamespace, except that for Storage Pools the storage backend specific details can be stored within the _virStoragePoolOptions unlike the domain processing code which manages its xmlopt's via the virDomainXMLOption which is allocated/passed around for each domain. This patch defines the add the parse, format, free, and href methods required to process the XML and callout from the Storage Pool Source parse, format, and clear/free API's to perform the action on the XML data for/from the backend. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 24 +++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 55db7a96f5..5fae9b3a41 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -124,6 +124,9 @@ typedef virStoragePoolOptions *virStoragePoolOptionsPtr; struct _virStoragePoolOptions { unsigned int flags; int defaultFormat; + + virStoragePoolXMLNamespace ns; + virStoragePoolFormatToString formatToString; virStoragePoolFormatFromString formatFromString; }; @@ -318,6 +321,34 @@ virStoragePoolOptionsForPoolType(int type) } +/* virStoragePoolOptionsPoolTypeSetXMLNamespace: + * @type: virStoragePoolType + * @ns: xmlopt namespace pointer + * + * Store the @ns in the pool options for the particular backend. + * This allows the parse/format code to then directly call the Namespace + * method space (parse, format, href, free) as needed during processing. + * + * Returns: 0 on success, -1 on failure. + */ +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns) +{ + int ret = -1; + virStoragePoolTypeInfoPtr backend = virStoragePoolTypeInfoLookup(type); + + if (!backend) + goto cleanup; + + backend->poolOptions.ns = *ns; + ret = 0; + + cleanup: + return ret; +} + + static virStorageVolOptionsPtr virStorageVolOptionsForPoolType(int type) { @@ -378,6 +409,9 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) virStorageAuthDefFree(source->auth); VIR_FREE(source->vendor); VIR_FREE(source->product); + + if (source->namespaceData && source->ns.free) + (source->ns.free)(source->namespaceData); } @@ -549,6 +583,13 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->vendor = virXPathString("string(./vendor/@name)", ctxt); source->product = virXPathString("string(./product/@name)", ctxt); + /* Make a copy of all the callback pointers here for easier use, + * especially during the virStoragePoolSourceClear method */ + source->ns = options->ns; + if (source->ns.parse && + (source->ns.parse)(ctxt, &source->namespaceData) < 0) + goto cleanup; + ret = 0; cleanup: ctxt->node = relnode; @@ -965,6 +1006,11 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferEscapeString(buf, "<vendor name='%s'/>\n", src->vendor); virBufferEscapeString(buf, "<product name='%s'/>\n", src->product); + if (src->namespaceData && src->ns.format) { + if ((src->ns.format)(buf, src->namespaceData) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); return 0; @@ -989,7 +1035,10 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, _("unexpected pool type")); return -1; } - virBufferAsprintf(buf, "<pool type='%s'>\n", type); + virBufferAsprintf(buf, "<pool type='%s'", type); + if (def->source.namespaceData && def->source.ns.href) + virBufferAsprintf(buf, " %s", (def->source.ns.href)()); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<name>%s</name>\n", def->name); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index dc0aa2ab29..f388596f7c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -33,6 +33,26 @@ # include <libxml/tree.h> +/* Various callbacks needed to parse/create Storage Pool XML's using + * a private namespace */ +typedef int (*virStoragePoolDefNamespaceParse)(xmlXPathContextPtr, void **); +typedef void (*virStoragePoolDefNamespaceFree)(void *); +typedef int (*virStoragePoolDefNamespaceXMLFormat)(virBufferPtr, void *); +typedef const char *(*virStoragePoolDefNamespaceHref)(void); + +typedef struct _virStoragePoolXMLNamespace virStoragePoolXMLNamespace; +typedef virStoragePoolXMLNamespace *virStoragePoolXMLNamespacePtr; +struct _virStoragePoolXMLNamespace { + virStoragePoolDefNamespaceParse parse; + virStoragePoolDefNamespaceFree free; + virStoragePoolDefNamespaceXMLFormat format; + virStoragePoolDefNamespaceHref href; +}; + +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns); + /* * How the volume's data is stored on underlying * physical devices - can potentially span many @@ -197,6 +217,10 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Pool backend specific XML namespace data */ + void *namespaceData; + virStoragePoolXMLNamespace ns; }; typedef struct _virStoragePoolTarget virStoragePoolTarget; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..4d82797ee1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -919,6 +919,7 @@ virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolFormatLogicalTypeToString; +virStoragePoolOptionsPoolTypeSetXMLNamespace; virStoragePoolSaveConfig; virStoragePoolSaveState; virStoragePoolSourceClear; -- 2.20.1
On Tue, Jan 08, 2019 at 12:52:21PM -0500, John Ferlan wrote:
Introduce the infrastructure necessary to manage a Storage Pool XML Namespace. The general concept is similar to virDomainXMLNamespace, except that for Storage Pools the storage backend specific details can be stored within the _virStoragePoolOptions unlike the domain processing code which manages its xmlopt's via the virDomainXMLOption which is allocated/passed around for each domain.
This patch defines the add the parse, format, free, and href methods required to process the XML and callout from the Storage Pool Source parse, format, and clear/free API's to perform the action on the XML data for/from the backend.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 24 +++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 55db7a96f5..5fae9b3a41 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -124,6 +124,9 @@ typedef virStoragePoolOptions *virStoragePoolOptionsPtr; struct _virStoragePoolOptions { unsigned int flags; int defaultFormat; + + virStoragePoolXMLNamespace ns; + virStoragePoolFormatToString formatToString; virStoragePoolFormatFromString formatFromString; }; @@ -318,6 +321,34 @@ virStoragePoolOptionsForPoolType(int type) }
+/* virStoragePoolOptionsPoolTypeSetXMLNamespace: + * @type: virStoragePoolType + * @ns: xmlopt namespace pointer + * + * Store the @ns in the pool options for the particular backend. + * This allows the parse/format code to then directly call the Namespace + * method space (parse, format, href, free) as needed during processing. + * + * Returns: 0 on success, -1 on failure. + */ +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns) +{ + int ret = -1; + virStoragePoolTypeInfoPtr backend = virStoragePoolTypeInfoLookup(type); + + if (!backend) + goto cleanup; + + backend->poolOptions.ns = *ns; + ret = 0; + + cleanup: + return ret; +} + + static virStorageVolOptionsPtr virStorageVolOptionsForPoolType(int type) { @@ -378,6 +409,9 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) virStorageAuthDefFree(source->auth); VIR_FREE(source->vendor); VIR_FREE(source->product); + + if (source->namespaceData && source->ns.free) + (source->ns.free)(source->namespaceData); }
@@ -549,6 +583,13 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, source->vendor = virXPathString("string(./vendor/@name)", ctxt); source->product = virXPathString("string(./product/@name)", ctxt);
+ /* Make a copy of all the callback pointers here for easier use, + * especially during the virStoragePoolSourceClear method */ + source->ns = options->ns; + if (source->ns.parse && + (source->ns.parse)(ctxt, &source->namespaceData) < 0) + goto cleanup; + ret = 0; cleanup: ctxt->node = relnode; @@ -965,6 +1006,11 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferEscapeString(buf, "<vendor name='%s'/>\n", src->vendor); virBufferEscapeString(buf, "<product name='%s'/>\n", src->product);
+ if (src->namespaceData && src->ns.format) { + if ((src->ns.format)(buf, src->namespaceData) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); return 0; @@ -989,7 +1035,10 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, _("unexpected pool type")); return -1; } - virBufferAsprintf(buf, "<pool type='%s'>\n", type); + virBufferAsprintf(buf, "<pool type='%s'", type); + if (def->source.namespaceData && def->source.ns.href) + virBufferAsprintf(buf, " %s", (def->source.ns.href)()); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<name>%s</name>\n", def->name);
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index dc0aa2ab29..f388596f7c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -33,6 +33,26 @@
# include <libxml/tree.h>
+/* Various callbacks needed to parse/create Storage Pool XML's using + * a private namespace */ +typedef int (*virStoragePoolDefNamespaceParse)(xmlXPathContextPtr, void **); +typedef void (*virStoragePoolDefNamespaceFree)(void *); +typedef int (*virStoragePoolDefNamespaceXMLFormat)(virBufferPtr, void *); +typedef const char *(*virStoragePoolDefNamespaceHref)(void); + +typedef struct _virStoragePoolXMLNamespace virStoragePoolXMLNamespace; +typedef virStoragePoolXMLNamespace *virStoragePoolXMLNamespacePtr; +struct _virStoragePoolXMLNamespace { + virStoragePoolDefNamespaceParse parse; + virStoragePoolDefNamespaceFree free; + virStoragePoolDefNamespaceXMLFormat format; + virStoragePoolDefNamespaceHref href; +}; + +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns); + /* * How the volume's data is stored on underlying * physical devices - can potentially span many @@ -197,6 +217,10 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Pool backend specific XML namespace data */ + void *namespaceData; + virStoragePoolXMLNamespace ns; };
I don't like this placement. For the domain and network XML we put namespace data at the top level of the XML document. This puts it one level further down, which makes it unusable if we need it for parts of the storage pool which are not related to the <source>. I think consistency across all our XML documents is important here. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/9/19 12:06 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:21PM -0500, John Ferlan wrote:
Introduce the infrastructure necessary to manage a Storage Pool XML Namespace. The general concept is similar to virDomainXMLNamespace, except that for Storage Pools the storage backend specific details can be stored within the _virStoragePoolOptions unlike the domain processing code which manages its xmlopt's via the virDomainXMLOption which is allocated/passed around for each domain.
This patch defines the add the parse, format, free, and href methods required to process the XML and callout from the Storage Pool Source parse, format, and clear/free API's to perform the action on the XML data for/from the backend.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 24 +++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 75 insertions(+), 1 deletion(-)
[...]
+ +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns); + /* * How the volume's data is stored on underlying * physical devices - can potentially span many @@ -197,6 +217,10 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Pool backend specific XML namespace data */ + void *namespaceData; + virStoragePoolXMLNamespace ns; };
I don't like this placement. For the domain and network XML we put namespace data at the top level of the XML document. This puts it one level further down, which makes it unusable if we need it for parts of the storage pool which are not related to the <source>. I think consistency across all our XML documents is important here.
Understood... Still does it make sense at the <pool> level to have something that only matters to the <source>? That's what I struggled with for placement. What if someone wanted something <target> specific or <pool> specific. In the long run I don't care where it's placed - it's just moving code around. It just doesn't seem logical elsewhere though. For the domain, it's "logically" part of the "qemu", "lxc", or "vmx" domains in some manner when building the command line or setting environment variables for a command to be run. I didn't look at the network example. For storage, while it is backend/pool specific, it's even more specific to the source and some command to run or API to call to ensure the source is configured in some specific manner. John
On Wed, Jan 09, 2019 at 12:41:27PM -0500, John Ferlan wrote:
On 1/9/19 12:06 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:21PM -0500, John Ferlan wrote:
Introduce the infrastructure necessary to manage a Storage Pool XML Namespace. The general concept is similar to virDomainXMLNamespace, except that for Storage Pools the storage backend specific details can be stored within the _virStoragePoolOptions unlike the domain processing code which manages its xmlopt's via the virDomainXMLOption which is allocated/passed around for each domain.
This patch defines the add the parse, format, free, and href methods required to process the XML and callout from the Storage Pool Source parse, format, and clear/free API's to perform the action on the XML data for/from the backend.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 24 +++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 75 insertions(+), 1 deletion(-)
[...]
+ +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns); + /* * How the volume's data is stored on underlying * physical devices - can potentially span many @@ -197,6 +217,10 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Pool backend specific XML namespace data */ + void *namespaceData; + virStoragePoolXMLNamespace ns; };
I don't like this placement. For the domain and network XML we put namespace data at the top level of the XML document. This puts it one level further down, which makes it unusable if we need it for parts of the storage pool which are not related to the <source>. I think consistency across all our XML documents is important here.
Understood... Still does it make sense at the <pool> level to have something that only matters to the <source>? That's what I struggled with for placement. What if someone wanted something <target> specific or <pool> specific.
In the long run I don't care where it's placed - it's just moving code around. It just doesn't seem logical elsewhere though.
For the domain, it's "logically" part of the "qemu", "lxc", or "vmx" domains in some manner when building the command line or setting environment variables for a command to be run. I didn't look at the network example.
For storage, while it is backend/pool specific, it's even more specific to the source and some command to run or API to call to ensure the source is configured in some specific manner.
It will vary depending on the pool backend. With NFS there's only a single mount command, so a single <netfs:mountopts> is relevant. If other backends run many commands I can imagine seeing multiple distinct XML elements - one for each conceptual command that needs extensions. It can still be at the top level IMHO. Even for domain XML the BHyve driver has multiple commands, so if it had custom XML namespace it would need to allow options for each of these commands. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Introduce the virStoragePoolNetFSMountOptionsDef to be used to manage the NFS Storage Pool XML Namespace for mount options. Using a new virStorageBackendNamespaceInit function, set the virStoragePoolXMLNamespace into the _virStoragePoolOptions when the storage backend is loaded. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 127 +++++++++++++++++++++++++++++++ src/storage/storage_util.c | 16 ++++ src/storage/storage_util.h | 14 ++++ 3 files changed, 157 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index dc9869417e..4b8878f450 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -41,6 +41,7 @@ VIR_LOG_INIT("storage.storage_backend_fs"); #if WITH_STORAGE_FS +# include <libxml/xpathInternals.h> # include <mntent.h> struct _virNetfsDiscoverState { @@ -559,6 +560,121 @@ virStorageBackendFileSystemBuild(virStoragePoolObjPtr pool, } +#if WITH_STORAGE_FS + +# define STORAGE_POOL_NETFS_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/netfs/1.0" + +/* NetFS backend XML Namespace handling for nfs specific mount options to + * be added to the mount -o {options_list} command line. The XML will use + * the format, such as: + * + * <netfs:mount_opts> + * <netfs:option name='nodev'/> + * <netfs:option name='nosuid'/> + * </netfs:mount_opts> + * + * and the <pool type='netfs'> is required to have a "xmlns:netfs='%s'" + * attribute using the STORAGE_POOL_NETFS_NAMESPACE_HREF + */ + +static void +virStoragePoolDefNetFSNamespaceFree(void *nsdata) +{ + virStoragePoolNetFSMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) + VIR_FREE(cmdopts->options[i]); + + VIR_FREE(cmdopts); +} + + +static int +virStoragePoolDefNetFSNamespaceParse(xmlXPathContextPtr ctxt, + void **data) +{ + virStoragePoolNetFSMountOptionsDefPtr cmdopts = NULL; + xmlNodePtr *nodes = NULL; + int nnodes; + size_t i; + int ret = -1; + + if (xmlXPathRegisterNs(ctxt, BAD_CAST "netfs", + BAD_CAST STORAGE_POOL_NETFS_NAMESPACE_HREF) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register xml namespace '%s'"), + STORAGE_POOL_NETFS_NAMESPACE_HREF); + return -1; + } + + nnodes = virXPathNodeSet("./netfs:mount_opts/netfs:option", ctxt, &nodes); + if (nnodes < 0) + return -1; + + if (nnodes == 0) + return 0; + + if (VIR_ALLOC(cmdopts) < 0 || + VIR_ALLOC_N(cmdopts->options, nnodes) < 0) + goto cleanup; + + for (i = 0; i < nnodes; i++) { + if (!(cmdopts->options[cmdopts->noptions] = + virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no netfs mount option name specified")); + goto cleanup; + } + cmdopts->noptions++; + } + + VIR_STEAL_PTR(*data, cmdopts); + ret = 0; + + cleanup: + VIR_FREE(nodes); + virStoragePoolDefNetFSNamespaceFree(cmdopts); + return ret; +} + + +static int +virStoragePoolDefNetFSNamespaceFormatXML(virBufferPtr buf, + void *nsdata) +{ + size_t i; + virStoragePoolNetFSMountOptionsDefPtr def = nsdata; + + if (!def) + return 0; + + virBufferAddLit(buf, "<netfs:mount_opts>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < def->noptions; i++) + virBufferEscapeString(buf, "<netfs:option name='%s'/>\n", + def->options[i]); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</netfs:mount_opts>\n"); + + return 0; +} + + +static const char * +virStoragePoolDefNetFSNamespaceHref(void) +{ + return "xmlns:netfs='" STORAGE_POOL_NETFS_NAMESPACE_HREF "'"; +} + +#endif /* WITH_STORAGE_FS */ + + virStorageBackend virStorageBackendDirectory = { .type = VIR_STORAGE_POOL_DIR, @@ -617,6 +733,13 @@ virStorageBackend virStorageBackendNetFileSystem = { .downloadVol = virStorageBackendVolDownloadLocal, .wipeVol = virStorageBackendVolWipeLocal, }; + +static virStoragePoolXMLNamespace virStoragePoolNetFSXMLNamespace = { + .parse = virStoragePoolDefNetFSNamespaceParse, + .free = virStoragePoolDefNetFSNamespaceFree, + .format = virStoragePoolDefNetFSNamespaceFormatXML, + .href = virStoragePoolDefNetFSNamespaceHref, +}; #endif /* WITH_STORAGE_FS */ @@ -632,6 +755,10 @@ virStorageBackendFsRegister(void) if (virStorageBackendRegister(&virStorageBackendNetFileSystem) < 0) return -1; + + if (virStorageBackendNamespaceInit(VIR_STORAGE_POOL_NETFS, + &virStoragePoolNetFSXMLNamespace) < 0) + return -1; #endif /* WITH_STORAGE_FS */ return 0; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..ff0a0239e6 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -77,6 +77,22 @@ VIR_LOG_INIT("storage.storage_util"); + +/* virStorageBackendNamespaceInit: + * @poolType: virStoragePoolType + * @xmlns: Storage Pool specific namespace callback methods + * + * To be called during storage backend registration to configure the + * Storage Pool XML Namespace based on the backend's needs. + */ +int +virStorageBackendNamespaceInit(int poolType, + virStoragePoolXMLNamespacePtr xmlns) +{ + return virStoragePoolOptionsPoolTypeSetXMLNamespace(poolType, xmlns); +} + + #define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) #define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024) diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index c872468135..b11b1b23f7 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -26,6 +26,20 @@ # include "storage_driver.h" # include "storage_backend.h" +/* NetFS Storage Pool Namespace options to share w/ storage_backend_fs.c and + * the virStorageBackendFileSystemMountCmd method */ +typedef struct _virStoragePoolNetFSMountOptionsDef virStoragePoolNetFSMountOptionsDef; +typedef virStoragePoolNetFSMountOptionsDef *virStoragePoolNetFSMountOptionsDefPtr; +struct _virStoragePoolNetFSMountOptionsDef { + size_t noptions; + char **options; +}; + +int +virStorageBackendNamespaceInit(int poolType, + virStoragePoolXMLNamespacePtr xmlns); + + /* File creation/cloning functions used for cloning between backends */ int -- 2.20.1
On 1/8/19 6:52 PM, John Ferlan wrote:
Introduce the virStoragePoolNetFSMountOptionsDef to be used to manage the NFS Storage Pool XML Namespace for mount options.
Using a new virStorageBackendNamespaceInit function, set the virStoragePoolXMLNamespace into the _virStoragePoolOptions when the storage backend is loaded.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 127 +++++++++++++++++++++++++++++++ src/storage/storage_util.c | 16 ++++ src/storage/storage_util.h | 14 ++++ 3 files changed, 157 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index dc9869417e..4b8878f450 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -41,6 +41,7 @@ VIR_LOG_INIT("storage.storage_backend_fs");
#if WITH_STORAGE_FS
+# include <libxml/xpathInternals.h> # include <mntent.h>
struct _virNetfsDiscoverState { @@ -559,6 +560,121 @@ virStorageBackendFileSystemBuild(virStoragePoolObjPtr pool, }
+#if WITH_STORAGE_FS + +# define STORAGE_POOL_NETFS_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/netfs/1.0" + +/* NetFS backend XML Namespace handling for nfs specific mount options to + * be added to the mount -o {options_list} command line. The XML will use + * the format, such as: + * + * <netfs:mount_opts> + * <netfs:option name='nodev'/> + * <netfs:option name='nosuid'/> + * </netfs:mount_opts> + * + * and the <pool type='netfs'> is required to have a "xmlns:netfs='%s'" + * attribute using the STORAGE_POOL_NETFS_NAMESPACE_HREF + */ + +static void +virStoragePoolDefNetFSNamespaceFree(void *nsdata) +{ + virStoragePoolNetFSMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) + VIR_FREE(cmdopts->options[i]); + + VIR_FREE(cmdopts);
This is missing VIR_FREE(cmdopts->options);
+} +
Michal
Modify the storagepool.rng to allow for the usage of a different XML namespace to parse the netfs_mount_opts to be included with the storage source. Modify the storagepoolxml2xmltest to utilize a properly modified XML file to parse and format the namespace for a netfs storage pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 42 +++++++++++++++++++ docs/schemas/storagepool.rng | 20 +++++++++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.xml | 24 +++++++++++ .../pool-netfs-mountopts.xml | 24 +++++++++++ tests/storagepoolxml2xmltest.c | 6 +++ 6 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index b6bf3edbd2..308b94f5e5 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -395,6 +395,48 @@ <dd>Provides an optional product name of the storage device. This contains a single attribute <code>name</code> whose value is backend specific. <span class="since">Since 0.8.4</span></dd> + + <dt><code>netfs:mount_opts</code></dt> + <dd>Provides an XML namespace mechanism to optionally utilize + specifically named options for the mount command via the "-o" + option for the <code>netfs</code> type storage pools. In order + to designate that the Storage Pool will be using the mechanism, + the <code>pool</code> element must be modified to provide the + XML namespace attribute syntax as follows: + + <p> + xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0' + </p> + + <p> + The <code>netfs:mount_opts</code> defines the mount options by + specifying multiple <code>netfs:option</code> subelements with + the attribute <code>name</code> specifying the mount option to + be added. The value of the named option is not checked since + it's possible options don't exist on all distributions. It is + expected that proper and valid options will be supplied for the + target host. + </p> + + The following XML snippet shows the syntax required in order to + utilize + <pre> +<pool type="netfs" xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'>; + <name>nfsimages</name> +... + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <netfs:mount_opts> + <netfs:option name='nodev'/> + <netfs:option name='nosuid'/> + </netfs:mount_opts> + </source> +...</pre> + + <span class="since">Since 5.0.0.</span></dd> + </dl> <h3><a id="StoragePoolTarget">Target elements</a></h3> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 74f4363106..20c7ae5744 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -534,6 +534,9 @@ <optional> <ref name='sourceinfovendor'/> </optional> + <optional> + <ref name='netfs_mount_opts'/> + </optional> </interleave> </group> <group> @@ -675,4 +678,21 @@ </data> </define> + <!-- + Optional storage pool extensions in their own namespace: + NetFS + --> + + <define name="netfs_mount_opts"> + <element name="mount_opts" ns="http://libvirt.org/schemas/storagepool/source/netfs/1.0"> + <zeroOrMore> + <element name="option"> + <attribute name='name'> + <text/> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + </grammar> diff --git a/tests/Makefile.am b/tests/Makefile.am index f74d8463b6..373d347389 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -937,7 +937,9 @@ storagevolxml2xmltest_LDADD = $(LDADDS) storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h -storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_LDADD = $(LDADDS) \ + ../src/libvirt_driver_storage_impl.la \ + ../gnulib/lib/libgnu.la nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml new file mode 100644 index 0000000000..2b720656c7 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml @@ -0,0 +1,24 @@ +<pool type='netfs' xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'> + <name>nfsimages</name> + <uuid>7641d5a8-af11-f730-a34e-0a7dfcede71f</uuid> + <capacity>0</capacity> + <allocation>0</allocation> + <available>0</available> + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <netfs:mount_opts> + <netfs:option name='nodev'/> + <netfs:option name='nosuid'/> + </netfs:mount_opts> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml b/tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml new file mode 100644 index 0000000000..ec78ff0d2f --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml @@ -0,0 +1,24 @@ +<pool type='netfs' xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'> + <name>nfsimages</name> + <uuid>7641d5a8-af11-f730-a34e-0a7dfcede71f</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <netfs:mount_opts> + <netfs:option name='nodev'/> + <netfs:option name='nosuid'/> + </netfs:mount_opts> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 707d09f5c2..c08313d236 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -11,6 +11,8 @@ #include "testutilsqemu.h" #include "virstring.h" +#include "storage/storage_util.h" + #define VIR_FROM_THIS VIR_FROM_NONE static int @@ -70,6 +72,9 @@ mymain(void) testCompareXMLToXMLHelper, (name)) < 0) \ ret = -1 + if (storageRegisterAll() < 0) + return EXIT_FAILURE; + DO_TEST("pool-dir"); DO_TEST("pool-dir-naming"); DO_TEST("pool-fs"); @@ -85,6 +90,7 @@ mymain(void) DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); + DO_TEST("pool-netfs-mountopts"); DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host"); DO_TEST("pool-scsi-type-fc-host"); -- 2.20.1
On 1/8/19 6:52 PM, John Ferlan wrote:
Modify the storagepool.rng to allow for the usage of a different XML namespace to parse the netfs_mount_opts to be included with the storage source.
Modify the storagepoolxml2xmltest to utilize a properly modified XML file to parse and format the namespace for a netfs storage pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 42 +++++++++++++++++++ docs/schemas/storagepool.rng | 20 +++++++++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.xml | 24 +++++++++++ .../pool-netfs-mountopts.xml | 24 +++++++++++ tests/storagepoolxml2xmltest.c | 6 +++ 6 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml
diff --git a/tests/Makefile.am b/tests/Makefile.am index f74d8463b6..373d347389 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -937,7 +937,9 @@ storagevolxml2xmltest_LDADD = $(LDADDS) storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h -storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_LDADD = $(LDADDS) \ + ../src/libvirt_driver_storage_impl.la \ + ../gnulib/lib/libgnu.la
Or $(GNULIB_LIBS)
nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \
Michal
If the NetFS Storage Pool Namespace XML data exists, format the mount options on the MOUNT command line. When the pool is started, the options will be generated on the command line along with the exportfs options already defined. To view the options of the running pool, use either 'nfsstat -m' or 'grep $POOLNAME /proc/mounts' for the added Flags/options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 20 +++++++++++++++++++ .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 4 ++++ 3 files changed, 25 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index ff0a0239e6..debf5aa133 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4352,6 +4352,26 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, virStorageBackendFileSystemMountCIFSArgs(cmd, src, def); else virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); + + if (def->type == VIR_STORAGE_POOL_NETFS && def->source.namespaceData) { + size_t i; + virStoragePoolNetFSMountOptionsDefPtr opts = def->source.namespaceData; + virBuffer buf = VIR_BUFFER_INITIALIZER; + VIR_AUTOFREE(char *) mountOpts = NULL; + + for (i = 0; i < opts->noptions; i++) + virBufferAsprintf(&buf, "%s,", opts->options[i]); + + virBufferTrim(&buf, ",", -1); + + if (virBufferCheckError(&buf) < 0) + return NULL; + + mountOpts = virBufferContentAndReset(&buf); + + virCommandAddArgList(cmd, "-o", mountOpts, NULL); + } + return cmd; } diff --git a/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv b/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv new file mode 100644 index 0000000000..16d35bc175 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv @@ -0,0 +1 @@ +mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nodev,nosuid diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 2f2d40e027..338fe6d5f5 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -144,6 +144,9 @@ mymain(void) #define DO_TEST_FAIL(pool, ...) \ DO_TEST_FULL(true, pool) + if (storageRegisterAll() < 0) + return EXIT_FAILURE; + DO_TEST_FAIL("pool-dir"); DO_TEST_FAIL("pool-dir-naming"); DO_TEST("pool-fs"); @@ -159,6 +162,7 @@ mymain(void) DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); + DO_TEST("pool-netfs-mountopts"); DO_TEST_FAIL("pool-scsi"); DO_TEST_FAIL("pool-scsi-type-scsi-host"); DO_TEST_FAIL("pool-scsi-type-fc-host"); -- 2.20.1
Add the ability to add the mount options for the pool on the virsh command line for pool-{create|define}-as commands, such as. virsh pool-define-as nfstest netfs \ --source-host localhost \ --source-path "/var/lib/libvirt/images" \ --source-format nfs \ --source-mount-opts "nodev,nosuid" \ --target "/mnt" in order to generate XML: <pool type='netfs' xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'> <name>nfstest</name> <source> <host name='localhost'/> <dir path='/var/lib/libvirt/images'/> <format type='nfs'/> <netfs:mount_opts> <netfs:option name='nodev'/> <netfs:option name='nosuid'/> </netfs:mount_opts> </source> <target> <path>/mnt</path> </target> </pool> Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 39 +++++++++++++++++++++++++++++++++++---- tools/virsh.pod | 5 +++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 70ca39bd3d..7f8de4a08c 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -136,6 +136,10 @@ {.name = "adapter-parent-fabric-wwn", \ .type = VSH_OT_STRING, \ .help = N_("adapter parent scsi_hostN fabric_wwn to be used for underlying vHBA storage") \ + }, \ + {.name = "source-mount-opts", \ + .type = VSH_OT_STRING, \ + .help = N_("comma separated list for NFS pool mount options") \ } virStoragePoolPtr @@ -319,7 +323,9 @@ virshBuildPoolXML(vshControl *ctl, *secretUsage = NULL, *adapterName = NULL, *adapterParent = NULL, *adapterWwnn = NULL, *adapterWwpn = NULL, *secretUUID = NULL, *adapterParentWwnn = NULL, *adapterParentWwpn = NULL, - *adapterParentFabricWwn = NULL; + *adapterParentFabricWwn = NULL, *mountOpts = NULL; + size_t noptsList = 0; + char **optsList = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; VSH_EXCLUSIVE_OPTIONS("secret-usage", "secret-uuid"); @@ -345,10 +351,19 @@ virshBuildPoolXML(vshControl *ctl, vshCommandOptStringReq(ctl, cmd, "adapter-parent", &adapterParent) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwnn", &adapterParentWwnn) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwpn", &adapterParentWwpn) < 0 || - vshCommandOptStringReq(ctl, cmd, "adapter-parent-fabric-wwn", &adapterParentFabricWwn) < 0) + vshCommandOptStringReq(ctl, cmd, "adapter-parent-fabric-wwn", &adapterParentFabricWwn) < 0 || + vshCommandOptStringReq(ctl, cmd, "source-mount-opts", &mountOpts) < 0) + goto cleanup; + + if (mountOpts && + !(optsList = virStringSplitCount(mountOpts, ",", 0, &noptsList))) goto cleanup; - virBufferAsprintf(&buf, "<pool type='%s'>\n", type); + virBufferAsprintf(&buf, "<pool type='%s'", type); + if (mountOpts) + virBufferAsprintf(&buf, " xmlns:netfs='%s'", + "http://libvirt.org/schemas/storagepool/source/netfs/1.0"); + virBufferAddLit(&buf, ">\n"); virBufferAdjustIndent(&buf, 2); virBufferAsprintf(&buf, "<name>%s</name>\n", name); if (srcHost || srcPath || srcDev || srcFormat || srcName || @@ -394,6 +409,20 @@ virshBuildPoolXML(vshControl *ctl, if (srcName) virBufferAsprintf(&buf, "<name>%s</name>\n", srcName); + if (mountOpts) { + size_t i; + + virBufferAddLit(&buf, "<netfs:mount_opts>\n"); + virBufferAdjustIndent(&buf, 2); + + for (i = 0; i < noptsList; i++) + virBufferAsprintf(&buf, "<netfs:option name='%s'/>\n", + optsList[i]); + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</netfs:mount_opts>\n"); + } + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n"); } @@ -409,14 +438,16 @@ virshBuildPoolXML(vshControl *ctl, if (virBufferError(&buf)) { vshError(ctl, "%s", _("Failed to allocate XML buffer")); - return false; + goto cleanup; } + virStringListFree(optsList); *xml = virBufferContentAndReset(&buf); *retname = name; return true; cleanup: + virStringListFree(optsList); virBufferFreeAndReset(&buf); return false; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 86a4996cae..881981e159 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3884,6 +3884,7 @@ just I<--build> is provided, then B<pool-build> is called with no flags. =item B<pool-create-as> I<name> I<type> [I<--source-host hostname>] [I<--source-path path>] [I<--source-dev path>] +[I<--source-mount-opts mountOpts>] [I<--source-name name>] [I<--target path>] [I<--source-format format>] [I<--auth-type authtype> I<--auth-username username> [I<--secret-usage usage> | I<--secret-uuid uuid>]] @@ -3910,6 +3911,10 @@ gluster). [I<--source-path path>] provides the source directory path for pools backed by directories (pool type dir). +[<--source-mount-opts mountOpts>] provides a string to be used when creating +the mount command for a netfs type pool. The options must be in a comma +separated list format as described by the mount options command. + [I<--source-dev path>] provides the source path for pools backed by physical devices (pool types fs, logical, disk, iscsi, zfs). -- 2.20.1
Allow for adjustment of RBD configuration options via Storage Pool XML Namespace adjustments. Based off original patch/concept: https://www.redhat.com/archives/libvir-list/2014-May/msg00940.html Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 45 +++++ docs/schemas/storagepool.rng | 23 +++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- .../pool-rbd-configopts.xml | 17 ++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 1 + 6 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 308b94f5e5..4e0fe0a981 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -437,6 +437,51 @@ <span class="since">Since 5.0.0.</span></dd> + <dt><code>rbd:config_opts</code></dt> + <dd>Provides an XML namespace mechanism to optionally utilize + specifically named options for the RBD configuration options + via the rados_conf_set API for the <code>rbd</code> type + storage pools. In order to designate that the Storage Pool + will be using the mechanism, the <code>pool</code> element + must be modified to provide the XML namespace attribute + syntax as follows: + + <p> + xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0' + </p> + + <p> + The <code>rbd:config_opts</code> defines the configuration options + by specifying multiple <code>rbd:option</code> subelements with + the attribute <code>name</code> specifying the configuration option + to be added and <code>value</code> specifying the configuration + option value. The name and value for each option is only checked + to be not empty. The name and value provided are not checked since + it's possible options don't exist on all distributions. It is + expected that proper and valid options will be supplied for the + target host. + </p> + + The following XML snippet shows the syntax required in order to + utilize + <pre> +<pool type="rbd" xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'>; + <name>myrbdpool</name> +... + <source> + <name>rbdpool</name> + <host name='1.2.3.4'/> + <host name='my.ceph.monitor'/> + <host name='third.ceph.monitor' port='6789'/> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='20'/> + <rbd:option name='rados_osd_op_timeout' value='10'/> + </rbd:config_opts> + </source> +...</pre> + + <span class="since">Since 5.0.0.</span></dd> </dl> <h3><a id="StoragePoolTarget">Target elements</a></h3> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 20c7ae5744..54fa584828 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -647,6 +647,9 @@ <optional> <ref name='sourceinfoauth'/> </optional> + <optional> + <ref name='rbd_config_opts'/> + </optional> </interleave> </element> </define> @@ -695,4 +698,24 @@ </element> </define> + <!-- + Optional storage pool extensions in their own namespace: + RBD + --> + + <define name="rbd_config_opts"> + <element name="config_opts" ns="http://libvirt.org/schemas/storagepool/source/rbd/1.0"> + <zeroOrMore> + <element name="option"> + <attribute name='name'> + <text/> + </attribute> + <attribute name='value'> + <text/> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + </grammar> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 24dd1349ae..c419b12e2d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -36,6 +36,7 @@ #include "rbd/librbd.h" #include "secret_util.h" #include "storage_util.h" +#include <libxml/xpathInternals.h> #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -50,6 +51,138 @@ struct _virStorageBackendRBDState { typedef struct _virStorageBackendRBDState virStorageBackendRBDState; typedef virStorageBackendRBDState *virStorageBackendRBDStatePtr; +/* NetFS Storage Pool Namespace options to share w/ storage_backend_fs.c and + * the virStorageBackendFileSystemMountCmd method */ +typedef struct _virStoragePoolRBDMountOptionsDef virStoragePoolRBDMountOptionsDef; +typedef virStoragePoolRBDMountOptionsDef *virStoragePoolRBDMountOptionsDefPtr; +struct _virStoragePoolRBDMountOptionsDef { + size_t noptions; + char **names; + char **values; +}; + +#define STORAGE_POOL_RBD_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/rbd/1.0" + +static void +virStoragePoolDefRBDNamespaceFree(void *nsdata) +{ + virStoragePoolRBDMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) { + VIR_FREE(cmdopts->names[i]); + VIR_FREE(cmdopts->values[i]); + } + + VIR_FREE(cmdopts); +} + + +static int +virStoragePoolDefRBDNamespaceParse(xmlXPathContextPtr ctxt, + void **data) +{ + virStoragePoolRBDMountOptionsDefPtr cmdopts = NULL; + xmlNodePtr *nodes = NULL; + int nnodes; + size_t i; + int ret = -1; + + if (xmlXPathRegisterNs(ctxt, BAD_CAST "rbd", + BAD_CAST STORAGE_POOL_RBD_NAMESPACE_HREF) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register xml namespace '%s'"), + STORAGE_POOL_RBD_NAMESPACE_HREF); + return -1; + } + + nnodes = virXPathNodeSet("./rbd:config_opts/rbd:option", ctxt, &nodes); + if (nnodes < 0) + return -1; + + if (nnodes == 0) + return 0; + + if (VIR_ALLOC(cmdopts) < 0) + goto cleanup; + + if (VIR_ALLOC_N(cmdopts->names, nnodes) < 0 || + VIR_ALLOC_N(cmdopts->values, nnodes) < 0) + goto cleanup; + + for (i = 0; i < nnodes; i++) { + if (!(cmdopts->names[cmdopts->noptions] = + virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no rbd option name specified")); + goto cleanup; + } + if (*cmdopts->names[cmdopts->noptions] == '\0') { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("empty rbd option name specified")); + goto cleanup; + } + if (!(cmdopts->values[cmdopts->noptions] = + virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_XML_ERROR, + _("no rbd option value specified for name '%s'"), + cmdopts->names[cmdopts->noptions]); + goto cleanup; + } + if (*cmdopts->values[cmdopts->noptions] == '\0') { + virReportError(VIR_ERR_XML_ERROR, + _("empty rbd option value specified for name '%s'"), + cmdopts->names[cmdopts->noptions]); + goto cleanup; + } + cmdopts->noptions++; + } + + VIR_STEAL_PTR(*data, cmdopts); + ret = 0; + + cleanup: + VIR_FREE(nodes); + virStoragePoolDefRBDNamespaceFree(cmdopts); + return ret; +} + + +static int +virStoragePoolDefRBDNamespaceFormatXML(virBufferPtr buf, + void *nsdata) +{ + size_t i; + virStoragePoolRBDMountOptionsDefPtr def = nsdata; + + if (!def) + return 0; + + virBufferAddLit(buf, "<rbd:config_opts>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < def->noptions; i++) { + virBufferEscapeString(buf, "<rbd:option name='%s' ", def->names[i]); + virBufferEscapeString(buf, "value='%s'/>\n", def->values[i]); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</rbd:config_opts>\n"); + + return 0; +} + + +static const char * +virStoragePoolDefRBDNamespaceHref(void) +{ + return "xmlns:rbd='" STORAGE_POOL_RBD_NAMESPACE_HREF "'"; +} + + static int virStorageBackendRBDRADOSConfSet(rados_t cluster, const char *option, @@ -183,6 +316,17 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rbd_default_format) < 0) goto cleanup; + if (source->namespaceData) { + virStoragePoolRBDMountOptionsDefPtr cmdopts = source->namespaceData; + + for (i = 0; i < cmdopts->noptions; i++) { + if (virStorageBackendRBDRADOSConfSet(ptr->cluster, + cmdopts->names[i], + cmdopts->values[i]) < 0) + goto cleanup; + } + } + ptr->starttime = time(0); if ((r = rados_connect(ptr->cluster)) < 0) { virReportSystemError(-r, _("failed to connect to the RADOS monitor on: %s"), @@ -1277,6 +1421,7 @@ virStorageBackendRBDVolWipe(virStoragePoolObjPtr pool, return ret; } + virStorageBackend virStorageBackendRBD = { .type = VIR_STORAGE_POOL_RBD, @@ -1291,8 +1436,20 @@ virStorageBackend virStorageBackendRBD = { }; +static virStoragePoolXMLNamespace virStoragePoolRBDXMLNamespace = { + .parse = virStoragePoolDefRBDNamespaceParse, + .free = virStoragePoolDefRBDNamespaceFree, + .format = virStoragePoolDefRBDNamespaceFormatXML, + .href = virStoragePoolDefRBDNamespaceHref, +}; + + int virStorageBackendRBDRegister(void) { - return virStorageBackendRegister(&virStorageBackendRBD); + if (virStorageBackendRegister(&virStorageBackendRBD) < 0) + return -1; + + return virStorageBackendNamespaceInit(VIR_STORAGE_POOL_RBD, + &virStoragePoolRBDXMLNamespace); } diff --git a/tests/storagepoolxml2xmlin/pool-rbd-configopts.xml b/tests/storagepoolxml2xmlin/pool-rbd-configopts.xml new file mode 100644 index 0000000000..a23f559afb --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-rbd-configopts.xml @@ -0,0 +1,17 @@ +<pool type='rbd' xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <source> + <name>rbd</name> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <auth username='admin' type='ceph'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='10'/> + <rbd:option name='rados_osd_op_timeout' value='20'/> + </rbd:config_opts> + </source> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-rbd-configopts.xml b/tests/storagepoolxml2xmlout/pool-rbd-configopts.xml new file mode 100644 index 0000000000..121a69c0d7 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-rbd-configopts.xml @@ -0,0 +1,20 @@ +<pool type='rbd' xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <name>rbd</name> + <auth type='ceph' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='10'/> + <rbd:option name='rados_osd_op_timeout' value='20'/> + </rbd:config_opts> + </source> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index c08313d236..25f134fc23 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -105,6 +105,7 @@ mymain(void) DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); DO_TEST("pool-rbd"); + DO_TEST("pool-rbd-configopts"); DO_TEST("pool-vstorage"); DO_TEST("pool-iscsi-direct-auth"); DO_TEST("pool-iscsi-direct"); -- 2.20.1
On 1/8/19 6:52 PM, John Ferlan wrote:
Allow for adjustment of RBD configuration options via Storage Pool XML Namespace adjustments.
Based off original patch/concept:
https://www.redhat.com/archives/libvir-list/2014-May/msg00940.html
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 45 +++++ docs/schemas/storagepool.rng | 23 +++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- .../pool-rbd-configopts.xml | 17 ++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 1 + 6 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 308b94f5e5..4e0fe0a981 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -437,6 +437,51 @@
<span class="since">Since 5.0.0.</span></dd>
+ <dt><code>rbd:config_opts</code></dt> + <dd>Provides an XML namespace mechanism to optionally utilize + specifically named options for the RBD configuration options + via the rados_conf_set API for the <code>rbd</code> type + storage pools. In order to designate that the Storage Pool + will be using the mechanism, the <code>pool</code> element + must be modified to provide the XML namespace attribute + syntax as follows: + + <p> + xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0' + </p> + + <p> + The <code>rbd:config_opts</code> defines the configuration options + by specifying multiple <code>rbd:option</code> subelements with + the attribute <code>name</code> specifying the configuration option + to be added and <code>value</code> specifying the configuration + option value. The name and value for each option is only checked + to be not empty. The name and value provided are not checked since + it's possible options don't exist on all distributions. It is + expected that proper and valid options will be supplied for the + target host. + </p> + + The following XML snippet shows the syntax required in order to + utilize + <pre> +<pool type="rbd" xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'>; + <name>myrbdpool</name> +... + <source> + <name>rbdpool</name> + <host name='1.2.3.4'/> + <host name='my.ceph.monitor'/> + <host name='third.ceph.monitor' port='6789'/> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='20'/> + <rbd:option name='rados_osd_op_timeout' value='10'/> + </rbd:config_opts> + </source> +...</pre> + + <span class="since">Since 5.0.0.</span></dd> </dl>
<h3><a id="StoragePoolTarget">Target elements</a></h3> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 20c7ae5744..54fa584828 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -647,6 +647,9 @@ <optional> <ref name='sourceinfoauth'/> </optional> + <optional> + <ref name='rbd_config_opts'/> + </optional> </interleave> </element> </define> @@ -695,4 +698,24 @@ </element> </define>
+ <!-- + Optional storage pool extensions in their own namespace: + RBD + --> + + <define name="rbd_config_opts"> + <element name="config_opts" ns="http://libvirt.org/schemas/storagepool/source/rbd/1.0"> + <zeroOrMore> + <element name="option"> + <attribute name='name'> + <text/> + </attribute> + <attribute name='value'> + <text/> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + </grammar> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 24dd1349ae..c419b12e2d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -36,6 +36,7 @@ #include "rbd/librbd.h" #include "secret_util.h" #include "storage_util.h" +#include <libxml/xpathInternals.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -50,6 +51,138 @@ struct _virStorageBackendRBDState { typedef struct _virStorageBackendRBDState virStorageBackendRBDState; typedef virStorageBackendRBDState *virStorageBackendRBDStatePtr;
+/* NetFS Storage Pool Namespace options to share w/ storage_backend_fs.c and + * the virStorageBackendFileSystemMountCmd method */ +typedef struct _virStoragePoolRBDMountOptionsDef virStoragePoolRBDMountOptionsDef; +typedef virStoragePoolRBDMountOptionsDef *virStoragePoolRBDMountOptionsDefPtr; +struct _virStoragePoolRBDMountOptionsDef { + size_t noptions; + char **names; + char **values; +}; + +#define STORAGE_POOL_RBD_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/rbd/1.0" + +static void +virStoragePoolDefRBDNamespaceFree(void *nsdata) +{ + virStoragePoolRBDMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) { + VIR_FREE(cmdopts->names[i]); + VIR_FREE(cmdopts->values[i]); + } +
Again, VIR_FREE(cmdopts->names) and VIR_FREE(cmdopts->values);
+ VIR_FREE(cmdopts); +}
Michal
On 1/8/19 6:52 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Changes since v1:
* Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool XML namespace. Could be split again, but separately the patches were too small IMO.
* Patch 2: (NEW) Implementation of providing XML Namespace functions. The details of the operation are in the subsequent patch. I could combine 2 and 3, but perhaps it's easier to review separated.
* Patch 3: (REWORK) Rework the previous series patch1 in order to utilize the Storage Pool XML Namespace. Adjust the description of the configuration and modify the storagepoolxml2xmltest appropriately.
* Patch 4: (REWORK) Utilize the XML Namespace data to generate the command line which doesn't change since v1.
* Patch 5: (REWORK) Rework the virsh code in order to generate the proper syntax to utilize Storage Pool XML Namespaces.
* Patch 6: (NEW) Well, new as in this series, but it's essentially a rework of what was posted in the series jtomko referenced. The code will "prove" (to a degree) that the concept can work for multiple pool types. The "downside" is that there's no (easy) way that I could find to generate output to show the commands are executed.
John Ferlan (6): conf: Introduce virStoragePoolXMLNamespace nfs: Add infrastructure to manage XML namespace options docs,tests: Add schema, description, and tests for NFS namespace storage: Add NFS storage pool mount options to command line virsh: Add source-mount-opts for pool commands rbd: Utilize storage pool namespace to manage config options
docs/formatstorage.html.in | 87 ++++++++++ docs/schemas/storagepool.rng | 43 +++++ src/conf/storage_conf.c | 51 +++++- src/conf/storage_conf.h | 24 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 127 ++++++++++++++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- src/storage/storage_util.c | 36 ++++ src/storage/storage_util.h | 14 ++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 4 + .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 17 ++ .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 7 + tools/virsh-pool.c | 39 ++++- tools/virsh.pod | 5 + 19 files changed, 680 insertions(+), 7 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml
ACK My only worry is that we don't validate the options in any way. They are basically a text free form which we will put onto 'mount' cmd line directly. Michal
On 1/9/19 11:55 AM, Michal Privoznik wrote: [...]
ACK
Thanks for the review... although it seems I'll have to consider a 3rd approach as a result of Daniel's comments. Nothing is ever easy.
My only worry is that we don't validate the options in any way. They are basically a text free form which we will put onto 'mount' cmd line directly.
I thought about making sure of specific chars for the attributes (there are some examples in other .rng files), but I know as soon as I do, there's some request for some char that wasn't included. Going with <text/> was just the bail out especially since I find regex's to be unintelligible to fully decipher. John
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Can you give any info about what motivated this addition. Anything that is implemented via a separate XML namespace is generally considered "unsupported, you're on your own when it breaks" so if this feature is neeeded for any management application like oVirt, OpenStack, etc, then using a custom namespace is not going to be a suitable approach. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Can you give any info about what motivated this addition.
There's a bz: https://bugzilla.redhat.com/show_bug.cgi?id=1584663 which along the way has had private comments - I probably should add the bz to at least one of the patches, but I think because of the (probably unnecessary) private comments in the bz, I was hesitant to do so. I tried a different mechanism and Jano pointed out: https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html that previous attempts were rejected due to the arbitrary text.
Anything that is implemented via a separate XML namespace is generally considered
"unsupported, you're on your own when it breaks"
so if this feature is neeeded for any management application like oVirt, OpenStack, etc, then using a custom namespace is not going to be a suitable approach.
And I don't think it would be palatable to add N 'text' options to the XML that map to the same N 'text' options in the mount command. So this was the next best option as far as I saw it. That'd be one of those never ending tasks to add the favorite option. John
On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Can you give any info about what motivated this addition.
There's a bz:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
which along the way has had private comments - I probably should add the bz to at least one of the patches, but I think because of the (probably unnecessary) private comments in the bz, I was hesitant to do so.
I tried a different mechanism and Jano pointed out:
https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
that previous attempts were rejected due to the arbitrary text.
Ok, from the rather limited information in the BZ above, it is clear that this feature is required to be production supported, so that rules out use of private XML namespaces as a viable solution. I agree with Jano's rejected of arbitrary mount option passthrough in v1 too though. I think the right answer involves multiple approaches in the XML. For the NFS pool, we should have an explicit way to request the NFS protocol version in the XML. For the general nosuid, nodev flags, I think we can do something like we have for the <features/> element in the domain XML. Or on second thoughts, the only reason for the storage pools mounts is to provide VM disk images, so we should just unconditionally always set nosuid & nodev when running mount. No storage volume we report should be setuid or be a device node. If the NFS mount has a directory tree for container filesystems, then the previously discussed virFileSystem APIs should be actually implemented. We could none the less also still have a private XML namespace for doing arbitrary mount argument passthrough, but that shouldn't be the solution for this particular BZ. It is just a way to get a quick workaround for an option while we decide how to model the desired new mount option explicitly in the XML, as we do for QEMU options.
Anything that is implemented via a separate XML namespace is generally considered
"unsupported, you're on your own when it breaks"
so if this feature is neeeded for any management application like oVirt, OpenStack, etc, then using a custom namespace is not going to be a suitable approach.
And I don't think it would be palatable to add N 'text' options to the XML that map to the same N 'text' options in the mount command. So this was the next best option as far as I saw it. That'd be one of those never ending tasks to add the favorite option.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/9/19 12:40 PM, Daniel P. Berrangé wrote:
On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Can you give any info about what motivated this addition.
There's a bz:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
which along the way has had private comments - I probably should add the bz to at least one of the patches, but I think because of the (probably unnecessary) private comments in the bz, I was hesitant to do so.
I tried a different mechanism and Jano pointed out:
https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
that previous attempts were rejected due to the arbitrary text.
Ok, from the rather limited information in the BZ above, it is clear that this feature is required to be production supported, so that rules out use of private XML namespaces as a viable solution.
I agree with Jano's rejected of arbitrary mount option passthrough in v1 too though.
I understand the rejection and I also found namespaces to not be very user or developer friendly. Even less easy to understand were those "other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard place scenario. It's no small wonder why the patches that were rejected in June 2014 had no attempted followup that I found - it just didn't seem worth the effort and using their own private code to suit their own needs was far more simple!
I think the right answer involves multiple approaches in the XML.
For the NFS pool, we should have an explicit way to request the NFS protocol version in the XML.
You mean mount option "nfsvers=n"?
For the general nosuid, nodev flags, I think we can do something like we have for the <features/> element in the domain XML.
So that means to me that every option currently possible in mount would need to be listed. Doesn't that feel excessive? Not that mount changes that often, but I would think it's awfully painful to take that route. Especially for some of the "more complicated" mount options.
Or on second thoughts, the only reason for the storage pools mounts is to provide VM disk images, so we should just unconditionally always set nosuid & nodev when running mount. No storage volume we report should be setuid or be a device node. If the NFS mount has a directory tree for container filesystems, then the previously discussed virFileSystem APIs should be actually implemented.
That'd be the really simple simple fix at least for nosuid and nodev. I didn't approach it from the viewpoint of providing VM disk images as it seemed to be more "general" provide the ability to pick-n-choose which mount option to use. Side bar, I also have this weird recollection about usage of rsize=n and/or wsize=n in some previous NFS issue I've chased (I think it was at my previous employer). There's also mention of noexec in one of the private comments, so while nosuid and nodev would be easier, it still doesn't cover everything noted in the bz.
We could none the less also still have a private XML namespace for doing arbitrary mount argument passthrough, but that shouldn't be the solution for this particular BZ. It is just a way to get a quick workaround for an option while we decide how to model the desired new mount option explicitly in the XML, as we do for QEMU options.
Does it become more work than it's worth though ;-) How large of a wall needs to be built to stop the flow of code ;-) John
Anything that is implemented via a separate XML namespace is generally considered
"unsupported, you're on your own when it breaks"
so if this feature is neeeded for any management application like oVirt, OpenStack, etc, then using a custom namespace is not going to be a suitable approach.
And I don't think it would be palatable to add N 'text' options to the XML that map to the same N 'text' options in the mount command. So this was the next best option as far as I saw it. That'd be one of those never ending tasks to add the favorite option.
Regards, Daniel
On Wed, Jan 09, 2019 at 01:26:19PM -0500, John Ferlan wrote:
On 1/9/19 12:40 PM, Daniel P. Berrangé wrote:
On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review.
Can you give any info about what motivated this addition.
There's a bz:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
which along the way has had private comments - I probably should add the bz to at least one of the patches, but I think because of the (probably unnecessary) private comments in the bz, I was hesitant to do so.
I tried a different mechanism and Jano pointed out:
https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
that previous attempts were rejected due to the arbitrary text.
Ok, from the rather limited information in the BZ above, it is clear that this feature is required to be production supported, so that rules out use of private XML namespaces as a viable solution.
I agree with Jano's rejected of arbitrary mount option passthrough in v1 too though.
I understand the rejection and I also found namespaces to not be very user or developer friendly. Even less easy to understand were those "other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard place scenario.
It's no small wonder why the patches that were rejected in June 2014 had no attempted followup that I found - it just didn't seem worth the effort and using their own private code to suit their own needs was far more simple!
I think the right answer involves multiple approaches in the XML.
For the NFS pool, we should have an explicit way to request the NFS protocol version in the XML.
You mean mount option "nfsvers=n"?
Yes, which would be set using something like <source> .... <protocol ver="4"/> </source>
For the general nosuid, nodev flags, I think we can do something like we have for the <features/> element in the domain XML.
So that means to me that every option currently possible in mount would need to be listed. Doesn't that feel excessive? Not that mount changes that often, but I would think it's awfully painful to take that route. Especially for some of the "more complicated" mount options.
The mount command options are a tiny, trivial set compared to QEMU command line options. The goal here is not to have simple libvirt code, rather is to have portable & simple application code. Libvirt exists to provide an abstraction layer over OS specific commands line "mount" which can have different syntax on each OS. eg FreeBSD mount command options may differ from Linux mount command options. I'm not suggesting implementing support for every single mount option though. Only those that we actually have a compelling reason to support, just as we've not implemented every single QEMU arg.
Or on second thoughts, the only reason for the storage pools mounts is to provide VM disk images, so we should just unconditionally always set nosuid & nodev when running mount. No storage volume we report should be setuid or be a device node. If the NFS mount has a directory tree for container filesystems, then the previously discussed virFileSystem APIs should be actually implemented.
That'd be the really simple simple fix at least for nosuid and nodev. I didn't approach it from the viewpoint of providing VM disk images as it seemed to be more "general" provide the ability to pick-n-choose which mount option to use. Side bar, I also have this weird recollection about usage of rsize=n and/or wsize=n in some previous NFS issue I've chased (I think it was at my previous employer).
That was needed in NFSv2, but in modern NFSv3/4 IIUC the server decides those values.
There's also mention of noexec in one of the private comments, so while nosuid and nodev would be easier, it still doesn't cover everything noted in the bz.
noexec would be reasonable to turn on by default too IMHO.
We could none the less also still have a private XML namespace for doing arbitrary mount argument passthrough, but that shouldn't be the solution for this particular BZ. It is just a way to get a quick workaround for an option while we decide how to model the desired new mount option explicitly in the XML, as we do for QEMU options.
Does it become more work than it's worth though ;-) How large of a wall needs to be built to stop the flow of code ;-)
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
[...]
For the general nosuid, nodev flags, I think we can do something like we have for the <features/> element in the domain XML.
So that means to me that every option currently possible in mount would need to be listed. Doesn't that feel excessive? Not that mount changes that often, but I would think it's awfully painful to take that route. Especially for some of the "more complicated" mount options.
The mount command options are a tiny, trivial set compared to QEMU command line options. The goal here is not to have simple libvirt code, rather is to have portable & simple application code. Libvirt exists to provide an abstraction layer over OS specific commands line "mount" which can have different syntax on each OS. eg FreeBSD mount command options may differ from Linux mount command options.
And this last line is the biggest concern over providing or "by default" adding "-o nosuid,nodev,noexec" for NFS mount command line. Secondary to that is the "what if" by doing so we break some existing configuration for some reason that was making use of a pool volume in some really strange manner that we didn't expect just because one of those mount options wasn't set by default. Not that it seems the 3 mentioned ones would cause such pain, but the precedence of adding options by default in general.
I'm not suggesting implementing support for every single mount option though. Only those that we actually have a compelling reason to support, just as we've not implemented every single QEMU arg.
Understood. Of course as soon a couple are implemented, someone else's favorite is desired, etc. It's the picking and choosing which are compelling enough to support vs. just allowing or even providing a supportable mechanism to allow arbitrary command options vs. implementing "some" and leaving the rest to be added via namespace. Right now the "some" are: nfsvers='n' [add only if new XML exists] nosuid nodev noexec and maybe additionally 'ro' if [new] <readonly/> was set. Just trying to get all the "parameters" set before starting a v3... John [...]
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik