All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: lvm-devel@redhat.com
Subject: [PATCH] dmsetup: Set exit code to 1 if remove_all fails to remove all devices
Date: Sat, 6 Dec 2014 22:25:19 +0100	[thread overview]
Message-ID: <20141206212519.GA11165@wunner.de> (raw)
In-Reply-To: <547E2B3D.9050200@redhat.com>

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



      reply	other threads:[~2014-12-06 21:25 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-02 17:58 [PATCH] dmsetup: Set exit code to 1 if remove_all fails to remove all devices Lukas Wunner
2014-12-02 19:16 ` Zdenek Kabelac
2014-12-02 21:00   ` Lukas Wunner
2014-12-02 21:12     ` Zdenek Kabelac
2014-12-06 21:25       ` Lukas Wunner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141206212519.GA11165@wunner.de \
    --to=lukas@wunner.de \
    --cc=lvm-devel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.