[PATCH] wipefs: Add --force option

[PATCH] wipefs: Add --force option

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

https://bugzilla.redhat.com/show_bug.cgi?id=872831
https://bugzilla.redhat.com/show_bug.cgi?id=865961

Unfortunately this means you nearly always need to use the --force
option with wipefs when trying to wipe something which is not an
unmounted filesystem.

Tested using a modified version of libguestfs which detects if wipefs
supports the --force option, and if it does unconditionally adds it.
I verified that this fixes the virt-format utility on Fedora 18.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw

[-- Attachment #2: 0001-wipefs-Add-force-option-to-force-it-to-erase.patch --]
[-- Type: text/plain, Size: 3445 bytes --]

>From 87d3843615d2ada2f474e2804acfe5a7d7529d72 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 19 Nov 2012 15:02:13 +0000
Subject: [PATCH] wipefs: Add --force option to force it to erase.

Commit c550f728f724360f99aae0fdb45b0589d9a347e0 added O_EXCL when
opening the thing to erase.  This broke the wipefs utility when used
on anything which isn't an unmounted filesystem.  eg. If you use it on
a block device containing partitions, then it won't work because the
kernel recognizes the partitions and so thinks the device is in use.

This change adds the --force option which, when used, undoes the above
flag change.  However you still have to use --force most of the time
when erasing something that isn't a plain unmounted filesystem.
---
 misc-utils/wipefs.8 |  3 +++
 misc-utils/wipefs.c | 19 +++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/misc-utils/wipefs.8 b/misc-utils/wipefs.8
index 05a5496..67261ee 100644
--- a/misc-utils/wipefs.8
+++ b/misc-utils/wipefs.8
@@ -38,6 +38,9 @@ erased.
 .IP "\fB\-a, \-\-all\fP"
 Erase all available signatures. This set of erased signatures could be
 restricted by \fB\-t <list>\fP option.
+.IP "\fB\-f, \-\-force\fP"
+Force erasure, even if the filesystem is mounted.  This is required in
+order to erase the partition table on a block device.
 .IP "\fB\-h, \-\-help\fP"
 Print help and exit.
 .IP "\fB\-n, \-\-no\-act\fP"
diff --git a/misc-utils/wipefs.c b/misc-utils/wipefs.c
index 0ddc148..a29f3cc 100644
--- a/misc-utils/wipefs.c
+++ b/misc-utils/wipefs.c
@@ -307,12 +307,17 @@ static void do_wipe_real(blkid_probe pr, const char *devname, struct wipe_desc *
 }
 
 static struct wipe_desc *
-do_wipe(struct wipe_desc *wp, const char *devname, int noact, int all, int quiet)
+do_wipe(struct wipe_desc *wp, const char *devname, int noact, int all, int quiet, int force)
 {
-	blkid_probe pr = new_probe(devname, O_RDWR | O_EXCL);
+	int flags;
+	blkid_probe pr;
 	struct wipe_desc *w, *wp0 = clone_offset(wp);
 	int zap = all ? 1 : wp->zap;
 
+	flags = O_RDWR;
+	if (!force)
+		flags |= O_EXCL;
+	pr = new_probe(devname, flags);
 	if (!pr)
 		return NULL;
 
@@ -362,6 +367,7 @@ usage(FILE *out)
 
 	fputs(_("\nOptions:\n"), out);
 	fputs(_(" -a, --all           wipe all magic strings (BE CAREFUL!)\n"
+		" -f, --force         force erasure\n"
 		" -h, --help          show this help text\n"
 		" -n, --no-act        do everything except the actual write() call\n"
 		" -o, --offset <num>  offset to erase, in bytes\n"
@@ -380,11 +386,12 @@ int
 main(int argc, char **argv)
 {
 	struct wipe_desc *wp0 = NULL, *wp;
-	int c, all = 0, has_offset = 0, noact = 0, quiet = 0;
+	int c, all = 0, force = 0, has_offset = 0, noact = 0, quiet = 0;
 	int mode = WP_MODE_PRETTY;
 
 	static const struct option longopts[] = {
 	    { "all",       0, 0, 'a' },
+	    { "force",     0, 0, 'f' },
 	    { "help",      0, 0, 'h' },
 	    { "no-act",    0, 0, 'n' },
 	    { "offset",    1, 0, 'o' },
@@ -414,6 +421,9 @@ main(int argc, char **argv)
 		case 'a':
 			all++;
 			break;
+		case 'f':
+			force++;
+			break;
 		case 'h':
 			usage(stdout);
 			break;
@@ -463,7 +473,8 @@ main(int argc, char **argv)
 		 */
 		while (optind < argc) {
 			wp = clone_offset(wp0);
-			wp = do_wipe(wp, argv[optind++], noact, all, quiet);
+			wp = do_wipe(wp, argv[optind++], noact, all, quiet,
+				     force);
 			free_wipe(wp);
 		}
 	}
-- 
1.7.11.4

Re: [PATCH] wipefs: Add --force option

[Karel Zak]

On Mon, Nov 19, 2012 at 03:41:16PM +0000, Richard W.M. Jones wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=872831
> https://bugzilla.redhat.com/show_bug.cgi?id=865961
> 
> Unfortunately this means you nearly always need to use the --force
> option with wipefs when trying to wipe something which is not an
> unmounted filesystem.
> 
> Tested using a modified version of libguestfs which detects if wipefs
> supports the --force option, and if it does unconditionally adds it.
> I verified that this fixes the virt-format utility on Fedora 18.

 Applied, thanks.

> This change adds the --force option which, when used, undoes the above
> flag change.  However you still have to use --force most of the time
> when erasing something that isn't a plain unmounted filesystem.

 I'll improve the code. It's stupid disadvantage that --force is
 necessary for things like partitions tables. We have to use
 O_EXCL more carefully.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] wipefs: Add --force option

[Karel Zak]

On Tue, Nov 20, 2012 at 12:16:34PM +0100, Karel Zak wrote:
> On Mon, Nov 19, 2012 at 03:41:16PM +0000, Richard W.M. Jones wrote:
> > This change adds the --force option which, when used, undoes the above
> > flag change.  However you still have to use --force most of the time
> > when erasing something that isn't a plain unmounted filesystem.
> 
>  I'll improve the code. It's stupid disadvantage that --force is
>  necessary for things like partitions tables. We have to use
>  O_EXCL more carefully.

 ... it seems that this is unnecessary. It's possible to delete
 partition table (open with O_EXCL) if any of the partitions is not
 mounted. It seems that only mount(2) has any impact to O_EXCL.

 I guess that --force is enough.

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] wipefs: Add --force option

[Richard W.M. Jones]

On Tue, Nov 20, 2012 at 02:52:46PM +0100, Karel Zak wrote:
> On Tue, Nov 20, 2012 at 12:16:34PM +0100, Karel Zak wrote:
> > On Mon, Nov 19, 2012 at 03:41:16PM +0000, Richard W.M. Jones wrote:
> > > This change adds the --force option which, when used, undoes the above
> > > flag change.  However you still have to use --force most of the time
> > > when erasing something that isn't a plain unmounted filesystem.
> > 
> >  I'll improve the code. It's stupid disadvantage that --force is
> >  necessary for things like partitions tables. We have to use
> >  O_EXCL more carefully.
> 
>  ... it seems that this is unnecessary. It's possible to delete
>  partition table (open with O_EXCL) if any of the partitions is not
>  mounted. It seems that only mount(2) has any impact to O_EXCL.
> 
>  I guess that --force is enough.

Sorry, that was my mistake.  My initial report had a simplifying
assumption that wasn't correct.

In fact the problem case is where you try to wipe a device which
contains a volume groups and a logical volume; note the LVs don't
contain filesystems, and nothing is mounted.

A simple reproducer is:

$ guestfish -N lv wipefs /dev/sda
libguestfs: error: wipefs: wipefs: error: /dev/sda: probing initialization failed: Device or resource busy

Adding -x shows what's going on:

$ guestfish -N lv wipefs /dev/sda -x
libguestfs: trace: set_pgroup true
libguestfs: trace: set_pgroup = 0
libguestfs: trace: add_drive "test1.img"
libguestfs: trace: add_drive = 0
libguestfs: trace: is_config
libguestfs: trace: is_config = 1
libguestfs: trace: launch
libguestfs: trace: get_tmpdir
libguestfs: trace: get_tmpdir = "/tmp"
libguestfs: trace: get_cachedir
libguestfs: trace: get_cachedir = "/var/tmp"
libguestfs: trace: disk_format "/home/rjones/test1.img"
libguestfs: trace: disk_format = "raw"
libguestfs: trace: get_cachedir
libguestfs: trace: get_cachedir = "/var/tmp"
libguestfs: trace: launch = 0
libguestfs: trace: part_disk "/dev/sda" "mbr"
libguestfs: trace: part_disk = 0
libguestfs: trace: pvcreate "/dev/sda1"
libguestfs: trace: pvcreate = 0
libguestfs: trace: vgcreate "VG" "/dev/sda1"
libguestfs: trace: vgcreate = 0
libguestfs: trace: lvcreate_free "LV" "VG" 100
libguestfs: trace: lvcreate_free = 0
libguestfs: trace: wipefs "/dev/sda"
libguestfs: trace: wipefs = -1 (error)
libguestfs: error: wipefs: wipefs: error: /dev/sda: probing initialization failed: Device or resource busy
libguestfs: trace: close
libguestfs: trace: internal_autosync
libguestfs: trace: internal_autosync = 0

Use -v to get more details.

I don't want to claim this is a bug, but it is certainly a change in
behaviour, and we want the --force option in libguestfs so we can say
"just do it".

By the way:

Just a partition + a PV: wipefs doesn't need to be forced.
guestfish -N part pvcreate /dev/sda1 : wipefs /dev/sda

Just a partition + a PV + a VG: wipefs doesn't need to be forced.
guestfish -N part pvcreate /dev/sda1 : vgcreate VG /dev/sda1 : wipefs /dev/sda

Partition + PV + VG + LV (but no filesystem): wipefs needs --force.
[for example see above]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

Re: [PATCH] wipefs: Add --force option

[Milan Broz]

On 11/20/2012 03:41 PM, Richard W.M. Jones wrote:

> In fact the problem case is where you try to wipe a device which
> contains a volume groups and a logical volume; note the LVs don't
> contain filesystems, and nothing is mounted.

Well there is a problem. While you can wipe part table, kernel will
reload it and destroy all obsolete partitions.
There should not be problem if it contains lvm, only if it contains
active LVs mapped.

If there are active device-mapper devices (either created by kpartx,
lvm, crypt, ...) you will lost metadata and cannot remove them
using standard tools, you are forced to do it with dmsetup later.

So wipe part table is fine, but not if partition is mounted or
mapped... You should remove mapping first.

And --force here should be used as a last resort - why not deactivate
mapped device on top first?
It should not be big problem (I think someone wrote this using
lsblk --inverse).

Using force in this situation just creates problem later.

(I am not against the patch, it should have --force option but it
should be used very carefully.)

> libguestfs: trace: lvcreate_free "LV" "VG" 100
> libguestfs: trace: lvcreate_free = 0
> libguestfs: trace: wipefs "/dev/sda"
> libguestfs: trace: wipefs = -1 (error)

IMHO lvcreate implicitly activates device.
Try lvcreate -a n !

Milan

Re: [PATCH] wipefs: Add --force option

[Richard W.M. Jones]

On Tue, Nov 20, 2012 at 04:07:37PM +0100, Milan Broz wrote:
> On 11/20/2012 03:41 PM, Richard W.M. Jones wrote:
> 
> > In fact the problem case is where you try to wipe a device which
> > contains a volume groups and a logical volume; note the LVs don't
> > contain filesystems, and nothing is mounted.
> 
> Well there is a problem. While you can wipe part table, kernel will
> reload it and destroy all obsolete partitions.
> There should not be problem if it contains lvm, only if it contains
> active LVs mapped.
> 
> If there are active device-mapper devices (either created by kpartx,
> lvm, crypt, ...) you will lost metadata and cannot remove them
> using standard tools, you are forced to do it with dmsetup later.

I guess this is not a problem for libguestfs (virt-format in this
case), since we throw away the appliance completely after each run.
We only care about what gets written to the disk.

But yes, I see your point.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw