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&lt;crobinso(a)redhat.com&gt; >> --- >> 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