[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] storage: lvm: use correct lv* command parameters



On 04/13/2012 09:03 AM, Osier Yang wrote:
> On 04/13/2012 07:50 PM, Cole Robinson wrote:
>> lvcreate want's the parent pool's name, not the pool path
>> lvchange and lvremove want lv specified as $vgname/$lvname
>>
>> This largely worked before because these commands strip off a
>> starting /dev. But https://bugzilla.redhat.com/show_bug.cgi?id=714986
>> is from a user using a 'nested VG' that was having problems.
>>
>> I couldn't find any info on nested LVM and the reporter never responded,
>> but I reproduced with XML that specified a valid source name, and
>> set target path to a symlink.
>>
>> Signed-off-by: Cole Robinson<crobinso redhat com>
>> ---
>>   src/storage/storage_backend_logical.c |   21 +++++++++++----------
>>   1 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_logical.c
>> b/src/storage/storage_backend_logical.c
>> index 6a235f6..9a91dd9 100644
>> --- a/src/storage/storage_backend_logical.c
>> +++ b/src/storage/storage_backend_logical.c
>> @@ -672,7 +672,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>>       char size[100];
>>       const char *cmdargvnew[] = {
>>           LVCREATE, "--name", vol->name, "-L", size,
>> -        pool->def->target.path, NULL
>> +        pool->def->source.name, NULL
> 
> This makes sense.
> 
>>       };
>>       const char *cmdargvsnap[] = {
>>           LVCREATE, "--name", vol->name, "-L", size,
>> @@ -778,23 +778,23 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn
>> ATTRIBUTE_UNUSED,
>>                                     unsigned int flags)
>>   {
>>       int ret = -1;
>> +    char *volpath = NULL;
>>
>>       virCommandPtr lvchange_cmd = NULL;
>>       virCommandPtr lvremove_cmd = NULL;
>>
>>       virCheckFlags(0, -1);
>>
>> -    virFileWaitForDevices();
>> +    if (virAsprintf(&volpath, "%s/%s",
>> +                    pool->def->source.name, vol->name)<  0) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>>
>> -    lvchange_cmd = virCommandNewArgList(LVCHANGE,
>> -                                        "-aln",
>> -                                        vol->target.path,
>> -                                        NULL);
>> +    virFileWaitForDevices();
>>
>> -    lvremove_cmd = virCommandNewArgList(LVREMOVE,
>> -                                        "-f",
>> -                                        vol->target.path,
>> -                                        NULL);
>> +    lvchange_cmd = virCommandNewArgList(LVCHANGE, "-aln", volpath, NULL);
>> +    lvremove_cmd = virCommandNewArgList(LVREMOVE, "-f", volpath, NULL);
> 
> I tried with both vol->target.path, and $vgname/$lvname, both
> of them work. So do we really need these changes?

They both work, just like lvcreate /dev/myvgname also works. But we do need
this if this 'nested lvm' thing actually exists as the reporter mentioned, or
user uses a symlink as the target path (I'm not saying that's a valid use case
but it's a data point).

Also the man pages for lvremove and lvchange but use that format in their
examples.

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]