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

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

[Lukas Wunner]

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.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 tools/dmsetup.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/tools/dmsetup.c b/tools/dmsetup.c
index 4202dbb..77d7cf6 100644
--- a/tools/dmsetup.c
+++ b/tools/dmsetup.c
@@ -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

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

[Zdenek Kabelac]

Dne 2.12.2014 v 18:58 Lukas Wunner napsal(a):
> 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*.
>

 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

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

[Lukas Wunner]

Hi,

On Tue, Dec 02, 2014 at 08:16:00PM +0100, Zdenek Kabelac wrote:
> It really is meant to remove only not opened devices.

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

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

[Zdenek Kabelac]

Dne 2.12.2014 v 22:00 Lukas Wunner napsal(a):
> Hi,
>
> On Tue, Dec 02, 2014 at 08:16:00PM +0100, Zdenek Kabelac wrote:
>> It really is meant to remove only not opened devices.
>
> 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.
>

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.

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

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.

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

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.


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

lvm2 provides   blkdeactive.sh script to take down LVs.


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

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

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

[Lukas Wunner]

Hi,

On Tue, Dec 02, 2014 at 10:12:29PM +0100, Zdenek Kabelac wrote:
> 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.

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


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

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