storage: Fix virStorageBackendDiskDeleteVol for device mapper

Commit id 'df1011ca' modified virStorageBackendDiskDeleteVol to use
"dmsetup remove --force" to remove the volume, but left things in an
inconsistent state since the partition still existed on the disk and
only the device mapper device (/dev/dm-#) was removed.

Prior to commit '1895b421' (or '1ffd82bb' and '471e1c4e'), this could
go unnoticed since virStorageBackendDiskRefreshPool wasn't called.
However, the pool would be unusable since the /dev/dm-# device would
be removed even though the partition was not removed unless a multipathd
restart reset the link. That would of course make the volume appear again
in the pool after a refresh or pool start after libvirt reload.

This patch removes the 'dmsetup' logic and re-implements the partition
deletion logic for device mapper devices. The removal of the partition
via 'parted rm --script #' will cause udev device change logic to allow
multipathd to handle removing the dm-* device associated with the partition.

configure.ac

@@ -1958,19 +1958,12 @@ LIBPARTED_LIBS=
 if test "$with_storage_disk" = "yes" ||
    test "$with_storage_disk" = "check"; then
   AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin])
-  AC_PATH_PROG([DMSETUP], [dmsetup], [], [$PATH:/sbin:/usr/sbin])
   if test -z "$PARTED" ; then
     PARTED_FOUND=no
   else
     PARTED_FOUND=yes
   fi
 
-  if test -z "$DMSETUP" ; then
-    DMSETUP_FOUND=no
-  else
-    DMSETUP_FOUND=yes
-  fi
-
   if test "$PARTED_FOUND" = "yes" && test "x$PKG_CONFIG" != "x" ; then
     PKG_CHECK_MODULES([LIBPARTED], [libparted >= $PARTED_REQUIRED], [],
       [PARTED_FOUND=no])
@@ -1989,12 +1982,12 @@ if test "$with_storage_disk" = "yes" ||
   fi
 
   if test "$with_storage_disk" = "yes" &&
-     test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then
-    AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver])
+     test "$PARTED_FOUND" != "yes"; then
+    AC_MSG_ERROR([Need parted for disk storage driver])
   fi
 
   if test "$with_storage_disk" = "check"; then
-    if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then
+    if test "$PARTED_FOUND" != "yes"; then
       with_storage_disk=no
     else
       with_storage_disk=yes
@@ -2006,8 +1999,6 @@ if test "$with_storage_disk" = "yes" ||
       [whether Disk backend for storage driver is enabled])
     AC_DEFINE_UNQUOTED([PARTED],["$PARTED"],
       [Location or name of the parted program])
-    AC_DEFINE_UNQUOTED([DMSETUP],["$DMSETUP"],
-      [Location or name of the dmsetup program])
   fi
 fi
 AM_CONDITIONAL([WITH_STORAGE_DISK], [test "$with_storage_disk" = "yes"])

src/storage/parthelper.c

@@ -83,6 +83,8 @@ int main(int argc, char **argv)
         return 1;
     }
 
