[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH] Make logical pools independent on target path



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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]