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&gt; --- 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