4:16 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1265694
Posting as an RFC mainly to get buyin/feedback over the concept of using
a <flags> field in the <pool> <source> as a way to allow or direct the
pool/storage backend code to use specific algorithms.
The issue is described in the bz, but the shorter version is the disk
backend needs a way to tell libvirt_parthelper to format output for
dm-multipath volumes in a slight different manner. In order to do that
I chose a new flag to call libvirt_parthelper to indicate the way to
output, but I needed a way to have the disk backend determine whether
to add the flag or not. Could have read /etc/multipath.conf - not a
good option... Don't want to read their config file. If there was an
API to get that data, it would have been nice, but then we'd have to
rely on a specific version of multipath that had the API (a dependency
that we don't want). So I chose to roll our own.
I figure there could be other uses of such kind of flags - for example
currently logical volume pools use a model of thin volume support that
is "older". A newer model is there, but IIRC the "format" of the lvs
output wouldn't mesh. So if there was a way to say, use this format of
support in preference, then we could easily add that support with the
caveat that one couldn't expect an "old" thin pool to work. IOW, use
the much improved algorithm knowing that it's a "new" pool rather than
having to mess with "conversions"... Perhaps other pools may find uses,
but this one came to mind since I have a long dormant set of changes
that uses the new thin pool options...
John Ferlan (3):
conf: Add flags to the storage pool source
storage: Add new flag for libvirt_parthelper
conf: Parse/use a flags argument in the storage pool xml
docs/formatstorage.html.in | 33 ++++++++++++++++++++
docs/schemas/storagepool.rng | 9 ++++++
src/conf/storage_conf.c | 41 +++++++++++++++++++++++++
src/conf/storage_conf.h | 11 +++++++
src/storage/parthelper.c | 15 +++++++--
src/storage/storage_backend_disk.c | 7 +++++
tests/storagepoolxml2xmlin/pool-disk-flags.xml | 15 +++++++++
tests/storagepoolxml2xmlout/pool-disk-flags.xml | 15 +++++++++
tests/storagepoolxml2xmltest.c | 1 +
9 files changed, 144 insertions(+), 3 deletions(-)
create mode 100644 tests/storagepoolxml2xmlin/pool-disk-flags.xml
create mode 100644 tests/storagepoolxml2xmlout/pool-disk-flags.xml
--
2.5.0
4:16 p.m.
New subject: [libvirt] [PATCH RFC 1/3] conf: Add flags to the storage pool source
Add a flags argument to the storage pool source to handle allowing some
feature to be passed to the lower/backend for the purpose of executing
specific code paths or making decisions on algorithms.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index ec59c17..bb6bcc7 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -198,6 +198,14 @@ struct _virStoragePoolSourceAdapter {
} data;
};
+/* Flags to be used in pool source for some feature or dependency of
+ * the source that the backends may need to know about in order to make
+ * decisions on how data is presented
+ */
+typedef enum {
+ VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES = 1 << 0,
+} virStoragePoolSourceFlags;
+
typedef struct _virStoragePoolSource virStoragePoolSource;
typedef virStoragePoolSource *virStoragePoolSourcePtr;
struct _virStoragePoolSource {
@@ -234,6 +242,9 @@ struct _virStoragePoolSource {
* or lvm version, etc.
*/
int format;
+
+ /* Pool specific flags */
+ unsigned int flags; /* virStoragePoolSourceFlags */
};
typedef struct _virStoragePoolTarget virStoragePoolTarget;
--
2.5.0
4:16 p.m.
New subject: [libvirt] [PATCH RFC 2/3] storage: Add new flag for libvirt_parthelper
In order to be able to process disk storage pool's using a multipath
device to handle the partitions, libvirt_parthelper will need to know
about a feature available from multipath known as 'user_friendly_names'.
This feature causes multipath partitions to not include the "p"
partition separator unless the partition ends with a number in the
generated partition name of the device.
For example, device path "/dev/mapper/mpatha" would create partitions
"mpatha1", "mpatha2", etc. instead of "mpathap1",
"mpathap2", etc.
If however, the device path ended with a number "/dev/mapper/mpatha1",
then the existing algorithm to generate names "mpatha1p1",
"mpatha1p2",
etc. would be utilized.
In order to enable the feature, the appropriate storage pool flags bit
VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES will be checked and if so
pass a "-f" to the libvirt_parthelper which will process the "-f"
flag
to utilize the new algorithm to process the generated partition names
that get then passed back to libvirt and processed against the device
mapper multipath (dm-#) names.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/parthelper.c | 15 ++++++++++++---
src/storage/storage_backend_disk.c | 7 +++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index 8de32fd..7a658ae 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -10,7 +10,7 @@
* in a reliable fashion if merely after a list of partitions & sizes,
* though it is fine for creating partitions.
*
- * Copyright (C) 2007-2008, 2010, 2013 Red Hat, Inc.
+ * Copyright (C) 2007-2008, 2010, 2013, 2015 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -70,6 +70,7 @@ int main(int argc, char **argv)
const char *path;
char *canonical_path;
const char *partsep;
+ bool user_friendly_names = false;
if (setlocale(LC_ALL, "") == NULL ||
bindtextdomain(PACKAGE, LOCALEDIR) == NULL ||
@@ -80,14 +81,22 @@ int main(int argc, char **argv)
if (argc == 3 && STREQ(argv[2], "-g")) {
cmd = DISK_GEOMETRY;
+ } else if (argc == 3 && STREQ(argv[2], "-f")) {
+ user_friendly_names = true;
} else if (argc != 2) {
- fprintf(stderr, _("syntax: %s DEVICE [-g]\n"), argv[0]);
+ fprintf(stderr, _("syntax: %s DEVICE [-g]|[-f]\n"), argv[0]);
return 1;
}
path = argv[1];
if (virIsDevMapperDevice(path)) {
- partsep = "p";
+ /* The 'user_friendly_names' option will not append the partsep of
"p"
+ * onto the name unless the 'path' ends in a number
+ */
+ if (c_isdigit(path[strlen(path)-1]) || !user_friendly_names)
+ partsep ="p";
+ else
+ partsep = "";
if (VIR_STRDUP_QUIET(canonical_path, path) < 0)
return 2;
} else {
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 7baecc1..63481c8 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -324,6 +324,13 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
pool->def->source.devices[0].path,
NULL);
+ /* Check for the presence of the user_friendly_names flags. Pass this
+ * along to the libvirt_parthelper as option '-f'. This will create
+ * slightly different names in the returned list.
+ */
+ if (pool->def->source.flags & VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES)
+ virCommandAddArg(cmd, "-f");
+
/* If a volume is passed, virStorageBackendDiskMakeVol only updates the
* pool allocation for that single volume.
*/
--
2.5.0
4:16 p.m.
New subject: [libvirt] [PATCH RFC 3/3] conf: Parse/use a flags argument in the storage pool xml
https://bugzilla.redhat.com/show_bug.cgi?id=1265694
Add the capability to recognize and parse named/documented flags for
the storage pool. Chose to make rng flags just text rather than having
flags be some list of "expected" values (such as in nwfilter.rng) mostly
to make future expansion easier. The flags are expect to be a comma
separated list which can be parsed by existing virStringSplit API.
The flag will be used by the disk pool that's using device mapper
mulitpath device to expose partitions. The flag will follow the logic
of the /etc/multipath.conf feature 'user_friendly_names'.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
docs/formatstorage.html.in | 33 ++++++++++++++++++++
docs/schemas/storagepool.rng | 9 ++++++
src/conf/storage_conf.c | 41 +++++++++++++++++++++++++
src/storage/parthelper.c | 2 +-
tests/storagepoolxml2xmlin/pool-disk-flags.xml | 15 +++++++++
tests/storagepoolxml2xmlout/pool-disk-flags.xml | 15 +++++++++
tests/storagepoolxml2xmltest.c | 1 +
7 files changed, 115 insertions(+), 1 deletion(-)
create mode 100644 tests/storagepoolxml2xmlin/pool-disk-flags.xml
create mode 100644 tests/storagepoolxml2xmlout/pool-disk-flags.xml
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index a60e05e..9912a2b 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -90,6 +90,15 @@
<pre>
...
<source>
+ <device path='/dev/mapper/mpatha'/>
+ <format type='gpt'/>
+ <flags>user_friendly_names</flags>
+ </source>
+ ...</pre>
+
+ <pre>
+ ...
+ <source>
<adapter type='scsi_host' name='scsi_host1'/>
</source>
...</pre>
@@ -353,6 +362,30 @@
<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>flags</code></dt>
+ <dd>Provides an optional way to pass a flag via the storage pool
+ configuration for the storage backing drivers to utilize some
+ feature or request usage of a specific algorithm by the backend
+ driver in order find storage targets. The following is a list
+ of support flags for specific backends.
+ <span class="since">Since 1.3.1</span>
+ <dl>
+ <dt><code>user_friendly_names</code></dt>
+ <dd>Used by the storage pool <code>type</code> 'disk'
which
+ is using a multipath <code>device path</code> in order to
+ manage the partitions for the pool. The feature mimics the
+ same named feature for device mapper multipath (dm-multipath)
+ within its /etc/multipath.conf. The result is names libvirt
+ generates for partitions will not have the "p" separator
+ unless the <code>device path</code> ends with a number.
+ Thus "/dev/mapper/mpatha" will create "mpatha1",
"mpatha2",
+ etc. instead of "mpathap1", "mpathap2", etc. names for
the
+ partitions.
+ <span class="since">Since 1.3.1</span>
+ </dd>
+ </dl>
+ </dd>
</dl>
<h3><a name="StoragePoolTarget">Target
elements</a></h3>
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 3a61f04..edfc924 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -532,6 +532,9 @@
<optional>
<ref name='sourceinfovendor'/>
</optional>
+ <optional>
+ <ref name='sourceflags'/>
+ </optional>
</interleave>
</element>
</define>
@@ -610,4 +613,10 @@
</data>
</define>
+ <define name='sourceflags'>
+ <element name="flags">
+ <text/>
+ </element>
+ </define>
+
</grammar>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9b8abea..049bc9e 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -459,6 +459,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
char *port = NULL;
char *adapter_type = NULL;
char *managed = NULL;
+ char *flagstr = NULL;
+ char **flags = NULL;
int n;
relnode = ctxt->node;
@@ -663,6 +665,26 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
source->vendor = virXPathString("string(./vendor/@name)", ctxt);
source->product = virXPathString("string(./product/@name)", ctxt);
+ /* Find and parse a comma separated list of flags by name and
+ * associate with the virStoragePoolSourceFlags
+ */
+ flagstr = virXPathString("string(./flags)", ctxt);
+ if (flagstr) {
+ if (!(flags = virStringSplit(flagstr, ",", 0)))
+ goto cleanup;
+
+ for (i = 0; flags[i] != NULL; i++) {
+ if (STREQ(flags[i], "user_friendly_names")) {
+ source->flags |= VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES;
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unrecognized storage pool flag
'%s'"),
+ flags[i]);
+ goto cleanup;
+ }
+ }
+ }
+
ret = 0;
cleanup:
ctxt->node = relnode;
@@ -671,6 +693,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_FREE(nodeset);
VIR_FREE(adapter_type);
VIR_FREE(managed);
+ VIR_FREE(flagstr);
+ virStringFreeList(flags);
virStorageAuthDefFree(authdef);
return ret;
}
@@ -1128,6 +1152,23 @@ virStoragePoolSourceFormat(virBufferPtr buf,
virBufferEscapeString(buf, "<vendor name='%s'/>\n",
src->vendor);
virBufferEscapeString(buf, "<product name='%s'/>\n",
src->product);
+ if (src->flags) {
+ unsigned int flags = src->flags;
+ int c = 0;
+
+ virBufferAddLit(buf, "<flags>");
+ while (flags) {
+ if (c > 0)
+ virBufferAddLit(buf, ",");
+ if (flags & VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES) {
+ virBufferAddLit(buf, "user_friendly_names");
+ flags ^= VIR_STORAGE_POOL_SOURCE_USER_FRIENDLY_NAMES;
+ c++;
+ }
+ }
+ virBufferAddLit(buf, "</flags>\n");
+ }
+
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</source>\n");
return 0;
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c
index 7a658ae..046c3ca 100644
--- a/src/storage/parthelper.c
+++ b/src/storage/parthelper.c
@@ -94,7 +94,7 @@ int main(int argc, char **argv)
* onto the name unless the 'path' ends in a number
*/
if (c_isdigit(path[strlen(path)-1]) || !user_friendly_names)
- partsep ="p";
+ partsep = "p";
else
partsep = "";
if (VIR_STRDUP_QUIET(canonical_path, path) < 0)
diff --git a/tests/storagepoolxml2xmlin/pool-disk-flags.xml
b/tests/storagepoolxml2xmlin/pool-disk-flags.xml
new file mode 100644
index 0000000..9e867f4
--- /dev/null
+++ b/tests/storagepoolxml2xmlin/pool-disk-flags.xml
@@ -0,0 +1,15 @@
+<pool type='disk'>
+ <name>multipath</name>
+ <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid>
+ <capacity unit='bytes'>0</capacity>
+ <allocation unit='bytes'>0</allocation>
+ <available unit='bytes'>0</available>
+ <source>
+ <device path='/dev/mapper/mpatha'/>
+ <format type='gpt'/>
+ <flags>user_friendly_names</flags>
+ </source>
+ <target>
+ <path>/dev</path>
+ </target>
+</pool>
diff --git a/tests/storagepoolxml2xmlout/pool-disk-flags.xml
b/tests/storagepoolxml2xmlout/pool-disk-flags.xml
new file mode 100644
index 0000000..9e867f4
--- /dev/null
+++ b/tests/storagepoolxml2xmlout/pool-disk-flags.xml
@@ -0,0 +1,15 @@
+<pool type='disk'>
+ <name>multipath</name>
+ <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid>
+ <capacity unit='bytes'>0</capacity>
+ <allocation unit='bytes'>0</allocation>
+ <available unit='bytes'>0</available>
+ <source>
+ <device path='/dev/mapper/mpatha'/>
+ <format type='gpt'/>
+ <flags>user_friendly_names</flags>
+ </source>
+ <target>
+ <path>/dev</path>
+ </target>
+</pool>
diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index b03c4b0..1f8655e 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -80,6 +80,7 @@ mymain(void)
DO_TEST("pool-logical-nopath");
DO_TEST("pool-logical-create");
DO_TEST("pool-disk");
+ DO_TEST("pool-disk-flags");
DO_TEST("pool-iscsi");
DO_TEST("pool-iscsi-auth");
DO_TEST("pool-netfs");
--
2.5.0
3272
days inactive
3272
days old
3 comments
1 participants
participants (1)
-
John Ferlan