[libvirt] storage conf: Add key-value options to storage pools
This series of patches adds the ability to pass down options to the storage pool drivers. In the case of NFS users can specify mount options and in the case of RBD users can specify options for librados to influence some behavior. All options and values are checked on input validity to prevent injection of malicious commands. -- Wido den Hollander
From: Wido den Hollander <wido@42on.com> This allows the end-user to pass down options to the storage pool backend. For example NFS could get mount options or Ceph librados options passed down. --- docs/schemas/storagepool.rng | 16 +++++++++ docs/storage.html.in | 48 +++++++++++++++++++++++++ src/conf/storage_conf.c | 82 +++++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 15 ++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 8d7a94d..7b38dce 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -294,6 +294,22 @@ </element> </define> + <define name='sourceinfooptions'> + <oneOrMore> + <element name='option'> + <attribute name='name'> + <text/> + </attribute> + <optional> + <attribute name='value'> + <text/> + </attribute> + </optional> + <empty/> + </element> + </oneOrMore> + </define> + <define name='initiatorinfo'> <element name='initiator'> <element name='iqn'> diff --git a/docs/storage.html.in b/docs/storage.html.in index eb38b16..3acbd53 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -270,6 +270,30 @@ </target> </pool></pre> + <h3>Mount options</h3> + <p> + It is also possible to influence the mount options used for NFS by + adding extra options in the pool's XML definition. + </p> + <pre> + <pool type="netfs"> + <name>virtimages</name> + <source> + <host name="nfs.example.com"/> + <dir path="/var/lib/virt/images"/> + <format type='nfs'/> + <option name='noatime'/> + <option name='rsize' value='8k'/> + </source> + <target> + <path>/var/lib/virt/images</path> + </target> + </pool></pre> + <p> + Storage pool options are support since <strong>1.2.6</strong> + </p> + + <h3>Valid pool format types</h3> <p> The network filesystem pool supports the following formats: @@ -561,6 +585,30 @@ </source> </pool></pre> + <h3>RADOS cluster options</h3> + <p> + It is also possible to influence the RADOS cluster options used by librados + by adding extra options in the pool's XML definition. + </p> + <pre> + <pool type="rbd"> + <name>myrbdpool</name> + <source> + <name>rbdpool</name> + <host name='1.2.3.4' port='6789'/> + <host name='my.ceph.monitor' port='6789'/> + <host name='third.ceph.monitor' port='6789'/> + <auth username='admin' type='ceph'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + <option name='rados_mon_op_timeout' value='10'/> + <option name='rados_osd_op_timeout' value='10'/> + </source> + </pool></pre> + <p> + Storage pool options are support since <strong>1.2.6</strong> + </p> + <h3>Example volume output</h3> <pre> <volume> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8b6fd79..777e12d 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -32,6 +32,7 @@ #include <stdio.h> #include <fcntl.h> #include <string.h> +#include <regex.h> #include "virerror.h" #include "datatypes.h" @@ -370,6 +371,11 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) for (i = 0; i < source->ndevice; i++) virStoragePoolSourceDeviceClear(&source->devices[i]); VIR_FREE(source->devices); + for (i = 0; i < source->noptions; i++) { + VIR_FREE(source->options[i].name); + VIR_FREE(source->options[i].value); + } + VIR_FREE(source->options); VIR_FREE(source->dir); VIR_FREE(source->name); virStoragePoolSourceAdapterClear(source->adapter); @@ -551,6 +557,32 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, return ret; } +static int virStoragePoolDefVerifyOption(char *msg) +{ + regex_t regex; + int reti; + char msgbuf[100]; + int ret = -1; + + reti = regcomp(®ex, "^[A-Za-z0-9][A-Za-z0-9_-]*[A-Za-z0-9]$", 0); + if (reti) + goto cleanup; + + reti = regexec(®ex, msg, 0, NULL, 0); + if (!reti) { + ret = 0; + } else if (reti == REG_NOMATCH) { + ret = -1; + } else { + regerror(reti, ®ex, msgbuf, sizeof(msgbuf)); + ret = -2; + } + +cleanup: + regfree(®ex); + return ret; +} + static int virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolSourcePtr source, @@ -559,7 +591,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, { int ret = -1; xmlNodePtr relnode, *nodeset = NULL; - int nsource; + int nsource, noptions; size_t i; virStoragePoolOptionsPtr options; char *name = NULL; @@ -649,6 +681,44 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } } + noptions = virXPathNodeSet("./option", ctxt, &nodeset); + if (noptions > 0) { + if (VIR_ALLOC_N(source->options, noptions) < 0) { + VIR_FREE(nodeset); + goto cleanup; + } + + source->noptions = noptions; + + for (i = 0; i < noptions; i++) { + char *option = virXMLPropString(nodeset[i], "name"); + if (option == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing storage pool option attribute 'name'")); + goto cleanup; + } else if (virStoragePoolDefVerifyOption(option) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s: %s", + _("invalid storage pool option"), option); + goto cleanup; + } + source->options[i].name = option; + + /* + * We allow values to be NULL. + * E.g. for a NFS mount: <option name='soft'/> or <option name='intr'/> + */ + char *value = virXMLPropString(nodeset[i], "value"); + if (value != NULL) { + if (virStoragePoolDefVerifyOption(value) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s: %s", + _("invalid storage pool option value"), value); + goto cleanup; + } + source->options[i].value = value; + } + } + } + source->dir = virXPathString("string(./dir/@path)", ctxt); /* In gluster, a missing dir defaults to "/" */ if (!source->dir && pool_type == VIR_STORAGE_POOL_GLUSTER && @@ -1166,6 +1236,16 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferEscapeString(buf, "<vendor name='%s'/>\n", src->vendor); virBufferEscapeString(buf, "<product name='%s'/>\n", src->product); + if (src->noptions) { + for (i = 0; i < src->noptions; i++) { + virBufferAsprintf(buf, " <option name='%s'", src->options[i].name); + if (src->options[i].value != NULL) { + virBufferAsprintf(buf, " value='%s'", src->options[i].value); + } + virBufferAddLit(buf, "/>\n"); + } + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</source>\n"); return 0; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 04d99eb..715f129 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -147,6 +147,17 @@ struct _virStoragePoolSourceHost { int port; }; +/* + * Key-value pairs for passing down options to storage pools. + * Eg NFS mount options or Ceph RBD options + */ +typedef struct _virStoragePoolSourceOptions virStoragePoolSourceOptions; +typedef virStoragePoolSourceOptions *virStoragePoolSourceOptionsPtr; +struct _virStoragePoolSourceOptions { + char *name; + char *value; +}; + /* * For MSDOS partitions, the free area is important when @@ -259,6 +270,10 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Key-value pairs with options for the pool */ + int noptions; + virStoragePoolSourceOptionsPtr options; }; typedef struct _virStoragePoolTarget virStoragePoolTarget; -- 1.7.9.5
From: Wido den Hollander <wido@42on.com> This way users can manually set options in librados which might suite their needs better. --- docs/schemas/storagepool.rng | 1 + src/storage/storage_backend_rbd.c | 16 ++++++++++++++++ tests/storagepoolxml2xmlin/pool-rbd.xml | 3 +++ tests/storagepoolxml2xmlout/pool-rbd.xml | 3 +++ 4 files changed, 23 insertions(+) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 7b38dce..0cd119c 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -568,6 +568,7 @@ <ref name='sourceinfohost'/> <optional> <ref name='sourceinfoauth'/> + <ref name='sourceinfooptions'/> </optional> </interleave> </element> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 5d4ef79..3cb5b85 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -216,6 +216,22 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, VIR_DEBUG("Setting RADOS option rados_osd_op_timeout to %s", osd_op_timeout); rados_conf_set(ptr->cluster, "rados_osd_op_timeout", osd_op_timeout); + if (pool->def->source.noptions > 0) { + for (i = 0; i < pool->def->source.noptions; i++) { + if (pool->def->source.options[i].name != NULL && + pool->def->source.options[i].value != NULL) { + VIR_DEBUG("Setting RADOS option %s to %s", + pool->def->source.options[i].name, + pool->def->source.options[i].value); + if (rados_conf_set(ptr->cluster, pool->def->source.options[i].name, + pool->def->source.options[i].value) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set RADOS option %s"), + pool->def->source.options[i].name); + } + } + } + } + ptr->starttime = time(0); r = rados_connect(ptr->cluster); if (r < 0) { diff --git a/tests/storagepoolxml2xmlin/pool-rbd.xml b/tests/storagepoolxml2xmlin/pool-rbd.xml index 056ba6e..05991fc 100644 --- a/tests/storagepoolxml2xmlin/pool-rbd.xml +++ b/tests/storagepoolxml2xmlin/pool-rbd.xml @@ -7,5 +7,8 @@ <auth username='admin' type='ceph'> <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> </auth> + <option name='client_mount_timeout' value='30'/> + <option name='rados_mon_op_timeout' value='30'/> + <option name='rados_osd_op_timeout' value='30'/> </source> </pool> diff --git a/tests/storagepoolxml2xmlout/pool-rbd.xml b/tests/storagepoolxml2xmlout/pool-rbd.xml index 4fe2fce..cb9969e 100644 --- a/tests/storagepoolxml2xmlout/pool-rbd.xml +++ b/tests/storagepoolxml2xmlout/pool-rbd.xml @@ -11,5 +11,8 @@ <auth type='ceph' username='admin'> <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> </auth> + <option name='client_mount_timeout' value='30'/> + <option name='rados_mon_op_timeout' value='30'/> + <option name='rados_osd_op_timeout' value='30'/> </source> </pool> -- 1.7.9.5
This way users can provide mount options for for example NFS storage pools. --- src/storage/storage_backend_fs.c | 44 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 33551e7..66d7bec 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -377,6 +377,8 @@ static int virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) { char *src = NULL; + char *options = NULL; + char *optionsflag = NULL; /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs), * while plain 'mount' does. We have to craft separate argvs to * accommodate this */ @@ -385,8 +387,10 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) bool glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS && pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); virCommandPtr cmd = NULL; + virBuffer optionsbuf = VIR_BUFFER_INITIALIZER; int ret = -1; int rc; + int i; if (pool->def->type == VIR_STORAGE_POOL_NETFS) { if (pool->def->source.nhost != 1) { @@ -432,9 +436,45 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) if (VIR_STRDUP(src, pool->def->source.devices[0].path) < 0) return -1; } + + /* + * Mount options for NFS pool. + * For security reasons we do not simply build a string based on + * the given mount options. This is to prevent any shell injection + * or non-valid mount options. + */ + if (pool->def->type == VIR_STORAGE_POOL_NETFS) { + if (pool->def->source.noptions > 0) { + for (i = 0; i < pool->def->source.noptions; i++) { + char *name = pool->def->source.options[i].name; + char *value = pool->def->source.options[i].value; + if (name != NULL && value == NULL) + virBufferAsprintf(&optionsbuf, "%s,", name); + + if (name != NULL && value != NULL) + virBufferAsprintf(&optionsbuf, "%s=%s,", name, value); + } + + if (virBufferError(&optionsbuf)) + goto no_memory; + + /* + * Strip the last character from the options string since + * that will be a comma. + */ + options = virBufferContentAndReset(&optionsbuf); + if (options != NULL) { + options[strlen(options)-1] = 0; + if (virAsprintf(&optionsflag, "%s", "-o") == -1) + return -1; + } + } + } if (netauto) cmd = virCommandNewArgList(MOUNT, + optionsflag, + options, src, pool->def->target.path, NULL); @@ -455,6 +495,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) (pool->def->type == VIR_STORAGE_POOL_FS ? virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) : virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)), + optionsflag, + options, src, pool->def->target.path, NULL); @@ -463,6 +505,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) goto cleanup; ret = 0; + no_memory: + virReportOOMError(); cleanup: virCommandFree(cmd); VIR_FREE(src); -- 1.7.9.5
On 05/28/2014 11:38 AM, Wido den Hollander wrote:
This series of patches adds the ability to pass down options to the storage pool drivers.
In the case of NFS users can specify mount options and in the case of RBD users can specify options for librados to influence some behavior.
All options and values are checked on input validity to prevent injection of malicious commands.
Should we allow arbitrary options? I think only adding the configuration options we care about would be nicer, something like: <options> <timeout>30</timeout> <retries>5</retries> </options> where timeout would set all the three timeout options for RADOS (unless there is a need to tweak them separately) and retries would only work for NFS? Jan
On Tue, 3 Jun 2014, Ján Tomko wrote:
On 05/28/2014 11:38 AM, Wido den Hollander wrote:
This series of patches adds the ability to pass down options to the storage pool drivers.
In the case of NFS users can specify mount options and in the case of RBD users can specify options for librados to influence some behavior.
All options and values are checked on input validity to prevent injection of malicious commands.
Should we allow arbitrary options?
Ceph in particular has a tendency to add new options with each release, and I'm not really sure that it's feasible for libvirt to know about all of these options natively. For instance, I've been running libvirt with a local patch that let's me attach arbitrary options to RBD disks. This lets me configure things like "rbd cache size" and "rbd cache writethrough until flush" on a per-disk basis. Previous discussion on this list has indicated that some people would find Ceph's debugging options useful as well. - Michael
On Tue, Jun 03, 2014 at 05:33:13PM +0200, Ján Tomko wrote:
On 05/28/2014 11:38 AM, Wido den Hollander wrote:
This series of patches adds the ability to pass down options to the storage pool drivers.
In the case of NFS users can specify mount options and in the case of RBD users can specify options for librados to influence some behavior.
All options and values are checked on input validity to prevent injection of malicious commands.
Should we allow arbitrary options?
No, we shouldn't - at least not in this way. Libvirt's goal is always to to provide a clearly defined mapping, not do arbitrary unchecked argument passthrough. History has shown repeatedly that tools/libraries change their syntax and libvirt has demonstrated its value in adapting to these changes without breaking application compatibility. The only way I'd support passthrough is if it were done in he same way as QEMU passthrough where it used a separate XML namespace which was clearly marked "use at your own risk, unsupported if it breaks". Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/04/2014 12:14 PM, Daniel P. Berrange wrote:
On Tue, Jun 03, 2014 at 05:33:13PM +0200, Ján Tomko wrote:
On 05/28/2014 11:38 AM, Wido den Hollander wrote:
This series of patches adds the ability to pass down options to the storage pool drivers.
In the case of NFS users can specify mount options and in the case of RBD users can specify options for librados to influence some behavior.
All options and values are checked on input validity to prevent injection of malicious commands.
Should we allow arbitrary options?
No, we shouldn't - at least not in this way. Libvirt's goal is always to to provide a clearly defined mapping, not do arbitrary unchecked argument passthrough. History has shown repeatedly that tools/libraries change their syntax and libvirt has demonstrated its value in adapting to these changes without breaking application compatibility.
Ok, understood. I didn't expect these patches to make it, but I wanted to start the discussion.
The only way I'd support passthrough is if it were done in he same way as QEMU passthrough where it used a separate XML namespace which was clearly marked "use at your own risk, unsupported if it breaks".
Well, that would be something which is useful. For Ceph there are a lot of options, but NFS users might want to specify "noatime" or w and rsize as mount options. Wido
Regards, Daniel
participants (4)
-
Daniel P. Berrange -
Ján Tomko -
Michael Chapman -
Wido den Hollander