[libvirt] [PATCH RFC 0/3] Add a "flags" element to storage pool source
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
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@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
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@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
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@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
participants (1)
-
John Ferlan