Merge ~jibel/ubiquity:zfs_fixes into ubiquity:master

[Jean-Baptiste Lallement]

Various fixes from experiment:
  - Use version 5000 for bpool with some features disabled to stay compatible with grub and prevent users from upgrading and breaking their systems.
  - The partman confirmation dialog now displays the final layout of the partition that will be created instead of the true but confusing message from partman telling that an ext4 partition will be created (LP: #1847719)
  - Calculate the size of bpool to be 500M < 5% partition size < 2G
  - Always create an ESP and moved grub to the ESP.

[Didier Roche-Tolomelli (didrocks)]

I didn't test yet as we are going to have some iterations. Here are mostly nitpick, good work :)

[Jean-Baptiste Lallement (jibel)]

Thanks fir the review. Comments addressed.

[Didier Roche-Tolomelli (didrocks)]

- one more comment on the new changes
- still 2 pending on previous diff

[Didier Roche-Tolomelli (didrocks)]

LGTM! I didn't give it a try yet, but I didn't spot anything more. Just 2 notes:
- Is having predictable names in /tmp is a security issue? I remember we removed from flag filenames in the past due to this.
- Did you have a look at our latest exchanges with Richard (the ones before the 19.10 release) to ensure we address all of his remarks?

Ack (pending testing)

[Didier Roche-Tolomelli (didrocks)]

Did a test-run without UEFI enabled, and working well for me.

[Jean-Baptiste Lallement (jibel)]

I reviewed the comments from Richard in the spec and latest email exchanges and apparently they've all been addressed.

About the temporary directory in /tmp we could pass an additional argument to the script, but it's called from completely different scopes: from ubiquity itself (ubi-partman and gtk_ui) and from a subprocess, it would add lot of complexity for share the name of the temporary directory between these scopes. Passing it in the environment wouldn't improve the security either. At best we could umask the directory so it can only be accessed by root.

[Didier Roche-Tolomelli (didrocks)]

+1 for me then :)

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 8a152ab..9ba61ce 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,18 @@
 ubiquity (20.04.1) UNRELEASED; urgency=medium
 
