[PATCH v3] Make logical pools independent on target path

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

 



When using logical pools, we had to trust the target->path provided.
This parameter, however, can be completely ommited and we can get the
correct path using 'lvdisplay' command.  In order not to omit the
target.path completely, we rather default it to '/dev/<source.name>'
which is used to check the pool anyway.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=952973
Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---

Notes:
    v3:
     - just rebase
    v2:
     - don't drop target.path, but fix it instead to '/dev/<source.name>'
    
    Thanks to the change from v1, we can now safely drop all the hunks
    from logical backend and even the dependency on lvdisplay command.
    There might be some systems where the path is different and the part
    of this patch using lvdisplay command would make most of them work.
    However, checkPool() still depends on '/dev/<source.name>' and I
    haven't found any other way how to fix that, so feel free to ACK just
    the {conf,docs,tests}/ part in case you think that we shouldn't try to
    support anything else than '/dev/<source.name>'.

 configure.ac                                       |  4 +
 docs/schemas/storagepool.rng                       | 13 +++-
 src/conf/storage_conf.c                            | 19 +++--
 src/storage/storage_backend_logical.c              | 88 ++++++++++++++--------
 src/storage/storage_driver.c                       |  2 +-
 tests/storagepoolxml2xmlin/pool-logical-create.xml |  2 +-
 tests/storagepoolxml2xmlin/pool-logical-nopath.xml | 18 +++++
 .../storagepoolxml2xmlout/pool-logical-nopath.xml  | 19 +++++
 tests/storagepoolxml2xmltest.c                     |  1 +
 9 files changed, 125 insertions(+), 41 deletions(-)
 create mode 100644 tests/storagepoolxml2xmlin/pool-logical-nopath.xml
 create mode 100644 tests/storagepoolxml2xmlout/pool-logical-nopath.xml

diff --git a/configure.ac b/configure.ac
index b5af0d3..967e70a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1601,6 +1601,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
@@ -1615,6 +1616,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
@@ -1628,6 +1630,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
@@ -1646,6 +1649,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/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index 3c2158a..1b3f4bc 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -62,7 +62,7 @@
     <ref name='commonmetadata'/>
     <ref name='sizing'/>
     <ref name='sourcelogical'/>
-    <ref name='target'/>
+    <ref name='targetlogical'/>
   </define>

   <define name='pooldisk'>
@@ -207,6 +207,17 @@
     </element>
   </define>

+  <define name='targetlogical'>
+    <element name='target'>
+      <optional>
+        <element name='path'>
+          <ref name='absFilePath'/>
+        </element>
+      </optional>
+      <ref name='permissions'/>
+    </element>
+  </define>
+
   <define name='sourceinfohost'>
     <oneOrMore>
       <element name='host'>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 524a4d6..8fe9e3a 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
@@ -953,15 +953,22 @@ 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 (virAsprintf(&target_path, "/dev/%s", ret->source.name) < 0) {
+                virReportOOMError();
+                goto error;
+            }
+        } else {
+            target_path = virXPathString("string(./target/path)", ctxt);
+            if (!target_path) {
+                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;
-
         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 416a042..310286d 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)
@@ -113,8 +144,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
     }

     if (vol->target.path == NULL) {
-        if (virAsprintf(&vol->target.path, "%s/%s",
-                        pool->def->target.path, vol->name) < 0)
+        if (VIR_STRDUP(vol->target.path, groups[10]) < 0)
             goto cleanup;
     }

@@ -125,9 +155,10 @@ 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)
+    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;
@@ -263,21 +294,21 @@ 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_attr,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#-wi-ao#/dev/VGNAME/RootLV
+     * SwapLV##oHviCK-8Ik0-paqS-V20c-nkhY-Bm1e-zgzU0M#/dev/hda2(156)#1040187392#33554432#1040187392#-wi-ao#/dev/VGNAME/SwapLV
+     * Test2##3pg3he-mQsA-5Sui-h0i6-HNmc-Cz7W-QSndcR#/dev/hda2(219)#1073741824#33554432#1073741824#owi-a-#/dev/VGNAME/Test2
+     * Test3##UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(251)#2181038080#33554432#2181038080#-wi-a-#/dev/VGNAME/Test3
+     * Test3#Test2#UB5hFw-kmlm-LSoX-EI1t-ioVd-h7GL-M0W8Ht#/dev/hda2(187)#1040187392#33554432#1040187392#swi-a-#/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).
@@ -285,7 +316,7 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
      *    striped, so "," is not a suitable separator either (rhbz 727474).
      */
     const char *regexes[] = {
-       "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#?\\s*$"
+       "^\\s*(\\S+)#(\\S*)#(\\S+)#(\\S+)#(\\S+)#([0-9]+)#(\\S+)#([0-9]+)#([0-9]+)#(\\S+)#(\\S+)#?\\s*$"
     };
     int vars[] = {
         10
@@ -300,7 +331,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_attr,lv_path",
                                pool->def->source.name,
                                NULL);
     if (virStorageBackendRunProgRegex(pool,
@@ -454,17 +485,7 @@ virStorageBackendLogicalCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                   virStoragePoolObjPtr pool,
                                   bool *isActive)
 {
-    char *path;
-
-    *isActive = false;
-    if (virAsprintf(&path, "/dev/%s", pool->def->source.name) < 0)
-        return -1;
-
-    if (access(path, F_OK) == 0)
-        *isActive = true;
-
-    VIR_FREE(path);
-
+    *isActive = (access(pool->def->target.path, F_OK) == 0);
     return 0;
 }

@@ -684,6 +705,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,
@@ -699,11 +721,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
         VIR_FREE(vol->target.path);
     }

