[PATCH 00/10] fixes for kpartx -d

[PATCH 00/10] fixes for kpartx -d

[Martin Wilck]

Working on a bug report about kpartx not properly removing
partitions for loop devices, I realized a number of glitches
and improperly handled corner cases in the kpartx code for
deleting partitions. Some mappings are not deleted although
they should be, and others are deleted although that is clearly
wrong.

This patch series fixes the issues I found. The series starts
with a test program demonstrating the problems. The program
succeeds only after all patches of this series are applied.

Here is my summary of what I think how kpartx should behave:

  1) kpartx should delete all mappings it created beforehand.
  2) kpartx should handle partitions on dm devices and other devices
     (e.g. loop devices) equally well.
  3) kpartx should only delete "partitions", which are single-target
     linear mappings into a block device. Other maps should not be touched.
  4) kpartx should only delete mappings it created itself beforehand.
     In particular, it shouldn't delete LVM LVs, even if they are fully
     contained in the block device at hand and thus look like partitions
     in the first place. (For historical compatibility reasons, allow
     such mappings to be deleted with the -f/--force flag).
  5) DM map names may be changed, thus kpartx shouldn't rely on them to
     check whether a mapping is a partition of a particular device. It is
     legal for a partition of /dev/loop0 to be named "loop0".

One patch has an obvious libdevmapper equivalent and is therefore
included although this series is otherwise focused only on kpartx.

Feedback is welcome.

Martin Wilck (10):
  kpartx: test-kpartx: new unit test program
  kpartx: avoid ioctl error for loop devices
  kpartx: remove is_loop_device
  kpartx: dm_remove_partmaps: support non-dm devices
  kpartx: dm_devn: return error for non-existent device
  kpartx: don't treat multi-linear mappings as partitions
  libmultipath: don't treat multi-linear mappings as partitions
  kpartx: use partition UUID for non-DM devices
  kpartx: use absolute path for regular files
  kpartx: find_loop_by_file: use sysfs

 kpartx/devmapper.c       |  39 +++++---
 kpartx/devmapper.h       |   2 +-
 kpartx/kpartx.c          |  42 +++++++-
 kpartx/lopart.c          |  73 ++++++--------
 kpartx/lopart.h          |   1 -
 kpartx/test-kpartx       | 253 +++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/devmapper.c |  15 +--
 7 files changed, 356 insertions(+), 69 deletions(-)
 create mode 100755 kpartx/test-kpartx

-- 
2.12.2

[PATCH 01/10] kpartx: test-kpartx: new unit test program

[Martin Wilck]

This is a unit test program for kpartx, in particular for deleting
partitions.

NOTE: This test program fails with current kpartx; full patch series
needed to make it work.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/test-kpartx | 253 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 253 insertions(+)
 create mode 100755 kpartx/test-kpartx

diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
new file mode 100755
index 00000000..03e8030e
--- /dev/null
+++ b/kpartx/test-kpartx
@@ -0,0 +1,253 @@
+#! /bin/bash
+
+# This is a unit test program for kpartx, in particular for deleting partitions.
+#
+# The rationale is the following:
+#
+#  1) kpartx should delete all mappings it created beforehand.
+#  2) kpartx should handle partitions on dm devices and other devices
+#     (e.g. loop devices) equally well.
+#  3) kpartx should only delete "partitions", which are single-target
+#     linear mappings into a block device. Other maps should not be touched.
+#  4) kpartx should only delete mappings it created itself beforehand.
+#     In particular, it shouldn't delete LVM LVs, even if they are fully
+#     contained in the block device at hand and thus look like partitions
+#     in the first place. (For historical compatibility reasons, we allow
+#     such mappings to be deleted with the -f/--force flag).
+#  5) DM map names may be changed, thus kpartx shouldn't rely on them to
+#     check whether a mapping is a partition of a particular device. It is
+#     legal for a partition of /dev/loop0 to be named "loop0".
+
+# Note: This program tries hard to clean up, but if tests fail,
+# stale DM or loop devices may keep lurking around.
+
+# Set WORKDIR in environment to existing dir to for persistence
+# WARNING:  exisiting files will be truncated.
+# If empty, test will be done in temporary dir
+: ${WORKDIR:=}
+# Set this environment variable to test an alternative kpartx executable
+: ${KPARTX:=}
+# Time to wait for device nodes to appear (microseconds)
+: ${WAIT_US:=100000}
+
+# IMPORTANT: The ERR trap is essential for this program to work correctly!
+trap 'LINE=$LINENO; trap - ERR; echo "== error in $BASH_COMMAND on line $LINE ==" >&2; exit 1' ERR
+trap 'cleanup' 0
+
+CLEANUP=:
+cleanup() {
+    trap - ERR
+    trap - 0
+    if [[ $OK ]]; then
+	echo == all tests completed successfully == >&2
+    else
+	echo == step $STEP failed == >&2
+    fi
+    eval "$CLEANUP" &>/dev/null
+}
+
+push_cleanup() {
+    CLEANUP="$@;$CLEANUP"
+}
+
+pop_cleanup() {
+    # CAUTION: simplistic
+    CLEANUP=${CLEANUP#*;}
+}
+
+step() {
+    STEP="$@"
+    echo == Test step: $STEP == >&2
+}
+
+mk_partitions() {
+    parted -s $1 mklabel msdos
+    parted -s -- $1 mkpart prim ext2 1MiB -1s
+}
+
+step preparation
+
+[[ $UID -eq 0 ]]
+[[ $KPARTX ]] || {
+    if [[ -x $PWD/kpartx/kpartx ]]; then
+	KPARTX=$PWD/kpartx/kpartx
+    else
+	KPARTX=$(which kpartx)
+    fi
+}
+[[ $KPARTX ]]
+
+FILE1=kpartx1
+FILE2=kpartx2
+FILE3=kpartx3
+SIZE=$((1024*1024*1024))  # use bytes as units here
+SECTSIZ=512
+OFFS=32                # offset of linear mapping into dev, sectors
+VG=kpvg  # volume group name
+LV=kplv  # logical vol name
+LVMCONF='devices { filter = [ "a|/dev/loop.*|", r".*" ] }'
+
+OK=
+
+[[ $WORKDIR ]] || {
+    WORKDIR=$(mktemp -d /tmp/kpartx-XXXXXX)
+    push_cleanup 'rm -rf $WORKDIR'
+}
+
+push_cleanup "cd $PWD"
+cd "$WORKDIR"
+
+step "create loop devices"
+truncate -s $SIZE $FILE1
+truncate -s $SIZE $FILE2
+truncate -s $SIZE $FILE3
+
+LO1=$(losetup -f $FILE1 --show)
+push_cleanup 'losetup -d $LO1'
+LO2=$(losetup -f $FILE2 --show)
+push_cleanup 'losetup -d $LO2'
+LO3=$(losetup -f $FILE3 --show)
+push_cleanup 'losetup -d $LO3'
+
+[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
+DEV1=$(stat -c "%t:%T" $LO1)
+DEV2=$(stat -c "%t:%T" $LO2)
+DEV3=$(stat -c "%t:%T" $LO3)
+
+usleep $WAIT_US
+
+step "create DM devices (spans)"
+# Create two linear mappings spanning two loopdevs.
+# One of them gets a pathological name colliding with
+# the loop device name.
+# These mappings must not be removed by kpartx.
+# They also serve as DM devices to test partition removal on those.
+
+TABLE="\
+0 $((SIZE/SECTSIZ-OFFS)) linear $DEV1 $OFFS
+$((SIZE/SECTSIZ-OFFS)) $((SIZE/SECTSIZ-OFFS)) linear $DEV2 $OFFS"
+
+SPAN1=kpt
+SPAN2=$(basename $LO2)
+dmsetup create $SPAN1 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN1'
+
+dmsetup create $SPAN2 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN2'
+
+usleep $WAIT_US
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+step "create vg on $LO3"
+# On the 3rd loop device, we create a VG and an LV
+# The LV should not be removed by kpartx.
+pvcreate --config "$LVMCONF" -f $LO3
+vgcreate --config "$LVMCONF" $VG $LO3
+push_cleanup 'vgremove --config "$LVMCONF" -f $VG'
+lvcreate --config "$LVMCONF" -L $((SIZE/2))B -n $LV $VG
+push_cleanup 'lvremove --config "$LVMCONF" -f $VG/$LV'
+usleep $WAIT_US
+
+[[ -b /dev/mapper/$VG-$LV ]]
+
+# dmsetup table /dev/mapper/$VG-$LV
+# dmsetup info /dev/mapper/$VG-$LV
+
+step "create partitions on loop devices"
+
+mk_partitions $LO1
+mk_partitions $LO2
+
+# Test invocation of kpartx with regular file here
+LO2P1=/dev/mapper/$(basename $LO2)-foo1
+$KPARTX -a -v -p -foo $FILE2
+push_cleanup 'dmsetup remove -f $(basename $LO2P1)'
+
+LO1P1=/dev/mapper/$(basename $LO1)-eggs1
+$KPARTX -a -v -p -eggs $LO1
+push_cleanup 'dmsetup remove -f $(basename $LO1P1)'
+
+usleep $WAIT_US
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+
+# dmsetup info $LO2P1
+
+# Set pathological name for partition on $LO1 (same as loop device itself)
+dmsetup rename $(basename $LO1P1) $(basename $LO1)
+LO1P1=/dev/mapper/$(basename $LO1)
+pop_cleanup
+push_cleanup 'dmsetup remove -f $(basename $LO1P1)'
+
+# dmsetup info $LO1P1
+
+step "create partitions on DM devices"
+mk_partitions /dev/mapper/$SPAN2
+
+$KPARTX -a -v -p -bar /dev/mapper/$SPAN2
+SPAN2P1=/dev/mapper/${SPAN2}-bar1
+push_cleanup 'dmsetup remove -f $(basename $SPAN2P1)'
+
+$KPARTX -a -v -p -spam /dev/mapper/$SPAN1
+SPAN1P1=/dev/mapper/${SPAN1}-spam1
+push_cleanup 'dmsetup remove -f $(basename $SPAN1P1)'
+
+usleep $WAIT_US
+[[ -b $SPAN2P1 ]]
+[[ -b $SPAN1P1 ]]
+
+step "delete partitions on DM devices"
+$KPARTX -d -v /dev/mapper/$SPAN1
+usleep $WAIT_US
+
+[[ -b $SPAN2P1 ]]
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+[[ ! -b $SPAN1P1 ]]
+pop_cleanup
+
+$KPARTX -d -v /dev/mapper/$SPAN2
+usleep $WAIT_US
+
+[[ -b $LO1P1 ]]
+[[ -b $LO2P1 ]]
+[[ ! -b $SPAN2P1 ]]
+pop_cleanup
+
+# udev rules may have created partition mappings without UUIDs
+# which aren't removed by default (if system standard kpartx doesn't
+# set the UUID).
+# Remove them using -f
+$KPARTX -f -d -v /dev/mapper/$SPAN1
+$KPARTX -f -d -v /dev/mapper/$SPAN2
+
+step "delete partitions on loop devices"
+
+$KPARTX -d -v $LO3
+
+# This will also delete the loop device
+$KPARTX -d -v $FILE2
+$KPARTX -d -v $LO1
+usleep $WAIT_US
+
+# ls -l /dev/mapper
+[[ ! -b $LO1P1 ]]
+pop_cleanup
+[[ ! -b $LO2P1 ]]
+pop_cleanup
+# spans should not have been removed
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+# LVs neither
+[[ -b /dev/mapper/$VG-$LV ]]
+
+step "delete partitions on $LO3 with -f"
+
+$KPARTX -f -d -v $LO3
+# -d -f should delete the LV, too
+[[ ! -b /dev/mapper/$VG-$LV ]]
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+OK=yes
-- 
2.12.2

[PATCH 02/10] kpartx: avoid ioctl error for loop devices

[Martin Wilck]

Commit 3d709241 causes kpartx to attempt a dm ioctl on a loop
device. This causes an error message
"device-mapper: table ioctl on loop4 failed: No such device or address".

Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 58e60ffe..e9b09492 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -362,7 +362,7 @@ main(int argc, char **argv){
 
 	if (!mapname)
 		mapname = device + off;
-	if (!force_devmap &&
+	else if (!force_devmap &&
 		 dm_no_partitions(mapname)) {
 		/* Feature 'no_partitions' is set, return */
 		return 0;
-- 
2.12.2

Re: [PATCH 02/10] kpartx: avoid ioctl error for loop devices

[Benjamin Marzinski]

dm_no_partitions() checks if features includes "no_partitions", but the
upstream kernel doesn't allow that feature.  The patch to add it got
NAKed when Hannes posted it, IIRC. That's why I went the route of using
udev flags to avoid running kpartx.  Unless SUSE (or some other distro)
is using the "no_partitions" patch in their kernel, we can pull the
dm_no_partitions code completely.

If you're still using it, I'm fine with this patch.
-Ben

On Sat, May 06, 2017 at 12:05:51AM +0200, Martin Wilck wrote:
> Commit 3d709241 causes kpartx to attempt a dm ioctl on a loop
> device. This causes an error message
> "device-mapper: table ioctl on loop4 failed: No such device or address".
> 
> Fixes: 3d709241 "kpartx: sanitize delete partitions"
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  kpartx/kpartx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 58e60ffe..e9b09492 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -362,7 +362,7 @@ main(int argc, char **argv){
>  
>  	if (!mapname)
>  		mapname = device + off;
> -	if (!force_devmap &&
> +	else if (!force_devmap &&
>  		 dm_no_partitions(mapname)) {
>  		/* Feature 'no_partitions' is set, return */
>  		return 0;
> -- 
> 2.12.2

Re: [PATCH 02/10] kpartx: avoid ioctl error for loop devices

[Martin Wilck]

On Mon, 2017-05-08 at 15:46 -0500, Benjamin Marzinski wrote:
> dm_no_partitions() checks if features includes "no_partitions", but
> the
> upstream kernel doesn't allow that feature.  The patch to add it got
> NAKed when Hannes posted it, IIRC. That's why I went the route of
> using
> udev flags to avoid running kpartx.  Unless SUSE (or some other
> distro)
> is using the "no_partitions" patch in their kernel, we can pull the
> dm_no_partitions code completely.
> 
> If you're still using it, I'm fine with this patch.

The SUSE kernel supports this feature. But after discussing with
Hannes, I see no problem with removing it from upstream multipath-tools 
and making it a SUSE-specific feature in our code base.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSELinux GmbH, GF: Felix Imendörffer, Jane Smithard,  Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[PATCH 03/10] kpartx: remove is_loop_device

[Martin Wilck]

This function is not used any more.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 31 -------------------------------
 kpartx/lopart.h |  1 -
 2 files changed, 32 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 2eb3f631..44f0c277 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -62,37 +62,6 @@ xstrdup (const char *s)
 	return t;
 }
 
-int is_loop_device(const char *device)
-{
-	struct stat statbuf;
-	int loopmajor;
-#if 1
-	loopmajor = 7;
-#else
-	FILE *procdev;
-	char line[100], *cp;
-
-	loopmajor = 0;
-
-	if ((procdev = fopen(PROC_DEVICES, "r")) != NULL) {
-
-		while (fgets (line, sizeof(line), procdev)) {
-
-			if ((cp = strstr (line, " loop\n")) != NULL) {
-				*cp='\0';
-				loopmajor=atoi(line);
-				break;
-			}
-		}
-
-		fclose(procdev);
-	}
-#endif
-	return (loopmajor && stat(device, &statbuf) == 0 &&
-		S_ISBLK(statbuf.st_mode) &&
-		major(statbuf.st_rdev) == loopmajor);
-}
-
 #define SIZE(a) (sizeof(a)/sizeof(a[0]))
 
 char *find_loop_by_file(const char *filename)
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index a512353b..d3bad10a 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,6 +1,5 @@
 extern int verbose;
 extern int set_loop (const char *, const char *, int, int *);
 extern int del_loop (const char *);
-extern int is_loop_device (const char *);
 extern char * find_unused_loop_device (void);
 extern char * find_loop_by_file (const char *);
-- 
2.12.2

[PATCH 04/10] kpartx: dm_remove_partmaps: support non-dm devices

[Martin Wilck]

Commit  3d709241 improved partition removal, but broke support
for handling partitions of non-dm devices such as loop devices
or RAM disks.

This requires passing the dev_t of the device down to
do_foreach_partmap(). Doing so, there's little use in trying
to derive major/minor numbers from the "mapname" any more
(which actually is the device name for non-DM devices).
But we can use this to find out whether the device in question
is a dm device.

The test for UUID match doesn't work for non-DM devices (this
shall be improved in a later patch in this series).

The test for equal name of parent dev and partition is only valid
for dm devices. For non-dm devices such as loop, "/dev/mapper/loop0"
could, theoretically, be a partition of "/dev/loop0"
(and we don't want to rely on map names).

Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 26 +++++++++++++++++---------
 kpartx/devmapper.h |  2 +-
 kpartx/kpartx.c    |  3 ++-
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index cf6650c6..8f48a705 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -8,6 +8,7 @@
 #include <libdevmapper.h>
 #include <ctype.h>
 #include <errno.h>
+#include <sys/sysmacros.h>
 #include "devmapper.h"
 
 #define UUID_PREFIX "part%d-"
@@ -418,8 +419,8 @@ dm_compare_uuid(const char *mapuuid, const char *partname)
 		return 1;
 
 	if (!strncmp(partuuid, "part", 4)) {
-		char *p = strstr(partuuid, "mpath-");
-		if (p && !strcmp(mapuuid, p))
+		char *p = strchr(partuuid, '-');
+		if (p && !strcmp(mapuuid, p + 1))
 			r = 0;
 	}
 	free(partuuid);
@@ -432,6 +433,7 @@ struct remove_data {
 
 static int
 do_foreach_partmaps (const char * mapname, const char *uuid,
+		     dev_t devt,
 		     int (*partmap_func)(const char *, void *),
 		     void *data)
 {
@@ -443,6 +445,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 	int major, minor;
 	char dev_t[32];
 	int r = 1;
+	int is_dmdev = 1;
 
 	if (!(dmt = dm_task_create(DM_DEVICE_LIST)))
 		return 1;
@@ -460,15 +463,20 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		goto out;
 	}
 
-	if (dm_devn(mapname, &major, &minor))
-		goto out;
+	if (dm_devn(mapname, &major, &minor) ||
+	    (major != major(devt) || minor != minor(devt)))
+		/*
+		 * The latter could happen if a dm device "/dev/mapper/loop0"
+		 * exits while kpartx is called on "/dev/loop0".
+		 */
+		is_dmdev = 0;
 
-	sprintf(dev_t, "%d:%d", major, minor);
+	sprintf(dev_t, "%d:%d", major(devt), minor(devt));
 	do {
 		/*
 		 * skip our devmap
 		 */
-		if (!strcmp(names->name, mapname))
+		if (is_dmdev && !strcmp(names->name, mapname))
 			goto next;
 
 		/*
@@ -496,7 +504,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if uuids don't match
 		 */
-		if (dm_compare_uuid(uuid, names->name)) {
+		if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
 			if (rd->verbose)
 				printf("%s: is not a kpartx partition. Not removing\n",
 				       names->name);
@@ -537,10 +545,10 @@ remove_partmap(const char *name, void *data)
 }
 
 int
-dm_remove_partmaps (char * mapname, char *uuid, int verbose)
+dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose)
 {
 	struct remove_data rd = { verbose };
-	return do_foreach_partmaps(mapname, uuid, remove_partmap, &rd);
+	return do_foreach_partmaps(mapname, uuid, devt, remove_partmap, &rd);
 }
 
 #define FEATURE_NO_PART "no_partitions"
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 9988ec0f..2e28c780 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -18,7 +18,7 @@ char * dm_mapname(int major, int minor);
 dev_t dm_get_first_dep(char *devname);
 char * dm_mapuuid(const char *mapname);
 int dm_devn (const char * mapname, int *major, int *minor);
-int dm_remove_partmaps (char * mapname, char *uuid, int verbose);
+int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
 int dm_no_partitions(char * mapname);
 
 #endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e9b09492..e2056b7f 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -451,7 +451,8 @@ main(int argc, char **argv){
 			break;
 
 		case DELETE:
-			r = dm_remove_partmaps(mapname, uuid, verbose);
+			r = dm_remove_partmaps(mapname, uuid, buf.st_rdev,
+					       verbose);
 			if (loopdev) {
 				if (del_loop(loopdev)) {
 					if (verbose)
-- 
2.12.2

[PATCH 05/10] kpartx: dm_devn: return error for non-existent device

[Martin Wilck]

For non-existent maps (ENXIO from ioctl()), dm_task_run and
dm_task_get_info return success. We need to check info.exists.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 8f48a705..d6ccd000 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -293,7 +293,7 @@ dm_devn (const char * mapname, int *major, int *minor)
 	if (!dm_task_run(dmt))
 		goto out;
 
-	if (!dm_task_get_info(dmt, &info))
+	if (!dm_task_get_info(dmt, &info) || info.exists == 0)
 		goto out;
 
 	*major = info.major;
-- 
2.12.2

[PATCH 06/10] kpartx: don't treat multi-linear mappings as partitions

[Martin Wilck]

kpartx -d treats any map that has a "linear" mapping into a
device as first target as a partition of this device.
This is wrong, because linear mappings may consist of
several pieces, combining multiple devices into one
(LVM logical volumes are an example of such a
mapping). Partitions have to be single-target mappings.

Fix this by returning an error in dm_type() if a map with
multiple targets is encountered. The "type" of a map with
two or more targets is a difficult concept, anyway.

test case:
truncate -s 1G test0 test1
losetup /dev/loop0 test0
losetup /dev/loop1 test1
dmsetup create <<EOF
0 1000000 linear 7:0 4096
1000000 1000000 linear 7:1 4096
EOF
kpartx -d /dev/loop0 -v

The map "join" should NOT be deleted.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index d6ccd000..5380b2e9 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -390,10 +390,11 @@ dm_type(const char * name, char * type)
 		goto out;
 
 	/* Fetch 1st target */
-	dm_get_next_target(dmt, NULL, &start, &length,
-			   &target_type, &params);
-
-	if (!target_type)
+	if (dm_get_next_target(dmt, NULL, &start, &length,
+			       &target_type, &params) != NULL)
+		/* more than one target */
+		r = -1;
+	else if (!target_type)
 		r = -1;
 	else if (!strcmp(target_type, type))
 		r = 1;
@@ -494,7 +495,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if devmap target is not "linear"
 		 */
-		if (!dm_type(names->name, "linear")) {
+		if (dm_type(names->name, "linear") != 1) {
 			if (rd->verbose)
 				printf("%s: is not a linear target. Not removing\n",
 				       names->name);
-- 
2.12.2

[PATCH 07/10] libmultipath: don't treat multi-linear mappings as partitions

[Martin Wilck]

dm_type is used in libmultipath only to check whether a mapping
is "linear", with the intention to test if it represents a
"partition". This test returns TRUE also for mappings with
multiple targets, the first of which happens to be a linear
mapping into the target device. This is questionable, it's
hard to assign a "type" to such maps anyway.

Fix this by returning an error for multi-target maps.
This is analogous to the patch
"kpartx: don't treat multi-linear mappings as partitions".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 5fb9d9ac..c19dcb62 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -572,7 +572,7 @@ out:
  * returns:
  *    1 : match
  *    0 : no match
- *   -1 : empty map
+ *   -1 : empty map, or more than 1 target
  */
 int dm_type(const char *name, char *type)
 {
@@ -594,10 +594,11 @@ int dm_type(const char *name, char *type)
 		goto out;
 
 	/* Fetch 1st target */
-	dm_get_next_target(dmt, NULL, &start, &length,
-			   &target_type, &params);
-
-	if (!target_type)
+	if (dm_get_next_target(dmt, NULL, &start, &length,
+			       &target_type, &params) != NULL)
+		/* multiple targets */
+		r = -1;
+	else if (!target_type)
 		r = -1;
 	else if (!strcmp(target_type, type))
 		r = 1;
@@ -1185,9 +1186,9 @@ do_foreach_partmaps (const char * mapname,
 	do {
 		if (
 		    /*
-		     * if devmap target is "linear"
+		     * if there is only a single "linear" target
 		     */
-		    (dm_type(names->name, TGT_PART) > 0) &&
+		    (dm_type(names->name, TGT_PART) == 1) &&
 
 		    /*
 		     * and both uuid end with same suffix starting
-- 
2.12.2

[PATCH 08/10] kpartx: use partition UUID for non-DM devices

[Martin Wilck]

For dm devices, kpartx uses an UUID check at partition removal
to make sure it only deletes partitions it previously created.
Non-DM parent devices such as loop devices don't generally have
a UUID.

Introduce a "fake" UUID for these devices to make sure kpartx
deletes only devices it had created previously. Otherwise kpartx
might e.g. delete LVM LVs that are inside a device it is trying
to delete partitions for. It seems to be wiser to make sure the
user delete these manually before running "kpartx -d".

With the fake UUID in place, we can re-introduce the UUID check
for non-DM device that was removed in the earlier patch
"kpartx: dm_remove_partmaps: support non-dm devices".

This disables also deletion of partition mappings created
by earlier versions of kpartx. If kpartx has been updated after
partition mappings were created, the "-f" flag can be used
to force delting these partitions, too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c |  2 +-
 kpartx/kpartx.c    | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 5380b2e9..40ec26cf 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -505,7 +505,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
 		/*
 		 * skip if uuids don't match
 		 */
-		if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
+		if (uuid && dm_compare_uuid(uuid, names->name)) {
 			if (rd->verbose)
 				printf("%s: is not a kpartx partition. Not removing\n",
 				       names->name);
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e2056b7f..d9057fba 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -60,6 +60,26 @@ struct pt {
 int ptct = 0;
 int udev_sync = 0;
 
+/*
+ * UUID format for partitions created on non-DM devices
+ * ${UUID_PREFIX}${MAJOR}:${MINOR}-${NONDM_UUID_SUFFIX}"
+ * The suffix should be sufficiently random to avoid unintended conflicts,
+ * and shouldn't be changed.
+ * The value below is a bas64-encoded 96-bit random number.
+ */
+#define NONDM_UUID_SUFFIX "-kpartx-Wh5pYvM7uc60hh74"
+
+static char *
+nondm_create_uuid(dev_t devt)
+{
+#define NONDM_UUID_BUFLEN (32 + sizeof(NONDM_UUID_SUFFIX))
+	static char uuid_buf[NONDM_UUID_BUFLEN];
+	snprintf(uuid_buf, sizeof(uuid_buf), "%u:%u%s",
+		 major(devt), minor(devt), NONDM_UUID_SUFFIX);
+	uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
+	return uuid_buf;
+}
+
 static void
 addpts(char *t, ptreader f)
 {
@@ -360,6 +380,15 @@ main(int argc, char **argv){
 			uuid = dm_mapuuid(mapname);
 	}
 
+	/*
+	 * We are called for a non-DM device.
+	 * Make up a fake UUID for the device, unless "-d -f" is given.
+	 * This allows deletion of partitions created with older kpartx
+	 * versions which didn't use the fake UUID during creation.
+	 */
+	if (!uuid && !(what == DELETE && force_devmap))
+		uuid = nondm_create_uuid(buf.st_rdev);
+
 	if (!mapname)
 		mapname = device + off;
 	else if (!force_devmap &&
-- 
2.12.2

Re: [PATCH 08/10] kpartx: use partition UUID for non-DM devices

[Alasdair G Kergon]

On Sat, May 06, 2017 at 12:05:57AM +0200, Martin Wilck wrote:
> Introduce a "fake" UUID for these devices to make sure kpartx
> deletes only devices it had created previously. Otherwise kpartx
> might e.g. delete LVM LVs that are inside a device it is trying
> to delete partitions for. It seems to be wiser to make sure the
> user delete these manually before running "kpartx -d".
> 
> With the fake UUID in place, we can re-introduce the UUID check
> for non-DM device that was removed in the earlier patch
> "kpartx: dm_remove_partmaps: support non-dm devices".
> 
> This disables also deletion of partition mappings created
> by earlier versions of kpartx. If kpartx has been updated after
> partition mappings were created, the "-f" flag can be used
> to force delting these partitions, too.
 
Indeed - always supply a uuid for every dm device created.
Add a standard prefix to the beginning of it, ideally "KPARTX-" to be
consistent with other dm tools ("LVM-", "CRYPT-" etc.) if you're
able to break compatibility, but "part" is still unique and adequate.  
(Stacked devices can end up with a stack of prefixes - only the leftmost one
counts.)

Alasdair

Re: [PATCH 08/10] kpartx: use partition UUID for non-DM devices

[Martin Wilck]

On Fri, 2017-05-05 at 23:30 +0100, Alasdair G Kergon wrote:
> On Sat, May 06, 2017 at 12:05:57AM +0200, Martin Wilck wrote:
> > Introduce a "fake" UUID for these devices to make sure kpartx
> > deletes only devices it had created previously. Otherwise kpartx
> > might e.g. delete LVM LVs that are inside a device it is trying
> > to delete partitions for. It seems to be wiser to make sure the
> > user delete these manually before running "kpartx -d".
> > 
> > With the fake UUID in place, we can re-introduce the UUID check
> > for non-DM device that was removed in the earlier patch
> > "kpartx: dm_remove_partmaps: support non-dm devices".
> > 
> > This disables also deletion of partition mappings created
> > by earlier versions of kpartx. If kpartx has been updated after
> > partition mappings were created, the "-f" flag can be used
> > to force delting these partitions, too.
> 
>  
> Indeed - always supply a uuid for every dm device created.
> Add a standard prefix to the beginning of it, ideally "KPARTX-" to be
> consistent with other dm tools ("LVM-", "CRYPT-" etc.) if you're
> able to break compatibility, but "part" is still unique and
> adequate.  
> (Stacked devices can end up with a stack of prefixes - only the
> leftmost one
> counts.)

As noted in my reply to Hannes' mail, I'm using the prefix "part%u-",
the same prefix that kpartx currently used for partitions of DM
devices.

The full format e.g. for /dev/mapper/loop0p1 is 

"part1-7:0-kpartx-Wh5pYvM7uc60hh74" 

and correspondingly, the "fake UUID" used for /dev/loop0 would be 

      "7:0-kpartx-Wh5pYvM7uc60hh74"

If you don't like the fact that this starts with "7:0", changing this
to e.g. "KPARTX-7.0-$something" is no problem as long as patch isn't
applied.

The last part ("Wh...") is a hard-coded string, just to avoid
accidental conflicts. I was wondering whether it would make sense to
some sort of encoded "host ID" to make this UUID truly "universal". My
personal opinion is that this would be over-engineered, because these
UUID will be visible only only on the host that called "kpartx -a" in
the first place.

Cheers,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[PATCH 09/10] kpartx: use absolute path for regular files

[Martin Wilck]

kpartx supports being called for a regular file, in which
case it assumes that it should set up a loop device or use
an existing one. Because the loopinfo.lo_name field contains
an absolute path, matching with existing loop devices fails
for relative paths. Matching by basename only would be unreliable.

Therefore, convert relative to absolute path for regular files.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index d9057fba..3bd16452 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -350,7 +350,13 @@ main(int argc, char **argv){
 
 	if (S_ISREG (buf.st_mode)) {
 		/* already looped file ? */
-		loopdev = find_loop_by_file(device);
+		char rpath[PATH_MAX];
+		if (realpath(device, rpath) == NULL) {
+			fprintf(stderr, "Error: %s: %s\n", device,
+				strerror(errno));
+			exit (1);
+		}
+		loopdev = find_loop_by_file(rpath);
 
 		if (!loopdev && what == DELETE)
 			exit (0);
-- 
2.12.2

[PATCH 10/10] kpartx: find_loop_by_file: use sysfs

[Martin Wilck]

Rather then searching through all of /dev, look up loop devices
in /sys/devices/virtual/block. This is cleaner and more robust
(/dev/loop$Xp$Y symlinks may confuse kpartx).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 44f0c277..0521d7dc 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -68,25 +68,46 @@ char *find_loop_by_file(const char *filename)
 {
 	DIR *dir;
 	struct dirent *dent;
-	char dev[64], *found = NULL;
+	char dev[64], *found = NULL, *p;
 	int fd;
 	struct stat statbuf;
 	struct loop_info loopinfo;
+	const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
+	char path[PATH_MAX];
 
-	dir = opendir("/dev");
+	dir = opendir(VIRT_BLOCK);
 	if (!dir)
 		return NULL;
 
 	while ((dent = readdir(dir)) != NULL) {
 		if (strncmp(dent->d_name,"loop",4))
 			continue;
-		if (!strcmp(dent->d_name, "loop-control"))
-			continue;
-		sprintf(dev, "/dev/%s", dent->d_name);
 
-		fd = open (dev, O_RDONLY);
+		if (snprintf(path, PATH_MAX, "%s/%s/dev", VIRT_BLOCK,
+			     dent->d_name) >= PATH_MAX)
+			continue;
+
+		fd = open(path, O_RDONLY);
 		if (fd < 0)
-			break;
+			continue;
+
+		if (read(fd, dev, sizeof(dev)) <= 0) {
+			close(fd);
+			continue;
+		}
+
+		close(fd);
+
+		dev[sizeof(dev)-1] = '\0';
+		p = strchr(dev, '\n');
+		if (p != NULL)
+			*p = '\0';
+		if (snprintf(path, PATH_MAX, "/dev/block/%s", dev) >= PATH_MAX)
+			continue;
+
+		fd = open (path, O_RDONLY);
+		if (fd < 0)
+			continue;
 
 		if (fstat (fd, &statbuf) != 0 ||
 		    !S_ISBLK(statbuf.st_mode)) {
@@ -99,13 +120,12 @@ char *find_loop_by_file(const char *filename)
 			continue;
 		}
 
+		close (fd);
+
 		if (0 == strcmp(filename, loopinfo.lo_name)) {
-			close (fd);
-			found = xstrdup(dev);
+			found = realpath(path, NULL);
 			break;
 		}
-
-		close (fd);
 	}
 	closedir(dir);
 	return found;
-- 
2.12.2

Re: [PATCH 00/10] fixes for kpartx -d

[Alasdair G Kergon]

On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
>   3) kpartx should only delete "partitions", which are single-target
>      linear mappings into a block device. Other maps should not be touched.

The prefix on the dm device's uuid should guarantee this: all devices
kpartx creates should have the same initial characters (a
not-quite-standard form "part" IIRC instead of "KPARTX-") and any
devices without those initial characters must be ignored.

Alasdair

Re: [PATCH 00/10] fixes for kpartx -d

[Martin Wilck]

On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote:
> On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
> >   3) kpartx should only delete "partitions", which are single-
> > target
> >      linear mappings into a block device. Other maps should not be
> > touched.
> 
> The prefix on the dm device's uuid should guarantee this: all devices
> kpartx creates should have the same initial characters (a
> not-quite-standard form "part" IIRC instead of "KPARTX-") and any
> devices without those initial characters must be ignored.

This works only for partitions on DM devices, not e.g. for loop
devices. These devices obviously have no DM UUID; and thus kpartx also
doesn't set an UUID for the partition devices it creates.
That's the main point of this series.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 00/10] fixes for kpartx -d

[Martin Wilck]

On Sat, 2017-05-06 at 00:30 +0200, Martin Wilck wrote:
> On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote:
> > On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
> > >   3) kpartx should only delete "partitions", which are single-
> > > target
> > >      linear mappings into a block device. Other maps should not
> > > be
> > > touched.
> > 
> > The prefix on the dm device's uuid should guarantee this: all
> > devices
> > kpartx creates should have the same initial characters (a
> > not-quite-standard form "part" IIRC instead of "KPARTX-") and any
> > devices without those initial characters must be ignored.
> 
> This works only for partitions on DM devices, not e.g. for loop
> devices. These devices obviously have no DM UUID; and thus kpartx
> also
> doesn't set an UUID for the partition devices it creates.
> That's the main point of this series.

Moreover: before even looking at the UUID, kpartx discards mappings
that are not "linear" and don't map into the device in question. I
added the additional case that it should also disregard mappings with
two or more targets which can't be regarded as simple "linear" type
(but in the current code, they are).

With the UUID test in place, this may seem kind of redundant, but it
follows the general logic that obvious non-partition devices should be
discarded before checking the UUID.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 00/10] fixes for kpartx -d

[Hannes Reinecke]

On 05/06/2017 12:30 AM, Martin Wilck wrote:
> On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote:
>> On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
>>>   3) kpartx should only delete "partitions", which are single-
>>> target
>>>      linear mappings into a block device. Other maps should not be
>>> touched.
>>
>> The prefix on the dm device's uuid should guarantee this: all devices
>> kpartx creates should have the same initial characters (a
>> not-quite-standard form "part" IIRC instead of "KPARTX-") and any
>> devices without those initial characters must be ignored.
> 
> This works only for partitions on DM devices, not e.g. for loop
> devices. These devices obviously have no DM UUID; and thus kpartx also
> doesn't set an UUID for the partition devices it creates.
> That's the main point of this series.
> 
I beg to differ.

The main point is to _set_ an UUID for all kpartx-generated devices.
But to make matters consistent I'm siding with Alasdair here, namely
that any kpartx-generated UUID should be starting with 'part-'.
So I'm fine with adding a UUID, but this UUID _must_ start with 'part-'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 00/10] fixes for kpartx -d

[Martin Wilck]

On Mon, 2017-05-08 at 08:33 +0200, Hannes Reinecke wrote:
> On 05/06/2017 12:30 AM, Martin Wilck wrote:
> > On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote:
> > > On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
> > > >   3) kpartx should only delete "partitions", which are single-
> > > > target
> > > >      linear mappings into a block device. Other maps should not
> > > > be
> > > > touched.
> > > 
> > > The prefix on the dm device's uuid should guarantee this: all
> > > devices
> > > kpartx creates should have the same initial characters (a
> > > not-quite-standard form "part" IIRC instead of "KPARTX-") and any
> > > devices without those initial characters must be ignored.
> > 
> > This works only for partitions on DM devices, not e.g. for loop
> > devices. These devices obviously have no DM UUID; and thus kpartx
> > also
> > doesn't set an UUID for the partition devices it creates.
> > That's the main point of this series.
> > 
> 
> I beg to differ.
> 
> The main point is to _set_ an UUID for all kpartx-generated devices.
> But to make matters consistent I'm siding with Alasdair here, namely
> that any kpartx-generated UUID should be starting with 'part-'.
> So I'm fine with adding a UUID, but this UUID _must_ start with
> 'part-'.

Well, not quite. It must start with "part%u-". It does so in my patch
set. Like UUIDs created partitions on DM devices, kpartx-generated
partitions for non-DM devices start with part$N, where $N is the
partition number (without my patch set, these partitions devices have
_no UUID_).

The full partition UUID for partitions used by my code looks like this
(see patch 08/10):

{UUID_PREFIX}${MAJOR}:${MINOR}-${NONDM_UUID_SUFFIX}"

This format fulfills the constraints of dm_compare_uuid(). The only
change I had to introduce for dm_compare_uuid() was to remove the check
for "mpath-" (patch 04/10), which was obviously wrong for non-multipath 
devices. It's correct to have that check in libmultipath/devmapper.c,
but not in kpartx/devmapper.c.

Moreover, by including $MAJOR:$MINOR of the parent device, this format
makes it possible to check whether the given partition device matches
the given parent device whose partitions are supposed to be deleted,
and with the nontrivial suffix, takes precations against casual UUID
conflicts.

For DM devices, my patch set introduces no changes (of course).

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 00/10] fixes for kpartx -d

[Martin Wilck]

On Mon, 2017-05-08 at 09:47 +0200, Martin Wilck wrote:
> The full partition UUID for partitions used by my code looks like
> this
> (see patch 08/10):
> 
> {UUID_PREFIX}${MAJOR}:${MINOR}-${NONDM_UUID_SUFFIX}"

... where UUID_PREFIX is what kpartx uses anyway, i.e. "part$N".

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 00/10] fixes for kpartx -d

[Benjamin Marzinski]

On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote:
> Working on a bug report about kpartx not properly removing
> partitions for loop devices, I realized a number of glitches
> and improperly handled corner cases in the kpartx code for
> deleting partitions. Some mappings are not deleted although
> they should be, and others are deleted although that is clearly
> wrong.
> 
> This patch series fixes the issues I found. The series starts
> with a test program demonstrating the problems. The program
> succeeds only after all patches of this series are applied.
> 
> Here is my summary of what I think how kpartx should behave:
> 
>   1) kpartx should delete all mappings it created beforehand.
>   2) kpartx should handle partitions on dm devices and other devices
>      (e.g. loop devices) equally well.
>   3) kpartx should only delete "partitions", which are single-target
>      linear mappings into a block device. Other maps should not be touched.
>   4) kpartx should only delete mappings it created itself beforehand.
>      In particular, it shouldn't delete LVM LVs, even if they are fully
>      contained in the block device at hand and thus look like partitions
>      in the first place. (For historical compatibility reasons, allow
>      such mappings to be deleted with the -f/--force flag).
>   5) DM map names may be changed, thus kpartx shouldn't rely on them to
>      check whether a mapping is a partition of a particular device. It is
>      legal for a partition of /dev/loop0 to be named "loop0".
> 
> One patch has an obvious libdevmapper equivalent and is therefore
> included although this series is otherwise focused only on kpartx.

ACK for the set (with the possible exception of "kpartx: avoid ioctl
error for loop devices", if we don't need to keep the dm_no_partitions
code anymore).

-Ben

> 
> Feedback is welcome.
> 
> Martin Wilck (10):
>   kpartx: test-kpartx: new unit test program
>   kpartx: avoid ioctl error for loop devices
>   kpartx: remove is_loop_device
>   kpartx: dm_remove_partmaps: support non-dm devices
>   kpartx: dm_devn: return error for non-existent device
>   kpartx: don't treat multi-linear mappings as partitions
>   libmultipath: don't treat multi-linear mappings as partitions
>   kpartx: use partition UUID for non-DM devices
>   kpartx: use absolute path for regular files
>   kpartx: find_loop_by_file: use sysfs
> 
>  kpartx/devmapper.c       |  39 +++++---
>  kpartx/devmapper.h       |   2 +-
>  kpartx/kpartx.c          |  42 +++++++-
>  kpartx/lopart.c          |  73 ++++++--------
>  kpartx/lopart.h          |   1 -
>  kpartx/test-kpartx       | 253 +++++++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/devmapper.c |  15 +--
>  7 files changed, 356 insertions(+), 69 deletions(-)
>  create mode 100755 kpartx/test-kpartx
> 
> -- 
> 2.12.2