[libvirt] [PATCH] storage: lvm: use correct lv* command parameters
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 }; 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); if (virCommandRun(lvremove_cmd, NULL) < 0) { if (virCommandRun(lvchange_cmd, NULL) < 0) { @@ -807,6 +807,7 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, ret = 0; cleanup: + VIR_FREE(volpath); virCommandFree(lvchange_cmd); virCommandFree(lvremove_cmd); return ret; -- 1.7.7.6
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?
if (virCommandRun(lvremove_cmd, NULL)< 0) { if (virCommandRun(lvchange_cmd, NULL)< 0) { @@ -807,6 +807,7 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
ret = 0; cleanup: + VIR_FREE(volpath); virCommandFree(lvchange_cmd); virCommandFree(lvremove_cmd); return ret;
Osier
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
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@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
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@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
participants (2)
-
Cole Robinson -
Osier Yang