[PATCH] Don't create grubenv on ZFS

[PATCH] Don't create grubenv on ZFS

[Richard Laager]

[-- Attachment #1.1: Type: text/plain, Size: 214 bytes --]

GRUB can't write to ZFS.  Creating a grubenv file leads to a misleading
"sparse file not allowed" error on boot.  The attached patch for
grub-install skips the creation of a grubenv file on ZFS.

-- 
Richard

[-- Attachment #1.2: zfs-no-grubenv.patch --]
[-- Type: text/x-patch, Size: 580 bytes --]

--- grub2-1.99-orig/util/grub-install.in	2011-04-03 08:36:21.000000000 -0500
+++ grub2-1.99/util/grub-install.in	2012-01-10 00:21:52.159253000 -0600
@@ -460,7 +460,9 @@
 grub_device="`"$grub_probe" --device-map="${device_map}" --target=device "${grubdir}"`" || exit 1
 
 if ! test -f "${grubdir}"/grubenv; then
-    "$grub_editenv" "${grubdir}"/grubenv create
+    if ! test "x`"$grub_probe" --target=fs "${grubdir}"`" = xzfs; then
+        "$grub_editenv" "${grubdir}"/grubenv create
+    fi
 fi
 
 # Create the core image. First, auto-detect the filesystem module.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Don't create grubenv on ZFS

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 02.02.2012 12:16, Richard Laager wrote:
> GRUB can't write to ZFS.  Creating a grubenv file leads to a misleading
> "sparse file not allowed" error on boot.  The attached patch for
> grub-install skips the creation of a grubenv file on ZFS.
This isn't specific to ZFS. BtrFS, squash4, all variants of cpio, tar 
and romfs are affected by the same problem. Could you expand your patch 
to handle those?
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Re: [PATCH] Don't create grubenv on ZFS

[Richard Laager]

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

On Fri, 2012-02-03 at 11:05 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
wrote:
> On 02.02.2012 12:16, Richard Laager wrote:
> > GRUB can't write to ZFS.  Creating a grubenv file leads to a misleading
> > "sparse file not allowed" error on boot.  The attached patch for
> > grub-install skips the creation of a grubenv file on ZFS.
> This isn't specific to ZFS. BtrFS, squash4, all variants of cpio, tar 
> and romfs are affected by the same problem. Could you expand your patch 
> to handle those?

I suspect that handling them by name is not the best way to do this. How
can I tell if a filesystem doesn't support writing?

-- 
Richard

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Don't create grubenv on ZFS

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 03.02.2012 11:07, Richard Laager wrote:
> On Fri, 2012-02-03 at 11:05 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
> wrote:
>> On 02.02.2012 12:16, Richard Laager wrote:
>>> GRUB can't write to ZFS.  Creating a grubenv file leads to a misleading
>>> "sparse file not allowed" error on boot.  The attached patch for
>>> grub-install skips the creation of a grubenv file on ZFS.
>> This isn't specific to ZFS. BtrFS, squash4, all variants of cpio, tar
>> and romfs are affected by the same problem. Could you expand your patch
>> to handle those?
> I suspect that handling them by name is not the best way to do this. How
> can I tell if a filesystem doesn't support writing?
I think it's the best way. One could use grub-fstest blocklist but it 
has other problems.  While btrfs and zfs don't hook blocklist on purpose 
(COW and, more importantly, checksums), others are just almost never 
sector-aligned or are compressed.


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

Re: [PATCH] Don't create grubenv on ZFS

[Richard Laager]

[-- Attachment #1.1: Type: text/plain, Size: 265 bytes --]

I've attached an updated patch.

I also noticed a whitespace inconsistency when looking at another case
statement. I've attached a patch.

Both patches are -p0 and have a ChangeLog entry (in a separate file,
which I think is the convention).

-- 
Richard

[-- Attachment #1.2: zfs-no-grubenv.patch --]
[-- Type: text/x-patch, Size: 929 bytes --]

--- util/grub-install.in	2012-02-03 04:16:36.032265000 -0600
+++ util/grub-install.in	2012-02-03 05:35:50.362628000 -0600
@@ -469,7 +469,12 @@
 grub_device="`"$grub_probe" --device-map="${device_map}" --target=device "${grubdir}"`" || exit 1
 
 if ! test -f "${grubdir}"/grubenv; then
-    "$grub_editenv" "${grubdir}"/grubenv create
+    case "`"$grub_probe" --target=fs "${grubdir}"`" in
+        btrfs | cpiofs | newc | odc | romfs | squash4 | tarfs | zfs)
+	    ;;
+        *)
+            "$grub_editenv" "${grubdir}"/grubenv create
+    esac
 fi
 
 # Create the core image. First, auto-detect the filesystem module.
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ ChangeLog.zfs-no-grubenv	2012-02-03 05:49:27.977376000 -0600
@@ -0,0 +1,5 @@
+2012-02-03  Richard Laager <rlaager@wiktel.com>
+
+	* util/grub-install.in: Don't create a grubenv file on filesystems
+	to which GRUB cannot write.
+

[-- Attachment #1.3: grub-install-whitespace.patch --]
[-- Type: text/x-patch, Size: 1037 bytes --]

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ ChangeLog.grub-install-whitespace	2012-02-03 05:52:51.439942000 -0600
@@ -0,0 +1,4 @@
+2012-02-03  Richard Laager <rlaager@wiktel.com>
+
+	* util/grub-install.in: Fix some inconsistent whitespace.
+
--- util/grub-install.in	2012-02-03 05:53:36.039465769 -0600
+++ util/grub-install.in	2012-02-03 05:54:04.771697000 -0600
@@ -486,13 +486,13 @@
 # filesystem will be accessible).
 partmap_module=
 for x in `echo "${grub_device}" | xargs "$grub_probe" --device-map="${device_map}" --target=partmap --device 2> /dev/null`; do
-   case "$x" in
-       netbsd | openbsd) 
-	   partmap_module="$partmap_module part_bsd";;
-       "") ;;
-       *)
-	   partmap_module="$partmap_module part_$x";;
-   esac
+    case "$x" in
+        netbsd | openbsd)
+            partmap_module="$partmap_module part_bsd";;
+        "") ;;
+        *)
+            partmap_module="$partmap_module part_$x";;
+    esac
 done
 
 # Device abstraction module, if any (lvm, raid).

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Don't create grubenv on ZFS

[Jordan Uggla]

On Thu, Feb 2, 2012 at 3:16 AM, Richard Laager <rlaager@wiktel.com> wrote:
> GRUB can't write to ZFS.  Creating a grubenv file leads to a misleading
> "sparse file not allowed" error on boot.  The attached patch for
> grub-install skips the creation of a grubenv file on ZFS.

I think that not writing /boot/grub/grubenv is a poor work around for
the problem of grubenv not being writable.

1: Even if it doesn't now, I would expect an error message when
save_env is run and no grubenv is present, so there should be an error
message either way.

2: There are uses for grubenv that do not involve writing to it at
boot. Fedora's grub2 documentation recommends grub-set-default as the
best way to change the default menu entry:
http://fedoraproject.org/wiki/Grub2#Setting_default_entry and if I
understand correctly this should continue to work on any filesystem
which grub can read.

3: It can't be determined with certainty that grubenv will be
writeable at boot. I have run into problems where I installed grub to
an SD card and then flipped the write protect switch on the card,
which could not have been anticipated at grub-install time. For this
reason I think the focus should be on graceful degradation and
understandable error messages when save_env is called with a read only
grubenv (and possibly there should be ways to disable calls to grubenv
in grub-mkconfig, especially in the case of Ubuntu's recordfail
modifications).

I do however think that some utilities should warn (but not otherwise
change their behavior) when grubenv does not appear to be writable.
The prime example of this would be grub-reboot. When grub-reboot is
called with a filesytem / abstraction which will prevent grub from
writing to the grubenv at boot, grub-reboot cannot possibly function.
It already degrades to basically having no effect, so there is no harm
in making the changes to grubenv on the off chance the user knows
better, but it should produce a clear warning that grub-reboot cannot
function without a grubenv writeable by grub at boot.

-- 
Jordan Uggla (Jordan_U on irc.freenode.net)

Re: [PATCH] Don't create grubenv on ZFS

[Richard Laager]

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

You make good points. I don't have a problem with the idea that save_env
should throw an error when it can't actually *save* the grubenv.

So a better way to look at my problem is to say, "I don't want my
grub.cfg to call save_env by default."

This then becomes an downstream Ubuntu bug, because my grub.cfg is only
calling save_env by default for recordfail. I've submitted a patch to
Ubuntu which disables recordfail for these filesystems:
https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/913336/comments/7

So I withdraw zfs-no-grubenv.patch.

-- 
Richard

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]