https://bugzilla.redhat.com/show_bug.cgi?id=1346566 If libvirt_parthelper is erroneously told to append the partition separator 'p' onto the generated output for a disk pool using device mapper that has 'user_friendly_names' set to true, then the error recovery path will fail to find volume resulting in the pool being in an unusable state. So, augment the documentation to provide the better hint that the part_separator='yes' should be set when user_friendly_names are not being used. Additionally, once we're in the error path where the returned name doesn't match the expected partition name try to see if the reason is because the 'p' was erroneosly added. If so alter the about to be removed vol->target.path so that the DiskDeleteVol code can find the partition that was created and remove it. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatstorage.html.in | 5 ++++- src/storage/storage_backend_disk.c | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index ed1f0d1..7690114 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -138,7 +138,10 @@ causes libvirt to attempt to generate and find target volume path's using a "p" separator. The default algorithm used by device mapper is to add the "p" separator only when the source device path ends - with a number. + with a number; however, it's possible to configure the devmapper + device to not use 'user_friendly_names' thus creating partitions + with the "p" separator even when the device source path does not + end with a number. <span class="since">Since 1.3.1</span></p></dd> <dt><code>dir</code></dt> <dd>Provides the source for pools backed by directories (pool diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 666ad03..07a04f1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -95,6 +95,29 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, virReportError(VIR_ERR_INVALID_ARG, _("invalid partition name '%s', expected '%s'"), vol->name, partname); + + /* Let's see if we by chance parthelper created a name that won't be + * found later when we try to delete. We tell parthelper to add a 'p' + * to the output via the part_separator flag, but if devmapper has + * user_friendly_names set, the creation won't happen that way, thus + * our deletion will fail because the name we generated is wrong. + * Check for our conditions and see if the generated name is the + * same as StablePath returns and has the 'p' in it */ + if (pool->def->source.devices[0].part_separator == + VIR_TRISTATE_BOOL_YES && + !virIsDevMapperDevice(vol->target.path) && + STREQ(groups[0], vol->target.path) && + (tmp = strrchr(groups[0], 'p'))) { + + /* If we remove the 'p' from groups[0] and the resulting + * device is a devmapper device, then we know parthelper + * was told to create the wrong name based on the results. + * So just remove the 'p' from the vol->target.path too. */ + memmove(tmp, tmp + 1, strlen(tmp)); + if (virIsDevMapperDevice(groups[0]) && + (tmp = strrchr(vol->target.path, 'p'))) + memmove(tmp, tmp + 1, strlen(tmp)); + } return -1; }

ping? Thanks - John On 12/03/2016 09:06 AM, John Ferlan wrote: > > ping? > > Tks - > > John > > On 11/17/2016 09:55 AM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1346566 >> >> If libvirt_parthelper is erroneously told to append the partition >> separator 'p' onto the generated output for a disk pool using device >> mapper that has 'user_friendly_names' set to true, then the error >> recovery path will fail to find volume resulting in the pool being >> in an unusable state. >> >> So, augment the documentation to provide the better hint that the >> part_separator='yes' should be set when user_friendly_names are not >> being used. Additionally, once we're in the error path where the >> returned name doesn't match the expected partition name try to see >> if the reason is because the 'p' was erroneosly added. If so alter >> the about to be removed vol->target.path so that the DiskDeleteVol >> code can find the partition that was created and remove it. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> docs/formatstorage.html.in | 5 ++++- >> src/storage/storage_backend_disk.c | 23 +++++++++++++++++++++++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in >> index ed1f0d1..7690114 100644 >> --- a/docs/formatstorage.html.in >> +++ b/docs/formatstorage.html.in >> @@ -138,7 +138,10 @@ >> causes libvirt to attempt to generate and find target volume path's >> using a "p" separator. The default algorithm used by device mapper >> is to add the "p" separator only when the source device path ends >> - with a number. >> + with a number; however, it's possible to configure the devmapper >> + device to not use 'user_friendly_names' thus creating partitions >> + with the "p" separator even when the device source path does not >> + end with a number. >> <span class="since">Since 1.3.1</span></p></dd> >> <dt><code>dir</code></dt> >> <dd>Provides the source for pools backed by directories (pool >> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c >> index 666ad03..07a04f1 100644 >> --- a/src/storage/storage_backend_disk.c >> +++ b/src/storage/storage_backend_disk.c >> @@ -95,6 +95,29 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, >> virReportError(VIR_ERR_INVALID_ARG, >> _("invalid partition name '%s', expected '%s'"), >> vol->name, partname); >> + >> + /* Let's see if we by chance parthelper created a name that won't be >> + * found later when we try to delete. We tell parthelper to add a 'p' >> + * to the output via the part_separator flag, but if devmapper has >> + * user_friendly_names set, the creation won't happen that way, thus >> + * our deletion will fail because the name we generated is wrong. >> + * Check for our conditions and see if the generated name is the >> + * same as StablePath returns and has the 'p' in it */ >> + if (pool->def->source.devices[0].part_separator == >> + VIR_TRISTATE_BOOL_YES && >> + !virIsDevMapperDevice(vol->target.path) && >> + STREQ(groups[0], vol->target.path) && >> + (tmp = strrchr(groups[0], 'p'))) { >> + >> + /* If we remove the 'p' from groups[0] and the resulting >> + * device is a devmapper device, then we know parthelper >> + * was told to create the wrong name based on the results. >> + * So just remove the 'p' from the vol->target.path too. */ >> + memmove(tmp, tmp + 1, strlen(tmp)); >> + if (virIsDevMapperDevice(groups[0]) && >> + (tmp = strrchr(vol->target.path, 'p'))) >> + memmove(tmp, tmp + 1, strlen(tmp)); >> + } >> return -1; >> } >> >> > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list