dmsetup: Set exit code to 1 if remove_all fails to remove all devices

There are scripts out there which expect "dmsetup remove_all" to not exit with 0 if some devices couldn't be removed, e.g. dracut:
https://git.kernel.org/cgit/boot/dracut/dracut.git/tree/modules.d/90dm/dm-shutdown.sh

Up until now the exit code of "dmsetup remove_all" is only non-zero if the call to ioctl() fails, causing _do_dm_ioctl() to return NULL instead of a struct dm_ioctl*.

Fix this by counting the remaining devices after the call to _simple() even if --force is not used, and by returning success only if the call to simple() was succesful AND no devices remain.

---
 tools/dmsetup.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

index 4202dbb..77d7cf6 100644
@@ -1528,27 +1528,23 @@ static int _remove_all(CMD_ARGS)
 	/* Remove all closed devices */
 	r =  _simple(DM_DEVICE_REMOVE_ALL, "", 0, 0) | dm_mknodes(NULL);
 
-	if (!_switches[FORCE_ARG])
-		return r;
-
 	_num_devices = 0;
-	r |= _process_all(cmd, argc, argv, 1, _count_devices);
+	r &= _process_all(cmd, argc, argv, 1, _count_devices);
 
-	/* No devices left? */
-	if (!_num_devices)
-		return r;
+	if ((r && !_num_devices) || !_switches[FORCE_ARG])
+		goto out;
 
 	r |= _process_all(cmd, argc, argv, 1, _error_device);
 	r |= _simple(DM_DEVICE_REMOVE_ALL, "", 0, 0) | dm_mknodes(NULL);
 
 	_num_devices = 0;
-	r |= _process_all(cmd, argc, argv, 1, _count_devices);
-	if (!_num_devices)
-		return r;
+	r &= _process_all(cmd, argc, argv, 1, _count_devices);
 
-	fprintf(stderr, "Unable to remove %d device(s).\n", _num_devices);
+out:
+	if (_num_devices)
+		fprintf(stderr, "Unable to remove %d device(s).\n", _num_devices);
 
-	return r;
+	return r && !_num_devices;
 }
 
 static void _display_dev(struct dm_task *dmt, const char *name)

1.8.5.2

Dne 2.12.2014 v 18:58 Lukas Wunner napsal(a):

From man page:
--

Attempts to remove all device definitions i.e. reset the driver. This also runs mknodes afterwards. Use with care! Open devices cannot be removed, but adding --force will replace the table with one that fails all I/O. --deferred will enable deferred removal of open devices - the device will be removed when the last user closes it. The deferred removal feature is supported since version 4.27.0 of the device-mapper driver available in upstream kernel version 3.13.

--

It really is meant to remove only not opened devices.

https://git.kernel.org/cgit/boot/dracut/dracut.git/tree/modules.d/90dm/dm-shutdown.sh

I'm afraid you need that script.

Moreover 'remove_all' is rather 'developer's' feature - it should never be be used 'randomly' by a script.

So IMHO if some public script tries to use it - the script is IMHO broken. You really are not supposed to randomly turn off device - only if you really know what you are doing (it's very similar to put in there rm -rf....)

Zdenek

Hi,

Sorry but that's not the point.

The interpretation of a program's exit status by the Shell is inverse to the semantics in C. An exit status of 0 denotes success, a non-zero exit status denotes failure.

The point is that, as evidenced by the dracut shutdown script I mentioned, people expect the exit status of "dmsetup remove_all" to be non-zero if the program failed to remove all devices. It's perfectly okay if it failed to remove all devices. No need to remove devices that are still open.

But the program should *tell* the user that some devices could not be closed (at the very least by returning with a non-zero exit status).

Right now the exit status of "dmsetup remove_all" is always 0, thus pretending success.

As for the dracut shutdown script allegedly being broken: That script was written by Harald Hoyer who works for the same company as you do and is shipped with RHEL as well as Fedora. Just saying...

To provide some context: Dracut has a number of shutdown scripts that are called round-robin until all succeed. This is useful for complex LVM setups, e.g. a device mapper target layered on top of ZFS, with the ZFS pool itself being backed by another device mapper target. To properly untangle this, the dm-shutdown script should close the top-layered device mapper target, next a zfs-shutdown script will export the zpool, and finally another invocation of the dm-shutdown script will close the bottom-layered device mapper target.

For this to work, the exit status of "dmsetup remove_all" must be non-zero on failure, otherwise the dm-shutdown script cannot determine whether it's work is done.

Therefore, please consider merging this patch.

Kind regards,

Lukas

Dne 2.12.2014 v 22:00 Lukas Wunner napsal(a):

remove_all - means to remove all CLOSED (unused) devices.

It should return 'fail' if there is failure during command execution (i.e. mem alloc fail) - but it will not report error if there are left devices.

Exit status of current dmsetup is OK and it's matching documented behaviour. If you want a different one - write a wrapper script - just don't propose to break existing valid and expected behaviour because of some script.

So please open BZ if you think there is one for dracut package. I'm sure it will be properly resolved.

I'm just 100% sure that usage of 'remove_all' is not a generally usable feature - it's purely mean for developers to help then cleanup dm tables - it's not meant to be used by 'scripts' to cleanup user devices and return errors if there are some device left - it's plain miss-use.

lvm2 provides blkdeactive.sh script to take down LVs.

Nope - as said 'dmsetup' years stable API will not change because of dracut script - fix the script and detect there are still devices in table - it's one line fix in dracut script.

Regards

Zdenek

Hi,

The blkdeactivate.sh script (which you've suggested) uses "dmsetup remove".

It turns out that "dmsetup remove" does fail with a non-zero exit status if one tries to remove a device which is still in use. On the other hand, as stated before "dmsetup remove_all" does *not* fail with a non-zero exit status if it couldn't remove all devices.

So the behaviour of dmsetup is inconsistent. That precisely is fixed by the proposed patch. I find it bewildering that you call this a "valid and expected behaviour" even though it is inconsistent.

I went ahead and changed the dracut shutdown script to loop over all devices and call "dmsetup remove" on each of them. When testing it I discovered that calling "dmsetup remove" on an (unused) crypt device causes the system to hang on shutdown. Perhaps the ioctl() is blocking? Should crypt devices only be removed with cryptsetup, not with "dmsetup remove"?

Kernel 3.14.15, LVM2 2.02.109

That is not documented in the manpage. The manpage merely says:
"Use with care!"

I propose that the manpage be amended so as to prevent others from "miss-using" this command, as your colleague Harald Hoyer apparently did.

Kind regards,

Lukas