[PATCH 00/12] Grub-shell improvements

[PATCH 00/12] Grub-shell improvements

[Glenn Washburn]

I've extracted this patch series from the GitLab CI patch series, which some
minor modifications. Mostly these are improvements to grub-shell to facilitate
debugging.

The only patch that might be considered a fix, as opposed to an improvement,
would be patch #11, which fixes the issue where qemu-mips is given a
non-existant machine type. And while this was discussed here on the list as a
possible solution, I couldn't get the mips tests working, so I'm not sure it's
the right fix.

Glenn

Glenn Washburn (12):
  grub-shell: Allow specifying non-default trim line contents
  grub-shell: Trim line should always be matched from the beginning of
    the line
  grub-shell: Only show grub-mkrescue output if it returns an error
  grub-shell: Allow setting default timeout via
    GRUB_SHELL_DEFAULT_TIMEOUT envvar
  grub-shell: Put all generated files into working dir and use better
    file names
  grub-shell: Add grub output logfile with grub-shell --debug
  grub-shell: Set exit status to qemu exit status
  tests: Allow turning on shell tracing from environment variables
  grub-shell: Add --verbose to mkrescue when $debug is greater than 2
  grub-shell: Only turn on qemu head when large debug value is specified
  grub-shell: Use malta qemu-mips machine type instead off non-existant
    indy
  grub-shell: Update qemu UEFI firmware names to be more generic

 tests/util/grub-fs-tester.in |  2 +
 tests/util/grub-shell.in     | 89 ++++++++++++++++++++++++------------
 2 files changed, 63 insertions(+), 28 deletions(-)

-- 
2.27.0

[PATCH 01/12] grub-shell: Allow specifying non-default trim line contents

[Glenn Washburn]

This will be useful for tests that have unwanted output from setup. This is
not documented because its only intended to be internal at the moment. Also,
--no-trim is allowed to explicitly turn off trim.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 93e9f5148..b409962f1 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -32,6 +32,7 @@ PATH="${builddir}:$PATH"
 export PATH
 
 trim=0
+trim_head=664cbea8-132f-4770-8aa4-1696d59ac35c
 
 # Usage: usage
 # Print the usage.
@@ -212,8 +213,13 @@ for option in "$@"; do
 	echo "$0 (GNU GRUB ${PACKAGE_VERSION})"
 	exit 0 ;;
     --trim)
-	trim=1
+	trim=1 ;;
+    --trim=*)
+	trim=2
+	trim_head=`echo "$option" | sed -e 's/--trim=//' -e 's/,/ /g'`
 	;;
+    --no-trim)
+	trim=0 ;;
     --debug)
         debug=1 ;;
     --modules=*)
@@ -336,8 +342,6 @@ terminal_input ${term}
 terminal_output ${term}
 EOF
 
-trim_head=664cbea8-132f-4770-8aa4-1696d59ac35c
-
 if [ $trim = 1 ]; then
     echo "echo $trim_head" >>${cfgfile}
 fi
@@ -452,8 +456,8 @@ fi
 
 do_trim ()
 {
-    if [ $trim = 1 ]; then
-	awk '{ if (have_head == 1) print $0; } /664cbea8-132f-4770-8aa4-1696d59ac35c/ { have_head=1; }'
+    if [ $trim = 1 ] || [ $trim = 2 ]; then
+	awk '{ if (have_head == 1) print $0; } /'"$trim_head"'/ { have_head=1; }'
     else
 	cat
     fi
-- 
2.27.0

[PATCH 02/12] grub-shell: Trim line should always be matched from the beginning of the line

[Glenn Washburn]

When turning on shell tracing the trim line will be output before we
actually want to start the trim. However, in this case the trim line never
starts from the beginning of the line. So start trimming from the correct
line by matching from the beginning of the line.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index b409962f1..602b16f3e 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -343,7 +343,7 @@ terminal_output ${term}
 EOF
 
 if [ $trim = 1 ]; then
-    echo "echo $trim_head" >>${cfgfile}
+    echo "echo; echo $trim_head" >>${cfgfile}
 fi
 
 rom_directory=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
@@ -457,7 +457,7 @@ fi
 do_trim ()
 {
     if [ $trim = 1 ] || [ $trim = 2 ]; then
-	awk '{ if (have_head == 1) print $0; } /'"$trim_head"'/ { have_head=1; }'
+	awk '{ if (have_head == 1) print $0; } /^'"$trim_head"'/ { have_head=1; }'
     else
 	cat
     fi
-- 
2.27.0

[PATCH 03/12] grub-shell: Only show grub-mkrescue output if it returns an error

[Glenn Washburn]

The previous behavior ignored an error and the output from grub-mkrescue.
This made it a pain to discover that grub-mkrescue was the reason that tests
which rely on grub-shell were failing.  Even after discovering grub-mkrescue
was the culprit, there was no output to indicate why it was failing.  It
turns out that grub-mkrescue is a thin wrapper around xorriso. So if you do
not have xorriso installed it will fail with an error message about not
being able to find xorriso.

This change will allow grub-mkrescue output to be written to stderr, only if
grub-mkrescue fails.  If grub-mkrescue succeeds, there will be no output
from grub-mkrescue so as not to interfere with the functioning of tests.
This change should have no effect on the running of tests or other uses of
grub-shell as it only modifies the error path.

Also, if grub-mkrescue fails, the script exits early.  Since grub-shell
needs the iso image created by grub-mkresue to boot the qemu instance, a
failure here should be considered fatal.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 602b16f3e..9d8c417da 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -60,6 +60,17 @@ Report bugs to <bug-grub@gnu.org>.
 EOF
 }
 
+# Exec given argv and only show its output on STDERR if it returns an
+# error status.
+exec_show_error () {
+    v=`$@ 2>&1`
+    ret=$?
+    if [ "$ret" != 0 ]; then
+        echo "$v" >&2
+        exit $ret
+    fi
+}
+
 . "${builddir}/grub-core/modinfo.sh"
 qemuopts="${GRUB_QEMU_OPTS}"
 serial_port=com0
@@ -383,13 +394,15 @@ if test -z "$debug"; then
 fi
 
 if [ x$boot != xnet ] && [ x$boot != xemu ]; then
-    pkgdatadir="@builddir@" "@builddir@/grub-mkrescue" "--output=${isofile}" "--override-directory=${builddir}/grub-core" \
+    pkgdatadir="@builddir@" \
+    exec_show_error "@builddir@/grub-mkrescue" "--output=${isofile}" \
+	"--override-directory=${builddir}/grub-core" \
 	--rom-directory="${rom_directory}" \
 	--locale-directory="@srcdir@/po" \
 	--themes-directory="@srcdir@/themes" \
 	$mkimage_extra_arg ${mkrescue_args} \
 	"/boot/grub/grub.cfg=${cfgfile}" "/boot/grub/testcase.cfg=${source}" \
-	${files} >/dev/null 2>&1
+	${files} || exit $?
 fi
 if [ x$boot = xhd ]; then
     if [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = arm64-efi ] || [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = arm-efi ]; then
-- 
2.27.0

[PATCH 04/12] grub-shell: Allow setting default timeout via GRUB_SHELL_DEFAULT_TIMEOUT envvar

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 9d8c417da..e80471126 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -211,7 +211,7 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
 	console=console;;
 esac
 
-timeout=60
+timeout=${GRUB_SHELL_DEFAULT_TIMEOUT:-60}
 mkimage_extra_arg=
 
 # Check the arguments.
-- 
2.27.0

[PATCH 05/12] grub-shell: Put all generated files into working dir and use better file names

[Glenn Washburn]

When running tests there are many invocations of grub-shell, and because the
output files are all random names in the same tmp directory, it becomes more
work to figure out which files went with which grub-shell invocations. So
all generated files from one invocation of grub-shell are put into a
randomly named directory, so as not to collide with other grub-shell
invocations. And now that the generated files can be put in a location where
they will not get stepped on, and they can be named sensible names.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index e80471126..8c6ed76d7 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -71,6 +71,8 @@ exec_show_error () {
     fi
 }
 
+work_directory=${WORKDIR:-`mktemp -d "${TMPDIR:-/tmp}/grub-shell.XXXXXXXXXX"`} || exit 1
+
 . "${builddir}/grub-core/modinfo.sh"
 qemuopts="${GRUB_QEMU_OPTS}"
 serial_port=com0
@@ -80,7 +82,7 @@ pseries=n
 disk="hda "
 case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
     *-emu)
-	device_map=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+	device_map="$work_directory/device.map"
 	boot=emu
 	console=console
 	disk=0
@@ -318,14 +320,14 @@ for option in "$@"; do
 done
 
 if [ "x${source}" = x ] ; then
-    tmpfile=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+    tmpfile="$work_directory/testcase.cfg"
     while read REPLY; do
 	echo "$REPLY" >> ${tmpfile}
     done
     source=${tmpfile}
 fi
 
-cfgfile=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+cfgfile="$work_directory/grub.cfg"
 cat <<EOF >${cfgfile}
 grubshell=yes
 enable_progress_indicator=0
@@ -357,7 +359,8 @@ if [ $trim = 1 ]; then
     echo "echo; echo $trim_head" >>${cfgfile}
 fi
 
-rom_directory=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+rom_directory="$work_directory/rom_directory"
+mkdir -p "$rom_directory"
 
 for mod in ${modules}
 do
@@ -378,7 +381,7 @@ test -z "$debug" || echo "GRUB script: ${cfgfile}" >&2
 test -z "$debug" || echo "GRUB testcase script: ${tmpfile}" >&2
 test -z "$debug" || echo "Boot device: ${boot}" >&2
 
-isofile=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+isofile="$work_directory/grub.iso"
 test -z "$debug" || echo "GRUB ISO file: ${isofile}" >&2
 test -z "$debug" || echo "GRUB ROM directory: ${rom_directory}" >&2
 
@@ -450,7 +453,7 @@ if [ x$boot = xmips_qemu ]; then
 fi
 
 if [ x$boot = xcoreboot ]; then
-    imgfile=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+    imgfile="$work_directory/coreboot.img"
     cp "${GRUB_COREBOOT_ROM}" "${imgfile}"
     "${GRUB_CBFSTOOL}" "${imgfile}" add-payload -f "${rom_directory}/coreboot.elf" -n fallback/payload
     bootdev="-bios ${imgfile}"
@@ -493,14 +496,15 @@ copy_extra_files() {
 }
 
 if [ x$boot = xnet ]; then
-    netdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
+    netdir="$work_directory/netdir"
+    mkdir -p "$netdir"
     pkgdatadir="@builddir@" "@builddir@/grub-mknetdir" "--grub-mkimage=${builddir}/grub-mkimage" "--directory=${builddir}/grub-core" "--net-directory=$netdir" ${mkrescue_args} > /dev/null
     cp "${cfgfile}" "$netdir/boot/grub/grub.cfg"
     cp "${source}" "$netdir/boot/grub/testcase.cfg"
     [ -z "$files" ] || copy_extra_files "$netdir" $files
     timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -boot n -net "user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"  -net nic  | cat | tr -d "\r" | do_trim
 elif [ x$boot = xemu ]; then
-    rootdir="$(mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX")"
+    rootdir="$work_directory/rootdir"
     grubdir="$rootdir/boot/grub"
     mkdir -p "$grubdir/fonts"
     mkdir -p "$grubdir/themes"
@@ -515,7 +519,7 @@ elif [ x$boot = xemu ]; then
     cp "${cfgfile}" "$grubdir/grub.cfg"
     cp "${source}" "$grubdir/testcase.cfg"
     [ -z "$files" ] || copy_extra_files "$rootdir" $files
