10:05 a.m.
When using logical pools, we had to trust the target->pth provided.
This parameter, however, can be completely ommited and we can get the
correct path using 'lvdisplay' command.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Notes:
I'm not sure we can drop the target.path from the XML (see tests), so
another variant is to keep it there the same way it was defined by
user/mgmt app. If that's desired, mentioning it with an ack is enough
for me to know I should drop the conf/storage_conf.c hunk as well as
the hunks patching tests.
configure.ac | 4 ++
src/conf/storage_conf.c | 19 +++---
src/storage/storage_backend_logical.c | 79 +++++++++++++++-------
tests/storagepoolxml2xmlin/pool-logical-create.xml | 1 -
tests/storagepoolxml2xmlin/pool-logical.xml | 1 -
.../storagepoolxml2xmlout/pool-logical-create.xml | 1 -
tests/storagepoolxml2xmlout/pool-logical.xml | 1 -
7 files changed, 67 insertions(+), 39 deletions(-)
diff --git a/configure.ac b/configure.ac
index 5d1bc6b..753139d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1603,6 +1603,7 @@ if test "$with_storage_lvm" = "yes" || test
"$with_storage_lvm" = "check"; then
AC_PATH_PROG([PVS], [pvs], [], [$PATH:/sbin:/usr/sbin])
AC_PATH_PROG([VGS], [vgs], [], [$PATH:/sbin:/usr/sbin])
AC_PATH_PROG([LVS], [lvs], [], [$PATH:/sbin:/usr/sbin])
+ AC_PATH_PROG([LVDISPLAY], [lvdisplay], [], [$PATH:/sbin:/usr/sbin])
if test "$with_storage_lvm" = "yes" ; then
if test -z "$PVCREATE" ; then AC_MSG_ERROR([We need pvcreate for LVM
storage driver]) ; fi
@@ -1617,6 +1618,7 @@ if test "$with_storage_lvm" = "yes" || test
"$with_storage_lvm" = "check"; then
if test -z "$PVS" ; then AC_MSG_ERROR([We need pvs for LVM storage driver])
; fi
if test -z "$VGS" ; then AC_MSG_ERROR([We need vgs for LVM storage driver])
; fi
if test -z "$LVS" ; then AC_MSG_ERROR([We need lvs for LVM storage driver])
; fi
+ if test -z "$LVDISPLAY" ; then AC_MSG_ERROR([We need lvdisplay for LVM
storage driver]) ; fi
else
if test -z "$PVCREATE" ; then with_storage_lvm=no ; fi
if test -z "$VGCREATE" ; then with_storage_lvm=no ; fi
@@ -1630,6 +1632,7 @@ if test "$with_storage_lvm" = "yes" || test
"$with_storage_lvm" = "check"; then
if test -z "$PVS" ; then with_storage_lvm=no ; fi
if test -z "$VGS" ; then with_storage_lvm=no ; fi
if test -z "$LVS" ; then with_storage_lvm=no ; fi
+ if test -z "$LVDISPLAY" ; then with_storage_lvm=no ; fi
if test "$with_storage_lvm" = "check" ; then with_storage_lvm=yes
; fi
fi
@@ -1648,6 +1651,7 @@ if test "$with_storage_lvm" = "yes" || test
"$with_storage_lvm" = "check"; then
AC_DEFINE_UNQUOTED([PVS],["$PVS"],[Location of pvs program])
AC_DEFINE_UNQUOTED([VGS],["$VGS"],[Location of vgs program])
AC_DEFINE_UNQUOTED([LVS],["$LVS"],[Location of lvs program])
+ AC_DEFINE_UNQUOTED([LVDISPLAY],["$LVDISPLAY"],[Location of lvdisplay
program])
fi
fi
AM_CONDITIONAL([WITH_STORAGE_LVM], [test "$with_storage_lvm" =
"yes"])
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index cc3d3d9..948e190 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1,7 +1,7 @@
/*
* storage_conf.c: config handling for storage driver
*
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2013 Red Hat, Inc.
* Copyright (C) 2006-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -947,15 +947,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
/* When we are working with a virtual disk we can skip the target
* path and permissions */
if (!(options->flags & VIR_STORAGE_POOL_SOURCE_NETWORK)) {
- if (!(target_path = virXPathString("string(./target/path)", ctxt))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing storage pool target path"));
- goto error;
+ if (ret->type != VIR_STORAGE_POOL_LOGICAL) {
+ if (!(target_path = virXPathString("string(./target/path)", ctxt)))
{
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing storage pool target path"));
+ goto error;
+ }
+ ret->target.path = virFileSanitizePath(target_path);
+ if (!ret->target.path)
+ goto error;
}
- ret->target.path = virFileSanitizePath(target_path);
- if (!ret->target.path)
- goto error;
-
if (virStorageDefParsePerms(ctxt, &ret->target.perms,
"./target/permissions",
DEFAULT_POOL_PERM_MODE) < 0)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 944aa0e..66a4e42 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -45,6 +45,37 @@
#define PV_BLANK_SECTOR_SIZE 512
+static char *
+virStorageBackendGetVolPath(const char *poolname, const char *volname)
+{
+ char *start = NULL;
+ char *lvpath = NULL;
+ char *output = NULL;
+
+ virCommandPtr cmd = virCommandNewArgList(LVDISPLAY,
+ "--columns",
+ "--options", "lv_path",
+ "--noheadings",
+ "--unbuffered",
+ NULL);
+
+ virCommandAddArgFormat(cmd, "%s/%s", poolname, volname);
+ virCommandSetOutputBuffer(cmd, &output);
+
+ if (virCommandRun(cmd, NULL) < 0 ||
+ !(start = strchr(output, '/'))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get LV
path"));
+ goto cleanup;
+ }
+
+ ignore_value(VIR_STRDUP(lvpath, start));
+
+ cleanup:
+ VIR_FREE(output);
+ virCommandFree(cmd);
+ return lvpath;
+}
+
static int
virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
int on)
@@ -116,11 +147,8 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
}
if (vol->target.path == NULL) {
- if (virAsprintf(&vol->target.path, "%s/%s",
- pool->def->target.path, vol->name) < 0) {
- virReportOOMError();
+ if (VIR_STRDUP(vol->target.path, groups[9]) < 0)
goto cleanup;
- }
}
/* Skips the backingStore of lv created with "--virtualsize",
@@ -130,12 +158,11 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
* (lvs outputs "[$lvname_vorigin] for field "origin" if the
* lv is created with "--virtualsize").
*/
- if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] !=
'[')) {
- if (virAsprintf(&vol->backingStore.path, "%s/%s",
- pool->def->target.path, groups[1]) < 0) {
- virReportOOMError();
+ if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] !=
'[')) {
+ vol->backingStore.path =
virStorageBackendGetVolPath(pool->def->source.name,
+ groups[1]);
+ if (!vol->backingStore.path)
goto cleanup;
- }
vol->backingStore.format = VIR_STORAGE_POOL_LOGICAL_LVM2;
}
@@ -277,21 +304,20 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
/*
- * # lvs --separator , --noheadings --units b --unbuffered --nosuffix --options \
- * "lv_name,origin,uuid,devices,seg_size,vg_extent_size,size,lv_attr"
VGNAME
+ * # lvs --separator '#' --noheadings --units b --unbuffered --nosuffix
--options "lv_name,origin,uuid,devices,seg_size,vg_extent_size,size,lv_path"
VGNAME
*
- *
RootLV,,06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky,/dev/hda2(0),5234491392,33554432,5234491392,-wi-ao
- *
SwapLV,,oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M,/dev/hda2(156),1040187392,33554432,1040187392,-wi-ao
- *
Test2,,3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR,/dev/hda2(219),1073741824,33554432,1073741824,owi-a-
- *
Test3,,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(251),2181038080,33554432,2181038080,-wi-a-
- *
Test3,Test2,UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht,/dev/hda2(187),1040187392,33554432,1040187392,swi-a-
+ *
RootLV##06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky#/dev/hda2(0)#5234491392#33554432#5234491392#/dev/VGNAME/RootLV
+ *
SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#1040187392#33554432#1040187392#/dev/VGNAME/SwapLV
+ *
Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#1073741824#33554432#1073741824#/dev/VGNAME/Test2
+ *
Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#2181038080#33554432#2181038080#/dev/VGNAME/Test3
+ *
Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#1040187392#33554432#1040187392#/dev/VGNAME/Test3
*
* Pull out name, origin, & uuid, device, device extent start #,
* segment size, extent size, size, attrs
*
* NB can be multiple rows per volume if they have many extents
*
- * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing "," on each
line
+ * NB lvs from some distros (e.g. SLES10 SP2) outputs trailing separator on each
line
*
* NB Encrypted logical volumes can print ':' in their name, so it is
* not a suitable separator (rhbz 470693).
@@ -314,7 +340,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
"--unbuffered",
"--nosuffix",
"--options",
-
"lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr",
+
"lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_path",
pool->def->source.name,
NULL);
if (virStorageBackendRunProgRegex(pool,
@@ -702,6 +728,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
int fd = -1;
virCommandPtr cmd = NULL;
virErrorPtr err;
+ bool created = false;
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -717,13 +744,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
VIR_FREE(vol->target.path);
}
- if (virAsprintf(&vol->target.path, "%s/%s",
- pool->def->target.path,
- vol->name) == -1) {
- virReportOOMError();
- return -1;
- }
-
cmd = virCommandNewArgList(LVCREATE,
"--name", vol->name,
NULL);
@@ -742,9 +762,15 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
if (virCommandRun(cmd, NULL) < 0)
goto error;
+ created = true;
virCommandFree(cmd);
cmd = NULL;
+ vol->target.path = virStorageBackendGetVolPath(pool->def->source.name,
+ vol->name);
+ if (!vol->target.path)
+ goto error;
+
if ((fd = virStorageBackendVolOpen(vol->target.path)) < 0)
goto error;
@@ -784,7 +810,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
error:
err = virSaveLastError();
VIR_FORCE_CLOSE(fd);
- virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
+ if (created)
+ virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
virCommandFree(cmd);
virSetError(err);
virFreeError(err);
diff --git a/tests/storagepoolxml2xmlin/pool-logical-create.xml
b/tests/storagepoolxml2xmlin/pool-logical-create.xml
index 4c67089..e1bb4db 100644
--- a/tests/storagepoolxml2xmlin/pool-logical-create.xml
+++ b/tests/storagepoolxml2xmlin/pool-logical-create.xml
@@ -10,7 +10,6 @@
<device path="/dev/sdb3"/>
</source>
<target>
- <path>/dev/HostVG</path>
<permissions>
<mode>0700</mode>
<owner>0</owner>
diff --git a/tests/storagepoolxml2xmlin/pool-logical.xml
b/tests/storagepoolxml2xmlin/pool-logical.xml
index c4bfa07..307e2ab 100644
--- a/tests/storagepoolxml2xmlin/pool-logical.xml
+++ b/tests/storagepoolxml2xmlin/pool-logical.xml
@@ -9,7 +9,6 @@
<format type='lvm2'/>
</source>
<target>
- <path>/dev/HostVG</path>
<permissions>
<mode>0700</mode>
<owner>0</owner>
diff --git a/tests/storagepoolxml2xmlout/pool-logical-create.xml
b/tests/storagepoolxml2xmlout/pool-logical-create.xml
index 7413f1c..fbce90e 100644
--- a/tests/storagepoolxml2xmlout/pool-logical-create.xml
+++ b/tests/storagepoolxml2xmlout/pool-logical-create.xml
@@ -12,7 +12,6 @@
<format type='lvm2'/>
</source>
<target>
- <path>/dev/HostVG</path>
<permissions>
<mode>0700</mode>
<owner>0</owner>
diff --git a/tests/storagepoolxml2xmlout/pool-logical.xml
b/tests/storagepoolxml2xmlout/pool-logical.xml
index 07860ef..7fad0d8 100644
--- a/tests/storagepoolxml2xmlout/pool-logical.xml
+++ b/tests/storagepoolxml2xmlout/pool-logical.xml
@@ -9,7 +9,6 @@
<format type='lvm2'/>
</source>
<target>
- <path>/dev/HostVG</path>
<permissions>
<mode>0700</mode>
<owner>0</owner>
--
1.8.2.1