On 03/06/13 23:05, Martin Kletzander wrote:
When using logical pools, we had to trust the target->pth provided.
s/pth/path/,
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@xxxxxxxxxx>
---
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.
How about one file a bug "The created volume is not under specified pool's
target path"? Since with this patch, the specified pool's target path is
silently
ignored.
If you have a good answer/reason/fix for above question, I see no problem of
ignore the pool->target.path, but the path you got from lvdisplay (with
vol name
is truncated) should be reflected to the pool's XML.
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;
RNG schema needs to be changed too..
}
- 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",
I won't want to see what I added (lv_attr) is removed by mistake, :-)
+ "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>
At least one test should have the specified target path, to make sure
the old XML still work..
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list