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

[Cole Robinson]

On 04/15/2012 09:22 PM, Osier Yang wrote:
> On 2012幎04月13日 22:09, Cole Robinson wrote:
>> 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 at 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
> 
> Makes sense, and ACK then.
> 

Thanks, pushed now.

- Cole