-  * script/zsys-setup: fix tag name from org.zsys to com.ubuntu.zsys.
-  * Added an advanced features dialog to the partitioning page. LVM, LVM with
-    encryption and experimental ZFS support have been moved to this dialog.
+  * zfs:
+    - Added an advanced features dialog to the partitioning page. LVM, LVM
+      with encryption and experimental ZFS support have been moved to this
+      dialog.
+    - Fix tag name from org.zsys to com.ubuntu.zsys.
+    - Use version 5000 for bpool with some features disabled to stay
+      compatible with grub and prevent users from upgrading and breaking their
+      systems.
+    - The partman confirmation dialog now displays the final layout of the
+      partition that will be created instead of the true but confusing message
+      from partman telling that an ext4 partition will be created (LP: #1847719)
+    - Calculate the size of bpool to be 500M < 5% partition size < 2G
+    - Always create an ESP and moved grub to the ESP.
 
  -- Jean-Baptiste Lallement <[e-mail]>  Mon, 28 Oct 2019 09:07:57 +0100
 
diff --git a/debian/ubiquity.templates b/debian/ubiquity.templates
index f56ac69..4342a2a 100644
--- a/debian/ubiquity.templates
+++ b/debian/ubiquity.templates
@@ -1597,3 +1597,8 @@ _Description:
 Template: ubiquity/secureboot_key
 Type: password
 _Description: SecureBoot key for MokPW
+
+Template: ubiquity/text/partman_confirm_zfs
+Type: text
+_Description:
+ partition #%(partid)s as %(parttype)s used for %(partusage)s
\ No newline at end of file
diff --git a/scripts/zsys-setup b/scripts/zsys-setup
index 439a9d7..554adb2 100755
--- a/scripts/zsys-setup
+++ b/scripts/zsys-setup
@@ -43,7 +43,12 @@ set -eu
 REQUIREDPKGS="zfsutils-linux"
 TARGET="/target"
 ESP="${TARGET}/boot/efi"
-INIT_FLAG="/tmp/.zsys-init.done"
+ZSYSTMP="/tmp/$(basename $0)"
+INIT_FLAG="${ZSYSTMP}/init.done"
+FSTAB_PARTMAN="${ZSYSTMP}/fstab.partman"
+PARTITION_LAYOUT="${ZSYSTMP}/layout"
+
+mkdir -p "${ZSYSTMP}"
 
 usage() {
     # Display script usage
@@ -122,7 +127,7 @@ prepare_target() {
 	# Save fstab generated by partman
 	if [ -f "${target}/etc/fstab" ]; then
 	    echo "I: Saving existing fstab"
-		cp "${target}/etc/fstab" /tmp/fstab.partman
+		cp "${target}/etc/fstab" "${FSTAB_PARTMAN}"
 	else
 		echo "W: ${target}/etc/fstab doesn't exist"
 	fi
@@ -156,46 +161,116 @@ prepare_target() {
 	done
 }
 
+get_layout() {
+	# Returns disk, base name of the partition and partition numbers to create
+	target="$1"
+
+	# The entire disk has been formatted with use_device
+	# There is either one ext4 partition or one ext4 and one ESP
+	part="$(grep -E "\s${target}\s" /proc/mounts | awk '{print $1}')"
+	disk=""
+	partbase=""
+
+	if [ -n "${part}" ]; then
+		disk="$(lsblk -lns -o TYPE,PATH ${part}| grep disk| awk '{print $2}')"
+		if [ -z "${disk}" ]; then
+			echo "E: Couldn't identify disk for partition ${part}. Exiting!"
+			exit 1
+		fi
+		# Some disks have letters in the partition number like /dev/nvme0n1p1
+		# In this case we want to retrieve 'p' so we deal only with partition number
+		# in the rest of the script and prepend the base.
+		partbase="$(echo ${part} | sed -e 's/[0-9]*$//' | sed -e "s#${disk}##")"
+	fi
+
+	partesp=1
+	if in_efi_mode; then
+		# No extended partition on EFI + GPT
+		# The layout is
+		# 1: ESP
+		# 2: swap
+		# 3: bpool
+		# 4: rpool
+		partswap=2
+		partbpool=3
+		partrpool=4
+	else
+		# MBR pools are on extended partition
+		# The layout is:
+		# 1: ESP
+		# 2: Extended
+		# 5: swap
+		# 6: bpool
+		# 7: rpool
+		partswap=5
+		partbpool=6
+		partrpool=7
+	fi
+
+	echo "OK|${disk}|${partbase}|${partesp}|${partswap}|${partbpool}|${partrpool}"
+}
+
 format_disk() {
 	disk="$1"
-	partgrub="$2"
-	partbpool="$3"
-	partrpool="$4"
-	ss="$5"
+	partbase="$2"
+	partesp="$3"
+	partbpool="$4"
+	partrpool="$5"
+	ss="$6"
 	partswap=$(( partbpool - 1 ))
-	partext=$(( partgrub + 1 ))
+	partext=$(( partesp + 1 ))
 	sfdiskopts=""
-	partprefix="${disk}${PARTBASE}"
+	partprefix="${disk}${partbase}"
 
-	echo "I: Formatting disk $disk with partitions grub:${partgrub} ext:${partext} bpool:${partbpool} rpool:${partrpool}"
+	sfdisktmp="${ZSYSTMP}/sfdisk.cfg"
+	rm -f "${sfdisktmp}"
 
-	if [ "${IS_EFI}" = "true" ]; then
+	echo "I: Formatting disk $disk with partitions ESP:${partesp} ext:${partext} swap:${partswap} bpool:${partbpool} rpool:${partrpool}"
 
+	# bpool size: 500M < 5% of ZFS allocated space < 2G
+	# partext4 is the ext4 partition created by partman.
+	# Its id is 2 or 1 depending on being on and EFI system of not.
+	partext4=1
+	if in_efi_mode; then
+		partext4=$(( partesp + 1 ))
+	fi
+
+	size_percent=$(expr \( $(blockdev --getsize64 ${disk}${partext4}) / 1024 / 1024 \) \* 5 / 100)
+	bpool_size=500
+	[ ${size_percent} -gt ${bpool_size} ] && bpool_size=${size_percent}
+	[ ${bpool_size} -gt 2048 ] && bpool_size=2048
+
+	if in_efi_mode; then
 		# Improvement: Delete all the partitions but the ESP
-		# We should be only 1 or 2 partittion but well it can be made generic
-		sfdisk --delete "${disk}" ${partgrub}
-
-		cat > /tmp/sfdisk.cfg <<EOF
-${partprefix}${partgrub}  :   size=    50M, type=0FC63DAF-8483-4772-8E79-3D69D8477DE4
-${partprefix}${partswap}  :   size= ${ss}M, type=0657FD6D-A4AB-43C4-84E5-0933C84B4F4F
-${partprefix}${partbpool} :   size=     2G, type=6A82CB45-1DD2-11B2-99A6-080020736631
-${partprefix}${partrpool} :                 type=6A85CF4D-1DD2-11B2-99A6-080020736631
+		# There should be only 1 or 2 partitions but it can be made generic
+		sfdisk --delete "${disk}" ${partswap}
+
+		cat > "${sfdisktmp}" <<EOF
+${partprefix}${partswap}  :   size= ${ss}M,         type=0657FD6D-A4AB-43C4-84E5-0933C84B4F4F
+${partprefix}${partbpool} :   size= ${bpool_size}M, type=6A82CB45-1DD2-11B2-99A6-080020736631
+${partprefix}${partrpool} :                         type=6A85CF4D-1DD2-11B2-99A6-080020736631
 EOF
 		sfdiskopts="--append"
 	else
-		start=$(sfdisk -l "${disk}"|grep "^${partprefix}${partgrub}"|awk '{print $2}')
-
-		cat > /tmp/sfdisk.cfg <<EOF
-${partprefix}${partgrub}  : start=        $start, size=    50M, type=83, bootable
-${partprefix}${partext}   :                                   type=5
-${partprefix}${partswap}  :                     size= ${ss}M, type=82
-${partprefix}${partbpool} :                     size=     2G, type=a5
-${partprefix}${partrpool} :                                   type=a5
+		if ! esp_exists; then
+			start=$(sfdisk -l "${disk}"|grep "^${partprefix}${partesp}"|awk '{print $2}')
+			cat > "${sfdisktmp}" <<EOF
+${partprefix}${partesp}   : start= $start, size=    512M,        type=ef, bootable
+EOF
+		else
+			sfdisk --delete "${disk}" ${partswap}
+		fi
+
+		cat >> "${sfdisktmp}" <<EOF
+${partprefix}${partext}   :                                      type=5
+${partprefix}${partswap}  :                size= ${ss}M,         type=82
+${partprefix}${partbpool} :                size= ${bpool_size}M, type=a5
+${partprefix}${partrpool} :                                      type=a5
 EOF
 
 	fi
 
-	cat /tmp/sfdisk.cfg | sfdisk ${sfdiskopts} "${disk}"
+	cat "${sfdisktmp}" | sfdisk ${sfdiskopts} "${disk}"
 
 	# Force a re-read of the partition table
 	echo "I: Re-reading partition table"
@@ -228,12 +303,26 @@ init_zfs() {
 		-O mountpoint=/ -R "${target}" rpool "${partrpool}"
 
 	# bpool
+	# The version of bpool is set to the default version to prevent users from upgrading
+	# Then only features supported by grub are enabled.
 	zpool create -f \
-		-o version=28 \
 		-o ashift=12 \
 		-d \
-		-O xattr=sa \
+		-o feature@async_destroy=enabled \
+		-o feature@bookmarks=enabled \
+		-o feature@embedded_data=enabled \
+		-o feature@empty_bpobj=enabled \
+		-o feature@enabled_txg=enabled \
+		-o feature@extensible_dataset=enabled \
+		-o feature@filesystem_limits=enabled \
+		-o feature@hole_birth=enabled \
+		-o feature@large_blocks=enabled \
+		-o feature@lz4_compress=enabled \
+		-o feature@spacemap_histogram=enabled \
+		-o feature@userobj_accounting=enabled \
+		-O compression=lz4 \
 		-O acltype=posixacl \
+		-O xattr=sa \
 		-O relatime=on \
 		-O normalization=formD \
 		-O canmount=off \
@@ -297,18 +386,27 @@ init_system_partitions() {
 	partefi="$2"
 	partgrub="$3"
 
-	echo "I: Creating grub partition $partgrub"
+	# ESP
+	mkdir -p "${target}/boot/efi"
+	if ! in_efi_mode; then
+		echo "I: Creating ESP"
+		mkfs.vfat "${partefi}"
+	fi
+	mount -t vfat "${partefi}" "${target}/boot/efi"
+	mkdir -p "${target}/boot/efi/grub"
+
+	echo "I: Mount grub directory"
 	# Finalize grub directory
 	mkdir -p "${target}/boot/grub"
-	mke2fs -F -t ext4 "${partgrub}"
-	mount -t ext4 "${partgrub}" "${target}/boot/grub"
-
-	if [ "${IS_EFI}" = "true" ]; then
-		# ESP
-		mkdir -p "${target}/boot/efi"
-		# No need to reformat the partition has been prepared by partman-efi
-		mount -t vfat "${partefi}" "${target}/boot/efi"
-	fi
+	mount -o bind "${target}/boot/efi/grub" "${target}/boot/grub"
+}
+
+in_efi_mode() {
+    [ -d /proc/efi ] || [ -d /sys/firmware/efi ]
+}
+
+esp_exists() {
+    grep -q "\s${ESP}\s" /proc/mounts
 }
 
 check_prerequisites ${REQUIREDPKGS}
@@ -318,57 +416,48 @@ echo "I: Running $(basename "$0") ${COMMAND}"
 if [ -z "${COMMAND}" ]; then
 	echo "E: ${COMMAND} is mandatory. Exiting!"
 	exit 1
-elif [ "${COMMAND}" = "init" ]; then
-	# The entire disk has been formatted with use_device
-	# There is either one ext4 partition or one ext4 and one ESP
-	PART=$(grep -E "\s${TARGET}\s" /proc/mounts | awk '{print $1}')
+elif [ "${COMMAND}" = "layout" ]; then
+	# Just displays de layout that will be created without any change to the disk.
+	# At this stage we don't now yet the size of the partition that will be created.
+	IFS="|" read ERR DISK PARTBASE PARTESP PARTSWAP PARTBPOOL PARTRPOOL<<EOF
+$(get_layout ${TARGET})
+EOF
 
-	if [ -z "${PART}" ]; then
-		echo "E: Couldn't identify partition matching ${TARGET}. Exiting!"
+	if [ "${ERR}" != "OK" ]; then
+		echo "${ERR}"
 		exit 1
 	fi
 
-	DISK="$(lsblk -lns -o TYPE,PATH ${PART}| grep disk| awk '{print $2}')"
+	cat > "${PARTITION_LAYOUT}" <<EOF
+disk:${DISK}
+EOF
+	if ! in_efi_mode || ! esp_exists; then
+		cat >> "${PARTITION_LAYOUT}" <<EOF
+part:vfat:ESP:${DISK}${PARTBASE}${PARTESP}
+EOF
+	fi
 
-	echo "I: Partition table before init of ZFS"
-	partx --show "${DISK}"
+	cat >> "${PARTITION_LAYOUT}" <<EOF
+part:swap:swap:${DISK}${PARTBASE}${PARTSWAP}
+part:zfs:bpool:${DISK}${PARTBASE}${PARTBPOOL}
+part:zfs:rpool:${DISK}${PARTBASE}${PARTRPOOL}
+EOF
 
-	# Some disks have letters in the partion number like /dev/nvme0n1p1
-	# In this case we want to retrieve 'p' so we deal only with partition number
-	# in the rest of the script and prepend the base.
-	PARTBASE="$(echo ${PART} | sed -e 's/[0-9]*$//' | sed -e "s#${DISK}##")"
+elif [ "${COMMAND}" = "init" ]; then
+	rm -f "${INIT_FLAG}"
 
-	# Better check if the ESP has been created than if it's a EFI system
-	IS_EFI="false"
-	if grep -q "\s${ESP}\s" /proc/mounts; then
-		IS_EFI="true"
-	fi
+	IFS="|" read ERR DISK PARTBASE PARTESP PARTSWAP PARTBPOOL PARTRPOOL<<EOF
+$(get_layout ${TARGET})
+EOF
 
-	PARTGRUB=$(lsblk -n -o TYPE ${DISK}|grep ^part| wc -l)
-	if [ "$IS_EFI" = "true" ]; then
-		# No extended partition on EFI + GPT
-		# The layout is
-		# 1: ESP
-		# 2: grub
-		# 3: swap
-		# 4: bpool
-		# 5: rpool
-		PARTSWAP=3
-		PARTBPOOL=4
-		PARTRPOOL=5
-	else
-		# MBR pools are on extended partition
-		# The layout is:
-		# 1: grub
-		# 2: Extended
-		# 5: swap
-		# 6: bpool
-		# 7: rpool
-		PARTSWAP=5
-		PARTBPOOL=6
-		PARTRPOOL=7
+	if [ "${ERR}" != "OK" ]; then
+		echo "${ERR}"
+		exit 1
 	fi
 
+	echo "I: Partition table before init of ZFS"
+	partx --show "${DISK}"
+
 	# Swap files are not supported on ZFS, we use a swap partition instead:
 	SWAPFILE="$(grep "^${TARGET}" /proc/swaps |awk '{print $1}')"
 	# Give us a minimum swap partition size of 4MB in case we decide on
@@ -385,20 +474,26 @@ elif [ "${COMMAND}" = "init" ]; then
 	SWAPVOLSIZE=$(( SWAPSIZE / 1024 / 1024 ))
 
 	prepare_target "${TARGET}"
-	format_disk "${DISK}" "${PARTGRUB}" "${PARTBPOOL}" "${PARTRPOOL}" "${SWAPVOLSIZE}"
+	format_disk "${DISK}" "${PARTBASE}" "${PARTESP}" "${PARTBPOOL}" "${PARTRPOOL}" "${SWAPVOLSIZE}"
 	init_zfs "${TARGET}" "${DISK}${PARTBASE}${PARTBPOOL}" "${DISK}${PARTBASE}${PARTRPOOL}"
-	init_system_partitions "${TARGET}" "${DISK}${PARTBASE}1" "${DISK}${PARTBASE}${PARTGRUB}"
+	init_system_partitions "${TARGET}" "${DISK}${PARTBASE}1" "${DISK}${PARTBASE}${PARTESP}"
 
 	# Generate fstab
 	# $TARGET/etc has been destroyed by the creation of the zfs partitition
 	# Recreate it
 	mkdir -p "${TARGET}/etc"
-	if [ -f /tmp/fstab.partman ]; then
+	if [ -f "${FSTAB_PARTMAN}" ]; then
 		echo "I: Creating fstab"
-		grep -Ev '\s/\s|/swapfile' /tmp/fstab.partman > "${TARGET}/etc/fstab"
+		grep -Ev '\s/\s|/swapfile' "${FSTAB_PARTMAN}" > "${TARGET}/etc/fstab"
+	fi
+
+	if ! grep -q "boot/efi" "${TARGET}/etc/fstab"; then
+		espuuid=$(blkid -s UUID -o value "${DISK}${PARTBASE}${PARTESP}")
+		echo "UUID=${espuuid}\t/boot/efi\tvfat\tumask=0077\t0\t1" >> "${TARGET}/etc/fstab"
 	fi
-	grubuuid=$(blkid -s UUID -o value "${DISK}${PARTBASE}${PARTGRUB}")
-	echo "UUID=${grubuuid}\t/boot/grub\text4\terrors=remount-ro\t0\t1" >> "${TARGET}/etc/fstab"
+
+	# Bind mount grub from ESP to the expected location
+	echo "/boot/efi/grub\t/boot/grub\tnone\tdefaults,bind\t0\t0" >> "${TARGET}/etc/fstab"
 
 	if [ -n "${SWAPFILE}" ]; then
 		SWAPDEVICE="${DISK}${PARTBASE}${PARTSWAP}"
diff --git a/ubiquity/plugins/ubi-partman.py b/ubiquity/plugins/ubi-partman.py
index 5601d7d..086ac2b 100644
--- a/ubiquity/plugins/ubi-partman.py
+++ b/ubiquity/plugins/ubi-partman.py
@@ -3215,9 +3215,15 @@ class Page(plugin.Plugin):
                 raise AssertionError("Arrived at %s unexpectedly" % question)
 
         elif question.startswith('partman/confirm'):
+            description = self.extended_description(question)
+
+            if hasattr(self.ui, "use_zfs"):
+                if (self.ui.use_zfs.get_active() and self.ui.use_device.get_active()):
+                    description = self.update_zfs_description(self.extended_description(question))
+
             response = self.frontend.question_dialog(
                 self.description(question),
-                self.extended_description(question),
+                description,
                 ('ubiquity/text/go_back', 'ubiquity/text/continue'))
             if response == 'ubiquity/text/continue':
                 self.db.set('ubiquity/partman-confirm', question[8:])
@@ -3293,6 +3299,40 @@ class Page(plugin.Plugin):
 
         return plugin.Plugin.run(self, priority, question)
 
+    def update_zfs_description(self, description):
+        """Update the description in the partman dialog to display custom
+           messages"""
+
+        misc.execute_root('/usr/share/ubiquity/zsys-setup', 'layout')
+
+        zsys_layout = '/tmp/zsys-setup/layout'
+        lines = description.splitlines()
+        # Remove the last line of the output from partman which corresponds to
+        # the ext4 partition
+        del(lines[-1])
+
+        if not os.path.exists(zsys_layout):
+            return description
+
+        with open(zsys_layout, 'r') as f:
+            layout = f.readlines()
+
+        partlabel = misc.utf8(self.db.metaget('ubiquity/text/partman_confirm_zfs', 'extended_description'),
+                              errors='replace')
+        for line in layout:
+            if not line.startswith("part:"):
+                continue
+
+            line = line.strip()
+            (t, f, u, p) = line.split(':')
+
+            lines.append("    " + partlabel % {
+                'partid': os.path.basename(p),
+                'parttype': f,
+                'partusage': u})
+
+        return "\n".join(lines)
+
     def ok_handler(self):
         if self.install_bootloader and not self.is_bootdev_preseeded():
             self.preseed('grub-installer/bootdev', self.ui.get_grub_choice())