Bug#824895: [PATCH] warn against likely data loss

[Dimitri John Ledkov]

On 5 September 2016 at 17:35, Nicholas D Steeves  wrote:
> On Tue, Jul 26, 2016 at 12:35:56PM +0100, Dimitri John Ledkov wrote:
>> On 20 May 2016 at 22:48, Nicholas D Steeves  wrote:
>> >
>> > Because btrfs-convert is known to fail, and is known to causes
>> > data-loss that may take some months to manifest, we must provide a
>> > warning, because data-loss is serious.  This patch also includes a
>> > contrib initramfs hook which is installed to
>> > /usr/share/doc/btrfs-progs; it must be manually copied into place;
>> > I've included it because this is the first place a sophisticated user
>> > would look.
>> >
>>
>> Adding a btrfs-convert initramfs hook (albeit disabled) and warning
>> not to use it, sounds a lot like negative suggestion encouraging
>> people to use it.
>>
>> I do not want to ship btrfs-convert (nor provide suggestions) in the
>> initramfs precisely because upstream discourages people from using it.
>>
>> I also fail to see how this is an RC bug. btrfs-convert is a crutch
>> for people to toy and try out btrfs, and always has been.
>
> Primary issue: btrfs-convert provides no warning
> Secondary issue introduced by proposed fix: btrfs-convert is a diversion 
> script
>
> To solve the primary issue, would you prefer a quilt patch to the source?  
> The problem of encouraging masochistic users is only relevant to the 
> workaround for the issue a diversion script creates.
>
> Raid5/6 profiles are also now on the list of features upstream considers 
> dangerous, but there is yet no warning.
>

Please submit warnings as patches upstream

-- 
Regards,

Dimitri.

Bug#824895: [PATCH] warn against likely data loss

[Nicholas D Steeves]

On Tue, Jul 26, 2016 at 12:35:56PM +0100, Dimitri John Ledkov wrote:
> On 20 May 2016 at 22:48, Nicholas D Steeves  wrote:
> >
> > Because btrfs-convert is known to fail, and is known to causes
> > data-loss that may take some months to manifest, we must provide a
> > warning, because data-loss is serious.  This patch also includes a
> > contrib initramfs hook which is installed to
> > /usr/share/doc/btrfs-progs; it must be manually copied into place;
> > I've included it because this is the first place a sophisticated user
> > would look.
> >
> 
> Adding a btrfs-convert initramfs hook (albeit disabled) and warning
> not to use it, sounds a lot like negative suggestion encouraging
> people to use it.
> 
> I do not want to ship btrfs-convert (nor provide suggestions) in the
> initramfs precisely because upstream discourages people from using it.
> 
> I also fail to see how this is an RC bug. btrfs-convert is a crutch
> for people to toy and try out btrfs, and always has been.

Primary issue: btrfs-convert provides no warning
Secondary issue introduced by proposed fix: btrfs-convert is a diversion script

To solve the primary issue, would you prefer a quilt patch to the source?  The 
problem of encouraging masochistic users is only relevant to the workaround for 
the issue a diversion script creates.

Raid5/6 profiles are also now on the list of features upstream considers 
dangerous, but there is yet no warning.

Cheers,
Nicholas

Bug#824895: [PATCH] warn against likely data loss

[Dimitri John Ledkov]

Control: severity -1 wishlist
Control: tag -1 wontfix

Hello,

On 20 May 2016 at 22:48, Nicholas D Steeves  wrote:
> Package: btrfs-progs
> Version: 4.5.2-1
> Tags: patch
> Control: severity -1 severe
>
> Because btrfs-convert is known to fail, and is known to causes
> data-loss that may take some months to manifest, we must provide a
> warning, because data-loss is serious.  This patch also includes a
> contrib initramfs hook which is installed to
> /usr/share/doc/btrfs-progs; it must be manually copied into place;
> I've included it because this is the first place a sophisticated user
> would look.
>
> Signed-off-by: Nicholas D Steeves 

Adding a btrfs-convert initramfs hook (albeit disabled) and warning
not to use it, sounds a lot like negative suggestion encouraging
people to use it.

I do not want to ship btrfs-convert (nor provide suggestions) in the
initramfs precisely because upstream discourages people from using it.

I also fail to see how this is an RC bug. btrfs-convert is a crutch
for people to toy and try out btrfs, and always has been.

