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?

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&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 Makes sense, and ACK then.