[libvirt] [PATCH v2] logical: Check for existing vgname before building

[John Ferlan]

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 at 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