-    roottar="$(mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX")"
+    roottar="$work_directory/root.tar"
     (cd "$rootdir"; tar cf "$roottar" .)
     @builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r memdisk -d "/boot/grub" | tr -d "\r" | do_trim
     test -n "$debug" || rm -rf "$rootdir"
-- 
2.27.0

[PATCH 06/12] grub-shell: Add grub output logfile with grub-shell --debug

[Glenn Washburn]

This allows seeing full qemu output of grub-shell, which can be invaluable
when debugging failing tests.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 8c6ed76d7..c6d7860e9 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -379,6 +379,9 @@ echo "${halt_cmd}" >>${cfgfile}
 
 test -z "$debug" || echo "GRUB script: ${cfgfile}" >&2
 test -z "$debug" || echo "GRUB testcase script: ${tmpfile}" >&2
+
+goutfile="$work_directory/grub-qemu.log"
+test -z "$debug" || echo "GRUB output log: ${goutfile}" >&2
 test -z "$debug" || echo "Boot device: ${boot}" >&2
 
 isofile="$work_directory/grub.iso"
@@ -502,7 +505,7 @@ if [ x$boot = xnet ]; then
     cp "${cfgfile}" "$netdir/boot/grub/grub.cfg"
     cp "${source}" "$netdir/boot/grub/testcase.cfg"
     [ -z "$files" ] || copy_extra_files "$netdir" $files
-    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -boot n -net "user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"  -net nic  | cat | tr -d "\r" | do_trim
+    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -boot n -net "user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"  -net nic  | cat | tr -d "\r" | tee "${goutfile}" | do_trim
 elif [ x$boot = xemu ]; then
     rootdir="$work_directory/rootdir"
     grubdir="$rootdir/boot/grub"
@@ -521,11 +524,11 @@ elif [ x$boot = xemu ]; then
     [ -z "$files" ] || copy_extra_files "$rootdir" $files
     roottar="$work_directory/root.tar"
     (cd "$rootdir"; tar cf "$roottar" .)
-    @builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r memdisk -d "/boot/grub" | tr -d "\r" | do_trim
+    @builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r memdisk -d "/boot/grub" | tr -d "\r" | tee "${goutfile}" | do_trim
     test -n "$debug" || rm -rf "$rootdir"
     test -n "$debug" || rm -f "$roottar"
 else
-    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -${device}"${isofile}" ${bootdev} | cat | tr -d "\r" | do_trim
+    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -${device}"${isofile}" ${bootdev} | cat | tr -d "\r" | tee "${goutfile}" | do_trim
 fi
 if [ x$boot = xcoreboot ]; then
     test -n "$debug" || rm -f "${imgfile}"
-- 
2.27.0

[PATCH 07/12] grub-shell: Set exit status to qemu exit status

[Glenn Washburn]

This allows us to test if unexpected output in test scripts is because of a
bug in grub, because there was an error in qemu, or qemu was killed due to a
timeout.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index c6d7860e9..40c62b9fd 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -498,6 +498,9 @@ copy_extra_files() {
     done
 }
 
+ret=0
+mkfifo "$work_directory/qemu-pipe"
+cat < "$work_directory/qemu-pipe" | tr -d "\r" | tee "${goutfile}" | do_trim &
 if [ x$boot = xnet ]; then
     netdir="$work_directory/netdir"
     mkdir -p "$netdir"
@@ -505,7 +508,7 @@ if [ x$boot = xnet ]; then
     cp "${cfgfile}" "$netdir/boot/grub/grub.cfg"
     cp "${source}" "$netdir/boot/grub/testcase.cfg"
     [ -z "$files" ] || copy_extra_files "$netdir" $files
-    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -boot n -net "user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"  -net nic  | cat | tr -d "\r" | tee "${goutfile}" | do_trim
+    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -boot n -net "user,tftp=$netdir,bootfile=/boot/grub/${grub_modinfo_target_cpu}-${grub_modinfo_platform}/core.$netbootext"  -net nic > "$work_directory/qemu-pipe" || ret=$?
 elif [ x$boot = xemu ]; then
     rootdir="$work_directory/rootdir"
     grubdir="$rootdir/boot/grub"
@@ -524,11 +527,11 @@ elif [ x$boot = xemu ]; then
     [ -z "$files" ] || copy_extra_files "$rootdir" $files
     roottar="$work_directory/root.tar"
     (cd "$rootdir"; tar cf "$roottar" .)
-    @builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r memdisk -d "/boot/grub" | tr -d "\r" | tee "${goutfile}" | do_trim
+    @builddir@/grub-core/grub-emu -m "$device_map" --memdisk "$roottar" -r memdisk -d "/boot/grub" > "$work_directory/qemu-pipe" || ret=$?
     test -n "$debug" || rm -rf "$rootdir"
     test -n "$debug" || rm -f "$roottar"
 else
-    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -${device}"${isofile}" ${bootdev} | cat | tr -d "\r" | tee "${goutfile}" | do_trim
+    timeout -s KILL $timeout "${qemu}" ${qemuopts} ${serial_null} -serial file:/dev/stdout -${device}"${isofile}" ${bootdev} > "$work_directory/qemu-pipe" || ret=$?
 fi
 if [ x$boot = xcoreboot ]; then
     test -n "$debug" || rm -f "${imgfile}"
@@ -536,6 +539,7 @@ fi
 test -n "$debug" || rm -f "${isofile}"
 test -n "$debug" || rm -rf "${rom_directory}"
 test -n "$debug" || rm -f "${tmpfile}" "${cfgfile}"
-exit 0
+rm -f "$work_directory/qemu-pipe"
+exit $ret
 
 
-- 
2.27.0

[PATCH 08/12] tests: Allow turning on shell tracing from environment variables

[Glenn Washburn]

This allows turning on shell tracing when its not practical or not possible
to use commandline arguments. Turn on tracing when the envvar is an integer
greater than 1, since these can log a lot of messages.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-fs-tester.in | 2 ++
 tests/util/grub-shell.in     | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index bfc425e1f..adeffbfba 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -2,6 +2,8 @@
 
 set -e
 