-- 
Regards,

Dimitri.

Bug#824895: [PATCH] warn against likely data loss

[Nicholas D Steeves]

Justification: Problems like these [1] still exist.

[1] https://www.spinics.net/lists/linux-btrfs/msg56596.html

Bug#824895: [PATCH] warn against likely data loss

[Nicholas D Steeves]

Package: btrfs-progs
Version: 4.5.2-1
Tags: patch
Control: severity -1 severe

Because btrfs-convert is known to fail, and is known to causes
data-loss that may take some months to manifest, we must provide a
warning, because data-loss is serious.  This patch also includes a
contrib initramfs hook which is installed to
/usr/share/doc/btrfs-progs; it must be manually copied into place;
I've included it because this is the first place a sophisticated user
would look.

Signed-off-by: Nicholas D Steeves 
diff -ur --new-file btrfs-progs-4.5.2/debian/local/btrfs-convert.hook btrfs-progs-4.5.2.changed/debian/local/btrfs-convert.hook
--- btrfs-progs-4.5.2/debian/local/btrfs-convert.hook	1969-12-31 19:00:00.0 -0500
+++ btrfs-progs-4.5.2.changed/debian/local/btrfs-convert.hook	2016-05-20 17:00:42.460132609 -0400
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+. /usr/share/initramfs-tools/hook-functions
+
+if command -v /bin/btrfs-convert >/dev/null 2>&1; then
+copy_exec /bin/btrfs-convert bin/btrfs-convert
+copy_exec /bin/btrfs-convert.real bin/btrfs-convert.real
+fi
diff -ur --new-file btrfs-progs-4.5.2/debian/local/btrfs-convert.wrapper btrfs-progs-4.5.2.changed/debian/local/btrfs-convert.wrapper
--- btrfs-progs-4.5.2/debian/local/btrfs-convert.wrapper	1969-12-31 19:00:00.0 -0500
+++ btrfs-progs-4.5.2.changed/debian/local/btrfs-convert.wrapper	2016-05-20 17:01:01.084103049 -0400
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+cat << !EOF!
+  WARNING 
+Btrfs-convert is known to fail. An upstream rewrite of the
+utility is currently in progress. At this point in time,
+btrfs-convert is provided for experimental purposes only.
+
+Please do not convert filesystems filled with data you value.
+Expect that you will need to restore from backup.  Beware,
+the dataloss may not be immediately apparent, and may take
+some time to manifest.
+
+If you choose to experiment with btrfs-convert, please do so
+with the willingness to work with linux-bt...@vger.kernel.org
+
+!EOF!
+echo -n 'Proceed (Yes/No)?: '
+
+while true; do
+read -r choice
+case "$choice" in
+Yes )
+exec /bin/btrfs-convert.real "$@"
+exit $?
+;;
+no | n | No | NO )
+echo "Thank you for making the safe choice"
+exit 0
+;;
+* )
+echo -n "Please answer Yes or No: "
+;;
+esac
+done
diff -ur --new-file btrfs-progs-4.5.2/debian/rules btrfs-progs-4.5.2.changed/debian/rules
--- btrfs-progs-4.5.2/debian/rules	2016-05-10 05:15:55.0 -0400
+++ btrfs-progs-4.5.2.changed/debian/rules	2016-05-20 16:59:16.708269112 -0400
@@ -27,6 +27,12 @@
 	# Adding initramfs-tools integration
 	install -D -m 0755 debian/local/btrfs.hook debian/btrfs-progs/usr/share/initramfs-tools/hooks/btrfs
 	install -D -m 0755 debian/local/btrfs.local-premount debian/btrfs-progs/usr/share/initramfs-tools/scripts/local-premount/btrfs
+	install -D -m 0755 debian/local/btrfs-convert.hook debian/btrfs-progs/usr/share/doc/btrfs-convert.hook
+
+# divert btrfs-convert to btrfs-convert.real
+# and use a wrapper script to notify it is dangerous
+	mv debian/btrfs-progs/bin/btrfs-convert debian/btrfs-progs/bin/btrfs-convert.real
+	install -D -m 0755 debian/local/btrfs-convert.wrapper debian/btrfs-progs/bin/btrfs-convert
 
 # Needs autopkgtest
 override_dh_auto_test: