Re: [gentoo-dev] [PATCH] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

["Michał Górny" <mgorny@gentoo.org>]

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

On Fri, 2019-12-06 at 14:11 +0100, Ulrich Müller wrote:
> The eclass failed to remount a read-only mounted /boot, because package
> collision sanity checks in recent Portage versions prevented it from
> reaching pkg_pretend() at all. Furthermore, with the "mount-sandbox"

Did you mean: pkg_preinst?

> feature enabled, the mount won't be propagated past pkg_preinst() and
> installed files would end up under the (shadowed) mount point.
> 
> Therefore don't even attempt to mount /boot ourselves, but error out
> if it isn't mounted read/write and ask the user to mount /boot.
> 
> Also clean up and simplify. (For example, awk is a grown-up program
> which doesn't need any help from egrep or sed. :-)
> 
> Closes: https://bugs.gentoo.org/532264
> Signed-off-by: Ulrich Müller <ulm@gentoo.org>
> ---
>  eclass/mount-boot.eclass | 137 ++++++++++++---------------------------
>  1 file changed, 43 insertions(+), 94 deletions(-)
> 
> diff --git a/eclass/mount-boot.eclass b/eclass/mount-boot.eclass
> index 938df6732f4..1d7eb8bfc29 100644
> --- a/eclass/mount-boot.eclass
> +++ b/eclass/mount-boot.eclass
> @@ -1,156 +1,105 @@
> -# Copyright 1999-2015 Gentoo Foundation
> +# Copyright 1999-2019 Gentoo Authors
>  # Distributed under the terms of the GNU General Public License v2
>  
>  # @ECLASS: mount-boot.eclass
>  # @MAINTAINER:
>  # base-system@gentoo.org
>  # @BLURB: functions for packages that install files into /boot
>  # @DESCRIPTION:
>  # This eclass is really only useful for bootloaders.
>  #
>  # If the live system has a separate /boot partition configured, then this
>  # function tries to ensure that it's mounted in rw mode, exiting with an
> -# error if it can't. It does nothing if /boot isn't a separate partition.
> +# error if it can't.  It does nothing if /boot isn't a separate partition.
> +
> +case ${EAPI:-0} in
> +	4|5|6|7) ;;
> +	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> +esac
>  
>  EXPORT_FUNCTIONS pkg_pretend pkg_preinst pkg_postinst pkg_prerm pkg_postrm
>  
>  # @FUNCTION: mount-boot_disabled
>  # @INTERNAL
>  # @DESCRIPTION:
>  # Detect whether the current environment/build settings are such that we do not
>  # want to mess with any mounts.
>  mount-boot_is_disabled() {
>  	# Since this eclass only deals with /boot, skip things when ROOT is active.
> -	if [[ "${ROOT:-/}" != "/" ]] ; then
> +	if [[ ${ROOT:-/} != "/" ]] ; then

I suppose you can unquote RHS too since it doesn't contain any pattern
characters, if you are already touching quoting.

>  		return 0
>  	fi
>  
>  	# If we're only building a package, then there's no need to check things.
> -	if [[ "${MERGE_TYPE}" == "buildonly" ]] ; then
> +	if [[ ${MERGE_TYPE} == "buildonly" ]] ; then
>  		return 0
>  	fi
>  
>  	# The user wants us to leave things be.
> -	if [[ -n ${DONT_MOUNT_BOOT} ]] ; then
> +	if [[ -n ${I_KNOW_WHAT_I_AM_DOING} ]] ; then
>  		return 0
>  	fi
>  
>  	# OK, we want to handle things ourselves.
>  	return 1
>  }
>  
>  # @FUNCTION: mount-boot_check_status
>  # @INTERNAL
>  # @DESCRIPTION:
> -# Figure out what kind of work we need to do in order to have /boot be sane.
> -# Return values are:
> -# 0 - Do nothing at all!
> -# 1 - It's mounted, but is currently ro, so need to remount rw.
> -# 2 - It's not mounted, so need to mount it rw.
> +# Check if /boot is sane, i.e., mounted read/write if on a separate
> +# partition.  Return 0 if conditions are fulfilled, otherwise die.

I don't think there's a point in explicitly defining the return value
if there is no alternative.