+[ "${GRUB_TEST_DEFAULT_DEBUG:-0}" -gt 1 ] && set -x
+
 fs="$1"
 
 GRUBFSTEST="@builddir@/grub-fstest"
diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 40c62b9fd..198b03057 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -215,6 +215,7 @@ esac
 
 timeout=${GRUB_SHELL_DEFAULT_TIMEOUT:-60}
 mkimage_extra_arg=
+debug=${GRUB_SHELL_DEFAULT_DEBUG:-$GRUB_TEST_DEFAULT_DEBUG}
 
 # Check the arguments.
 for option in "$@"; do
@@ -234,7 +235,7 @@ for option in "$@"; do
     --no-trim)
 	trim=0 ;;
     --debug)
-        debug=1 ;;
+        debug=$((debug+1)) ;;
     --modules=*)
 	ms=`echo "$option" | sed -e 's/--modules=//' -e 's/,/ /g'`
 	modules="$modules $ms" ;;
@@ -319,6 +320,8 @@ for option in "$@"; do
     esac
 done
 
+[ "${debug:-0}" -gt 1 ] && set -x
+
 if [ "x${source}" = x ] ; then
     tmpfile="$work_directory/testcase.cfg"
     while read REPLY; do
-- 
2.27.0

[PATCH 09/12] grub-shell: Add --verbose to mkrescue when $debug is greater than 2

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 198b03057..854508750 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -404,7 +404,9 @@ fi
 
 if [ x$boot != xnet ] && [ x$boot != xemu ]; then
     pkgdatadir="@builddir@" \
-    exec_show_error "@builddir@/grub-mkrescue" "--output=${isofile}" \
+    exec_show_error "@builddir@/grub-mkrescue" \
+	${debug:+$([ "$debug" -gt 2 ] && echo -n "--verbose")} \
+	"--output=${isofile}" \
 	"--override-directory=${builddir}/grub-core" \
 	--rom-directory="${rom_directory}" \
 	--locale-directory="@srcdir@/po" \
-- 
2.27.0

[PATCH 10/12] grub-shell: Only turn on qemu head when large debug value is specified

[Glenn Washburn]

There are several levels of debug and more may be added. But the qemu head
should be one of the highest debug levels. Set debug to a high value of 10
to turn on the head. We do not want to accidentally turn it on when adding
a new debug level to the testing system.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index 854508750..fbc809a8c 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -391,7 +391,7 @@ isofile="$work_directory/grub.iso"
 test -z "$debug" || echo "GRUB ISO file: ${isofile}" >&2
 test -z "$debug" || echo "GRUB ROM directory: ${rom_directory}" >&2
 
-if test -z "$debug"; then
+if test "${debug:-0}" -lt 10; then
   qemuopts="${qemuopts} -nographic -monitor file:/dev/null"
   # SeaBIOS 1.11.0 added support for VGA emulation over a serial port.  If
   # this is configured in SeaBIOS, then -nographic causes some extra junk to
-- 
2.27.0

[PATCH 11/12] grub-shell: Use malta qemu-mips machine type instead off non-existant indy

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index fbc809a8c..c69fbd080 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -119,7 +119,7 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
     mips-arc)
 	boot=cd
 	qemu=qemu-system-mips64
-	qemuopts="-M indy $qemuopts"
+	qemuopts="-M malta $qemuopts"
 	serial_port=arc/serial0/line0
 	console=
 	trim=1
-- 
2.27.0

[PATCH 12/12] grub-shell: Update qemu UEFI firmware names to be more generic

[Glenn Washburn]

The current qemu firmware paths for arm-efi and arm64-efi are hardcoded
and not available on Ubuntu/Debian. Make these paths relative and basenames
generic. The idea is that regardless where or what names firmware images
are installed under they can be copied and renamed into a temporary
directory. Then use the -L argument to qemu to point to that directory.

