[libvirt] [PATCH v2] logical: Check for existing vgname before building
https://bugzilla.redhat.com/show_bug.cgi?id=1373711 Add a check for an existing volume group name before trying to build the volume group. Since the process of building a vg involves wiping the first 512 bytes and using pvcreate on each source device path before creating the vg - we could conceivably overwrite something that we shouldn't be. Also, once a pv is part of a vg, the pvcreate would fail unless we chose to overwrite the volume. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Difference w/ v2 is that I'm taking a different approach - disallow the second pool-build since the vg already exists. NB: libvirt has no API to extend an existing vg - that's left to an admin anyway. src/storage/storage_backend_logical.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca05fe1..b241495 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -682,14 +682,31 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, unsigned int flags) { - virCommandPtr vgcmd; + virCommandPtr vgcmd = NULL; int fd; char zeros[PV_BLANK_SECTOR_SIZE]; int ret = -1; size_t i; + virStoragePoolSourceList sourceList; virCheckFlags(0, -1); + /* Let's make sure the about to be created vg doesn't already exist */ + memset(&sourceList, 0, sizeof(sourceList)); + sourceList.type = VIR_STORAGE_POOL_LOGICAL; + + if (virStorageBackendLogicalGetPoolSources(&sourceList) < 0) + return -1; + + for (i = 0; i < sourceList.nsources; i++) { + if (STREQ(sourceList.sources[i].name, pool->def->source.name)) { + virReportError(VIR_ERR_STORAGE_POOL_BUILT, + _("volume group with name '%s' already exists"), + pool->def->source.name); + goto cleanup; + } + } + memset(zeros, 0, sizeof(zeros)); vgcmd = virCommandNewArgList(VGCREATE, pool->def->source.name, NULL); @@ -751,6 +768,9 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, ret = 0; cleanup: + for (i = 0; i < sourceList.nsources; i++) + virStoragePoolSourceClear(&sourceList.sources[i]); + VIR_FREE(sourceList.sources); virCommandFree(vgcmd); return ret; } -- 2.7.4
On Wed, Nov 16, 2016 at 09:28:55AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1373711
Add a check for an existing volume group name before trying to build the volume group. Since the process of building a vg involves wiping the first 512 bytes and using pvcreate on each source device path before creating the vg - we could conceivably overwrite something that we shouldn't be.
That sounds like a bug in the pool build process. We should not be overwriting the source devices without the OVERWRITE flag.
Also, once a pv is part of a vg, the pvcreate would fail unless we chose to overwrite the volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
Difference w/ v2 is that I'm taking a different approach - disallow the second pool-build since the vg already exists. NB: libvirt has no API to extend an existing vg - that's left to an admin anyway.
src/storage/storage_backend_logical.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca05fe1..b241495 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -682,14 +682,31 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, unsigned int flags) { - virCommandPtr vgcmd; + virCommandPtr vgcmd = NULL; int fd; char zeros[PV_BLANK_SECTOR_SIZE]; int ret = -1; size_t i; + virStoragePoolSourceList sourceList;
virCheckFlags(0, -1);
+ /* Let's make sure the about to be created vg doesn't already exist */ + memset(&sourceList, 0, sizeof(sourceList)); + sourceList.type = VIR_STORAGE_POOL_LOGICAL; + + if (virStorageBackendLogicalGetPoolSources(&sourceList) < 0) + return -1; +
Rather than introducing one more place where we use this lovely function, I'd prefer to stick to non-destructive steps and let the lvm tools error out if the vg already exists. Jan
On 11/16/2016 09:50 AM, Ján Tomko wrote:
On Wed, Nov 16, 2016 at 09:28:55AM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1373711
Add a check for an existing volume group name before trying to build the volume group. Since the process of building a vg involves wiping the first 512 bytes and using pvcreate on each source device path before creating the vg - we could conceivably overwrite something that we shouldn't be.
That sounds like a bug in the pool build process. We should not be overwriting the source devices without the OVERWRITE flag.
I didn't check if the chicken (logical pool build function) or egg (OVERWRITE flag) came first in this instance, but since you asked... It seems overwrite was added with commit id '27758859' and the logical pool before that with commit id 'ac736602f' although there was commit to check the flags being 0 - commit id '64bd1b9d' probably slightly before the OVERWRITE flag was added. In any case, I think the 'assumption' in logical pool build has been we're overwriting no matter what because that's our only option. IOW: an optional parameter would become a required parameter, something I believe we cannot enforce.
Also, once a pv is part of a vg, the pvcreate would fail unless we chose to overwrite the volume.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
Difference w/ v2 is that I'm taking a different approach - disallow the second pool-build since the vg already exists. NB: libvirt has no API to extend an existing vg - that's left to an admin anyway.
src/storage/storage_backend_logical.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca05fe1..b241495 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -682,14 +682,31 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, unsigned int flags) { - virCommandPtr vgcmd; + virCommandPtr vgcmd = NULL; int fd; char zeros[PV_BLANK_SECTOR_SIZE]; int ret = -1; size_t i; + virStoragePoolSourceList sourceList;
virCheckFlags(0, -1);
+ /* Let's make sure the about to be created vg doesn't already exist */ + memset(&sourceList, 0, sizeof(sourceList)); + sourceList.type = VIR_STORAGE_POOL_LOGICAL; + + if (virStorageBackendLogicalGetPoolSources(&sourceList) < 0) + return -1; +
Rather than introducing one more place where we use this lovely function, I'd prefer to stick to non-destructive steps and let the lvm tools error out if the vg already exists.
Jan
The current mechanism is destructive insomuch as it will wipe the first 512 bytes clear on the device before failing on the pvcreate command. IDC either way, really. John
participants (2)
-
John Ferlan -
Ján Tomko