>  mount-boot_check_status() {
>  	# Get out fast if possible.
>  	mount-boot_is_disabled && return 0
>  
>  	# note that /dev/BOOT is in the Gentoo default /etc/fstab file
> -	local fstabstate=$(awk '!/^#|^[[:blank:]]+#|^\/dev\/BOOT/ {print $2}' /etc/fstab | egrep "^/boot$" )
> -	local procstate=$(awk '$2 ~ /^\/boot$/ {print $2}' /proc/mounts)
> -	local proc_ro=$(awk '{ print $2 " ," $4 "," }' /proc/mounts | sed -n '/^\/boot .*,ro,/p')
> -
> -	if [ -n "${fstabstate}" ] && [ -n "${procstate}" ] ; then
> -		if [ -n "${proc_ro}" ] ; then
> -			echo
> -			einfo "Your boot partition, detected as being mounted at /boot, is read-only."
> -			einfo "It will be remounted in read-write mode temporarily."
> -			return 1
> -		else
> -			echo
> -			einfo "Your boot partition was detected as being mounted at /boot."
> -			einfo "Files will be installed there for ${PN} to function correctly."
> -			return 0
> -		fi
> -	elif [ -n "${fstabstate}" ] && [ -z "${procstate}" ] ; then
> -		echo
> -		einfo "Your boot partition was not mounted at /boot, so it will be automounted for you."
> -		einfo "Files will be installed there for ${PN} to function correctly."
> -		return 2
> -	else
> -		echo
> +	local fstabstate=$(awk '!/^[[:blank:]]*#|^\/dev\/BOOT/ && $2 == "/boot" \
> +		{print $2}' /etc/fstab)

The 'print' here is used as a boolean... why not use a boolean output
instead?

> +
> +	if [[ -z ${fstabstate} ]] ; then
>  		einfo "Assuming you do not have a separate /boot partition."
>  		return 0
>  	fi
> -}
>  
> -mount-boot_pkg_pretend() {
> -	# Get out fast if possible.
> -	mount-boot_is_disabled && return 0
> +	local procstate=$(awk '$2 == "/boot" \
> +		{print gensub(/^(.*,)?(ro|rw)(,.*)?$/, "\\2", 1, $4)}' /proc/mounts)

Shouldn't this use /proc/self/mounts?

>  
> -	elog "To avoid automounting and auto(un)installing with /boot,"
> -	elog "just export the DONT_MOUNT_BOOT variable."
> -	mount-boot_check_status
> +	if [[ -z ${procstate} ]] ; then
> +		eerror "Your boot partition is not mounted at /boot."
> +		eerror "Please mount it and retry."
> +		die	"/boot not mounted"
> +	fi
> +
> +	if [[ ${procstate} == "ro" ]] ; then
> +		eerror "Your boot partition, detected as being mounted at /boot," \
> +			"is read-only."
> +		eerror "Please remount it read/write and retry."
> +		die	"/boot mounted read-only"
> +	fi
> +
> +	einfo "Your boot partition was detected as being mounted at /boot."
> +	einfo "Files will be installed there for ${PN} to function correctly."
> +	return 0
>  }
>  
> -mount-boot_mount_boot_partition() {
> +mount-boot_pkg_pretend() {
>  	mount-boot_check_status
> -	case $? in
> -	0)	# Nothing to do.
> -		;;
> -	1)	# Remount it rw.
> -		mount -o remount,rw /boot
> -		if [ $? -ne 0 ] ; then
> -			echo
> -			eerror "Unable to remount in rw mode. Please do it manually!"
> -			die "Can't remount in rw mode. Please do it manually!"
> -		fi
> -		touch /boot/.e.remount
> -		;;
> -	2)	# Mount it rw.
> -		mount /boot -o rw
> -		if [ $? -ne 0 ] ; then
> -			echo
> -			eerror "Cannot automatically mount your /boot partition."
> -			eerror "Your boot partition has to be mounted rw before the installation"
> -			eerror "can continue. ${PN} needs to install important files there."
> -			die "Please mount your /boot partition manually!"
> -		fi
> -		touch /boot/.e.mount
> -		;;
> -	esac
>  }
>  
>  mount-boot_pkg_preinst() {
> -	# Handle older EAPIs.
> -	case ${EAPI:-0} in
> -	[0-3]) mount-boot_pkg_pretend ;;
> -	esac
> -
> -	mount-boot_mount_boot_partition
> +	mount-boot_check_status
>  }
>  
>  mount-boot_pkg_prerm() {
> -	touch "${ROOT}"/boot/.keep 2>/dev/null
> -	mount-boot_mount_boot_partition
> -	touch "${ROOT}"/boot/.keep 2>/dev/null
> -}
> -
> -mount-boot_umount_boot_partition() {
> -	# Get out fast if possible.
> -	mount-boot_is_disabled && return 0
> -
> -	if [ -e /boot/.e.remount ] ; then
> -		einfo "Automatically remounting /boot as ro as it was previously."
> -		rm -f /boot/.e.remount
> -		mount -o remount,ro /boot
> -	elif [ -e /boot/.e.mount ] ; then
> -		einfo "Automatically unmounting /boot as it was previously."
> -		rm -f /boot/.e.mount
> -		umount /boot
> +	mount-boot_check_status
> +	if ! ( shopt -s failglob; : "${ROOT}"/boot/.keep* ) 2>/dev/null ; then

EROOT?

> +		# Create a .keep file, in case it is shadowed at the mount point
> +		touch "${ROOT}"/boot/.keep 2>/dev/null
>  	fi
>  }
>  
> -mount-boot_pkg_postinst() {
> -	mount-boot_umount_boot_partition
> -}
> +# No-op phases for backwards compatibility
> +mount-boot_pkg_postinst() { :; }
>  
> -mount-boot_pkg_postrm() {
> -	mount-boot_umount_boot_partition
> -}
> +mount-boot_pkg_postrm() { :; }

-- 
Best regards,
Michał Górny


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