For convenience, the GRUB source directory which build grub-shell will be
added to the list of firmware paths that qemu will search. So all that is
needed is to copy the firmwares to the source directory with potential
renames.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 tests/util/grub-shell.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index c69fbd080..1bdfbbeba 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -74,7 +74,7 @@ exec_show_error () {
 work_directory=${WORKDIR:-`mktemp -d "${TMPDIR:-/tmp}/grub-shell.XXXXXXXXXX"`} || exit 1
 
 . "${builddir}/grub-core/modinfo.sh"
-qemuopts="${GRUB_QEMU_OPTS}"
+qemuopts="${GRUB_QEMU_OPTS} -L ${srcdir}"
 serial_port=com0
 serial_null=
 halt_cmd=halt
@@ -194,7 +194,7 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
 	boot=hd
 	console=console
 	trim=1
-	qemuopts="-machine virt -cpu cortex-a57 -bios /usr/share/qemu-efi/QEMU_EFI.fd $qemuopts"
+	qemuopts="-machine virt -cpu cortex-a57 -bios OVMF-aarch64.fd $qemuopts"
 	disk="device virtio-blk-device,drive=hd1 -drive if=none,id=hd1,file="
 	serial_port=
 	;;
@@ -203,7 +203,7 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
 	boot=hd
 	console=console
 	trim=1
-	qemuopts="-machine virt -bios /usr/share/ovmf-arm/QEMU_EFI.fd $qemuopts"
+	qemuopts="-machine virt -bios OVMF-arm.fd $qemuopts"
 	disk="device virtio-blk-device,drive=hd1 -drive if=none,id=hd1,file="
 	serial_port=efi0
 	;;
-- 
2.27.0

Re: [PATCH 00/12] Grub-shell improvements

[Glenn Washburn]

Hi Daniel,

What are the chances this patch series can be reviewed in the near
future? Some feedback would be greatly appreciated. 

Glenn

On Wed, 31 Mar 2021 20:57:39 -0500
Glenn Washburn <development@efficientek.com> wrote:

> I've extracted this patch series from the GitLab CI patch series,
> which some minor modifications. Mostly these are improvements to
> grub-shell to facilitate debugging.
> 
> The only patch that might be considered a fix, as opposed to an
> improvement, would be patch #11, which fixes the issue where
> qemu-mips is given a non-existant machine type. And while this was
> discussed here on the list as a possible solution, I couldn't get the
> mips tests working, so I'm not sure it's the right fix.
> 
> Glenn
> 
> Glenn Washburn (12):
>   grub-shell: Allow specifying non-default trim line contents
>   grub-shell: Trim line should always be matched from the beginning of
>     the line
>   grub-shell: Only show grub-mkrescue output if it returns an error
>   grub-shell: Allow setting default timeout via
>     GRUB_SHELL_DEFAULT_TIMEOUT envvar
>   grub-shell: Put all generated files into working dir and use better
>     file names
>   grub-shell: Add grub output logfile with grub-shell --debug
>   grub-shell: Set exit status to qemu exit status
>   tests: Allow turning on shell tracing from environment variables
>   grub-shell: Add --verbose to mkrescue when $debug is greater than 2
>   grub-shell: Only turn on qemu head when large debug value is
> specified grub-shell: Use malta qemu-mips machine type instead off
> non-existant indy
>   grub-shell: Update qemu UEFI firmware names to be more generic
> 
>  tests/util/grub-fs-tester.in |  2 +
>  tests/util/grub-shell.in     | 89
> ++++++++++++++++++++++++------------ 2 files changed, 63
> insertions(+), 28 deletions(-)
> 

Re: [PATCH 00/12] Grub-shell improvements

[Daniel Kiper]

Hi Glenn,

On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> Hi Daniel,
>
> What are the chances this patch series can be reviewed in the near
> future? Some feedback would be greatly appreciated.

I can see the following patches from you waiting in my review queue:
  - [CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2 and a few other things.
    https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
  - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
    https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
  - [PATCH 0/5] Testing improvements
    https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
  - [CI 00/17] Gitlab CI and test framework improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html
  - [PROCFS 0/5] Add and improve (proc) entries
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
  - [PATCH 0/4] Various LUKS2 improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
  - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
  - [PATCH] fs: Allow number of blocks in block list to be optional, defaulting length to device length
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
  - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
  - [PATCH] command: Add silent mode to read command to suppress input echo
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
  - [PATCH 0/2] Allow overriding commands
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
  - [PATCH 00/12] Grub-shell improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
  - [PATCH v2 0/8] Various fixes/improvements for tests
    https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
  - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto modules
    https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html

Please sort them in the order of importance/preference/... Then I will be
looking at them (more or less) in that order, one patch set at a time.

I hope I did not miss any of your patches.

Daniel

Re: [PATCH 00/12] Grub-shell improvements

[Glenn Washburn]

Hi Daniel,

On Thu, 26 Aug 2021 20:00:32 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> Hi Glenn,
> 
> On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > Hi Daniel,
> >
> > What are the chances this patch series can be reviewed in the near
> > future? Some feedback would be greatly appreciated.
> 
> I can see the following patches from you waiting in my review queue:
>   - [CRYPTO-LUKS v1 00/19] Fixes and improvements for
> cryptodisks+luks2 and a few other things.
> https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
>   - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
>     https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
>   - [PATCH 0/5] Testing improvements
>     https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
>   - [CI 00/17] Gitlab CI and test framework improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html
>   - [PROCFS 0/5] Add and improve (proc) entries
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
>   - [PATCH 0/4] Various LUKS2 improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
>   - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
>   - [PATCH] fs: Allow number of blocks in block list to be optional,
> defaulting length to device length
> https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
>   - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
> dash-insensitive manner
> https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
>   - [PATCH] command: Add silent mode to read command to suppress
> input echo
> https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
>   - [PATCH 0/2] Allow overriding commands
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
>   - [PATCH 00/12] Grub-shell improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
>   - [PATCH v2 0/8] Various fixes/improvements for tests
>     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
>   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
> modules
> https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> 
> Please sort them in the order of importance/preference/... Then I
> will be looking at them (more or less) in that order, one patch set
> at a time.
> 
> I hope I did not miss any of your patches.

As far as I can tell, this is the full list.

My order preference is as follows:

These two patches are only first because it should be a quick review.
  - [PATCH] command: Add silent mode to read command to suppress
    input echo
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
  - [PATCH] fs: Allow number of blocks in block list to be optional,
    defaulting length to device length
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html

This is a high priority because I think it should be merged before the
keyfile and detached header support patch series. If this is merged,
I'll submit and updated keyfile and detached header patch series that
works with this patch series.
  - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
    modules
    https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html

Based on discussion with Thomas Schmitt, I think the first patch of
this series should be dropped. Should I make a v3, or would you review
it and if acceptable drop the first patch?
  - [PATCH v2 0/8] Various fixes/improvements for tests
    https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html

Then these in this order.
  - [PATCH 00/12] Grub-shell improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
  - [PATCH 0/4] Various LUKS2 improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
  - [PROCFS 0/5] Add and improve (proc) entries
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
  - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
    dash-insensitive manner
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
  - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
  - [PATCH 0/2] Allow overriding commands
    https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html

====================================================================
Patch series with dependencies

This patch series requires keyfile support via the -k argument for
cryptomount. So it must be reviewed after a patch series implementing
this. Also, iirc, this series relies on "Grub-shell improvements".
Ideally this would get reviewed soon after its prerequisites get
merged, so that we can be testing LUKS support. I'll probably also need
to update it for changes, so let's hold off on this until I resubmit an
updated version.
  - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
    https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html

====================================================================
Excluded/Discarded patch series

I'm excluding the following patch series because most of it has already
been accepted or is in one of the above patch series. So no need to
review this one.
  - [CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2
    and a few other things.
    https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html

Likewise this patch series is mostly a subset of "Various
fixes/improvements for tests", and can be discarded.
  - [PATCH 0/5] Testing improvements
    https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html

This patch series is the patch series "Testing improvements" combined
with "Grub-shell improvements" with the CI patch on top. It makes sense
to first deal with the patch series as separate, then the CI patch,
which I'll submit at a later date. So let's ignore this one also.
  - [CI 00/17] Gitlab CI and test framework improvements
    https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html

Much appreciated,
Glenn

Re: [PATCH 00/12] Grub-shell improvements

[Daniel Kiper]

CC-ing Denis and Patrick...

On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote:
> Hi Daniel,
>
> On Thu, 26 Aug 2021 20:00:32 +0200
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> > Hi Glenn,
> >
> > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > > Hi Daniel,
> > >
> > > What are the chances this patch series can be reviewed in the near
> > > future? Some feedback would be greatly appreciated.
> >
> > I can see the following patches from you waiting in my review queue:
> >   - [CRYPTO-LUKS v1 00/19] Fixes and improvements for
> > cryptodisks+luks2 and a few other things.
> > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
> >   - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
> >     https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
> >   - [PATCH 0/5] Testing improvements
> >     https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
> >   - [CI 00/17] Gitlab CI and test framework improvements
> >     https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html
> >   - [PROCFS 0/5] Add and improve (proc) entries
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
> >   - [PATCH 0/4] Various LUKS2 improvements
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
> >   - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
> >   - [PATCH] fs: Allow number of blocks in block list to be optional,
> > defaulting length to device length
> > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> >   - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
> > dash-insensitive manner
> > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
> >   - [PATCH] command: Add silent mode to read command to suppress
> > input echo
> > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> >   - [PATCH 0/2] Allow overriding commands
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
> >   - [PATCH 00/12] Grub-shell improvements
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
> >   - [PATCH v2 0/8] Various fixes/improvements for tests
> >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
> >   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
> > modules
> > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> >
> > Please sort them in the order of importance/preference/... Then I
> > will be looking at them (more or less) in that order, one patch set
> > at a time.
> >
> > I hope I did not miss any of your patches.
>
> As far as I can tell, this is the full list.

Great!

> My order preference is as follows:
>
> These two patches are only first because it should be a quick review.
>   - [PATCH] command: Add silent mode to read command to suppress
>     input echo
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
>   - [PATCH] fs: Allow number of blocks in block list to be optional,
>     defaulting length to device length
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html

OK.

> This is a high priority because I think it should be merged before the
> keyfile and detached header support patch series. If this is merged,
> I'll submit and updated keyfile and detached header patch series that
> works with this patch series.
>   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
>     modules
>     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html

Denis, Patrick, are you both OK with this?

> Based on discussion with Thomas Schmitt, I think the first patch of
> this series should be dropped. Should I make a v3, or would you review
> it and if acceptable drop the first patch?
>   - [PATCH v2 0/8] Various fixes/improvements for tests
>     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html

No, you do not need to repost. I will take a look at v2 and ignore first
patch in it.

> Then these in this order.
>   - [PATCH 00/12] Grub-shell improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
>   - [PATCH 0/4] Various LUKS2 improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
>   - [PROCFS 0/5] Add and improve (proc) entries
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
>   - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
>     dash-insensitive manner
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
>   - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
>   - [PATCH 0/2] Allow overriding commands
>     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
>
> ====================================================================
> Patch series with dependencies
>
> This patch series requires keyfile support via the -k argument for
> cryptomount. So it must be reviewed after a patch series implementing
> this. Also, iirc, this series relies on "Grub-shell improvements".
> Ideally this would get reviewed soon after its prerequisites get
> merged, so that we can be testing LUKS support. I'll probably also need
> to update it for changes, so let's hold off on this until I resubmit an
> updated version.
>   - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
>     https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
>
> ====================================================================
> Excluded/Discarded patch series
>
> I'm excluding the following patch series because most of it has already
> been accepted or is in one of the above patch series. So no need to
> review this one.
>   - [CRYPTO-LUKS v1 00/19] Fixes and improvements for cryptodisks+luks2
>     and a few other things.
>     https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
>
> Likewise this patch series is mostly a subset of "Various
> fixes/improvements for tests", and can be discarded.
>   - [PATCH 0/5] Testing improvements
>     https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
>
> This patch series is the patch series "Testing improvements" combined
> with "Grub-shell improvements" with the CI patch on top. It makes sense
> to first deal with the patch series as separate, then the CI patch,
> which I'll submit at a later date. So let's ignore this one also.
>   - [CI 00/17] Gitlab CI and test framework improvements
>     https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html

Granted!

Daniel

Re: [PATCH 00/12] Grub-shell improvements

[Denis 'GNUtoo' Carikli]

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

On Mon, 30 Aug 2021 17:23:44 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> CC-ing Denis and Patrick...
> 
> On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote:
> > Hi Daniel,
> >
> > On Thu, 26 Aug 2021 20:00:32 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > Hi Glenn,
> > >
> > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > > > Hi Daniel,
> > > >
> > > > What are the chances this patch series can be reviewed in the
> > > > near future? Some feedback would be greatly appreciated.
> > >
> > > I can see the following patches from you waiting in my review
> > > queue:
> > >   - [CRYPTO-LUKS v1 00/19] Fixes and improvements for
> > > cryptodisks+luks2 and a few other things.
> > > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
> > >   - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
> > >     https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
> > >   - [PATCH 0/5] Testing improvements
> > >     https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
> > >   - [CI 00/17] Gitlab CI and test framework improvements
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html
> > >   - [PROCFS 0/5] Add and improve (proc) entries
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
> > >   - [PATCH 0/4] Various LUKS2 improvements
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
> > >   - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
> > >   - [PATCH] fs: Allow number of blocks in block list to be
> > > optional, defaulting length to device length
> > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> > >   - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
> > > dash-insensitive manner
> > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
> > >   - [PATCH] command: Add silent mode to read command to suppress
> > > input echo
> > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> > >   - [PATCH 0/2] Allow overriding commands
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
> > >   - [PATCH 00/12] Grub-shell improvements
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
> > >   - [PATCH v2 0/8] Various fixes/improvements for tests
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
> > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to
> > > crypto modules
> > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> > >
> > > Please sort them in the order of importance/preference/... Then I
> > > will be looking at them (more or less) in that order, one patch
> > > set at a time.
> > >
> > > I hope I did not miss any of your patches.
> >
> > As far as I can tell, this is the full list.
> 
> Great!
> 
> > My order preference is as follows:
> >
> > These two patches are only first because it should be a quick
> > review.
> >   - [PATCH] command: Add silent mode to read command to suppress
> >     input echo
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> >   - [PATCH] fs: Allow number of blocks in block list to be optional,
> >     defaulting length to device length
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> 
> OK.
> 
> > This is a high priority because I think it should be merged before
> > the keyfile and detached header support patch series. If this is
> > merged, I'll submit and updated keyfile and detached header patch
> > series that works with this patch series.
> >   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
> >     modules
> >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> 
> Denis, Patrick, are you both OK with this?
I'm OK with it.

The "Refactor/improve cryptomount data passing to crypto modules" looks
way cleaner than what we had before: it can scale better than the
previous design because it's more generic, it can be extended more
easily, and we can have more fine grained communication between the
cryptodisk and the backends.

Glenn Washburn <development@efficientek.com> wrote:
> My intention is for this patch series to lay the foundation for an
> improved patch series providing detached header and keyfile support
> (I already have the series updated and ready to send once this is
> accepted).
Thanks a lot for that work and for taking care of the patch serie I
sent.

Denis.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/12] Grub-shell improvements

[Patrick Steinhardt]

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

On Mon, Aug 30, 2021 at 06:33:15PM +0200, Denis 'GNUtoo' Carikli wrote:
> On Mon, 30 Aug 2021 17:23:44 +0200
> Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> > CC-ing Denis and Patrick...
> > 
> > On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote:
> > > Hi Daniel,
> > >
> > > On Thu, 26 Aug 2021 20:00:32 +0200
> > > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > >
> > > > Hi Glenn,
> > > >
> > > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > > > > Hi Daniel,
> > > > >
> > > > > What are the chances this patch series can be reviewed in the
> > > > > near future? Some feedback would be greatly appreciated.
> > > >
> > > > I can see the following patches from you waiting in my review
> > > > queue:
> > > >   - [CRYPTO-LUKS v1 00/19] Fixes and improvements for
> > > > cryptodisks+luks2 and a few other things.
> > > > https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00088.html
> > > >   - [CRYPTOMOUNT-TEST 0/7] Add LUKS1/2 tests for cryptomount
> > > >     https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00010.html
> > > >   - [PATCH 0/5] Testing improvements
> > > >     https://lists.gnu.org/archive/html/grub-devel/2020-12/msg00246.html
> > > >   - [CI 00/17] Gitlab CI and test framework improvements
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-02/msg00071.html
> > > >   - [PROCFS 0/5] Add and improve (proc) entries
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00264.html
> > > >   - [PATCH 0/4] Various LUKS2 improvements
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00272.html
> > > >   - [PATCH 0/4] Miscellaneous changes to aid in troubleshooting
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00279.html
> > > >   - [PATCH] fs: Allow number of blocks in block list to be
> > > > optional, defaulting length to device length
> > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> > > >   - [PATCH v2 0/2] cryptodisk: Allows UUIDs to be compared in a
> > > > dash-insensitive manner
> > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00344.html
> > > >   - [PATCH] command: Add silent mode to read command to suppress
> > > > input echo
> > > > https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> > > >   - [PATCH 0/2] Allow overriding commands
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00292.html
> > > >   - [PATCH 00/12] Grub-shell improvements
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00390.html
> > > >   - [PATCH v2 0/8] Various fixes/improvements for tests
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
> > > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to
> > > > crypto modules
> > > > https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> > > >
> > > > Please sort them in the order of importance/preference/... Then I
> > > > will be looking at them (more or less) in that order, one patch
> > > > set at a time.
> > > >
> > > > I hope I did not miss any of your patches.
> > >
> > > As far as I can tell, this is the full list.
> > 
> > Great!
> > 
> > > My order preference is as follows:
> > >
> > > These two patches are only first because it should be a quick
> > > review.
> > >   - [PATCH] command: Add silent mode to read command to suppress
> > >     input echo
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> > >   - [PATCH] fs: Allow number of blocks in block list to be optional,
> > >     defaulting length to device length
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> > 
> > OK.
> > 
> > > This is a high priority because I think it should be merged before
> > > the keyfile and detached header support patch series. If this is
> > > merged, I'll submit and updated keyfile and detached header patch
> > > series that works with this patch series.
> > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
> > >     modules
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> > 
> > Denis, Patrick, are you both OK with this?
> I'm OK with it.
> 
> The "Refactor/improve cryptomount data passing to crypto modules" looks
> way cleaner than what we had before: it can scale better than the
> previous design because it's more generic, it can be extended more
> easily, and we can have more fine grained communication between the
> cryptodisk and the backends.

Agreed, this has been a pain point in the current architecture.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

IS: Glenn's patches status WAS: Re: [PATCH 00/12] Grub-shell improvements

[Daniel Kiper]

On Mon, Aug 30, 2021 at 05:23:44PM +0200, Daniel Kiper wrote:
> CC-ing Denis and Patrick...
>
> On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote:
> > Hi Daniel,
> >
> > On Thu, 26 Aug 2021 20:00:32 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > > Hi Glenn,
> > >
> > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > > > Hi Daniel,

[...]

> > As far as I can tell, this is the full list.
>
> Great!

It looks I missed at least this one:
  - [PATCH 00/17] Fixes and improvements for cryptodisks+luks2 and a few other things.
    https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00050.html

Do you still care about it?

> > My order preference is as follows:
> >
> > These two patches are only first because it should be a quick review.
> >   - [PATCH] command: Add silent mode to read command to suppress
> >     input echo
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> >   - [PATCH] fs: Allow number of blocks in block list to be optional,
> >     defaulting length to device length
> >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
>
> OK.

I merged them yesterday.

> > This is a high priority because I think it should be merged before the
> > keyfile and detached header support patch series. If this is merged,
> > I'll submit and updated keyfile and detached header patch series that
> > works with this patch series.
> >   - [PATCH 0/3] Refactor/improve cryptomount data passing to crypto
> >     modules
> >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
>
> Denis, Patrick, are you both OK with this?

I can see a discussion about this patch set. Should I wait for a new
version for review?

> > Based on discussion with Thomas Schmitt, I think the first patch of
> > this series should be dropped. Should I make a v3, or would you review
> > it and if acceptable drop the first patch?
> >   - [PATCH v2 0/8] Various fixes/improvements for tests
> >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html

Next on my list for review...

Daniel

Re: IS: Glenn's patches status WAS: Re: [PATCH 00/12] Grub-shell improvements

[Glenn Washburn]

On Tue, 14 Sep 2021 13:04:31 +0200
Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Mon, Aug 30, 2021 at 05:23:44PM +0200, Daniel Kiper wrote:
> > CC-ing Denis and Patrick...
> >
> > On Thu, Aug 26, 2021 at 05:08:21PM -0500, Glenn Washburn wrote:
> > > Hi Daniel,
> > >
> > > On Thu, 26 Aug 2021 20:00:32 +0200
> > > Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > >
> > > > Hi Glenn,
> > > >
> > > > On Wed, Aug 25, 2021 at 06:06:30PM -0500, Glenn Washburn wrote:
> > > > > Hi Daniel,
> 
> [...]
> 
> > > As far as I can tell, this is the full list.
> >
> > Great!
> 
> It looks I missed at least this one:
>   - [PATCH 00/17] Fixes and improvements for cryptodisks+luks2 and a
> few other things.
> https://lists.gnu.org/archive/html/grub-devel/2020-07/msg00050.html
> 
> Do you still care about it?

No, these can be skipped. The changes in that patch series that aren't
integrated, are in other patch series or I'll send to the list in the
future.

> > > My order preference is as follows:
> > >
> > > These two patches are only first because it should be a quick
> > > review.
> > >   - [PATCH] command: Add silent mode to read command to suppress
> > >     input echo
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00291.html
> > >   - [PATCH] fs: Allow number of blocks in block list to be
> > > optional, defaulting length to device length
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-03/msg00286.html
> >
> > OK.
> 
> I merged them yesterday.

Thanks!

> > > This is a high priority because I think it should be merged
> > > before the keyfile and detached header support patch series. If
> > > this is merged, I'll submit and updated keyfile and detached
> > > header patch series that works with this patch series.
> > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to
> > > crypto modules
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> >
> > Denis, Patrick, are you both OK with this?
> 
> I can see a discussion about this patch set. Should I wait for a new
> version for review?

Yes, there will be a new version of the patch series coming. I'm
waiting to tie up the last concern of Patrick's.

> > > Based on discussion with Thomas Schmitt, I think the first patch
> > > of this series should be dropped. Should I make a v3, or would
> > > you review it and if acceptable drop the first patch?
> > >   - [PATCH v2 0/8] Various fixes/improvements for tests
> > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00110.html
> 
> Next on my list for review...
> 
> Daniel

Great! I also have some more testing fixes / changes coming in this
vein.

Glenn

Re: IS: Glenn's patches status WAS: Re: [PATCH 00/12] Grub-shell improvements

[Daniel Kiper]

On Tue, Sep 14, 2021 at 05:27:55PM +0000, Glenn Washburn wrote:
> On Tue, 14 Sep 2021 13:04:31 +0200
> Daniel Kiper <daniel.kiper@oracle.com> wrote:

[...]

> > > > This is a high priority because I think it should be merged
> > > > before the keyfile and detached header support patch series. If
> > > > this is merged, I'll submit and updated keyfile and detached
> > > > header patch series that works with this patch series.
> > > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to
> > > > crypto modules
> > > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> > >
> > > Denis, Patrick, are you both OK with this?
> >
> > I can see a discussion about this patch set. Should I wait for a new
> > version for review?
>
> Yes, there will be a new version of the patch series coming. I'm
> waiting to tie up the last concern of Patrick's.

May I ask you to add James Bottomley to the list of recipients of this
patch series?

Daniel

Re: IS: Glenn's patches status WAS: Re: [PATCH 00/12] Grub-shell improvements

[Daniel Kiper]

On Wed, Sep 15, 2021 at 06:15:51PM +0200, Daniel Kiper wrote:
> On Tue, Sep 14, 2021 at 05:27:55PM +0000, Glenn Washburn wrote:
> > On Tue, 14 Sep 2021 13:04:31 +0200
> > Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> [...]
>
> > > > > This is a high priority because I think it should be merged
> > > > > before the keyfile and detached header support patch series. If
> > > > > this is merged, I'll submit and updated keyfile and detached
> > > > > header patch series that works with this patch series.
> > > > >   - [PATCH 0/3] Refactor/improve cryptomount data passing to
> > > > > crypto modules
> > > > >     https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00129.html
> > > >
> > > > Denis, Patrick, are you both OK with this?
> > >
> > > I can see a discussion about this patch set. Should I wait for a new
> > > version for review?
> >
> > Yes, there will be a new version of the patch series coming. I'm
> > waiting to tie up the last concern of Patrick's.
>
> May I ask you to add James Bottomley to the list of recipients of this
> patch series?

... and Denis 'GNUtoo' Carikli...

Daniel