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

[Osier Yang]

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.

Osier