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

[PATCH] 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 difficult 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 bb7b285e9..ce431757c 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -59,6 +59,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
@@ -384,13 +395,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.34.1

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

[Daniel Kiper]

On Thu, Aug 11, 2022 at 12:06:43PM -0500, Glenn Washburn wrote:
> The previous behavior ignored an error and the output from grub-mkrescue.
> This made it difficult 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>

This completely breaks messages formatting, especially multi-line ones,
but is better than silent error.

So, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel