Re: ZFS boot environment patch

[Konrad Rzeszutek Wilk]

On Fri, Sep 01, 2017 at 01:31:28PM +0200, Paul Lagerweij wrote:
> Hello,
> 
> I've created a patch for util/grub.d/10_linux.in for dynamic ZFS boot
> environment entries. I've only tested the patch in Ubuntu 16.04.2 with GRUB
> 2.02~beta2, ZFS on Linux 0.6.5.6, Linux 4.4.0-83 and 4.4.0-81, but the
> patch should work with any root ZFS configuration, because it only changes
> the 10_linux shell script. It is designed to work with FreeBSD's beadm
> tool, for which I'm currently making a Debian package, but isn't dependent
> on beadm. I added the patch as an attachment and would like to request a
> review.

You seem to be missing an Signed off?

And perhaps an commit description?
> 
> Thanks.

> diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> index de9044c..c9a318e 100644
> --- a/util/grub.d/10_linux.in
> +++ b/util/grub.d/10_linux.in
> @@ -20,6 +20,7 @@ set -e
>  prefix="@prefix@"
>  exec_prefix="@exec_prefix@"
>  datarootdir="@datarootdir@"
> +zfs_be="0"

Why not just

zfs_be=

And then you can just check for zfs_be having an value instead of for 1 or 0?

Or alternatively,

zfs_be=0

and then you can do if [ $zfs_be -ne 0 ]; .. or such?

>  
>  . "$pkgdatadir/grub-mkconfig_lib"
>  
> @@ -62,9 +63,15 @@ case x"$GRUB_FS" in
>       fi;;
>      xzfs)
>       rpool=`${grub_probe} --device ${GRUB_DEVICE} --target=fs_label 
> 2>/dev/null || true`
> -     bootfs="`make_system_path_relative_to_its_root / | sed -e "s,@$,,"`"
> -     LINUX_ROOT_DEVICE="ZFS=${rpool}${bootfs}"
> -     ;;
> +     zfs_active_bootfs="`zpool list -H -o bootfs ${rpool} || true`"

Is zpool usually in /sbin or such? Perhaps a full path?
> +     if [ -n "${zfs_active_bootfs}" ] && [ "${zfs_active_bootfs}" != "-" ] 
> && \
> +        [ `echo ${zfs_active_bootfs} | grep -o '/' | wc -l` -gt 1 ]; then
> +         zfs_be="1"
> +         LINUX_ROOT_DEVICE="ZFS=${zfs_active_bootfs}"
> +     else
> +         bootfs="`make_system_path_relative_to_its_root / | sed -e "s,@$,,"`"
> +         LINUX_ROOT_DEVICE="ZFS=${rpool}${bootfs}"
> +     fi;;

Did you test this where zpool is not installed?
>  esac
>  
>  title_correction_code=
> @@ -177,6 +184,16 @@ title_correction_code=
>  # yet, so it's empty. In a submenu it will be equal to '\t' (one tab).
>  submenu_indentation=""
>  
> +if [ "${zfs_be}" = 1 ]; then
> +  while read ZFS_BE_NAME ZFS_ORIGIN; do
> +    if [ "${ZFS_ORIGIN}" != "-" ]; then
> +      zfs_be_list="${zfs_be_list} ${ZFS_BE_NAME}"
> +    fi
> +  done << EOF
> +`zfs list -H -t filesystem -S creation -o name,origin -d 1 
> ${zfs_active_bootfs%/*} || true`
> +EOF
> +fi
> +
>  is_top_level=true
>  while [ "x$list" != "x" ] ; do
>    linux=`version_find_latest $list`
> @@ -184,6 +201,10 @@ while [ "x$list" != "x" ] ; do
>    basename=`basename $linux`
>    dirname=`dirname $linux`
>    rel_dirname=`make_system_path_relative_to_its_root $dirname`
> +  # If ZFS BE support and /boot is in ZFS.
> +  if [ "${zfs_be}" = 1 ] && [ -n "${rel_dirname}" ]; then
> +    rel_dirname="/""${zfs_active_bootfs#*/}""@/boot"
> +  fi
>    version=`echo $basename | sed -e "s,^[^0-9]*-,,g"`
>    alt_version=`echo $version | sed -e "s,\.old$,,g"`
>    linux_root_device_thisversion="${LINUX_ROOT_DEVICE}"
> @@ -238,12 +259,53 @@ while [ "x$list" != "x" ] ; do
>      is_top_level=false
>    fi
>  
> -  linux_entry "${OS}" "${version}" advanced \
> -              "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
> -  if [ "x${GRUB_DISABLE_RECOVERY}" != "xtrue" ]; then
> -    linux_entry "${OS}" "${version}" recovery \
> -                "single ${GRUB_CMDLINE_LINUX}"
> -  fi
> +  for menu_entries_group in default ${zfs_be_list}; do
> +    found_entry="0"
> +    if [ "${menu_entries_group}" = "default" ]; then
> +      found_entry="1"
> +      os_title="${OS}"
> +    else
> +      found_zfs_be="0"
> +      zfs_be_name=${menu_entries_group}
> +      # If /boot is not in ZFS.
> +      if [ -z "${rel_dirname}" ]; then
> +        found_zfs_be="1"
> +      else
> +        zfs_be_mounted="`zfs list -H -o mounted ${zfs_be_name}`"
> +        if [ "${zfs_be_mounted}" = "yes" ]; then
> +          ZFSMNT=`mount | grep -m 1 "^${zfs_be_name} " | cut -d' ' -f3)`
> +        else
> +          ZFSMNT=`mktemp -d "/tmp/$(echo ${zfs_be_name} | sed 
> 's^/^-^g').XXX" || true`
> +          zfs set mountpoint=legacy ${zfs_be_name} || true
> +          mount -t zfs -o ro ${zfs_be_name} ${ZFSMNT} || true
> +        fi
> +        if [ -n "${ZFSMNT}" ] && [ -f "${ZFSMNT}/boot/vmlinuz-${version}" ]; 
> then
> +          found_zfs_be="1"
> +          rel_dirname="/""${zfs_be_name#*/}""@/boot"
> +        fi
> +        if [ "${zfs_be_mounted}" != "yes" ]; then
> +          umount ${ZFSMNT} && rm -rf ${ZFSMNT} || true
> +          zfs set mountpoint=/ ${zfs_be_name} || true
> +        fi
> +      fi
> +
> +      if [ "${found_zfs_be}" = 1 ]; then
> +        found_entry="1"
> +        os_title="${OS} (${zfs_be_name##*/} BE)"
> +        gettext_printf "Found ZFS boot environment: (%s BE) %s\n" 
> "${zfs_be_name##*/}" "$linux" >&2
> +        linux_root_device_thisversion="ZFS=${zfs_be_name}"
> +      fi
> +    fi
> +
> +    if [ "${found_entry}" = 1 ]; then
> +      linux_entry "${os_title}" "${version}" advanced \
> +                  "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
> +      if [ "x${GRUB_DISABLE_RECOVERY}" != "xtrue" ]; then
> +        linux_entry "${os_title}" "${version}" recovery \
> +                    "single ${GRUB_CMDLINE_LINUX}"
> +      fi
> +    fi
> +  done
>  
>    list=`echo $list | tr ' ' '\n' | fgrep -vx "$linux" | tr '\n' ' '`
>  done

> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel