[PATCH v2 0/4] mbuto: Assorted fixes and simplifications

[PATCH v2 0/4] mbuto: Assorted fixes and simplifications

[David Gibson]

While debugging a problem Laurent Vivier was having with the passt
test suite, I discovered mbuto's archivemount support is entirely
broken: if archivemount is on the system, mbuto won't work.  While
fixing that, I discovered some slightly related simplfications that
can be made.  Here they all are.

Changes since v1:
 * Updated commit message in 2/4 to clarify why the desired advantages
   of using archivemount can't be used in practice
 * Improve accuracy of comment on compress_select in 3/4
 * Clarify removal of update existing archive functionality.

David Gibson (4):
  ${wd} is always set, no need to test for it
  Remove stale archivemount support
  Split "auto" compression mode into its own path
  Remove unnecessary cpio_init function

 mbuto | 128 +++++++++++++++-------------------------------------------
 1 file changed, 33 insertions(+), 95 deletions(-)

-- 
2.44.0

[PATCH v2 1/4] ${wd} is always set, no need to test for it

[David Gibson]

We unconditionally set ${wd} to a temporary directory, but there are some
places where we test if it is empty.  This appears to be stale code from
some earlier version, so simply remove it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 mbuto | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mbuto b/mbuto
index 0609e7e..a6f80ab 100755
--- a/mbuto
+++ b/mbuto
@@ -369,7 +369,7 @@ cmd_check() {
 # cleanup() - Remove left-overs on exit, used from trap
 cleanup() {
 	[ -n "${ARCHIVEMOUNT}" ] && "${UMOUNT}" "${wd}" 2>/dev/null
-	[ -n "${wd}" ] && "${RM}" -rf "${wd}"
+	"${RM}" -rf "${wd}"
 	[ -n "${pkg_tmp}" ] && "${RM}" -f "${pkg_tmp}"
 	[ -n "${compress_test1}" ] && "${RM}" -f "${compress_test1}"
 	[ -n "${compress_test2}" ] && "${RM}" -f "${compress_test2}"
@@ -1044,7 +1044,6 @@ build() {
 # add() - Add contents to existing image
 # $1:	Item to be added
 add() {
-	[ -z "${wd}" ] && cpio_init
 	kmod_init
 
 	[ -e "${1}" ] && __path="${1}" || __path=$(command -v "${1}") || :
-- 
@@ -369,7 +369,7 @@ cmd_check() {
 # cleanup() - Remove left-overs on exit, used from trap
 cleanup() {
 	[ -n "${ARCHIVEMOUNT}" ] && "${UMOUNT}" "${wd}" 2>/dev/null
-	[ -n "${wd}" ] && "${RM}" -rf "${wd}"
+	"${RM}" -rf "${wd}"
 	[ -n "${pkg_tmp}" ] && "${RM}" -f "${pkg_tmp}"
 	[ -n "${compress_test1}" ] && "${RM}" -f "${compress_test1}"
 	[ -n "${compress_test2}" ] && "${RM}" -f "${compress_test2}"
@@ -1044,7 +1044,6 @@ build() {
 # add() - Add contents to existing image
 # $1:	Item to be added
 add() {
-	[ -z "${wd}" ] && cpio_init
 	kmod_init
 
 	[ -e "${1}" ] && __path="${1}" || __path=$(command -v "${1}") || :
-- 
2.44.0

[PATCH v2 2/4] Remove stale archivemount support

[David Gibson]

mbuto has two ways of building the initramfs.  One is the typical approach
of staging its contents in a temporary directory, then building the
initramfs with cpio.  The other is to create an empty initramfs, mount
it with archivemount, and copy things into the mounted archive.

However, the archivemount approach is broken.  I'm not entirely sure why,
but it appears not to properly unmount the archive and retrieve the final.
filled version.  The upshot is that if archivemount is installed, then
mbuto generates an empty, gzip-compressed initramfs instead of whatever it
was supposed to.

It appears the archivemount approach was there to allow some later changes
to the archive without re-running mbuto.  However, it's essentially
impossible to use this safely - if archivemount still has the archive open
for writing, it's not safe to read it concurrently.  This appears to have
worked by accident with some earlier versions, but doesn't work now.

Just remove the archivemount approach entirely.

Reported-by: Laurent Vivier <lvivier@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 mbuto | 41 +++++------------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/mbuto b/mbuto
index a6f80ab..49d032c 100755
--- a/mbuto
+++ b/mbuto
@@ -368,7 +368,6 @@ cmd_check() {
 
 # cleanup() - Remove left-overs on exit, used from trap
 cleanup() {
-	[ -n "${ARCHIVEMOUNT}" ] && "${UMOUNT}" "${wd}" 2>/dev/null
 	"${RM}" -rf "${wd}"
 	[ -n "${pkg_tmp}" ] && "${RM}" -f "${pkg_tmp}"
 	[ -n "${compress_test1}" ] && "${RM}" -f "${compress_test1}"
@@ -564,16 +563,6 @@ cpio_init() {
 		else
 			OUT="$("${REALPATH}" "${OUT}")"
 		fi
-
-		if [ -n "${ARCHIVEMOUNT}" ]; then
-			: | "${CPIO}" --create -H newc --quiet | \
-				"${GZIP}" > "${OUT}"
-		fi
-	fi
-
-	if [ -n "${ARCHIVEMOUNT}" ]; then
-		"${ARCHIVEMOUNT}" "${OUT}" "${wd}"
-		info "Mounted CPIO archive ${OUT} at ${wd}"
 	fi
 }
 
@@ -1099,26 +1088,17 @@ cmds() {
 
 	[ "${NOSTRIP}" != "y" ] && strip_all
 
-	if [ -z "${ARCHIVEMOUNT}" ]; then
-		( __out="$("${REALPATH}" "${OUT}")"
-		  "${CD}" "${wd}"
-		  "${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
-		  cpio_compress "${__out}"
-		)
-	else
-		"${SYNC}"
-	fi
+	( __out="$("${REALPATH}" "${OUT}")"
+		"${CD}" "${wd}"
+		"${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
+		cpio_compress "${__out}"
+	)
 
 	stats
 
 	printf "%s" "${OUTPUT}"						| \
 		sed 's,__INITRD__,'"$("${REALPATH}" "${OUT}")"',g'	| \
 		sed 's,__KERNEL__,/boot/vmlinuz-'"${KERNEL}"',g'
-
-	if [ -n "${ARCHIVEMOUNT}" ]; then
-		trap - EXIT
-		notice "initramfs mounted at: ${wd}"
-	fi
 }
 
 # usage() - Print usage and exit
@@ -1271,17 +1251,6 @@ else
 	eval "profile_${PROFILE}" || err "profile ${PROFILE} not found"
 fi
 
-# Check if we can keep the CPIO mounted for convenience as we exit. This isn't
-# safe with fakeroot, as contents can't be touched before the environment
-# save-file is loaded again, so it needs root (and archivemount).
-if [ "${LD_PRELOAD}" = "libfakeroot-sysv.so" ]; then
-	if command -v archivemount >/dev/null 2>&1; then
-		notice "Not running as root, won't keep cpio mounted"
-	fi
-elif ! ARCHIVEMOUNT="$(command -v archivemount)"; then
-	warn "archivemount not available, won't keep cpio mounted"
-fi
-
 trap cleanup EXIT
 
 cmds "$@"
-- 
@@ -368,7 +368,6 @@ cmd_check() {
 
 # cleanup() - Remove left-overs on exit, used from trap
 cleanup() {
-	[ -n "${ARCHIVEMOUNT}" ] && "${UMOUNT}" "${wd}" 2>/dev/null
 	"${RM}" -rf "${wd}"
 	[ -n "${pkg_tmp}" ] && "${RM}" -f "${pkg_tmp}"
 	[ -n "${compress_test1}" ] && "${RM}" -f "${compress_test1}"
@@ -564,16 +563,6 @@ cpio_init() {
 		else
 			OUT="$("${REALPATH}" "${OUT}")"
 		fi
-
-		if [ -n "${ARCHIVEMOUNT}" ]; then
-			: | "${CPIO}" --create -H newc --quiet | \
-				"${GZIP}" > "${OUT}"
-		fi
-	fi
-
-	if [ -n "${ARCHIVEMOUNT}" ]; then
-		"${ARCHIVEMOUNT}" "${OUT}" "${wd}"
-		info "Mounted CPIO archive ${OUT} at ${wd}"
 	fi
 }
 
@@ -1099,26 +1088,17 @@ cmds() {
 
 	[ "${NOSTRIP}" != "y" ] && strip_all
 
-	if [ -z "${ARCHIVEMOUNT}" ]; then
-		( __out="$("${REALPATH}" "${OUT}")"
-		  "${CD}" "${wd}"
-		  "${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
-		  cpio_compress "${__out}"
-		)
-	else
-		"${SYNC}"
-	fi
+	( __out="$("${REALPATH}" "${OUT}")"
+		"${CD}" "${wd}"
+		"${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
+		cpio_compress "${__out}"
+	)
 
 	stats
 
 	printf "%s" "${OUTPUT}"						| \
 		sed 's,__INITRD__,'"$("${REALPATH}" "${OUT}")"',g'	| \
 		sed 's,__KERNEL__,/boot/vmlinuz-'"${KERNEL}"',g'
-
-	if [ -n "${ARCHIVEMOUNT}" ]; then
-		trap - EXIT
-		notice "initramfs mounted at: ${wd}"
-	fi
 }
 
 # usage() - Print usage and exit
@@ -1271,17 +1251,6 @@ else
 	eval "profile_${PROFILE}" || err "profile ${PROFILE} not found"
 fi
 
-# Check if we can keep the CPIO mounted for convenience as we exit. This isn't
-# safe with fakeroot, as contents can't be touched before the environment
-# save-file is loaded again, so it needs root (and archivemount).
-if [ "${LD_PRELOAD}" = "libfakeroot-sysv.so" ]; then
-	if command -v archivemount >/dev/null 2>&1; then
-		notice "Not running as root, won't keep cpio mounted"
-	fi
-elif ! ARCHIVEMOUNT="$(command -v archivemount)"; then
-	warn "archivemount not available, won't keep cpio mounted"
-fi
-
 trap cleanup EXIT
 
 cmds "$@"
-- 
2.44.0

[PATCH v2 3/4] Split "auto" compression mode into its own path

[David Gibson]

mbuto supports "auto" compression mode where we detect the fastest to
decompress and use it.  This is structured a bit oddly - cpio_compress()
first handles the case of an explicitly selected compressor, then handles
the auto-detected case, redundantly implementing the compression once it
has picked one.

Make this a bit clearer: first handle the "auto" case by calling out to
the testing code, and using that to set the parameter for the specific
compression path.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 mbuto | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/mbuto b/mbuto
index 49d032c..eb559fb 100755
--- a/mbuto
+++ b/mbuto
@@ -566,31 +566,11 @@ cpio_init() {
 	fi
 }
 
-# cpio_compress() - Compress archive, test available methods if none is selected
+# compress_select() - Try compressors and pick the fastest to decompress image
 # $1:	Existing CPIO archive
-cpio_compress() {
-	{ [ -z "${COMPRESS}" ] || [ "${COMPRESS}" = "none" ]; } && return
-
-	info "Compressing CPIO archive ${1}"
-
-	if [ "${COMPRESS}" != "auto" ]; then
-		[ "${COMPRESS}" = "lzo" ] && __cmd="lzop" || __cmd="${COMPRESS}"
-
-		cmd_check "${__cmd}"
-		if [ "${__cmd}" = "lz4" ]; then
-			"${__cmd}" -l -f -q -9 "${1}" "${1}.lz4"
-		else
-			"${__cmd}" -f -q -9 -S .lz4 "${1}"
-		fi
-
-		mv "${1}.lz4" "${1}"
-
-		return
-	fi
-
+compress_select() {
 	if [ ! -f "/boot/config-${KERNEL}" ]; then
-		"${GZIP}" -9 "${1}"
-		"${MV}" "${1}.gz" "${1}"
+		echo "gzip"
 		return
 	fi
 
@@ -639,14 +619,29 @@ cpio_compress() {
 		       "${__a}:" "${__size} bytes" "${__time}s"
 	done
 
-	[ "${__pick}" = "lzo" ] && __cmd="lzop" || __cmd="${__pick}"
-	[ "${__cmd}" = "lz4" ] && __opt="-l" || __opt=""
-
-	"${__cmd}" ${__opt} -q -9 -c "${1}" > "${compress_test1}"
 	notice "Picked ${__pick} compression for CPIO"
+	rm ${compress_test1} ${compress_test2}
+	echo "${__pick}"
+}
+
+# cpio_compress() - Compress archive, test available methods if none is selected
+# $1:	Existing CPIO archive
+cpio_compress() {
+	{ [ -z "${COMPRESS}" ] || [ "${COMPRESS}" = "none" ]; } && return
+	[ "${COMPRESS}" = "auto" ] && COMPRESS=$(compress_select "$1")
+
+	info "Compressing CPIO archive ${1}"
+
+	[ "${COMPRESS}" = "lzo" ] && __cmd="lzop" || __cmd="${COMPRESS}"
+
+	cmd_check "${__cmd}"
+	if [ "${__cmd}" = "lz4" ]; then
+		"${__cmd}" -l -f -q -9 "${1}" "${1}.lz4"
+	else
+		"${__cmd}" -f -q -9 -S .lz4 "${1}"
+	fi
 
-	mv "${compress_test1}" "${OUT}"
-	rm "${compress_test2}"
+	mv "${1}.lz4" "${1}"
 }
 
 ################################################################################
-- 
@@ -566,31 +566,11 @@ cpio_init() {
 	fi
 }
 
-# cpio_compress() - Compress archive, test available methods if none is selected
+# compress_select() - Try compressors and pick the fastest to decompress image
 # $1:	Existing CPIO archive
-cpio_compress() {
-	{ [ -z "${COMPRESS}" ] || [ "${COMPRESS}" = "none" ]; } && return
-
-	info "Compressing CPIO archive ${1}"
-
-	if [ "${COMPRESS}" != "auto" ]; then
-		[ "${COMPRESS}" = "lzo" ] && __cmd="lzop" || __cmd="${COMPRESS}"
-
-		cmd_check "${__cmd}"
-		if [ "${__cmd}" = "lz4" ]; then
-			"${__cmd}" -l -f -q -9 "${1}" "${1}.lz4"
-		else
-			"${__cmd}" -f -q -9 -S .lz4 "${1}"
-		fi
-
-		mv "${1}.lz4" "${1}"
-
-		return
-	fi
-
+compress_select() {
 	if [ ! -f "/boot/config-${KERNEL}" ]; then
-		"${GZIP}" -9 "${1}"
-		"${MV}" "${1}.gz" "${1}"
+		echo "gzip"
 		return
 	fi
 
@@ -639,14 +619,29 @@ cpio_compress() {
 		       "${__a}:" "${__size} bytes" "${__time}s"
 	done
 
-	[ "${__pick}" = "lzo" ] && __cmd="lzop" || __cmd="${__pick}"
-	[ "${__cmd}" = "lz4" ] && __opt="-l" || __opt=""
-
-	"${__cmd}" ${__opt} -q -9 -c "${1}" > "${compress_test1}"
 	notice "Picked ${__pick} compression for CPIO"
+	rm ${compress_test1} ${compress_test2}
+	echo "${__pick}"
+}
+
+# cpio_compress() - Compress archive, test available methods if none is selected
+# $1:	Existing CPIO archive
+cpio_compress() {
+	{ [ -z "${COMPRESS}" ] || [ "${COMPRESS}" = "none" ]; } && return
+	[ "${COMPRESS}" = "auto" ] && COMPRESS=$(compress_select "$1")
+
+	info "Compressing CPIO archive ${1}"
+
+	[ "${COMPRESS}" = "lzo" ] && __cmd="lzop" || __cmd="${COMPRESS}"
+
+	cmd_check "${__cmd}"
+	if [ "${__cmd}" = "lz4" ]; then
+		"${__cmd}" -l -f -q -9 "${1}" "${1}.lz4"
+	else
+		"${__cmd}" -f -q -9 -S .lz4 "${1}"
+	fi
 
-	mv "${compress_test1}" "${OUT}"
-	rm "${compress_test2}"
+	mv "${1}.lz4" "${1}"
 }
 
 ################################################################################
-- 
2.44.0

[PATCH v2 4/4] Remove unnecessary cpio_init function

[David Gibson]

The cpio_init function is now only every called with the "discard" option.
But, moreover, what it does is create an initial mostly empty archive which
will just get overwritten by the final archive.

So, it's entirely unnecessary except for one subtlety.  Our use of realpath
when generating the final output requires that a file already exist in the
output location.  We can fix that by shuffling some things out of a
subshell, removing the need for realpath.

This removes functionality to modify an existing initramfs image.  However
that was already broken in a several ways.  Just remove it, and update the
help messages to match.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 mbuto | 39 +++++++--------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/mbuto b/mbuto
index eb559fb..247b63d 100755
--- a/mbuto
+++ b/mbuto
@@ -544,28 +544,6 @@ subopts_get() {
 
 ### CPIO #######################################################################
 
-# cpio_init() - Source existing CPIO archive, or create if needed
-# $1:	Path to CPIO archive, might exist, might be discarded if existing
-cpio_init() {
-	if [ -f "${OUT}" ] && [ "${1}" != "discard" ]; then
-		info "Sourcing CPIO archive from ${OUT}"
-
-		if ! "${GZIP}" -dfc "${OUT}" |
-		   "${CPIO}" --quiet -iD "${wd}"; then
-			err "Invalid CPIO archive ${OUT}"
-		fi
-	else
-		info "Creating new CPIO archive"
-
-		if [ -z "${OUT}" ]; then
-			OUT="$("${MKTEMP}")"
-			notice "Creating image: ${OUT}"
-		else
-			OUT="$("${REALPATH}" "${OUT}")"
-		fi
-	fi
-}
-
 # compress_select() - Try compressors and pick the fastest to decompress image
 # $1:	Existing CPIO archive
 compress_select() {
@@ -987,7 +965,6 @@ pkg_add() {
 
 # build() - Build a new image, sourcing contents
 build() {
-	cpio_init discard
 	kmod_init
 
 	for __d in ${DIRS}; do
@@ -1083,11 +1060,11 @@ cmds() {
 
 	[ "${NOSTRIP}" != "y" ] && strip_all
 
-	( __out="$("${REALPATH}" "${OUT}")"
+	(
 		"${CD}" "${wd}"
-		"${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
-		cpio_compress "${__out}"
-	)
+		"${FIND}" . | "${CPIO}" --create -H newc --quiet
+	) > "${OUT}"
+	cpio_compress "${OUT}"
 
 	stats
 
@@ -1139,9 +1116,9 @@ usage() {
 	echo "	-v: verbose"
 	echo "	-h: show this help"
 	echo
-	echo "Build initramfs image unless an existing one is passed."
-	echo "Additional programs, kernel modules, device nodes, generic files"
-	echo "can be passed as a list of ADD_ON parameters."
+	echo "Build initramfs image.  Additional programs, kernel modules,"
+	echo "device nodes, generic files can be passed as a list of ADD_ON"
+	echo "parameters."
 	echo
 	echo "Distribution packages (deb and RPM currently supported) can be"
 	echo "added too, but they will simply be extracted, not installed."
@@ -1173,8 +1150,6 @@ usage() {
 	echo "		Build a base image as a temporary file"
 	echo "	${0} grep"
 	echo "		Build a new image including grep and needed libraries"
-	echo "	${0} -f kata.img zsh_5.6.2-3_amd64.deb"
-	echo "		Install zsh package to pre-existing kata.img"
 	echo "	${0} -v -f kata.img -p kata_debug -c lz4"
 	echo "		Use lz4 compression, run a shell before proceeding"
 	exit 1
-- 
@@ -544,28 +544,6 @@ subopts_get() {
 
 ### CPIO #######################################################################
 
-# cpio_init() - Source existing CPIO archive, or create if needed
-# $1:	Path to CPIO archive, might exist, might be discarded if existing
-cpio_init() {
-	if [ -f "${OUT}" ] && [ "${1}" != "discard" ]; then
-		info "Sourcing CPIO archive from ${OUT}"
-
-		if ! "${GZIP}" -dfc "${OUT}" |
-		   "${CPIO}" --quiet -iD "${wd}"; then
-			err "Invalid CPIO archive ${OUT}"
-		fi
-	else
-		info "Creating new CPIO archive"
-
-		if [ -z "${OUT}" ]; then
-			OUT="$("${MKTEMP}")"
-			notice "Creating image: ${OUT}"
-		else
-			OUT="$("${REALPATH}" "${OUT}")"
-		fi
-	fi
-}
-
 # compress_select() - Try compressors and pick the fastest to decompress image
 # $1:	Existing CPIO archive
 compress_select() {
@@ -987,7 +965,6 @@ pkg_add() {
 
 # build() - Build a new image, sourcing contents
 build() {
-	cpio_init discard
 	kmod_init
 
 	for __d in ${DIRS}; do
@@ -1083,11 +1060,11 @@ cmds() {
 
 	[ "${NOSTRIP}" != "y" ] && strip_all
 
-	( __out="$("${REALPATH}" "${OUT}")"
+	(
 		"${CD}" "${wd}"
-		"${FIND}" . | "${CPIO}" --create -H newc --quiet > "${OUT}"
-		cpio_compress "${__out}"
-	)
+		"${FIND}" . | "${CPIO}" --create -H newc --quiet
+	) > "${OUT}"
+	cpio_compress "${OUT}"
 
 	stats
 
@@ -1139,9 +1116,9 @@ usage() {
 	echo "	-v: verbose"
 	echo "	-h: show this help"
 	echo
-	echo "Build initramfs image unless an existing one is passed."
-	echo "Additional programs, kernel modules, device nodes, generic files"
-	echo "can be passed as a list of ADD_ON parameters."
+	echo "Build initramfs image.  Additional programs, kernel modules,"
+	echo "device nodes, generic files can be passed as a list of ADD_ON"
+	echo "parameters."
 	echo
 	echo "Distribution packages (deb and RPM currently supported) can be"
 	echo "added too, but they will simply be extracted, not installed."
@@ -1173,8 +1150,6 @@ usage() {
 	echo "		Build a base image as a temporary file"
 	echo "	${0} grep"
 	echo "		Build a new image including grep and needed libraries"
-	echo "	${0} -f kata.img zsh_5.6.2-3_amd64.deb"
-	echo "		Install zsh package to pre-existing kata.img"
 	echo "	${0} -v -f kata.img -p kata_debug -c lz4"
 	echo "		Use lz4 compression, run a shell before proceeding"
 	exit 1
-- 
2.44.0

Re: [PATCH v2 0/4] mbuto: Assorted fixes and simplifications

[Stefano Brivio]

On Fri, 26 Apr 2024 12:01:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> While debugging a problem Laurent Vivier was having with the passt
> test suite, I discovered mbuto's archivemount support is entirely
> broken: if archivemount is on the system, mbuto won't work.  While
> fixing that, I discovered some slightly related simplfications that
> can be made.  Here they all are.
> 
> Changes since v1:
>  * Updated commit message in 2/4 to clarify why the desired advantages
>    of using archivemount can't be used in practice
>  * Improve accuracy of comment on compress_select in 3/4
>  * Clarify removal of update existing archive functionality.
> 
> David Gibson (4):
>   ${wd} is always set, no need to test for it
>   Remove stale archivemount support
>   Split "auto" compression mode into its own path
>   Remove unnecessary cpio_init function

Applied, thanks.

-- 
Stefano