-    if (virAsprintf(&vol->target.path, "%s/%s",
-                    pool->def->target.path,
-                    vol->name) == -1)
-        return -1;
-
     cmd = virCommandNewArgList(LVCREATE,
                                "--name", vol->name,
                                NULL);
@@ -722,9 +739,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;

@@ -764,7 +787,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/src/storage/storage_driver.c b/src/storage/storage_driver.c
index a8eb731..43bf6de 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1,7 +1,7 @@
 /*
  * storage_driver.c: core driver for storage APIs
  *
- * 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
diff --git a/tests/storagepoolxml2xmlin/pool-logical-create.xml b/tests/storagepoolxml2xmlin/pool-logical-create.xml
index 4c67089..fd551e0 100644
--- a/tests/storagepoolxml2xmlin/pool-logical-create.xml
+++ b/tests/storagepoolxml2xmlin/pool-logical-create.xml
@@ -10,7 +10,7 @@
     <device path="/dev/sdb3"/>
   </source>
   <target>
-    <path>/dev/HostVG</path>
+    <path>/invalid/nonexistent/path</path>
     <permissions>
       <mode>0700</mode>
       <owner>0</owner>
diff --git a/tests/storagepoolxml2xmlin/pool-logical-nopath.xml b/tests/storagepoolxml2xmlin/pool-logical-nopath.xml
new file mode 100644
index 0000000..307e2ab
--- /dev/null
+++ b/tests/storagepoolxml2xmlin/pool-logical-nopath.xml
@@ -0,0 +1,18 @@
+<pool type='logical'>
+  <name>HostVG</name>
+  <uuid>1c13165a-d0f4-3aee-b447-30fb38789091</uuid>
+  <capacity>99891544064</capacity>
+  <allocation>99220455424</allocation>
+  <available>671088640</available>
+  <source>
+    <name>HostVG</name>
+    <format type='lvm2'/>
+  </source>
+  <target>
+    <permissions>
+      <mode>0700</mode>
+      <owner>0</owner>
+      <group>0</group>
+    </permissions>
+  </target>
+</pool>
diff --git a/tests/storagepoolxml2xmlout/pool-logical-nopath.xml b/tests/storagepoolxml2xmlout/pool-logical-nopath.xml
new file mode 100644
index 0000000..07860ef
--- /dev/null
+++ b/tests/storagepoolxml2xmlout/pool-logical-nopath.xml
@@ -0,0 +1,19 @@
+<pool type='logical'>
+  <name>HostVG</name>
+  <uuid>1c13165a-d0f4-3aee-b447-30fb38789091</uuid>
+  <capacity unit='bytes'>0</capacity>
+  <allocation unit='bytes'>0</allocation>
+  <available unit='bytes'>0</available>
+  <source>
+    <name>HostVG</name>
+    <format type='lvm2'/>
+  </source>
+  <target>
+    <path>/dev/HostVG</path>
+    <permissions>
+      <mode>0700</mode>
+      <owner>0</owner>
+      <group>0</group>
+    </permissions>
+  </target>
+</pool>
diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c
index 53a7f83..d59cff9 100644
--- a/tests/storagepoolxml2xmltest.c
+++ b/tests/storagepoolxml2xmltest.c
@@ -87,6 +87,7 @@ mymain(void)
     DO_TEST("pool-dir");
     DO_TEST("pool-fs");
     DO_TEST("pool-logical");
+    DO_TEST("pool-logical-nopath");
     DO_TEST("pool-logical-create");
     DO_TEST("pool-disk");
     DO_TEST("pool-iscsi");
-- 
1.8.3.2

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]