+    /* NB: Changes to the following algorithm will need corresponding
+     * changes to virStorageBackendDiskDeleteVol */
     path = argv[1];
     if (virIsDevMapperDevice(path)) {
         /* If the path ends with a number or we explicitly request it for

src/storage/storage_backend_disk.c

@@ -799,6 +799,31 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool,
 }
 
 
+/* virStorageBackendDiskDeleteVol
+ * @conn: Pointer to a libvirt connection
+ * @pool: Pointer to the storage pool
+ * @vol: Pointer to the volume definition
+ * @flags: flags (unused for now)
+ *
+ * This API will remove the disk volume partition either from direct
+ * API call or as an error path during creation when the partition
+ * name provided during create doesn't match the name read from
+ * virStorageBackendDiskReadPartitions.
+ *
+ * For a device mapper device, device respresentation is dependant upon
+ * device mapper configuration, but the general rule of thumb is that at
+ * creation if a device name ends with a number, then a partition separator
+ * "p" is added to the created name; otherwise, if the device name doesn't
+ * end with a number, then there is no partition separator. This name is
+ * what ends up in the vol->target.path. This ends up being a link to a
+ * /dev/mapper/dm-# device which cannot be used in the algorithm to determine
+ * which partition to remove, but a properly handled target.path can be.
+ *
+ * For non device mapper devices, just need to resolve the link of the
+ * vol->target.path in order to get the path.
+ *
+ * Returns 0 on success, -1 on failure with error message set.
+ */
 static int
 virStorageBackendDiskDeleteVol(virConnectPtr conn,
                                virStoragePoolObjPtr pool,
@@ -807,7 +832,9 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
 {
     char *part_num = NULL;
     char *devpath = NULL;
-    char *dev_name, *srcname;
+    char *dev_name;
+    char *src_path = pool->def->source.devices[0].path;
+    char *srcname = last_component(src_path);
     virCommandPtr cmd = NULL;
     bool isDevMapperDevice;
     int rc = -1;
@@ -817,56 +844,60 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn,
     if (!vol->target.path) {
         virReportError(VIR_ERR_INVALID_ARG,
                        _("volume target path empty for source path '%s'"),
-                      pool->def->source.devices[0].path);
+                      src_path);
         return -1;
     }
 
-    if (virFileResolveLink(vol->target.path, &devpath) < 0) {
-        virReportSystemError(errno,
-                             _("Couldn't read volume target path '%s'"),
-                             vol->target.path);
-        goto cleanup;
+    /* NB: This is the corollary to the algorithm in libvirt_parthelper
+     *     (parthelper.c) that is used to generate the target.path name
+     *     for use by libvirt. Changes to either, need to be reflected
+     *     in both places */
+    isDevMapperDevice = virIsDevMapperDevice(vol->target.path);
+    if (isDevMapperDevice) {
+        dev_name = last_component(vol->target.path);
+    } else {
+        if (virFileResolveLink(vol->target.path, &devpath) < 0) {
+            virReportSystemError(errno,
+                                 _("Couldn't read volume target path '%s'"),
+                                 vol->target.path);
+            goto cleanup;
+        }
+        dev_name = last_component(devpath);
     }
 
-    dev_name = last_component(devpath);
-    srcname = last_component(pool->def->source.devices[0].path);
     VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname);
 
-    isDevMapperDevice = virIsDevMapperDevice(devpath);
-
-    if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) {
+    if (!STRPREFIX(dev_name, srcname)) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Volume path '%s' did not start with parent "
                          "pool source device name."), dev_name);
         goto cleanup;
     }
 
-    if (!isDevMapperDevice) {
-        part_num = dev_name + strlen(srcname);
+    part_num = dev_name + strlen(srcname);
 
-        if (*part_num == 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("cannot parse partition number from target "
-                             "'%s'"), dev_name);
-            goto cleanup;
-        }
+    /* For device mapper and we have a partition character 'p' as the
+     * current character, let's move beyond that before checking part_num */
+    if (isDevMapperDevice && *part_num == 'p')
+        part_num++;
 
-        /* eg parted /dev/sda rm 2 */
-        cmd = virCommandNewArgList(PARTED,
-                                   pool->def->source.devices[0].path,
-                                   "rm",
-                                   "--script",
-                                   part_num,
-                                   NULL);
-        if (virCommandRun(cmd, NULL) < 0)
-            goto cleanup;
-    } else {
-        cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath, NULL);
-
-        if (virCommandRun(cmd, NULL) < 0)
-            goto cleanup;
+    if (*part_num == 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("cannot parse partition number from target "
+                         "'%s'"), dev_name);
+        goto cleanup;
     }
 
+    /* eg parted /dev/sda rm 2 or /dev/mapper/mpathc rm 2 */
+    cmd = virCommandNewArgList(PARTED,
+                               src_path,
+                               "rm",
+                               "--script",
+                               part_num,
+                               NULL);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+
     /* Refreshing the pool is the easiest option as LOGICAL and EXTENDED
      * partition allocation/capacity management is handled within
      * virStorageBackendDiskMakeDataVol and trying to redo that logic