[PATCH v2 00/12] Grub-shell improvements
Updates since v1:
* Improve QEMU logging patch to make sure all data is written to the pipe before
exiting (otherwise tests can fail because they don't get the last bit of QEMU
output).
* Improve QEMU firmware handling to prefer using the -bios option (for older
setups) and prefer firmware files found in the source directory so that
system firmware can be overridden and systems without packages providing the
firmware can be used.
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: Add flexibility in QEMU firmware handling
tests/util/grub-fs-tester.in | 2 +
tests/util/grub-shell.in | 185 +--
2 files changed, 158 insertions(+), 29 deletions(-)
Range-diff against v1:
1: ee0b447e0 = 1: 52df3299f grub-shell: Allow specifying non-default trim
line contents
2: 34ce6 = 2: 7c8264aeb grub-shell: Trim line should always be matched
from the beginning of the line
3: e2826567c = 3: c17da94e7 grub-shell: Only show grub-mkrescue output if
it returns an error
4: 68fa49770 = 4: 27717b949 grub-shell: Allow setting default timeout via
GRUB_SHELL_DEFAULT_TIMEOUT envvar
5: e113eba70 = 5: 17dd72798 grub-shell: Put all generated files into
working dir and use better file names
6: 8acbe1a63 = 6: 428698acd grub-shell: Add grub output logfile with
grub-shell --debug
7: 925d155ea ! 7: cdd28473c grub-shell: Set exit status to qemu exit status
@@ tests/util/grub-shell.in: copy_extra_files() {
done
}
++setup_qemu_logger() {
++cat < "$work_directory/qemu-pipe" | tr -d "\r" | tee "${goutfile}" |
do_trim &
++}
++
+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"
@@ tests/util/grub-shell.in: if [ x$boot = xnet ]; then
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
++setup_qemu_logger
+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"
@@ tests/util/grub-shell.in: elif [ x$boot = xemu ]; then
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
++setup_qemu_logger
+@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
"${g
[PATCH v2 03/12] grub-shell: Only show grub-mkrescue output if it returns an error
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
---
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 .
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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 02/12] grub-shell: Trim line should always be matched from the beginning of the line
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
---
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.XX"` || 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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 06/12] grub-shell: Add grub output logfile with grub-shell --debug
This allows seeing full qemu output of grub-shell, which can be invaluable
when debugging failing tests.
Signed-off-by: Glenn Washburn
---
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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 08/12] tests: Allow turning on shell tracing from environment variables
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
---
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 2dcd09f5e..6b8028f9b 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 f4b733858..78f27339f 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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 04/12] grub-shell: Allow setting default timeout via GRUB_SHELL_DEFAULT_TIMEOUT envvar
Signed-off-by: Glenn Washburn
---
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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 17/19] [not for merge] print more debug info in mm
On Wed, 10 Nov 2021 14:47:07 +0100
Daniel Kiper wrote:
> CC-ing Glenn...
>
> On Tue, Oct 12, 2021 at 06:30:06PM +1100, Daniel Axtens wrote:
> > This is handy for debugging - I'm including it in case anyone else hacking
> > on this area finds it helpful.
> >
> > Signed-off-by: Daniel Axtens
> > ---
> > grub-core/kern/mm.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> > index 58d5b89e8860..811df1ab5ebb 100644
> > --- a/grub-core/kern/mm.c
> > +++ b/grub-core/kern/mm.c
> > @@ -135,6 +135,9 @@ grub_mm_init_region (void *addr, grub_size_t size)
> >for (p = _mm_base, q = *p; q; p = &(q->next), q = *p)
> > {
> >/* Does this region come _before_ an existing region? */
> > + grub_printf ("Extending w/ before %p + %" PRIxGRUB_SIZE " + %"
> > PRIxGRUB_SIZE " = %p ? %s\n",
> > +(grub_uint8_t *)addr, size, q->pre_size, (grub_uint8_t *)q,
> > +(grub_uint8_t *)addr + size + q->pre_size == (grub_uint8_t *)
> > q ? "yes" : "no");
>
> I think this kind of messages can be useful. Same applies to patch #18.
> Though I would use grub_dprintf() instead which should be wrapped with
> #ifdef MM_DEBUG. However, we have to be very careful with printing any
> messages from mm and do not exeecec 255 chars message length. If we go
> above that limit then we will trigger dynamic allocation in grub_dprintf()
> from mm which may lead to a recursion...
>
> Additionally, I think Glenn's patch allowing us to disable logging from
> certain subsystem would be useful here. Glenn, could you take a look at
> it once again?
Yes, I'm planning on it. Its been low priority for me and this month is
very busy fo me IRL. I don't think my patches should hold this up
though (or would this cause way to much logs if one didn't want them?)
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[LOCAL-CI 2/3] scripts: Add functions for CI stages and default variables to functions.sh
Signed-off-by: Glenn Washburn
---
scripts/ci/functions.sh | 899 +++-
1 file changed, 898 insertions(+), 1 deletion(-)
diff --git a/scripts/ci/functions.sh b/scripts/ci/functions.sh
index 2f4cecaa1..f94f90dc1 100644
--- a/scripts/ci/functions.sh
+++ b/scripts/ci/functions.sh
@@ -1,5 +1,412 @@
#!/bin/bash
+### Documentation on configuration environment variables
+
+# SDIR [dirname of $0]:
+# Path to directory the caller of this script resides in.
+# ROOT [./grub-tests]:
+# Path to directory where all file operations take place, unless
overridden
+# by another environment variable.
+# CI_PROJECT_DIR [$ROOT]:
+# Base directory to use in commands.
+# CI_TYPE [local]:
+# Value used to control sourcing of ${CI_SCRIPT_DIR}/functions.$CI_TYPE.sh
+# which overrides the variables/functions in this file.
+# SCRIPTS_DIR [${SDIR} if ${SDIR}/ci else ${SRCDIR}/scripts]:
+# Path to scripts directory from source.
+# CI_SCRIPTS_DIR [${SCRIPTS_DIR}/ci]:
+# Path to CI scripts directory, normally the ci subdirectory of SCRITS_DIR
+# and where this file resides.
+# SHELL_TRACE [n]:
+# If set, turn on shell tracing of everything. NOTE: TEST_VERBOSITY=3
turns
+# on more targeted shell tracing
+# TMPDIR [$ROOT/testtmp]:
+# Path to directory to use for temporary files.
+# MATRIX [see definition below]:
+# A list of $ARCH:$TARGET[,...]:$CROSS_ARCH, one per line, with lines
+# starting with # being ignored.
+# JOBS:
+# Number of make jobs to run in parallel. Defaulted in build.sh and
test.sh
+# to the number of available processors.
+# NUM_FAILED_LOG_LINES [100]:
+# Set to integer number of log lines to display from the end of a failed
+# log file.
+
+### Source checkout variables
+# GIT_REPO:
+# URL to git repo
+# GIT_BRANCH:
+# Branch name in git repo
+# GIT_CLONE_ARGS [--depth=1 --no-single-branch]:
+# Extra args to use when cloning repository
+# SRCDIR [${CI_PROJECT_DIR}/grub]:
+# Path to source checkout
+
+### Distro package variables
+# APT_OPTS [--no-upgrade]:
+# Extra options when running apt.
+# PACKAGES [see definition below]:
+# A list of packages to add to a default list of packages needing to be
+# installed. The default list can only be appended to.
+# PACKAGE_CACHE [${CI_PROJECT_DIR}/packages]:
+# Path to directory where downloaded packages will be cached.
+
+### Ccache variables
+# CCACHE_DIR [${CI_PROJECT_DIR}/ccache]:
+# Directory to use for ccache
+
+### Cross compiler tool chain variables
+# CROSS_DIR [$CI_PROJECT_DIR/cross]:
+# Path to directory containing cross compilers tool chains for each
+# architecture. If it doesn't exist, it will be created and required tool
+# chains will be downloaded/installed there.
+# CROSS_VERSION [10.1.0]:
+# Version of cross-compiler to use (suggested: 10.1.0, 9.3.0, 8.1.0,
+# 7.5.0 [riscv32/64 builds fail before 7.3.0]).
+
+### Configure/Build variables
+# CONFIGURE_OPTS [--enable-boot-time]:
+# Extra options to pass to configure.
+# FONT_SOURCE:
+# Path to unicode font named "unifont." where ext is one of:
+#pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# DJVU_FONT_SOURCE:
+# Path to DejaVu font named "DejaVuSans." where ext is one of:
+#pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# BUILD_ALL_TARGETS:
+# If set 'y', all targets defined in the build matrix will be
+# built ignoring DISABLED_BUILDS, but can still be disabled by
+# DISABLE_ALL_BUILDS.
+# DISABLED_BUILDS [see definition below]:
+# A list of targets for which builds are disabled, one per line. These
+# default targets are disabled generally because they do not work yet.
+# Lines beginning with '#' may be used as comments and are ignored. Some
+# hints as to why the tests fail are included in comment lines. Patches
+# which get these tests working are very welcome.
+# DISABLE_ALL_BUILDS [n]:
+# If set to 'y', disable completely building (eg. the run_build command).
+
+### Tests variables
+# TESTS_TIMEOUT:
+# Set timeout for completion of all make check tests (see "man timeout"
+# duration argument). This is most useful when the make check job is
+# taking longer than the job timeout configured by the runner. In this
+# case, set this to 5-10 minutes less than runner timeout so that there
+# is time to package up the logs for debugging.
+# IGNORE_TIMEDOUT_TEST_FAILURE [y]:
+# If set to 'y', tests which fail due to a timeout in grub-shell of qemu
+# will not be counted as a failure. These failures are almost always the
+# result of insufficient runner resources to complete the execution of
qemu
+# within the timeout period.
+# FAIL_ON_HARD_ERRORS [n]:
+# If set to 'y', hard errors will cause a failure. A hard error indicates
+# that the test was not able to be r
[LOCAL-CI 3/3] scripts: Add local-tester.sh script to run local CI tests
Signed-off-by: Glenn Washburn
---
scripts/ci/functions.local.sh | 37 +
scripts/local-tester.sh | 39 +++
2 files changed, 76 insertions(+)
create mode 100644 scripts/ci/functions.local.sh
create mode 100755 scripts/local-tester.sh
diff --git a/scripts/ci/functions.local.sh b/scripts/ci/functions.local.sh
new file mode 100644
index 0..7a6766c07
--- /dev/null
+++ b/scripts/ci/functions.local.sh
@@ -0,0 +1,37 @@
+#!/bin/bash
+
+TXT_RED="\e[31m"
+TXT_YELLOW="\e[33m"
+TXT_CLEAR="\e[0m"
+TXT_LOG_COLOR="\e[97;100m"
+
+function start_log() {
+ local LOG_COLLAPSE LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -c) LOG_COLLAPSE=1; shift;;
+ -n) LOG_NAME="$2"; shift;;
+ *) [ "$#" -eq 1 ] && LOG_MSG="$1"; shift;;
+esac
+ done
+
+ echo -e "=="
+ echo -e "== ${TXT_YELLOW}Start Section ${LOG_NAME}:
${LOG_MSG}${TXT_CLEAR}"
+ echo -e "=="
+ echo -e "${LOG_NAME} STARTTIME=`date +%s`" >&2
+}
+
+function end_log() {
+ local LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -n) LOG_NAME=$2; shift;;
+ *) shift;;
+esac
+ done
+
+ echo -e "${LOG_NAME} ENDTIME=`date +%s`" >&2
+ echo -e "== End Section ${LOG_NAME} =="
+}
+
+:;
diff --git a/scripts/local-tester.sh b/scripts/local-tester.sh
new file mode 100755
index 0..9cc7b8f43
--- /dev/null
+++ b/scripts/local-tester.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+set -e
+
+SDIR=$(realpath -e "${SDIR:-"${0%/*}"}")
+ROOT=$(realpath -m "${ROOT:-"./grub-tests"}")
+
+mkdir -p "$ROOT"
+cd "$ROOT"
+
+# Log stdout+stderr to file while keeping stdout going to original stdout
+exec 2>"$ROOT/$(basename "$0" .sh).$(date '+%Y%m%d%H%M%S').log" > >(exec 3>&1;
exec tee >(exec cat >&3) >&2)
+PTTY=$!
+
+if [ "x$SHELL_TRACE" = "xy" ]; then
+ set -x
+ export >&2
+fi
+
+export CI_TYPE=local
+if [ -d "${SDIR}/ci" ]; then
+ export SCRIPTS_DIR=${SCRIPTS_DIR:-"${SDIR}"}
+ export CI_SCRIPTS_DIR=${CI_SCRIPTS_DIR:-"${SCRIPTS_DIR}/ci"}
+fi
+
+# If found, source helper functions
+[ -f "${CI_SCRIPTS_DIR}/functions.sh" ] &&
+. "${CI_SCRIPTS_DIR}/functions.sh"
+
+
+onexit() {
+ set +x
+ # Must close our end of the pipe to tee, else it will not exit and
+ # we'll wait forever on it to exit.
+ exec >&-
+ wait_anypid $PTTY
+}
+trap onexit exit
+
+main "$@"
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[LOCAL-CI 0/3] Add support for local automated testing
This patch series aims to make automated testing of all supported targets easy. Not all targets are actually tested for a variety of reasons, but this series covers a significant portion of the target space and should make it easy to add more targets as we figure out how do run their tests. All supported targets (that I know of) are build tested. The main work horse is the scripts/local-tester.sh script which ties everything together. It is very configurable, but defaults to building and testing as much as it can. To start it just run "./scripts/local-tester.sh" from the root of the repository. By default a directory named 'grub-tests' in the current working directory is created and everything is put in there. See the beginning of ./scripts/ci/function.sh for available environment variables that can be used to configure how you want it to run. The file ./scripts/ci/function.sh has a bunch of functions most of which are for stages in the automated testing. It is intended to be sourced by config files for other CI systems so that they can reuse this code. One might ask, why not just have all CI systems use local-tester.sh and put everything in there? The issue is that currently local-tester.sh does not do things in parallel (the make subprocess can be made more parallel with the JOBS environment variable). So in many CI systems, one could have all targets building and testing at the same time, which local-tester.sh only does this serially. This makes local-tester.sh a lot slower than it needs to be. Also other CI systems allow the CI pipeline to e broken into many stages, each of which could be run on a different machine, with the ability to cache the output of certain stages. These scripts have been written and tested on debian based systems, specifically the reference system, Debian 11. Some of the stages are debian or debian- derivative specific, such as the package install stage which uses apt and dpkg. Most of the stages, I believe, are fairly distro agnostic and the ones that aren't should be able to be adapted for a specific distro fairly easily. Also, this patch series is meant to be used on top of the grub-shell patch series already submitted to the list. It will run without that series, but some of the features may not work or work as well. Noteably, the QEMU tests for targets i386-efi, arm-efi and arm64-efi will fail. Of particular note, there are some knobs that can provide a lot debugging output and save the intermediate files of failed tests for later analysis. local-tester.sh will try to download and install all packages it needs to function. Obviously, this will not work when not running as a privileged user. A further patch series is intended, which will add support for running the system successfully completely as an unprivileged user. If local-tester.sh is run as an unprivileged user, it will skip running of privileged commands, like the package installer. This means it can continue past the package install phase, but it assumes that the needed packages are already installed. Patch 1: Add general scripts for automated building/testing. Patch 2: Add automated building/testing stages and respective configuration variables. Patch 3: Add consumer of those stages, local-tester.sh, for running the automated building/testing locally. I'm not particularly in love with the name "local-tester.sh", so suggestions on alternative names are welcome. The same goes for the output, which I think could look better. Glenn Glenn Washburn (3): scripts: Add general scripts to aid automated testing scripts: Add functions for CI stages and default variables to functions.sh scripts: Add local-tester.sh script to run local CI tests scripts/ci/build.sh | 67 +++ scripts/ci/functions.local.sh | 37 ++ scripts/ci/functions.sh | 930 ++ scripts/ci/make-images.sh | 86 scripts/ci/process-tests.sh | 111 scripts/ci/test.sh| 120 + scripts/local-tester.sh | 39 ++ 7 files changed, 1390 insertions(+) create mode 100755 scripts/ci/build.sh create mode 100644 scripts/ci/functions.local.sh create mode 100644 scripts/ci/functions.sh create mode 100755 scripts/ci/make-images.sh create mode 100755 scripts/ci/process-tests.sh create mode 100755 scripts/ci/test.sh create mode 100755 scripts/local-tester.sh -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[LOCAL-CI 1/3] scripts: Add general scripts to aid automated testing
Signed-off-by: Glenn Washburn
---
scripts/ci/build.sh | 67
scripts/ci/functions.sh | 33 ++
scripts/ci/make-images.sh | 86 ++
scripts/ci/process-tests.sh | 111 +
scripts/ci/test.sh | 120
5 files changed, 417 insertions(+)
create mode 100755 scripts/ci/build.sh
create mode 100644 scripts/ci/functions.sh
create mode 100755 scripts/ci/make-images.sh
create mode 100755 scripts/ci/process-tests.sh
create mode 100755 scripts/ci/test.sh
diff --git a/scripts/ci/build.sh b/scripts/ci/build.sh
new file mode 100755
index 0..723dc292f
--- /dev/null
+++ b/scripts/ci/build.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+
+set -eo pipefail
+
+# Environment variables
+# JOBS: Number of concurrent jobs while building, defaults to number of
+# processors plus 1, unless number of processors is 1 in which case its 2.
+# SRCDIR: Path to source files
+# BUILDDIR: Directory in which to place the build
+# INSTALLDIR: Directory to install binaries
+# ARCH: Architecture to build for
+# PLATFORM: Platform to build for
+# CONFIGURE_OPTS: Extra configure options
+# FONT_SOURCE: Path to unicode font named "unifont." where ext is one of:
+# pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# DJVU_FONT_SOURCE: Path to DejaVu font named "DejaVuSans." where ext is
+# one of: pcf pcf.gz bdf bdf.gz ttf ttf.gz
+# SHELL_TRACE: Set to 'y' to enable shell tracing
+
+[ "x${SHELL_TRACE}" = "xy" ] && set -x
+
+[ -f "$(dirname "$0")/functions.sh" ] &&
+. "$(dirname "$0")/functions.sh"
+
+[ -f "$(dirname "$0")/functions.$CI_TYPE.sh" ] &&
+. "$(dirname "$0")/functions.$CI_TYPE.sh"
+
+JOBS=${JOBS:-`getconf _NPROCESSORS_ONLN 2> /dev/null || echo 2`}
+[ "$JOBS" = 1 ] || JOBS=$(($JOBS + 1))
+
+TARGET="${ARCH}-${PLATFORM}"
+
+mkdir -pv ${BUILDDIR}
+
+RET=0
+cd ${BUILDDIR}
+
+if test -f "$FONT_SOURCE"; then
+ ln -svf "$(realpath -e "$FONT_SOURCE")" ./
+fi
+
+if test -f "$DJVU_FONT_SOURCE"; then
+ ln -svf "$(realpath -e "$DJVU_FONT_SOURCE")" ./
+fi
+
+start_log -c -n "configure-$TARGET" "Configuring $TARGET"
+[ -x "$SRCDIR/configure" ] && $SRCDIR/configure --help
+$SRCDIR/configure --target=$ARCH --with-platform=$PLATFORM \
+ --prefix=${INSTALLDIR} $CONFIGURE_OPTS || RET=$?
+end_log -n "configure-$TARGET"
+
+if [ "$RET" -ne 0 ]; then
+ start_log -c -n "showlogs-$TARGET" "Configuring $TARGET failed, showing logs"
+ cat ${BUILDDIR}/config.log
+ end_log -n "showlogs-$TARGET"
+ exit $RET
+fi
+
+start_log -c -n "build-$TARGET" "Building $TARGET"
+make ${JOBS:+-j$JOBS} || RET=$?
+end_log -n "build-$TARGET"
+[ "$RET" -ne 0 ] && exit $RET
+
+start_log -c -n "install-$TARGET" "Installing $TARGET"
+make ${JOBS:+-j$JOBS} install || RET=$?
+end_log -n "install-$TARGET"
+exit $RET
diff --git a/scripts/ci/functions.sh b/scripts/ci/functions.sh
new file mode 100644
index 0..2f4cecaa1
--- /dev/null
+++ b/scripts/ci/functions.sh
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+TXT_RED="\e[31m"
+TXT_YELLOW="\e[33m"
+TXT_CLEAR="\e[0m"
+TXT_LOG_COLOR="\e[97;100m"
+
+function start_log() {
+ local LOG_COLLAPSE LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -c) LOG_COLLAPSE=1; shift;;
+ -n) LOG_NAME="$2"; shift;;
+ *) [ "$#" -eq 1 ] && LOG_MSG="$1"; shift;;
+esac
+ done
+
+ echo -e "Start:${LOG_NAME} ${LOG_MSG}"
+}
+
+function end_log() {
+ local LOG_NAME
+ while [ "$#" -gt 0 ]; do
+case "$1" in
+ -n) LOG_NAME=$2; shift;;
+ *) shift;;
+esac
+ done
+
+ echo -e "End:${LOG_NAME}"
+}
+
+:;
diff --git a/scripts/ci/make-images.sh b/scripts/ci/make-images.sh
new file mode 100755
index 0..3c5fd64f8
--- /dev/null
+++ b/scripts/ci/make-images.sh
@@ -0,0 +1,86 @@
+#!/bin/bash
+
+set -eo pipefail
+
+# Environment variables
+# SRCDIR: Path to source files
+# BUILDDIR: Directory in which to place the build
+# TARGET: Target to test
+# MAKE_ALL_IMAGE_TARGETS: If set to 'y', then all image targets, even disabled
+# ones, will be run.
+# DISABLED_IMAGES: String of target formats to disable when building grub
+# images delimited by new lines.
+# SHELL_TRACE: Set to 'y' to enable shell tracing
+# NUM_FAILED_LOG_LINES: Set to integer number of log lines to display from the
+# end of a failed log file
+
+if [ "x${SHELL_TRACE}" = "xy" ]; then
+ # If we export SHELL_OPTS, then all shells will be traced, most of which we
do not want
+ SHELL_OPTS=&
Re: [PATCH] bootstrap: When a commit hash is specified, do a shallow fetch if possible
On Fri, 22 Oct 2021 18:44:15 +0200
Daniel Kiper wrote:
> On Thu, Oct 21, 2021 at 12:49:19PM -0500, Glenn Washburn wrote:
> > The gnulib sources are large but more importantly have lots of changes. So
> > initial checkout of the repository can take a long time when network or
> > cpu resources are limited. The later is especially acute in a non-KVM QEMU
> > virtual machine (which can take 40+ minutes compared to <30 seconds with
> > this change[1]). The problem is specific to how GRUB uses gnulib, which is
> > by pegging the desired checkout to a specific git revision. In this case,
> > git can not do a shallow clone by using the --depth option because git does
> > not know ahead of time how deep the revision is from the tip. So git must
> > clone the whole repository.
> >
> > However, there is an alternate method that requires support from the git
> > server[2], namely by asking for a specific commit on fetch. Refactor to use
> > fetch and fallback to fetching the entire repository if fetching by commit
> > hash fails.
> >
> > Currently the git server hosting the official gnulib git repository does not
> > support fetch by commit hash[3]. However, there are mirrors which do support
> > this[4], and can be specified by setting the $GNULIB_URL.
This paragraph is now out of date, the git server configuration change
happened much more quickly than I expected. So before committing it
should be removed along with the last two references.
> >
> > [1] https://savannah.nongnu.org/support/index.php?110553#comment1
> > [2] https://stackoverflow.com/a/3489576/2108011
> > [3] https://savannah.nongnu.org/support/index.php?110553
> > [4] https://github.com/coreutils/gnulib
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > bootstrap | 16 ++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/bootstrap b/bootstrap
> > index 5b08e7e2d..914f911f8 100755
> > --- a/bootstrap
> > +++ b/bootstrap
> > @@ -665,9 +665,21 @@ if $use_gnulib; then
> >shallow=
> >if test -z "$GNULIB_REVISION"; then
> > git clone -h 2>&1 | grep -- --depth > /dev/null &&
> > shallow='--depth 2'
> > +git clone $shallow ${GNULIB_URL:-$default_gnulib_url}
> > "$gnulib_path" \
> > + || cleanup_gnulib
> > + else
> > +git fetch -h 2>&1 | grep -- --depth > /dev/null &&
> > shallow='--depth 2'
> > +mkdir -p "$gnulib_path"
> > +git -C "$gnulib_path" init
> > +git -C "$gnulib_path" remote add origin
> > ${GNULIB_URL:-$default_gnulib_url}
> > +# Can not do a shallow fetch if fetch by commit hash fails because
> > we
> > +# do not know how the to go to get to $GNULIB_REVISION, so we must
> > get
> > +# all commits.
> > +git -C "$gnulib_path" fetch $shallow origin "$GNULIB_REVISION" \
> > + || git -C "$gnulib_path" fetch origin \
> > + || cleanup_gnulib
> > +git -C "$gnulib_path" reset --hard FETCH_HEAD
>
> Is this hunk from gnulib upstream? If not I would prefer if you upstream
> it into gnulib. We should avoid custom patches for imported code as much
> as possible.
I agree in general. As mentioned in a follow up reply to this patch, I
did submit the patch upstream[1]. The only difference is the commit
message. It has not been responded to yet. However, even once accepted
upstream we'll have to cherry-pick it to the GRUB repo. Are you
thinking we'd need to get all changes to bootstrap at that time? It
doesn't look like there's anything that would be that useful (except
maybe using the https repo url). I'm curious how you'd like to handle
this.
I don't think the issue of a custom patch in this instance is a problem
though. Bootstrap very rarely changes, even in upstream (<10 changes in
the last 2 years). So there I'm not concerned about having to maintain
a lot of custom patches on top of it (like the situation of many
distros vis-a-vis GRUB). So I think if its not accepted upstream (I
don't see why it wouldn't be), I think we should use in GRUB because it
provides significant reduction in bandwith and a drastic reduction in
time to checkout on a qemu VM (but also significant not on a VM, 5min
vs 1min) depending on the CPU resources available. This is especially
useful for a CI/testing system that is frequently run.
I'm fine with waiting a few weeks and see what happens upstream.
Glenn
[1] https://lists.gnu.org/archive/html/bug-gnulib/2021-10/msg00052.html
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/4] Documentation improvements and fixes
These patches are pretty self explanatory. The first patch was requested by Daniel and Daniel suggested the link to the INSTALL file from the git web repo. Glenn Glenn Washburn (4): docs: Add sentence on where Debian packages can be searched for online docs: Update development docs to include information on running test suite docs: Fix broken links in development docs docs: Add documentation on packages for building documentation INSTALL| 12 ++-- docs/grub-dev.texi | 22 +- 2 files changed, 27 insertions(+), 7 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/4] docs: Fix broken links in development docs
Use the Git Book as a reference for documentation on Git as no other link
was provided. Other links were broken because they used @url instead of
@uref and needed a comma separator between link and link text.
Signed-off-by: Glenn Washburn
---
docs/grub-dev.texi | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
index 1beff25a7..3e05def1f 100644
--- a/docs/grub-dev.texi
+++ b/docs/grub-dev.texi
@@ -96,8 +96,8 @@ This edition documents version @value{VERSION}.
@node Getting the source code
@chapter Getting the source code
-GRUB is maintained using the @uref{GIT revision
-control system}. To fetch:
+GRUB is maintained using the @uref{https://git-scm.com/book/en/v2,
+GIT revision control system}. To fetch:
@example
git clone git://git.sv.gnu.org/grub.git
@@ -346,8 +346,8 @@ manual and try GRUB 2 out to see what you think is missing
from there.
Here are additional pointers:
@itemize
-@item @url{https://savannah.gnu.org/task/?group=grub GRUB's Task Tracker}
-@item @url{https://savannah.gnu.org/bugs/?group=grub GRUB's Bug Tracker}
+@item @uref{https://savannah.gnu.org/task/?group=grub, GRUB's Task Tracker}
+@item @uref{https://savannah.gnu.org/bugs/?group=grub, GRUB's Bug Tracker}
@end itemize
If you intended to make changes to GRUB Legacy (<=0.97) those are not accepted
@@ -461,7 +461,7 @@ and the FSF clerks have dealt with your copyright
assignment.
@section When you are approved for write access to project's files
As you might know, GRUB is hosted on
-@url{https://savannah.gnu.org/projects/grub Savannah}, thus the membership
+@uref{https://savannah.gnu.org/projects/grub, Savannah}, thus the membership
is managed by Savannah. This means that, if you want to be a member of this
project:
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/4] docs: Add sentence on where Debian packages can be searched for online
Signed-off-by: Glenn Washburn --- INSTALL | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/INSTALL b/INSTALL index 38d8d5a4d..8a2b617f3 100644 --- a/INSTALL +++ b/INSTALL @@ -5,7 +5,8 @@ This is the GRUB. Welcome. This file contains instructions for compiling and installing the GRUB. Where this document refers to packages names, they are named according to the -Debian 11 package repositories. +Debian 11 package repositories. These packages can be found by searching +https://packages.debian.org/. The Requirements -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/4] docs: Update development docs to include information on running test suite
Add a section with minimal description on setting up and running the test
suite with a link to the INSTALL documentation which is a little more
detailed in terms of package requirements.
Signed-off-by: Glenn Washburn
---
docs/grub-dev.texi | 12
1 file changed, 12 insertions(+)
diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
index fb2cc965e..1beff25a7 100644
--- a/docs/grub-dev.texi
+++ b/docs/grub-dev.texi
@@ -77,6 +77,7 @@ This edition documents version @value{VERSION}.
* Coding style::
* Finding your way around::
* Contributing Changes::
+* Setting up and running test suite::
* Updating External Code::
* Porting::
* Error Handling::
@@ -483,6 +484,17 @@ If your intention is to just get started, please do not
submit a inclusion
request. Instead, please subscribe to the mailing list, and communicate first
(e.g. sending a patch, asking a question, commenting on another message...).
+@node Setting up and running test suite
+@chapter Setting up and running test suite
+
+GRUB is basically a tiny operating system with read support for many file
+systems and which has been ported to a variety of architectures. As such, its
+test suite has quite a few dependencies required to fully run the suite.
+These dependencies are currently documented in the
+@uref{http://git.savannah.gnu.org/cgit/grub.git/tree/INSTALL, INSTALL}
+file in the source repository. Once installed, the test suite can be started
+by running the @command{make check} command from the GRUB build directory.
+
@node Updating External Code
@chapter Updating external code
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 4/4] docs: Add documentation on packages for building documentation
Signed-off-by: Glenn Washburn --- INSTALL | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/INSTALL b/INSTALL index 8a2b617f3..a64f63723 100644 --- a/INSTALL +++ b/INSTALL @@ -90,6 +90,10 @@ Note that `make check' will run and many tests may complete successfully with only a subset of these prerequisites. However, some tests may be skipped or fail due to missing prerequisites. +To build the documentation you'll need: +* texinfo, for the info and html documentation +* texlive, for building the dvi and pdf documentation (optional) + Configuring the GRUB @@ -146,7 +150,10 @@ The simplest way to compile this package is: 8. Type `make install' to install the programs and any data files and documentation. - 9. You can remove the program binaries and object files from the + 9. Type `make html' or `make pdf' to generate the html or pdf + documentation. Note, these are not built by default. + + 10. You can remove the program binaries and object files from the source code directory by typing `make clean'. To also remove the files that `configure' created (so you can compile the package for a different kind of computer), type `make distclean'. There is -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
On Wed, 17 Nov 2021 18:29:36 +0100
Daniel Kiper wrote:
> On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> > As an example, passing a password as a cryptomount argument is implemented.
>
> I am not very happy with that. Splitting this into separate patch or
> merging with patch #2 probably would not help either.
Its not clear to me what action you're desiring. I don't particularly
like it either, but haven't thought of something better. Ideas?
> > However, the backends are not implemented, so testing this will return a not
> > implemented error.
>
> The commit message lacks of explanation why this change is needed.
> I think you can copy part of the cover letter here.
I can add an explanation.
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 31 +--
> > grub-core/disk/geli.c | 6 +-
> > grub-core/disk/luks.c | 7 ++-
> > grub-core/disk/luks2.c | 7 ++-
> > include/grub/cryptodisk.h | 9 -
> > 5 files changed, 46 insertions(+), 14 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..577942088 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> > /* TRANSLATORS: It's still restricted to cryptodisks only. */
> > {"all", 'a', 0, N_("Mount all."), 0, 0},
> > {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> > +{"password", 'p', 0, N_("Password to open volumes."), 0,
> > ARG_TYPE_STRING},
>
> Should not you update docs/grub.texi as well?
Yep, good catch. I think the doc change should be in patch #2 because
that's where the option actually becomes useful. What do you think?
> > {0, 0, 0, 0, 0, 0}
> >};
> >
> > @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> > }
> >
> > static grub_err_t
> > -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> > +grub_cryptodisk_scan_device_real (const char *name,
> > + grub_disk_t source,
> > + grub_cryptomount_args_t cargs)
> > {
> >grub_err_t err;
> >grub_cryptodisk_t dev;
> > @@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> > grub_disk_t source)
> > if (!dev)
> >continue;
> >
> > -err = cr->recover_key (source, dev);
> > +err = cr->recover_key (source, dev, cargs);
> > if (err)
> > {
> >cryptodisk_close (dev);
> > @@ -1080,10 +1083,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,
> > const char *cheat)
> >
> > static int
> > grub_cryptodisk_scan_device (const char *name,
> > -void *data __attribute__ ((unused)))
> > +void *data)
> > {
> >grub_err_t err;
> >grub_disk_t source;
> > + grub_cryptomount_args_t cargs = data;
> >
> >/* Try to open disk. */
> >source = grub_disk_open (name);
> > @@ -1093,7 +1097,7 @@ grub_cryptodisk_scan_device (const char *name,
> >return 0;
> > }
> >
> > - err = grub_cryptodisk_scan_device_real (name, source);
> > + err = grub_cryptodisk_scan_device_real (name, source, cargs);
> >
> >grub_disk_close (source);
> >
> > @@ -1106,12 +1110,19 @@ static grub_err_t
> > grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> > {
> >struct grub_arg_list *state = ctxt->state;
> > + struct grub_cryptomount_args cargs = {0};
> >
> >if (argc < 1 && !state[1].set && !state[2].set)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
> >
> > + if (state[3].set) /* password */
> > +{
> > + cargs.key_data = (grub_uint8_t *) state[3].arg;
> > + cargs.key_len = grub_strlen (state[3].arg);
> > +}
> > +
> >have_it = 0;
> > - if (state[0].set)
> > + if (state[0].set) /* uuid */
>
> Nice but if you want to do that please put that change into separate patch.
> Or at least advise in the commit message you are doing this on occasion.
I'll add a sentence to the commit message.
> > {
> >grub_cryptodisk_t dev;
> >
> > @@ -1125,18 +1136,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt,
> > int argc,
Re: [PATCH v3 2/4] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
On Wed, 17 Nov 2021 20:10:21 +0100
Daniel Kiper wrote:
> On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > The crypto device modules should only be setting up the crypto devices and
> > not getting user input. This has the added benefit of simplifying the code
> > such that three essentially duplicate pieces of code are merged into one.
>
> Mostly nitpicks below...
>
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 52 ++---
> > grub-core/disk/geli.c | 26 ---
> > grub-core/disk/luks.c | 27 +++
> > grub-core/disk/luks2.c | 26 ---
> > include/grub/cryptodisk.h | 1 +
> > 5 files changed, 57 insertions(+), 75 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 577942088..a5f7b860c 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
> > grub_disk_t source,
> > grub_cryptomount_args_t cargs)
> > {
> > - grub_err_t err;
> > + grub_err_t ret = GRUB_ERR_NONE;
> >grub_cryptodisk_t dev;
> >grub_cryptodisk_dev_t cr;
> > + int askpass = 0;
> > + char *part = NULL;
> >
> >dev = grub_cryptodisk_get_by_source_disk (source);
> >
> > @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> >return grub_errno;
> > if (!dev)
> >continue;
> > -
> > -err = cr->recover_key (source, dev, cargs);
> > -if (err)
> > -{
> > - cryptodisk_close (dev);
> > - return err;
> > -}
> > +
> > +if (cargs->key_len == 0)
>
> I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
>
> > + {
> > + /* Get the passphrase from the user, if no key data. */
> > + askpass = 1;
> > + if (source->partition)
>
> ...but not for the pointers and enums.
>
> s/if (source->partition)/if (source->partition != NULL)/
>
> > + part = grub_partition_get_name (source->partition);
> > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > +source->partition ? "," : "", part ? : "",
>
> s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
>
> s/part ? : ""/part != NULL ? part : "NO NAME"/?
Ok, when moving code, I generally don't like to change it unless
necesary. I'll add these changes.
> > +dev->uuid);
> > + grub_free (part);
> > +
> > + cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > + if (!cargs->key_data)
>
> Ditto and below please...
>
> > + return grub_errno;
> > +
> > + if (!grub_password_get ((char *) cargs->key_data,
> > GRUB_CRYPTODISK_MAX_PASSPHRASE))
> > + {
> > + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
>
> All errors printed by grub_error() should start with lower case...
Ok, I'll try to keep that in mind. There's quite a few grub_error()
calls in cryptodisk that do not conform to that (and I expect else
where in the source).
> > + goto error;
> > + }
> > + cargs->key_len = grub_strlen ((char *) cargs->key_data);
> > + }
> > +
> > +ret = cr->recover_key (source, dev, cargs);
> > +if (ret)
>
> if (ret != GRUB_ERR_NONE)
>
> > + goto error;
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > have_it = 1;
> >
> > -return GRUB_ERR_NONE;
> > +goto cleanup;
> >}
> > - return GRUB_ERR_NONE;
> > + goto cleanup;
> > +
> > +error:
>
> Please put space before labels.
Are you saying you want the line to be " error:"? There are labels in
the source which are preceeded by whitespace, but they seem to be in
the minority. What's the rationale for this? or is it just personal
preference? I don't mind making this change, but I don't understand it.
>
> > + cryptodisk_close (dev);
>
> I would add empty line here.
>
> > +cleanup:
>
> Please put space before labels.
>
> > + if (askpass)
> > +{
> > + cargs->key_len = 0;
> > + grub_free (cargs->key_data);
>
Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
On Thu, 18 Nov 2021 15:06:56 +0100
Daniel Kiper wrote:
> On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 26 +-
> > grub-core/disk/geli.c | 9 -
> > grub-core/disk/luks.c | 11 +--
> > grub-core/disk/luks2.c | 6 +++---
> > include/grub/cryptodisk.h | 10 --
> > 5 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index a5f7b860c..5b38606ed 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >
> > #endif
> >
> > -static int check_boot, have_it;
> > -static char *search_uuid;
> > -
> > static void
> > cryptodisk_close (grub_cryptodisk_t dev)
> > {
> > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> >FOR_CRYPTODISK_DEVS (cr)
> >{
> > -dev = cr->scan (source, search_uuid, check_boot);
> > +dev = cr->scan (source, cargs);
> > if (grub_errno)
> >return grub_errno;
> > if (!dev)
> > @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > -have_it = 1;
> > +cargs->found_uuid = 1;
>
> Please say in the commit message you are changing variable/member name too.
> Or maybe it would be better if you do this in separate patch.
I'll see about moving the next patch such that this is not a concern.
> > goto cleanup;
> >}
> > @@ -1091,7 +1088,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,
> > const char *cheat)
> >
> >FOR_CRYPTODISK_DEVS (cr)
> >{
> > -dev = cr->scan (source, search_uuid, check_boot);
> > +dev = cr->scan (source, NULL);
> > if (grub_errno)
> >return grub_errno;
> > if (!dev)
> > @@ -1135,7 +1132,7 @@ grub_cryptodisk_scan_device (const char *name,
> >
> >if (err)
> > grub_print_error ();
> > - return have_it && search_uuid ? 1 : 0;
> > + return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
>
> I think this should be enough:
> return (cargs->found_uuid && cargs->search_uuid != NULL)
>
> > }
> >
> > static grub_err_t
> > @@ -1153,7 +1150,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> > argc, char **args)
> >cargs.key_len = grub_strlen (state[3].arg);
> > }
> >
> > - have_it = 0;
>
> cargs->found_uuid = 0?
>
> >if (state[0].set) /* uuid */
> > {
> >grub_cryptodisk_t dev;
> > @@ -1166,21 +1162,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt,
> > int argc, char **args)
> > return GRUB_ERR_NONE;
> > }
> >
> > - check_boot = state[2].set;
> > - search_uuid = args[0];
> > + cargs.check_boot = state[2].set;
> > + cargs.search_uuid = args[0];
> >grub_device_iterate (_cryptodisk_scan_device, );
> > - search_uuid = NULL;
>
> cargs.search_uuid = NULL?
The initializer used for the struct declaration should already take
care of this.
>
> > - if (!have_it)
> > + if (!cargs.found_uuid)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >return GRUB_ERR_NONE;
> > }
> >else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > {
> > - search_uuid = NULL;
> > - check_boot = state[2].set;
> > + cargs.check_boot = state[2].set;
> >grub_device_iterate (_cryptodisk_scan_device, );
> > - search_uuid = NULL;
>
> Ditto. If this is correct then I think it should be shortly explained in
> the commit message why you can drop these assignments.
Since the storage for search_uuid is not global anymore, the struct
initializer tkes care of this. I'll add a line in the commit message.
> >return GRUB_ERR_NONE;
> > }
> >else
> > @@ -1192,8 +1185,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> > argc, char **args)
> >char *disklast = NULL;
> >grub_size_t len;
> >
> > - search_uuid = NULL;
>
> Ditto.
>
> > - check_boot = state[2].set;
> > + cargs.check_boot = state[2].s
Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
On Sun, 14 Nov 2021 10:56:15 +0100
Patrick Steinhardt wrote:
> On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 26 +-
> > grub-core/disk/geli.c | 9 -
> > grub-core/disk/luks.c | 11 +--
> > grub-core/disk/luks2.c | 6 +++---
> > include/grub/cryptodisk.h | 10 --
> > 5 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index a5f7b860c..5b38606ed 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >
> > #endif
> >
> > -static int check_boot, have_it;
> > -static char *search_uuid;
> > -
> > static void
> > cryptodisk_close (grub_cryptodisk_t dev)
> > {
> > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> >FOR_CRYPTODISK_DEVS (cr)
> >{
> > -dev = cr->scan (source, search_uuid, check_boot);
> > +dev = cr->scan (source, cargs);
> > if (grub_errno)
> >return grub_errno;
> > if (!dev)
> > @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > -have_it = 1;
> > +cargs->found_uuid = 1;
> >
> > goto cleanup;
> >}
> > @@ -1091,7 +1088,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,
> > const char *cheat)
> >
> >FOR_CRYPTODISK_DEVS (cr)
> >{
> > -dev = cr->scan (source, search_uuid, check_boot);
> > +dev = cr->scan (source, NULL);
>
> If I didn't get this wrong, then all scan implementations
> unconditionally dereference the `grub_cryptomount_args_t` pointer.
> So why does this work, and shouldn't we pass down a struct which has the
> `search_uuid` and `check_boot` parameters properly set up?
I'm not sure that this does work, good catch. I don't have a setup to
test GRUB utils in conjunction with cryptodisk. If you do (or anyone
else does), testing would be greatly appreciated. Regardless, I believe
this is an issue.
My reading of the previous code is that search_uuid and check_boot are
not explicitly initialized here. The only reasonable conclusion I come
to is that they are initialized to zero by default by the compiler. So
I'll create a cargs struct with all fields initalized to zero and pass
that in. Does this sound good?
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt
Call grub_partition_get_name unconditionally to initialize the part
variable. Then part will only be NULL when grub_partition_get_name errors.
Note that when source->partition is NULL, then grub_partition_get_name
returns an allocated empty string. So no comma or partition will be printed,
as desired.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 99265097a..2381c3330 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1021,11 +1021,10 @@ grub_cryptodisk_scan_device_real (const char *name,
{
/* Get the passphrase from the user, if no key data. */
askpass = 1;
- if (source->partition != NULL)
- part = grub_partition_get_name (source->partition);
+ part = grub_partition_get_name (source->partition);
grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
source->partition != NULL ? "," : "",
-part != NULL ? part : "",
+part != NULL ? part : N_("UNKNOWN"),
dev->uuid);
grub_free (part);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded
This displays an error notifying the user that they'll want to load a
backend module to make cryptomount useful.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9df3d310f..27491871a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1125,6 +1125,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
+ if (grub_cryptodisk_list == NULL)
+return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
+
if (state[0].set)
{
int found_uuid;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
Previously, the cryptomount arguments were passed by global variable and
function call argument, neither of which are ideal. This change passes data
via a grub_cryptomount_args struct, which can be added to over time as
opposed to continually adding arguments to the cryptodisk scan and
recover_key.
As an example, passing a password as a cryptomount argument is implemented.
However, the backends are not implemented, so testing this will return a not
implemented error.
Also, add comments to cryptomount argument parsing to make it more obvious
which argument states are being handled.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 31 +--
grub-core/disk/geli.c | 6 +-
grub-core/disk/luks.c | 7 ++-
grub-core/disk/luks2.c | 7 ++-
include/grub/cryptodisk.h | 9 -
5 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index a5f1b0978..f1ccfbee3 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
/* TRANSLATORS: It's still restricted to cryptodisks only. */
{"all", 'a', 0, N_("Mount all."), 0, 0},
{"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
+{"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
{0, 0, 0, 0, 0, 0}
};
@@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
}
static grub_cryptodisk_t
-grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
+grub_cryptodisk_scan_device_real (const char *name,
+ grub_disk_t source,
+ grub_cryptomount_args_t cargs)
{
grub_err_t err;
grub_cryptodisk_t dev;
@@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_disk_t source)
if (!dev)
continue;
-err = cr->recover_key (source, dev);
+err = cr->recover_key (source, dev, cargs);
if (err)
{
cryptodisk_close (dev);
@@ -1080,11 +1083,12 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,
const char *cheat)
static int
grub_cryptodisk_scan_device (const char *name,
-void *data __attribute__ ((unused)))
+void *data)
{
int ret = 0;
grub_disk_t source;
grub_cryptodisk_t dev;
+ grub_cryptomount_args_t cargs = data;
grub_errno = GRUB_ERR_NONE;
/* Try to open disk. */
@@ -1095,7 +1099,7 @@ grub_cryptodisk_scan_device (const char *name,
return 0;
}
- dev = grub_cryptodisk_scan_device_real (name, source);
+ dev = grub_cryptodisk_scan_device_real (name, source, cargs);
if (dev)
{
ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid)
== 0);
@@ -1124,6 +1128,7 @@ static grub_err_t
grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
struct grub_arg_list *state = ctxt->state;
+ struct grub_cryptomount_args cargs = {0};
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
@@ -1131,7 +1136,13 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
if (grub_cryptodisk_list == NULL)
return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
- if (state[0].set)
+ if (state[3].set) /* password */
+{
+ cargs.key_data = (grub_uint8_t *) state[3].arg;
+ cargs.key_len = grub_strlen (state[3].arg);
+}
+
+ if (state[0].set) /* uuid */
{
int found_uuid;
grub_cryptodisk_t dev;
@@ -1146,7 +1157,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
check_boot = state[2].set;
search_uuid = args[0];
- found_uuid = grub_device_iterate (_cryptodisk_scan_device, NULL);
+ found_uuid = grub_device_iterate (_cryptodisk_scan_device, );
search_uuid = NULL;
if (found_uuid)
@@ -1163,11 +1174,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
}
return grub_errno;
}
- else if (state[1].set || (argc == 0 && state[2].set))
+ else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
search_uuid = NULL;
check_boot = state[2].set;
- grub_device_iterate (_cryptodisk_scan_device, NULL);
+ grub_device_iterate (_cryptodisk_scan_device, );
search_uuid = NULL;
return GRUB_ERR_NONE;
}
@@ -1208,7 +1219,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
return GRUB_ERR_NONE;
}
- dev = grub_cryptodisk_scan_device_real (diskname, disk);
+ dev = grub_cryptodisk_scan_device_real (diskname, disk, );
grub_disk_clo
[PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
The crypto device modules should only be setting up the crypto devices and
not getting user input. This has the added benefit of simplifying the code
such that three essentially duplicate pieces of code are merged into one.
Add documentation of passphrase option for cryptomount as it is now usable.
Signed-off-by: Glenn Washburn
---
docs/grub.texi | 9 --
grub-core/disk/cryptodisk.c | 56 +
grub-core/disk/geli.c | 26 -
grub-core/disk/luks.c | 27 +++---
grub-core/disk/luks2.c | 26 -
include/grub/cryptodisk.h | 1 +
6 files changed, 66 insertions(+), 79 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..e0a309574 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4307,13 +4307,16 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See
command @command{hashsum}
@node cryptomount
@subsection cryptomount
-@deffn Command cryptomount device|@option{-u} uuid|@option{-a}|@option{-b}
-Setup access to encrypted device. If necessary, passphrase
-is requested interactively. Option @var{device} configures specific grub device
+@deffn Command cryptomount [@option{-p} password] device|@option{-u}
uuid|@option{-a}|@option{-b}
+Setup access to encrypted device. If @option{-p} is not given, a passphrase
+is requested interactively. Otherwise, the given @var{password} will be used
and
+no passphrase will be requested interactively.
+Option @var{device} configures specific grub device
(@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
with specified @var{uuid}; option @option{-a} configures all detected encrypted
devices; option @option{-b} configures all geli containers that have boot flag
set.
+
GRUB suports devices encrypted using LUKS, LUKS2 and geli. Note that necessary
modules (@var{luks}, @var{luks2} and @var{geli}) have to be loaded manually
before this command can be used. For LUKS2 only the PBKDF2 key derivation
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f1ccfbee3..f8f6e3e25 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_disk_t source,
grub_cryptomount_args_t cargs)
{
- grub_err_t err;
+ grub_err_t ret = GRUB_ERR_NONE;
grub_cryptodisk_t dev;
grub_cryptodisk_dev_t cr;
+ int askpass = 0;
+ char *part = NULL;
dev = grub_cryptodisk_get_by_source_disk (source);
@@ -1017,21 +1019,53 @@ grub_cryptodisk_scan_device_real (const char *name,
return NULL;
if (!dev)
continue;
-
-err = cr->recover_key (source, dev, cargs);
-if (err)
-{
- cryptodisk_close (dev);
- return NULL;
-}
+
+if (!cargs->key_len)
+ {
+ /* Get the passphrase from the user, if no key data. */
+ askpass = 1;
+ if (source->partition != NULL)
+ part = grub_partition_get_name (source->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+source->partition != NULL ? "," : "",
+part != NULL ? part : "",
+dev->uuid);
+ grub_free (part);
+
+ cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+ if (cargs->key_data == NULL)
+ return NULL;
+
+ if (!grub_password_get ((char *) cargs->key_data,
GRUB_CRYPTODISK_MAX_PASSPHRASE))
+ {
+ grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
+ goto error;
+ }
+ cargs->key_len = grub_strlen ((char *) cargs->key_data);
+ }
+
+ret = cr->recover_key (source, dev, cargs);
+if (ret != GRUB_ERR_NONE)
+ goto error;
grub_cryptodisk_insert (dev, name, source);
-return dev;
+goto cleanup;
}
-
grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this
device");
- return NULL;
+ goto cleanup;
+
+ error:
+ cryptodisk_close (dev);
+ dev = NULL;
+
+ cleanup:
+ if (askpass)
+{
+ cargs->key_len = 0;
+ grub_free (cargs->key_data);
+}
+ return dev;
}
#ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 777da3a05..7299a47d1 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -135,8 +135,6 @@ const char *algorithms[] = {
[0x16] = "aes"
};
-#define MAX_PASSPHRASE 256
-
static gcry_err_code_t
geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno)
{
@@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev,
grub_cryptomount_args_t
grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN];
grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE];
grub_uint8_t geli_cipher_key[64];
- char passphr
[PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct
Note that cargs.search_uuid does not need to be initialized in various parts
of the cryptomount argument parsing, just once when cargs is declared with a
struct initializer. The previous code used a global variable which would
retain the value across cryptomount invocations.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 24 +---
grub-core/disk/geli.c | 9 -
grub-core/disk/luks.c | 9 -
grub-core/disk/luks2.c | 8
include/grub/cryptodisk.h | 9 +++--
5 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f8f6e3e25..99265097a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
#endif
-static int check_boot;
-static char *search_uuid;
-
static void
cryptodisk_close (grub_cryptodisk_t dev)
{
@@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
FOR_CRYPTODISK_DEVS (cr)
{
-dev = cr->scan (source, search_uuid, check_boot);
+dev = cr->scan (source, cargs);
if (grub_errno)
return NULL;
if (!dev)
@@ -1077,6 +1074,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const
char *cheat)
grub_cryptodisk_t dev;
grub_cryptodisk_dev_t cr;
grub_disk_t source;
+ struct grub_cryptomount_args cargs = {0};
/* Try to open disk. */
source = grub_disk_open (sourcedev);
@@ -1093,7 +1091,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const
char *cheat)
FOR_CRYPTODISK_DEVS (cr)
{
-dev = cr->scan (source, search_uuid, check_boot);
+dev = cr->scan (source, );
if (grub_errno)
return grub_errno;
if (!dev)
@@ -1136,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
dev = grub_cryptodisk_scan_device_real (name, source, cargs);
if (dev)
{
- ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid)
== 0);
+ ret = (cargs->search_uuid != NULL && grub_strcasecmp
(cargs->search_uuid, dev->uuid) == 0);
goto cleanup;
}
@@ -1147,7 +1145,7 @@ grub_cryptodisk_scan_device (const char *name,
if (grub_errno == GRUB_ERR_BAD_MODULE)
grub_error_pop ();
- if (search_uuid != NULL)
+ if (cargs->search_uuid != NULL)
/* Push error onto stack to save for cryptomount */
grub_error_push ();
else
@@ -1189,10 +1187,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
return GRUB_ERR_NONE;
}
- check_boot = state[2].set;
- search_uuid = args[0];
+ cargs.check_boot = state[2].set;
+ cargs.search_uuid = args[0];
found_uuid = grub_device_iterate (_cryptodisk_scan_device, );
- search_uuid = NULL;
if (found_uuid)
return GRUB_ERR_NONE;
@@ -1210,10 +1207,8 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
}
else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
grub_device_iterate (_cryptodisk_scan_device, );
- search_uuid = NULL;
return GRUB_ERR_NONE;
}
else
@@ -1224,8 +1219,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
char *disklast = NULL;
grub_size_t len;
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
diskname = args[0];
len = grub_strlen (diskname);
if (len && diskname[0] == '(' && diskname[len - 1] == ')')
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 7299a47d1..23789c43f 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
#endif
static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int boot_only)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t newdev;
struct grub_geli_phdr header;
@@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
+ if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) &
GRUB_GELI_FLAGS_BOOT))
{
grub_dprintf ("geli", "not a boot volume\n");
return NULL;
@@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid)
!= 0)
{
- grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
+
[PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message
When a cryptmount is specified with a UUID, but no cryptodisk backends find a disk with that UUID, return a more detailed message giving telling the user that they might not have a needed cryptobackend module loaded. Signed-off-by: Glenn Washburn --- grub-core/disk/cryptodisk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c index f71bc62eb..a5f1b0978 100644 --- a/grub-core/disk/cryptodisk.c +++ b/grub-core/disk/cryptodisk.c @@ -1159,7 +1159,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args) */ grub_error_pop (); if (grub_errno == GRUB_ERR_NONE) - return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found"); + return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found, perhaps a needed disk or cryptodisk module is not loaded"); } return grub_errno; } -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations
Update such that "cryptomount -u UUID" will not print two error messages
when an invalid passphrase is given and the most relevant error message
will be displayed.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 27491871a..f71bc62eb 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1109,7 +1109,10 @@ grub_cryptodisk_scan_device (const char *name,
if (grub_errno == GRUB_ERR_BAD_MODULE)
grub_error_pop ();
- if (grub_errno != GRUB_ERR_NONE)
+ if (search_uuid != NULL)
+/* Push error onto stack to save for cryptomount */
+grub_error_push ();
+ else
grub_print_error ();
cleanup:
@@ -1146,9 +1149,19 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
found_uuid = grub_device_iterate (_cryptodisk_scan_device, NULL);
search_uuid = NULL;
- if (!found_uuid)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
- return GRUB_ERR_NONE;
+ if (found_uuid)
+ return GRUB_ERR_NONE;
+ else if (grub_errno == GRUB_ERR_NONE)
+ {
+ /*
+ * Try to pop the next error on the stack. If there is not one, then
+ * no device matched the given UUID.
+ */
+ grub_error_pop ();
+ if (grub_errno == GRUB_ERR_NONE)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk
found");
+ }
+ return grub_errno;
}
else if (state[1].set || (argc == 0 && state[2].set))
{
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules
Signed-off-by: Glenn Washburn
---
grub-core/disk/luks2.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..fea196dd4 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -370,7 +370,10 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int
check_boot)
uuid[j] = '\0';
if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-return NULL;
+{
+ grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
+ return NULL;
+}
cryptodisk = grub_zalloc (sizeof (*cryptodisk));
if (!cryptodisk)
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules
Updates since v4: * Rework patch #2 to (hopefully) be easier to understand * Add more commentary to patch #2 commit message * Split previous patch #3 into three separate patches --- This patch series refactors the way cryptomount passes data to the crypto modules. Currently, the method has been by global variable and function call argument, neither of which are ideal. This method passes data via a grub_cryptomount_args struct, which can be added to over time as opposed to continually adding arguments to the cryptodisk recover_key (as is being proposed in the keyfile and detached header patches). To make thing simpler and easier to understand, the "have_it" global variable is gotten rid of first in patch #2. Taking advantage of this change, patches #3-5 improve some long standing issues in cryptomount error messaging. Then, the infrastructure for passing argument data to cryptodisk backends is implemented in patch #6 along with adding a new -p parameter to cryptomount partly as an example to show how a password would be passed to the crypto module backends. The backends do nothing with this data in this patch, but print a message saying that sending a password is unimplemented. Patch #7 takes advantage of this new data passing mechanism to refactor the essentially duplicated code in each crypto backend module for inputting the password and puts that functionality in the cryptodisk code. Conceptually, the crypto backends should not be getting user input anyway. Patch #8 gets rid of some long time globals in cryptodisk, moving them into the passed struct. Patch #9 improves handling of partition name in cryptomount password prompt. 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). I also believe tha this will somewhat simplify the patch series by James Bottomley in passing secrets to the crypto backends. Glenn Glenn Washburn (9): luks2: Add debug message to align with luks and geli modules cryptodisk: Refactor to discard have_it global cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded cryptodisk: Improve error messaging in cryptomount invocations cryptodisk: Improve cryptomount -u error message cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules cryptodisk: Refactor password input out of crypto dev modules into cryptodisk cryptodisk: Move global variables into grub_cryptomount_args struct cryptodisk: Improve handling of partition name in cryptomount password prompt docs/grub.texi | 9 +- grub-core/disk/cryptodisk.c | 164 +--- grub-core/disk/geli.c | 35 +++- grub-core/disk/luks.c | 37 +++- grub-core/disk/luks2.c | 38 - include/grub/cryptodisk.h | 19 - 6 files changed, 175 insertions(+), 127 deletions(-) Range-diff against v4: 1: 0ae554743 < -: - cryptodisk: Refactor to discard have_it global -: - > 1: 5056163ca cryptodisk: Refactor to discard have_it global -: - > 2: 224d7f9bc cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded 2: d5040065e ! 3: a0dfaeb5a cryptodisk: Improve error messaging in cryptomount invocations @@ Commit message when an invalid passphrase is given and the most relevant error message will be displayed. -Improve error message which is displayed when a UUID is specified, but no -cryptodisk backends find a disk with that UUID. - -Also, make cryptomount return failure when no cryptodisk modules are loaded, -which allows an error to be displayed notifying the user that they'll want -to load a backend module to make cryptomount useful. - ## grub-core/disk/cryptodisk.c ## @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name, + if (grub_errno == GRUB_ERR_BAD_MODULE) + grub_error_pop (); - grub_disk_close (source); - -- /* -- * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many unhelpful -- * error messages. -- */ -- if (err != GRUB_ERR_NONE && err != GRUB_ERR_EXISTS && err != GRUB_ERR_BAD_MODULE) -+ if (err == GRUB_ERR_NONE || err == GRUB_ERR_EXISTS) -+; /* Success, skip the error handling */ -+ else if (err == GRUB_ERR_BAD_MODULE) -+/* Do nothing to avoid printing unhelpful error messages */ -+grub_errno = GRUB_ERR_NONE; -+ else if (cargs->search_uuid != NULL) +- if (grub_errno != GRUB_ERR_NONE) ++ if (search_uuid != NULL) +/* Push error onto stack to save for cryptomount */ -+grub_error_push(); ++grub_error_push (); + else grub_print_error (); - retur
[PATCH v5 2/9] cryptodisk: Refactor to discard have_it global
The global "have_it" was never used by the crypto-backends, but was used to
determine if a crypto-backend successfully mounted a cryptodisk with a given
uuid. This is not needed however, because grub_device_iterate() will return
1 if and only if grub_cryptodisk_scan_device() returns 1. And
grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
been specified and a cryptodisk was successfully setup by a crypto-backend or
a cryptodisk of the requested uuid is already open.
To implement this grub_cryptodisk_scan_device_real is modified to return a
cryptodisk or NULL on failure and having the appropriate grub_errno set to
indicated failure. Note that grub_cryptodisk_scan_device_real will fail now
with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
modules succeed in identifying the source disk.
With this change grub_device_iterate() will return 1 when a crypto device is
successfully decrypted or when the source device has already been
successfully opened. Prior to this change, trying to mount an already
successfully opened device would trigger an error with the message "no such
cryptodisk found", which is at best misleading. The mount should silently
succeed in this case, which is what happens with this patch.
Signed-off-by: Glenn Washburn
---
grub-core/disk/cryptodisk.c | 56 +++--
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..9df3d310f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
#endif
-static int check_boot, have_it;
+static int check_boot;
static char *search_uuid;
static void
@@ -995,7 +995,7 @@ cryptodisk_close (grub_cryptodisk_t dev)
grub_free (dev);
}
-static grub_err_t
+static grub_cryptodisk_t
grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
{
grub_err_t err;
@@ -1005,13 +1005,13 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_disk_t source)
dev = grub_cryptodisk_get_by_source_disk (source);
if (dev)
-return GRUB_ERR_NONE;
+return dev;
FOR_CRYPTODISK_DEVS (cr)
{
dev = cr->scan (source, search_uuid, check_boot);
if (grub_errno)
- return grub_errno;
+ return NULL;
if (!dev)
continue;
@@ -1019,16 +1019,16 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_disk_t source)
if (err)
{
cryptodisk_close (dev);
- return err;
+ return NULL;
}
grub_cryptodisk_insert (dev, name, source);
-have_it = 1;
-
-return GRUB_ERR_NONE;
+return dev;
}
- return GRUB_ERR_NONE;
+
+ grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this
device");
+ return NULL;
}
#ifdef GRUB_UTIL
@@ -1082,8 +1082,10 @@ static int
grub_cryptodisk_scan_device (const char *name,
void *data __attribute__ ((unused)))
{
- grub_err_t err;
+ int ret = 0;
grub_disk_t source;
+ grub_cryptodisk_t dev;
+ grub_errno = GRUB_ERR_NONE;
/* Try to open disk. */
source = grub_disk_open (name);
@@ -1093,13 +1095,26 @@ grub_cryptodisk_scan_device (const char *name,
return 0;
}
- err = grub_cryptodisk_scan_device_real (name, source);
+ dev = grub_cryptodisk_scan_device_real (name, source);
+ if (dev)
+{
+ ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid)
== 0);
+ goto cleanup;
+}
- grub_disk_close (source);
-
- if (err)
+ /*
+ * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many unhelpful
+ * error messages.
+ */
+ if (grub_errno == GRUB_ERR_BAD_MODULE)
+grub_error_pop ();
+
+ if (grub_errno != GRUB_ERR_NONE)
grub_print_error ();
- return have_it && search_uuid ? 1 : 0;
+
+ cleanup:
+ grub_disk_close (source);
+ return ret;
}
static grub_err_t
@@ -1110,9 +1125,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
- have_it = 0;
if (state[0].set)
{
+ int found_uuid;
grub_cryptodisk_t dev;
dev = grub_cryptodisk_get_by_uuid (args[0]);
@@ -1125,10 +1140,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
argc, char **args)
check_boot = state[2].set;
search_uuid = args[0];
- grub_device_iterate (_cryptodisk_scan_device, NULL);
+ found_uuid = grub_device_iterate (_cryptodisk_scan_device, NULL);
search_uuid = NULL;
- if (!have_it)
+ if (!found_uuid)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
return GRUB_ERR_NONE;
}
@@ -1142,7 +1157,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
Re: [PATCH v2] misc: Allow selective disabling of debug facility names
On Tue, 7 Dec 2021 22:01:14 +0100 Michael Schierl wrote: > Hello Glenn, > > > Am 07.12.2021 um 10:17 schrieb Glenn Washburn: > > >> maybe move the conditional before the assignment of ret? > > > > I'm understanding you to be suggesting to move the assignment of ret to > > after the if statement that follows it. > > Correct. > > > The only point I see is saving > > an assignment in the case that debug=all. Is there more to it? > > No. It just bugged me to do a definitely never used assignment directly > before returning. Feel free to ignore it. I'll think about this. Originally, I didn't think it looked as nice, but on second look it seems fine. > > Yep, this is an issue. It was intentional when I had the code only > > running in the case "all" was present, in which case skipping the > > condition if its the at the start of $debug is fine because the > > condition is already enabled. After changing to run also without "all" > > being present, this becomes an issue. > > If "all" still was present I would probably have complained about > initializing it to +1 instead of +3 as the first three characters cannot > be a match if they are "all". :) Yes, but I didn't want to assume that "all" is the first item in the list. I decided that just the presence of an "all" item any where in the list of conditionals enables all conditionals at the beginning of processing. So debug="-btrfs,all" is the same as debug="all,-btrfs" and means all of the debug conditionals are turned on except btrfs. I decided that it doesn't make as much sense to have the position of "all" matter, and effectively be a macro for all conditionals, because at that point why append ",all,...", just set debug to "all,..." erasing the previous contents. I'm open to suggestions on what the preferred way to handle this might be. We could say that "all" must appear at the beginning, but that seems unnecessary and overly restrictive. What's less clear to me is what situation I might want to put have "all" not at the beginning of the string. I'm thinking there might be one, which is why I'm leaving that as an option, but maybe I'm wrong. > > > I was using grub_iswordseparator in the original patch, but decided > > against it this round because the documentation states that separators > > are whitespace or comma. > > Fair point. I don't think there are any common cases where this > backward-incompatible change really matters. Yeah, and its undocumented that other seperators would work. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] misc: Allow selective disabling of debug facility names
On Tue, 7 Dec 2021 23:07:32 +0100 Michael Schierl wrote: > > Hello Glenn, > > > Am 07.12.2021 um 22:59 schrieb Glenn Washburn: > > Yes, but I didn't want to assume that "all" is the first item in the > > list. > > More important, "all" does not have to be present at all. > > DEBUG=fat,btrfs,-fat > > should be perfectly legal (and it is). Agreed. > > I decided that it doesn't make as much sense to have the position of > > "all" matter, and effectively be a macro for all conditionals, because > > at that point why append ",all,...", just set debug to "all,..." > > erasing the previous contents. > > That is a fair point. Same applies to "-all" (just erase everything in > the string before it). > > Instead of > > DEBUG=btrfs,all,fat,-all,ext2 > > probably the user just wanted to enter > > DEBUG=ext2 Yep. > > I'm open to suggestions on what the preferred way to handle this might > > be. We could say that "all" must appear at the beginning, but that > > seems unnecessary and overly restrictive. What's less clear to me is > > what situation I might want to put have "all" not at the beginning of > > the string. I'm thinking there might be one, which is why I'm leaving > > that as an option, but maybe I'm wrong. > > I cannot think of any sensible one that would not confuse the user, like > your example with > > DEBUG=-btrfs,all,-fat > > But I don't think we have to put too much effort in it, especially since > the code should also work in case "all" is not present at all. The other thing is that the original code allows "all" to be any item in the string. So not allowing that would break backward-compatibility (maybe not a big deal though). And I also agree that it can be confusing. Maybe Daniel has an opinion. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe
On Thu, 9 Dec 2021 18:38:51 +0100 Josselin Poiret via Grub-devel wrote: > Hello, > > These two draft patches make devmapper set up LUKS2 cryptomount > properties when pulling, as well as report LUKS2 cryptomounts as > having GRUB_DEV_ABSTRACTION_LUKS. This makes grub-probe and > grub-install behave properly wrt. LUKS2 drives: `grub-probe -t > abstraction /` reports all the needed modules for the GRUB image, and > grub-install leads to a working GRUB without manually adding modules. > > One small part that I am unsure about, although I have tested it and > it does seem to work properly: if I understand correctly, all dm > devices have a 512 sector size, however LUKS2 lets one choose up to > 4096 for the encryption sector size. Which of these two should be > used as cryptodisk->sector_size? I put 512 here since we're reading > through a cheated mount, but I'm not so sure. Its not clear to me, did you test a LUKS2 device with sector size 4096 with this change? I believe DM does use 512-byte sectors internally, but it can create block devices that report and use other sector sizes. You can verfiy this by creating a 4096 sector size LUKS2 devices, open it with cryptsetup, and then run "blockdev --getbsz /dev/mapper/". When having a 4096 byte sector size LUKS2 device opened via cyptsetup, here's what dmsetup table --show returns "sector_size:4096" as part of the output for the device. I'm not familiar with this code, but I'm thinking tht might show up in the "params" variable for you to use when setting log_sector_size. I have a feeling that this is not going to work as is with non-512-byte sector size LUKS2 devices. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] tests: Refactor building xorriso command for iso9660 tests
The iso9660 tests test creating isos with different combinations of joliet,
rockridge, and iso9660 conformance level. Refactor xorriso argument
generation for more readability and extensibility.
Signed-off-by: Glenn Washburn
---
tests/util/grub-fs-tester.in | 43 +---
1 file changed, 20 insertions(+), 23 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 2dcd09f5e..959d0694d 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -1027,30 +1027,27 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
xorriso -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -R -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img" --
-add /="$MASTER" -- -zisofs default -set_filter_r --zisofs / -- ;;
- x"iso9660")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"joliet")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_joliet")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"iso9660_1999")
+ x"iso9660" | xjoliet | xrockridge | xrockridge_joliet |
x"iso9660_1999" | xjoliet_1999 | xrockridge_1999 | xrockridge_joliet_1999)
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"joliet_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_joliet_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
+ XORRISO_ARGS="-as mkisofs $XORRISOFS_CHARSET -graft-points"
+
+ if [ -z "${fs##*rockridge*}" ]; then
+ XORRISO_ARGS="-rockridge on $XORRISO_ARGS"
+ else
+ XORRISO_ARGS="-rockridge off $XORRISO_ARGS"
+ fi
+
+ if [ -z "${fs##*1999*}" ]; then
+ XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
+ else
+ XORRISO_ARGS="$XORRISO_ARGS -iso-level 3"
+ fi
+
+ if [ -z "${fs##*joliet*}" ]; then
+ XORRISO_ARGS="$XORRISO_ARGS -J -joliet-long"
+ fi
+
+ xorriso -compliance rec_mtime $XORRISO_ARGS -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
x"romfs")
genromfs -V "$FSLABEL" -f "${FSIMAGEP}0.img" -d "$MASTER" ;;
xsquash4_*)
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] configure: Fix misspelled variable BUILD_LDFAGS -> BUILD_LDFLAGS
Signed-off-by: Glenn Washburn --- I've been wondering why my build has been strangely failing for the last couple months and finally found the culprit. Quite an annoyance. I guess no one else actually uses BUILD_LDFLAGS. Glenn --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 8d1c81a73..4f649edaf 100644 --- a/configure.ac +++ b/configure.ac @@ -1649,7 +1649,7 @@ CC="$BUILD_CC" CPP="$BUILD_CPP" CFLAGS="$BUILD_CFLAGS" CPPFLAGS="$BUILD_CPPFLAGS" -LDFLAGS="$BUILD_LDFAGS" +LDFLAGS="$BUILD_LDFLAGS" unset ac_cv_c_bigendian unset ac_cv_header_ft2build_h -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 3/7] cryptodisk: Improve error messaging in cryptomount invocations
On Wed, 8 Dec 2021 17:41:32 +0100 Daniel Kiper wrote: > On Sat, Dec 04, 2021 at 01:15:46AM -0600, Glenn Washburn wrote: > > Update such that "cryptomount -u UUID" will not print two error messages > > when an invalid passphrase is given and the most relevant error message > > will be displayed. > > > > Improve error message which is displayed when a UUID is specified, but no > > cryptodisk backends find a disk with that UUID. > > > > Also, make cryptomount return failure when no cryptodisk modules are loaded, > > which allows an error to be displayed notifying the user that they'll want > > to load a backend module to make cryptomount useful. > > Is it possible to split this patch into 3 separate patches? I think the first and last hunks will be difficult to untangle, which is part of what I expect you're asking for. The commit message has three sections, which can be broken in to three patches easily, but two of the patches would be fairly trivial (one is the second hunk and the other is changing an error string to be more descriptive. Can you clarify that I've understood you correctly and if not, give more detail in what you're looking for? Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
On Wed, 8 Dec 2021 17:37:19 +0100
Daniel Kiper wrote:
> On Sat, Dec 04, 2021 at 01:15:45AM -0600, Glenn Washburn wrote:
> > The global "have_it" was never used by the crypto-backends, but was used to
> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate() will return
> > 1 if and only if grub_cryptodisk_scan_device() returns 1. And
> > grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
> > been specified and a cryptodisk was successfully setup by a crypto-backend.
> >
> > With this change grub_device_iterate() will return 1 when a crypto device is
> > successfully decrypted or when the source device has already been
> > successfully opened. Prior to this change, trying to mount an already
> > successfully opened device would trigger an error with the message "no such
> > cryptodisk found", which is at best misleading. The mount should silently
> > succeed in this case, which is what happens with this patch.
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 29 ++---
> > include/grub/err.h | 1 +
> > 2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..9224105ac 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >
> > #endif
> >
> > -static int check_boot, have_it;
> > +static int check_boot;
> > static char *search_uuid;
> >
> > static void
> > @@ -1005,7 +1005,12 @@ grub_cryptodisk_scan_device_real (const char *name,
> > grub_disk_t source)
> >dev = grub_cryptodisk_get_by_source_disk (source);
> >
> >if (dev)
> > -return GRUB_ERR_NONE;
> > +{
> > + if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> > + return GRUB_ERR_NONE;
> > + else
> > + return GRUB_ERR_EXISTS;
>
> Hmmm... This looks strange. Could you explain why you return
> GRUB_ERR_EXISTS if UUIDs do not match?
I've re-defined success (that is return == GRUB_ERR_NONE) in
grub_cryptodisk_scan_device_real to be such that, for the case where we
are looking for a particular UUID, either source we're given matches
that UUID is already opened or we successfully open it. If a UUID is
not being searched for, then the criteria is essentially the same,
except for the UUID check.
When searching, GRUB_ERR_EXISTS is returned when the source is
associated with an unlocked crypto device, but is not the UUID that is
being searched for. This in turn tells grub_cryptodisk_scan_device to
not return true (ie. do not stop searching for the requested UUID).
Looking at this again, I'm thinking it might be clearer if
grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
either opens the source or source is associated with an already opened
crypto device, and otehrwise return NULL. If the caller receives a
non-NULL, and does the UUID check itself, if it cares/needs to.
I'm also noticing that at a minimum, I think the if statement with the
UUID check needs to be updated to "search_uuid == NULL ||
grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
like being sent a NULL pointer. Though, I'm confused why this didn't
crash in my testing.
> > +}
> >
> >FOR_CRYPTODISK_DEVS (cr)
> >{
> > @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name,
> > grub_disk_t source)
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > -have_it = 1;
> > -
> > return GRUB_ERR_NONE;
> >}
> > - return GRUB_ERR_NONE;
> > + return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle
> > this device");
>
> Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?
If this return is reached, it means that all cryptodisk backend modules
(eg. luks, luks2, geli) have tried unsuccessfully to open source. We
need to return an error here does that grub_cryptodisk_scan_device does
not confuse a success here with meaning that the source was
successfully opened.
This was not needed before because "have_it" was being used to
determine if source was or has been successfully opened. With these
changes it is not the return code of grub_cryptodisk_scan_device_real
that is being used for this.
> > }
> >
> > #ifdef GRUB_UTIL
> > @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
> >
Re: [PATCH] tests: Refactor building xorriso command for iso9660 tests
On Wed, 08 Dec 2021 09:11:07 +0100
"Thomas Schmitt" wrote:
> Hi,
>
> i think this change is beneficial for the maintainability of the test.
>
> But this sequence looks a bit confusing, albeit it is ok on the second
> glimpse:
>
> + XORRISO_ARGS="-as mkisofs $XORRISOFS_CHARSET
> -graft-points"
> +
> + if [ -z "${fs##*rockridge*}" ]; then
> + XORRISO_ARGS="-rockridge on $XORRISO_ARGS"
> + else
> + XORRISO_ARGS="-rockridge off $XORRISO_ARGS"
> + fi
> +
> + if [ -z "${fs##*1999*}" ]; then
> + XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
> + else
> + XORRISO_ARGS="$XORRISO_ARGS -iso-level 3"
> + fi
>
> It is essential here, but not really obvious, that the native command
> -rockridge "on"|"off" must be _prepended_ to XORRISO_ARGS whereas the
> mkisofs emulation options must be _appended_ to the variable content.
>
> If not, then the xorriso run would fail:
>
> $ xorriso -as mkisofs -rockridge on
> ...
> xorriso : FAILURE : -as mkisofs: Unrecognized option '-rockridge'
> xorriso : aborting : -abort_on 'FAILURE' encountered 'FAILURE'
>
> $ xorriso -iso-level 4 -as mkisofs
> ...
> xorriso : FAILURE : Not a known command: '-iso-level'
>
> xorriso : FAILURE : Not a known command: '4'
>
> xorriso : aborting : -abort_on 'FAILURE' encountered 'FAILURE'
> $
>
> It would be more intuitive to build XORRISO_ARGS in the sequence that
> will be seen by xorriso.
>
> So consider to pull -compliance "rec_mtime" into XORRISO_ARGS too, and
> to set the native commands before the -as "mkisofs" command.
> Like:
>
>XORRISO_ARGS="-compliance rec_mtime"
>
>if [ -z "${fs##*rockridge*}" ]; then
>XORRISO_ARGS="$XORRISO_ARGS -rockridge on"
>else
>XORRISO_ARGS="$XORRISO_ARGS -rockridge off"
>fi
>
>XORRISO_ARGS="$XORRISO_ARGS -as mkisofs $XORRISOFS_CHARSET
> -graft-points"
>
>if [ -z "${fs##*1999*}" ]; then
>XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
>else
>XORRISO_ARGS="$XORRISO_ARGS -iso-level 3"
>fi
>
>if [ -z "${fs##*joliet*}" ]; then
>XORRISO_ARGS="$XORRISO_ARGS -J -joliet-long"
>fi
>
>xorriso $XORRISO_ARGS -V "$FSLABEL"
> --modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
> /="$MASTER" ;;
>
>
> Have a nice day :)
>
> Thomas
>
Thanks for the suggestion Thomas. I've updated the patch to add this.
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] tests: Refactor building xorriso command for iso9660 tests
The iso9660 tests test creating isos with different combinations of joliet,
rockridge, and iso9660 conformance level. Refactor xorriso argument
generation for more readability and extensibility.
Signed-off-by: Glenn Washburn
---
Updates since v1:
* Reorder such that command line arguments are built in order, per Thomas'
suggestion
---
tests/util/grub-fs-tester.in | 45 ++--
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index 2dcd09f5e..aa72b2174 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -1027,30 +1027,29 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
x"ziso9660")
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
xorriso -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -R -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img" --
-add /="$MASTER" -- -zisofs default -set_filter_r --zisofs / -- ;;
- x"iso9660")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"joliet")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_joliet")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 3 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"iso9660_1999")
+ x"iso9660" | xjoliet | xrockridge | xrockridge_joliet |
x"iso9660_1999" | xjoliet_1999 | xrockridge_1999 | xrockridge_joliet_1999)
FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"joliet_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge off -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
- x"rockridge_joliet_1999")
- FSUUID=$(date -u +%Y-%m-%d-%H-%M-%S-00);
- xorriso --rockridge on -compliance rec_mtime -as mkisofs
$XORRISOFS_CHARSET -iso-level 4 -graft-points -J -joliet-long -V "$FSLABEL"
--modification-date=$(echo ${FSUUID} | sed 's/-//g;') -o "${FSIMAGEP}0.img"
/="$MASTER" ;;
+ XORRISO_ARGS="-compliance rec_mtime"
+
+ if [ -z "${fs##*rockridge*}" ]; then
+ XORRISO_ARGS="$XORRISO_ARGS -rockridge on"
+ else
+ XORRISO_ARGS="$XORRISO_ARGS -rockridge off"
+ fi
+
+ XORRISO_ARGS="$XORRISO_ARGS -as mkisofs $XORRISOFS_CHARSET
-graft-points"
+
+ if [ -z "${fs##*1999*}" ]; then
+ XORRISO_ARGS="$XORRISO_ARGS -iso-level 4"
+ else
+ XORRISO_ARGS="$XORRISO_ARGS -iso-l
[PATCH v3] misc: Allow selective disabling of debug facility names
Sometimes you know only know which debug logging facility names you want to
turn off, not necessarily all the ones you want enabled. This patch allows
the debug string to contain facility names in the $debug variable which are
prefixed with a "-" to disable debug log messages for that conditional. Say
you want all debug logging on except for btrfs and scripting, then do:
"set debug=all,-btrfs,-scripting"
Note, that only the last occurence of the facility name with or without a
leading "-" is considered. So simply appending ",-facilityname" to the
$debug variable will disable that conditional. To illustrate, the command
"set debug=all,-btrfs,-scripting,btrfs" will enable btrfs.
Also, add documentation explaining this new behavior.
Signed-off-by: Glenn Washburn
---
Changes since v2:
* Fix issue where a facility at the start of the string wouldn't be matched
---
Range-diff against v2:
1: ef2662d0b ! 1: 31439a7fb misc: Allow selective disabling of debug facility
names
@@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
-return 1;
+ if (grub_strword (debug, "all"))
+{
-+ ret = 1;
+ if (debug[3] == '\0')
+ return 1;
++ ret = 1;
+}
- return 0;
+ clen = grub_strlen (condition);
-+ found = debug;
++ found = debug-1;
+ while(1)
+{
+ found = grub_strstr (found+1, condition);
@@ grub-core/kern/misc.c: __attribute__ ((alias("grub_printf")));
+ continue;
+
+ /*
-+ * If found condition is prefixed with '-' and the start is on a
word
++ * If found condition is at the start of debug, then enable debug.
Else
++ * if found condition is prefixed with '-' and the start is on a
word
+ * boundary, then disable debug. Otherwise, if the start is on a
word
-+ * boundary, enable debug. If neither, ignore.
++ * boundary, enable debug. If none of these cases, ignore.
+ */
-+ if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
++ if (found == debug)
++ ret = 1;
++ else if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2)
== ','
+ || grub_isspace (*(found-2)
+ ret = 0;
+ else if (*(found-1) == ',' || grub_isspace (*(found-1)))
docs/grub.texi| 13 ++---
grub-core/kern/misc.c | 43 +++
2 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..d13aa6600 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3388,9 +3388,16 @@ processed by commands @command{configfile}
(@pxref{configfile}) or @command{norm
@subsection debug
This variable may be set to enable debugging output from various components
-of GRUB. The value is a list of debug facility names separated by
-whitespace or @samp{,}, or @samp{all} to enable all available debugging
-output. The facility names are the first argument to grub_dprintf. Consult
+of GRUB. The value is an ordered list of debug facility names separated by
+whitespace or @samp{,}. If the special facility names @samp{all} is present
+then debugging output of all facility names is enabled at the start of
+processing the value of this variable. A facility's debug output can then be
+disabled by prefixing its name with a @samp{-}. The last occurence facility
+name with or without a leading @samp{-} takes precendent over any previous
+occurence. This allows the easy enabling or disabling of facilities by
+appending a @samp{,} and then the facility name with or without the leading
+@samp{-}, which will preserve the state of the rest of the facilities.
+The facility names are the first argument to grub_dprintf. Consult the
source for more details.
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 11b8592c8..fda69fe00 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -162,16 +162,51 @@ __attribute__ ((alias("grub_printf")));
int
grub_debug_enabled (const char * condition)
{
- const char *debug;
+ const char *debug, *found;
+ grub_size_t clen;
+ int ret = 0;
debug = grub_env_get ("debug");
if (!debug)
return 0;
- if (grub_strword (debug, "all") || grub_strword (debug, condition))
-return 1;
+ if (grub_strword (debug, "all"))
+{
+ if (debug[3] == '\0')
+ return 1;
+ ret = 1;
+}
- return 0;
+ clen = grub_strlen (condition);
+ found = debug-1;
+ while(1)
+{
+ found = grub_strstr (found+1, condition);
+
+ if (found == NULL)
+ break;
+
+ /* Found condition is not a whole word, so ignore it */
+ if (*(found + clen) != '\0' && *(found + clen) != ','
+&& !grub_isspace (*(found + clen)
Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args
On Thu, 18 Nov 2021 15:25:44 +0100
Daniel Kiper wrote:
> On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> > The member found_uuid was never used by the crypto-backends, but was used to
>
> Ha! Could you make this patch second in this patch series? Then we could
> avoid carrying over have_it/found_uuid cruft over succeeding patches.
Sure, I was avoiding do that work, but since you've requested it, I'll
take a stab at it. I'm thinking I'll make it the first patch though, so
its independent from the rest of the series.
> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate will return 1
> > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
>
> s/iff/if/
"iff" is short hand for "if and only if". I'll expand it.
> > will only return 1 if a search_uuid has been specified and a cryptodisk was
> > successfully setup by a crypto-backend. So the return value of
> > grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
> > exception of the case where a mount is requested or an already opened
> > crypto device.
> >
> > With this change grub_device_iterate will return 1 when
>
> I like if you add "()" to function names in comments, etc.
>
> > grub_cryptodisk_scan_device when a crypto device is successfully decrypted
>
> I think one "when" is redundant here. Or something else is wrong.
Indeed, I'll fix this.
> > or when the source device has already been successfully opened. Prior to
> > this change, trying to mount an already successfully opened device would
> > trigger an error with the message "no such cryptodisk found", which is at
> > best misleading. The mount should silently succeed in this case, which is
> > what happens with this patch.
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/cryptodisk.c | 9 -
> > include/grub/cryptodisk.h | 1 -
> > 2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 5b38606ed..8e5277314 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > -cargs->found_uuid = 1;
> > -
> > goto cleanup;
> >}
> >goto cleanup;
> > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
> >
> >if (err)
> > grub_print_error ();
> > - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > + return (!err && cargs->search_uuid) ? 1 : 0;
>
> err == GRUB_ERR_NONE && cargs->search_uuid != NULL
>
> > }
> >
> > static grub_err_t
> > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> > argc, char **args)
> >
> >if (state[0].set) /* uuid */
> > {
> > + int found_uuid = 0;
>
> Zero initialization seems redundant here.
It is. I'll remove the initializer.
> >grub_cryptodisk_t dev;
> >
> >dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> > argc, char **args)
> >
> >cargs.check_boot = state[2].set;
> >cargs.search_uuid = args[0];
> > - grub_device_iterate (_cryptodisk_scan_device, );
> > + found_uuid = grub_device_iterate (_cryptodisk_scan_device,
> > );
> >
> > - if (!cargs.found_uuid)
> > + if (!found_uuid)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >return GRUB_ERR_NONE;
> > }
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] misc: Allow selective disabling of debug facility names
Hi Michael,
Thanks for taking a look at this.
On Mon, 6 Dec 2021 22:01:22 +0100
Michael Schierl wrote:
>
> Hello Glenn,
>
> Comments below, note that I did not test the patch so maybe I am missing
> something.
>
> Am 06.12.2021 um 18:03 schrieb Glenn Washburn:
>
> > grub_debug_enabled (const char * condition)
> > {
> > - const char *debug;
> > + const char *debug, *found;
> > + grub_size_t clen;
> > + int ret = 0;
> >
> > debug = grub_env_get ("debug");
> > if (!debug)
> > return 0;
> >
> > - if (grub_strword (debug, "all") || grub_strword (debug, condition))
> > -return 1;
> > + if (grub_strword (debug, "all"))
> > +{
> > + ret = 1;
> > + if (debug[3] == '\0')
> > + return 1;
>
> maybe move the conditional before the assignment of ret?
I'm understanding you to be suggesting to move the assignment of ret to
after the if statement that follows it. The only point I see is saving
an assignment in the case that debug=all. Is there more to it?
> > +}
> >
> > - return 0;
> > + clen = grub_strlen (condition);
> > + found = debug;
> > + while(1)
> > +{
> > + found = grub_strstr (found+1, condition);
>
> Off by one error: in case the condition is the first one in debug, it
> won't be found. And after fixing this...
Yep, this is an issue. It was intentional when I had the code only
running in the case "all" was present, in which case skipping the
condition if its the at the start of $debug is fine because the
condition is already enabled. After changing to run also without "all"
being present, this becomes an issue.
> > +
> > + if (found == NULL)
> > + break;
> > +
> > + /* Found condition is not a whole word, so ignore it */
> > + if (*(found + clen) != '\0' && *(found + clen) != ','
> > +&& !grub_isspace (*(found + clen)))
> > + continue;
> > +
> > + /*
> > + * If found condition is prefixed with '-' and the start is on a word
> > + * boundary, then disable debug. Otherwise, if the start is on a word
> > + * boundary, enable debug. If neither, ignore.
> > + */
> > + if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
> > + || grub_isspace (*(found-2)
>
> you will read beyond the start of the string here.
Yep, that's why I had the +1 above to account for this.
> > + ret = 0;
> > + else if (*(found-1) == ',' || grub_isspace (*(found-1)))
>
> What about the other separators in grub_iswordseparator besides comma?
I was using grub_iswordseparator in the original patch, but decided
against it this round because the documentation states that separators
are whitespace or comma.
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] http module is not checking correctly HTTP headers
On Thu, 13 Jan 2022 23:14:54 +0100
Javier Moragon wrote:
> I'm sorry, it's my first time using a mailing list for publishing
> patches so I sent you the message directly instead of to grub-devel
> and my mail client wrapped the patch lines. I attached the patch
> Instead of pasting the diff, I hope this time the lines don't get
> wrapped.
Its okay, I had a few hiccups my first several times sending patches to
the list. Sending the patch as an attachment does fix the line wrapping
issue, but it creates another one. Its makes it more difficult for
reviewers to review it. This list is accustomed to having the patch
sent inline (in the text of the email). The advantage of this is that
reviewers get to comment inline on specific lines of the patch (like I
did for your original patch). Most people use git-format-patch and
git-send for creating and sending patches to the list.
Also, I've noticed that you changed the indentation of the second change
of this latest patch. Could you please create a new patch with the
indention exactly as it was? Your editor is probably changing the
indentation with out you realizing it. To be clear, the "sizeof" should
be preceeded by a string of 2 tabs followed by 2 spaces.
Glenn
>
> El jue, 13 ene 2022 a las 5:08, Glenn Washburn
> () escribió:
> >
> > On Wed, 12 Jan 2022 23:54:58 +0100
> > Javier Moragon wrote:
> >
> > > According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> > > shall be case insensitive and we are now forced to read headers like
> > > `Content-Length` capitalized.
> > >
> > > The problem with that is when a HTTP server responds with a
> > > `content-length` header in lowercase GRUB gets stuck because HTTP
> > > module doesn't know the length of the transmision and the call never
> > > ends. I've been able to reproduce it and after ignoring the text case
> > > it worked perfectly.
> > >
> > > Here is it my patch proposal:
> > >
> > > diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> > > index b616cf40b..570fa3934 100644
> > > --- a/grub-core/net/http.c
> > > +++ b/grub-core/net/http.c
> > > @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >data->first_line_recv = 1;
> > >return GRUB_ERR_NONE;
> > > }
> > > - if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ")
> > > - 1)
> > > + if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> > > ("Content-Length: ") )
> >
> > I don't think there should be a new line here. And why change to
> > grub_strlen? Now the length is calculated everytime at runtime instead
> > of once at compile time.
> >
> > >== 0 && !data->size_recv)
> > > {
> > >ptr += sizeof ("Content-Length: ") - 1;
> > > @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> > > char *ptr, grub_size_t len)
> > >data->size_recv = 1;
> > >return GRUB_ERR_NONE;
> > > }
> > > - if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> > > -sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> > > + if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> > > +grub_strlen ("Transfer-Encoding: chunked") ) == 0)
> >
> > Ditto on the grub_strlen. I also don't like the original indentation of
> > this line and think that it should align with "ptr".
> >
> > > {
> > >data->chunked = 1;
> > >return GRUB_ERR_NONE;
> >
> > Otherwise, it seems like good patch.
> >
> > Glenn
> >
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] http module is not checking correctly HTTP headers
On Fri, 14 Jan 2022 00:05:59 +0100
Jamo wrote:
> From: Javier Moragon
>
> I applied the last suggestion in this patch and I've finally realized
> how to use git send-email.
>
> I'm sorry for the unnecesary mails and I hope for next contributions
> I won't make the same mistakes, Thank you!
No worries, we all had to learn at some point. There are still a couple
issues. One is that this space is for the commit message. For instance,
based on your first email it might be:
According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
shall be case insensitive and we are now forced to read headers like
`Content-Length` capitalized.
The problem with that is when a HTTP server responds with a
`content-length` header in lowercase GRUB gets stuck because HTTP
module doesn't know the length of the transmision and the call never
ends.
Another issue is that when you update a patch, you should update its
version number. See the "-v" option in git-format-patch. Also, the list
convention is to start a new thread when submitting an updated patch.
So no need to do have an "In-Reply-To" header on the next update.
>
> ---
If you want to have some text that will not get recorded in the commit,
like the text after your email above, you can put that here.
> grub-core/net/http.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..7a56ec7ef 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data, char
> *ptr, grub_size_t len)
>data->first_line_recv = 1;
>return GRUB_ERR_NONE;
> }
> - if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> + if (grub_strncasecmp (ptr, "Content-Length: ", sizeof ("Content-Length: ")
> - 1)
>== 0 && !data->size_recv)
> {
>ptr += sizeof ("Content-Length: ") - 1;
> @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data, char
> *ptr, grub_size_t len)
>data->size_recv = 1;
>return GRUB_ERR_NONE;
> }
> - if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> -sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> + if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> +sizeof ("Transfer-Encoding: chunked") - 1) == 0)
Almost there. The code style guidelines for this project use a TAB
character for every 8 spaces. It looks like you used all spaces to do
the indent.
> {
>data->chunked = 1;
>return GRUB_ERR_NONE;
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] tests: Add check-native and check-nonnative make targets
This allows for testing only tests that run directly on the build machine or
only tests that run in a virtualized environment. When testing multiple
targets on the same build machine the native tests only need to be run once
for all targets. Whereas, the nonnative tests must be run for each target
because the test is potentially compiled differently for each target.
Signed-off-by: Glenn Washburn
---
Makefile.am | 9 +++
Makefile.util.def| 164 +--
conf/Makefile.common | 4 ++
gentpl.py| 6 +-
4 files changed, 97 insertions(+), 86 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index bf9c1ba64..10faf670b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -24,6 +24,15 @@ CCASFLAGS_PROGRAM += $(CCASFLAGS_GNULIB)
include $(srcdir)/Makefile.util.am
+check_SCRIPTS = $(check_SCRIPTS_native) $(check_SCRIPTS_nonnative)
+check_PROGRAMS = $(check_PROGRAMS_native) $(check_PROGRAMS_nonnative)
+TESTS = $(check_SCRIPTS) $(check_PROGRAMS)
+
+check-native:
+ $(MAKE) TESTS="$(check_PROGRAMS_native) $(check_SCRIPTS_native)" check
+check-nonnative:
+ $(MAKE) TESTS="$(check_PROGRAMS_nonnative) $(check_SCRIPTS_nonnative)"
check
+
# XXX Use Automake's LEX & YACC support
grub_script.tab.h: $(top_srcdir)/grub-core/script/parser.y
$(YACC) -d -p grub_script_yy -b grub_script
$(top_srcdir)/grub-core/script/parser.y
diff --git a/Makefile.util.def b/Makefile.util.def
index f8b356cc1..b098d5bba 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -749,470 +749,470 @@ script = {
};
script = {
- testcase;
+ testcase = native;
name = ext234_test;
common = tests/ext234_test.in;
};
script = {
- testcase;
+ testcase = native;
name = squashfs_test;
common = tests/squashfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = iso9660_test;
common = tests/iso9660_test.in;
};
script = {
- testcase;
+ testcase = native;
name = hfsplus_test;
common = tests/hfsplus_test.in;
};
script = {
- testcase;
+ testcase = native;
name = ntfs_test;
common = tests/ntfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = reiserfs_test;
common = tests/reiserfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = fat_test;
common = tests/fat_test.in;
};
script = {
- testcase;
+ testcase = native;
name = minixfs_test;
common = tests/minixfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = xfs_test;
common = tests/xfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = f2fs_test;
common = tests/f2fs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = nilfs2_test;
common = tests/nilfs2_test.in;
};
script = {
- testcase;
+ testcase = native;
name = romfs_test;
common = tests/romfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = exfat_test;
common = tests/exfat_test.in;
};
script = {
- testcase;
+ testcase = native;
name = tar_test;
common = tests/tar_test.in;
};
script = {
- testcase;
+ testcase = native;
name = udf_test;
common = tests/udf_test.in;
};
script = {
- testcase;
+ testcase = native;
name = hfs_test;
common = tests/hfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = jfs_test;
common = tests/jfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = btrfs_test;
common = tests/btrfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = zfs_test;
common = tests/zfs_test.in;
};
script = {
- testcase;
+ testcase = native;
name = cpio_test;
common = tests/cpio_test.in;
};
script = {
- testcase;
+ testcase = native;
name = example_scripted_test;
common = tests/example_scripted_test.in;
};
script = {
- testcase;
+ testcase = native;
name = gettext_strings_test;
common = tests/gettext_strings_test.in;
extra_dist = po/exclude.pot;
};
script = {
- testcase;
+ testcase = nonnative;
name = pata_test;
common = tests/pata_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = ahci_test;
common = tests/ahci_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = uhci_test;
common = tests/uhci_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = ohci_test;
common = tests/ohci_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = ehci_test;
common = tests/ehci_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = example_grub_script_test;
common = tests/example_grub_script_test.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = grub_script_eval;
common = tests/grub_script_eval.in;
};
script = {
- testcase;
+ testcase = nonnative;
name = grub_script_test;
common = tests/grub_script_test.in;
};
script = {
[PATCH 0/2] Add check-native and check-nonnative as make targets
Tests can be put into two categories, native (tests that run on the build system) and non-native (tests run in QEMU). For any two targets (even of completely different architectures), the native tests will be running the same binary code (because they will be compiled for and run on the build machine), and thus will have the same result. So when building and running tests for multiple targets on a build machine, the native tests on need be run once. This can decrease the runtime of a multi-target test run significantly (like hours). This patch series makes it possible to run only run the non-native tests (skipping the native tests) by partitioning the set of tests into native and non-native based on whether QEMU is used by the test. The first patch is not necessary, but makes things look cleaner. Phcoder, could you confirm that the logic above is sound? Glenn Glenn Washburn (2): conf/Makefile.common: Order alphabetically variables tests: Add check-native and check-nonnative make targets Makefile.am | 9 +++ Makefile.util.def| 164 +-- conf/Makefile.common | 22 +++--- gentpl.py| 6 +- 4 files changed, 106 insertions(+), 95 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] Fix a couple issues in moddep parsing
The first patch fixes an OOB read bug and the second outputs a less confusing error to the user when the moddep.lst line is too long. Really it would be better to support lines of unlimited length, but I'm not motivated to add that. The condition under which these issues are triggered should never really happen because no module (currently) has enough dependencies to generate such long lines in moddep.lst. I was triggering this under some odd conditions where the all_video module dependency line contained all grub modules. So I think having a max length for moddep.lst lines is reasonable at this point. Glenn Glenn Washburn (2): util/resolve.c: Do not read past the end of the array in read_dep_list util/resolve.c: Bail with error if moddep lst file line is too long util/resolve.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] util/resolve.c: Bail with error if moddep lst file line is too long
The code reads each line into a buffer of size 1024 and does not check if
the line is longer. So a line longer than 1024 will be read as a valid line
followed by an invalid line. Then an error confusing to the user is sent
with the test "invalid line format". But the line format is prefectly fine,
the problem is in GRUB's parser. Check if we've hit a line longer than the
size of the buffer, and if so send a more correct and reasonable error.
Signed-off-by: Glenn Washburn
---
util/resolve.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/util/resolve.c b/util/resolve.c
index 5e9afa10c..b0f2661f7 100644
--- a/util/resolve.c
+++ b/util/resolve.c
@@ -127,6 +127,9 @@ read_dep_list (FILE *fp)
mod->next = dep->list;
dep->list = mod;
}
+
+ if ((p - buf) == sizeof (buf))
+ grub_util_error (_("line too long, length greater than %lu: module
%s"), sizeof (buf), dep->name);
}
return dep_list;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] util/resolve.c: Do not read past the end of the array in read_dep_list
If the last non-NULL byte of 'buf' is not a white-space character (such as
when a read line is longer than the size of 'buf'), then 'p' will eventually
point to the byte after the last byte in 'buf'. After which 'p' will be
dereferenced in the while conditional leading to an out of bounds read. Make
sure that 'p' is inside 'buf' before dereferencing it.
Signed-off-by: Glenn Washburn
---
util/resolve.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/resolve.c b/util/resolve.c
index 3e887d2ff..5e9afa10c 100644
--- a/util/resolve.c
+++ b/util/resolve.c
@@ -102,7 +102,7 @@ read_dep_list (FILE *fp)
dep_list = dep;
/* Add dependencies. */
- while (*p)
+ while (p < (buf + sizeof (buf)) && *p)
{
struct mod_list *mod;
char *name;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/2] Make build more robust
I've found these two patches to be necessary under certain build conditions that I've not been able to narrow down to a specific cause. I suspect it is related to the values of some build environment variables (like *CFLAGS). Either way, these patches allow a successful build finishes without error and where the test suite succeeds. So I believe these patches are allowing a usable build. Under normal conditions, these changes should be superflous and thus not affect the build process. Glenn Glenn Washburn (2): gentpl.py: Fix issue where sometimes marker files have CPP defines Makefile: Only look for @MARKER@ at the start of a line when generating libgrub_a_init.lst Makefile.am | 4 ++-- gentpl.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] gentpl.py: Fix issue where sometimes marker files have CPP defines
When generating video.lst, modules whose marker file contains the string
VIDEO_LIST_MARKER are selected. But when the marker file contains the CPP
defines, one of the defines is VIDEO_LIST_MARKER and is present in all
marker files, so they are all selected. By removing the defines, the correct
modules are selected.
Signed-off-by: Glenn Washburn
---
gentpl.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gentpl.py b/gentpl.py
index 28ec24209..9f51e4fb6 100644
--- a/gentpl.py
+++ b/gentpl.py
@@ -700,7 +700,7 @@ def module(defn, platform):
output("""
""" + name + """.marker: $(""" + cname(defn) + """_SOURCES) $(nodist_""" +
cname(defn) + """_SOURCES)
$(TARGET_CPP) -DGRUB_LST_GENERATOR $(CPPFLAGS_MARKER) $(DEFS)
$(DEFAULT_INCLUDES) $(INCLUDES) $(""" + cname(defn) + """_CPPFLAGS) $(CPPFLAGS)
$^ > $@.new || (rm -f $@; exit 1)
- grep 'MARKER' $@.new > $@; rm -f $@.new
+ grep 'MARKER' $@.new | grep -v '^#' > $@; rm -f $@.new
""")
def kernel(defn, platform):
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/5] Various test fixes and improvements
I believe the patches are fairly self explantatory. Glenn Glenn Washburn (5): tests: Do not remove image file on error in pata_test tests: Skip pata_test on i386-efi tests: Remove $((BASE#NUM)) bashism in grub-fs-tester tests: Ensure that mountpoints are unmounted before exiting tests: Ensure that loopback devices and zfs devices are cleaned up tests/pata_test.in | 4 ++- tests/util/grub-fs-tester.in | 57 +++- 2 files changed, 52 insertions(+), 9 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/5] tests: Skip pata_test on i386-efi
In comparison to other i386 targets, on i386-efi the Q35 QEMU machine type
is used to do the testing to be able to make use of the EFI firmware in
QEMU. On the Q35 machine type there is no way to use ATA to communicate with
an IDE, only AHCI.
Signed-off-by: Glenn Washburn
---
tests/pata_test.in | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/pata_test.in b/tests/pata_test.in
index 27dccec19..31144a8fd 100644
--- a/tests/pata_test.in
+++ b/tests/pata_test.in
@@ -29,6 +29,9 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
# PLATFORM: no ATA on ARC platforms (they use SCSI)
*-arc)
exit 77;;
+# QEMU: no ATA on Q35 machine type (they use AHCI)
+i386-efi)
+ exit 77;;
# FIXME: No native drivers are available for those
powerpc-ieee1275 | sparc64-ieee1275 | arm*-efi)
exit 77;;
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 4/5] tests: Ensure that mountpoints are unmounted before exiting
When all tests complete successfully, filesystems mounted by grub-fs-tester
will be unmounted before exiting. However, on certain test failures the
tester will exit with a failure code and not unmount previously mounted
filesystems. Now keep track of mounts and umounts and run an exit handler
on exit or process interruption that will umount all mounts that haven't
already been unmounted.
Signed-off-by: Glenn Washburn
---
tests/util/grub-fs-tester.in | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index a1f3f299b..b39831f27 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -13,6 +13,17 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` ||
# FSLABEL. This is especially needed for the conversion to Joliet UCS-2.
XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
+MOUNTS=
+umount_all() {
+for MOUNT in $MOUNTS; do
+ umount "$MOUNT" &&
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MOUNT||g;")"
+done
+}
+trap umount_all EXIT INT
+# This is for bash, dash and ash do not recognize ERR
+trap umount_all ERR || :
+
# This wrapper is to ease insertion of valgrind or time statistics
run_it () {
LC_ALL=C "$GRUBFSTEST" "$@"
@@ -934,6 +945,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE"
1); do
done
exit 99;
fi
+ MOUNTS="$MOUNTS $MNTPOINTRW"
;;
esac
case x"$fs" in
@@ -1058,11 +1070,13 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
x"bfs")
sleep 1
fusermount -u "$MNTPOINTRW"
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
;;
xlvm*)
sleep 1
for try in $(range 0 20 1); do
if umount "$MNTPOINTRW" ; then
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
break;
fi
sleep 1;
@@ -1075,6 +1089,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
sleep 1
for try in $(range 0 20 1); do
if umount "$MNTPOINTRW" ; then
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
break;
fi
sleep 1;
@@ -1087,6 +1102,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
sleep 1
for try in $(range 0 20 1); do
if umount "$MNTPOINTRW" ; then
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
break;
fi
sleep 1;
@@ -1116,13 +1132,19 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
xlvm*)
vgchange -a y grub_test
sleep 1
- mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro ;;
+ mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro
+ MOUNTS="$MOUNTS $MNTPOINTRO"
+ ;;
xmdraid*)
mdadm --assemble /dev/md/"${fs}_$NDEVICES" $LODEVICES
sleep 1
- mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro ;;
+ mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro
+ MOUNTS="$MOUNTS $MNTPOINTRO"
+ ;;
*)
- mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro ;;
+ mount -t "$MOUNTFS" "${MOUNTDEVICE}" "$MNTPOINTRO" -o
${MOUNTOPTS}${SELINUXOPTS}ro
+ MOUNTS="$MOUNTS $MNTPOINTRO"
+ ;;
esac
run_grubfstest ls -- -la
@@ -1547,6 +1569,8 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
sleep 1
umount "$MNTPOINTRO" || true
umount "$MNTPOINTRW" || true
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRO||g;")"
+ MOUNTS="$(echo ${MOUNTS} | sed "s|$MNTPOINTRW||g;")"
esac
sleep 1
case x"$fs" in
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/2] conf/Makefile.common: Order alphabetically variables
Signed-off-by: Glenn Washburn --- conf/Makefile.common | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/conf/Makefile.common b/conf/Makefile.common index 2a1a886f6..f0bb6e160 100644 --- a/conf/Makefile.common +++ b/conf/Makefile.common @@ -102,24 +102,24 @@ MODULE_FILES = MARKER_FILES = KERNEL_HEADER_FILES = -man_MANS = -noinst_DATA = -pkgdata_DATA = bin_SCRIPTS = -sbin_SCRIPTS = bin_PROGRAMS = -platform_DATA = -sbin_PROGRAMS = check_SCRIPTS = -dist_grubconf_DATA = check_PROGRAMS = +dist_grubconf_DATA = +dist_noinst_DATA = +grubconf_SCRIPTS = +man_MANS = +noinst_DATA = noinst_SCRIPTS = noinst_PROGRAMS = -grubconf_SCRIPTS = noinst_LIBRARIES = -dist_noinst_DATA = +pkgdata_DATA = +platform_DATA = platform_SCRIPTS = platform_PROGRAMS = +sbin_SCRIPTS = +sbin_PROGRAMS = TESTS = EXTRA_DIST = -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/2] Makefile: Only look for @MARKER@ at the start of a line when generating libgrub_a_init.lst
Under certain conditions libgrub.pp gets generated with a such that it contains a bunch of CPP defines, at least one of which contains "@MARKER@". This line should not be used when generating libgrub_a_init.lst, otherwise we get compiler errors like: libgrub_a_init.c:22:18: error: stray ‘#’ in program 22 | extern void grub_#define_init (void); | ^ libgrub_a_init.c:22:19: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘define_init’ 22 | extern void grub_#define_init (void); | ^~~ libgrub_a_init.c:23:18: error: stray ‘#’ in program 23 | extern void grub_#define_fini (void); | ^ libgrub_a_init.c:23:19: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘define_fini’ 23 | extern void grub_#define_fini (void); | ^~~ ... When generating libgrub_a_init.lst only lines starting with "@MARKER@" are desired. Signed-off-by: Glenn Washburn --- Makefile.am | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index 10faf670b..81a196cdc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -52,7 +52,7 @@ libgrub.pp: config-util.h grub_script.tab.h grub_script.yy.h $(libgrubmods_a_SOU CLEANFILES += libgrub.pp libgrub_a_init.lst: libgrub.pp - cat $< | grep '@MARKER@' | sed 's/@MARKER@\(.*\)@/\1/g' | sort -u > $@ || (rm -f $@; exit 1) + cat $< | grep '^@MARKER@' | sed 's/@MARKER@\(.*\)@/\1/g' | sort -u > $@ || (rm -f $@; exit 1) CLEANFILES += libgrub_a_init.lst libgrub_a_init.c: libgrub_a_init.lst $(top_srcdir)/geninit.sh @@ -66,7 +66,7 @@ grub_fstest.pp: $(grub_fstest_SOURCES) CLEANFILES += grub_fstest.pp grub_fstest_init.lst: libgrub.pp grub_fstest.pp - cat $^ | grep '@MARKER@' | sed 's/@MARKER@\(.*\)@/\1/g' | sort -u > $@ || (rm -f $@; exit 1) + cat $^ | grep '^@MARKER@' | sed 's/@MARKER@\(.*\)@/\1/g' | sort -u > $@ || (rm -f $@; exit 1) CLEANFILES += grub_fstest_init.lst grub_fstest_init.c: grub_fstest_init.lst $(top_srcdir)/geninit.sh -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/5] tests: Remove $((BASE#NUM)) bashism in grub-fs-tester
This bashism allows converting NUM in base BASE to decimal. Its not needed
because the only place its used is to convert from hexidecimal and this can
also be done with the more portable $((0xHEXNUM)) syntax.
Signed-off-by: Glenn Washburn
---
tests/util/grub-fs-tester.in | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index aa72b2174..a1f3f299b 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -459,7 +459,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE"
1); do
# FIXME: not so sure about AFFS
# OS LIMITATION: minix2/minix3 could be formatted in a way
to permit more.
x"minix3" | x"minix2" | x"hfs"| x"affs" | xaffs_intl |
xreiserfs_old | xext2_old)
- BIGBLOCKCNT=$((16#7fff));;
+ BIGBLOCKCNT=$((0x7fff));;
# FS LIMITATION: redundant storage
# We have only limited space. Mirroring multiplies it very
effectively.
@@ -669,18 +669,18 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE"
"$MAXLOGSECSIZE" 1); do
# mkfs.hfs and mkfs.hfsplus don't fill UUID.
x"hfsplus")
"mkfs.hfsplus" -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
- dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1
seek=$((16#468)) conv=notrunc count=8 ;;
+ dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468))
conv=notrunc count=8 ;;
x"hfsplus_wrap")
"mkfs.hfsplus" -w -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
- dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1
seek=$((16#468)) conv=notrunc count=8
+ dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468))
conv=notrunc count=8
MOUNTFS="hfsplus";;
x"hfsplus_casesens")
"mkfs.hfsplus" -s -b $BLKSIZE -v "$FSLABEL" "${MOUNTDEVICE}"
- dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1
seek=$((16#468)) conv=notrunc count=8
+ dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x468))
conv=notrunc count=8
MOUNTFS="hfsplus";;
x"hfs")
"mkfs.hfs" -b $BLKSIZE -v "`echo $FSLABEL |recode
utf8..macroman`" -h "${MOUNTDEVICE}"
- dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1
seek=$((16#474)) conv=notrunc count=8
+ dd if=/dev/urandom of="${MOUNTDEVICE}" bs=1 seek=$((0x474))
conv=notrunc count=8
MOUNTOPTS="iocharset=utf8,codepage=macroman,"
;;
x"vfat"*|xmsdos*)
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/5] tests: Do not remove image file on error in pata_test
The image file can be useful in debugging an issue when the test fails.
Signed-off-by: Glenn Washburn
---
tests/pata_test.in | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/pata_test.in b/tests/pata_test.in
index 4fee0b0fb..27dccec19 100644
--- a/tests/pata_test.in
+++ b/tests/pata_test.in
@@ -47,7 +47,6 @@ tar cf "$imgfile" "$outfile"
v=$(echo "nativedisk; source '($indisk)/$outfile';" | "${grubshell}"
--qemu-opts="-$disk $imgfile")
if [ "$v" != "Hello World" ]; then
- rm "$imgfile"
rm "$outfile"
exit 1
fi
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 5/5] tests: Ensure that loopback devices and zfs devices are cleaned up
ZFS file systems are not unmounted using umount, but instead by exporting
them. So export the ZFS file system that has the same label as the one that
was created during the test, if such one exists. This is required to delete
the loopback device that uses the ZFS image file. Otherwise the added code
to delete all loopback devices setup during the test run will never be able
to finish because the loopback device can not be deleted while in use.
Signed-off-by: Glenn Washburn
---
tests/util/grub-fs-tester.in | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index b39831f27..5c23c41a1 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -14,15 +14,32 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XX"` ||
XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
MOUNTS=
-umount_all() {
+LODEVICES=
+cleanup() {
+if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
+ zpool list "$FSLABEL" 2>/dev/null &&
+ while ! zpool export "$FSLABEL" ; do
+ sleep 1;
+ done
+fi
+
for MOUNT in $MOUNTS; do
umount "$MOUNT" &&
MOUNTS="$(echo ${MOUNTS} | sed "s|$MOUNT||g;")"
done
+
+for lodev in $LODEVICES; do
+ local i=600
+ while losetup -l -O NAME | grep -q "^$lodev\$"; do
+ losetup -d "$lodev" || sleep 1
+ [ "$((i--))" = "0" ] && break
+ done
+done
+return 0
}
-trap umount_all EXIT INT
+trap cleanup EXIT INT
# This is for bash, dash and ash do not recognize ERR
-trap umount_all ERR || :
+trap cleanup ERR || :
# This wrapper is to ease insertion of valgrind or time statistics
run_it () {
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] http module is not checking correctly HTTP headers
On Wed, 12 Jan 2022 23:54:58 +0100
Javier Moragon wrote:
> According to https://www.ietf.org/rfc/rfc2616.txt 4.2, header names
> shall be case insensitive and we are now forced to read headers like
> `Content-Length` capitalized.
>
> The problem with that is when a HTTP server responds with a
> `content-length` header in lowercase GRUB gets stuck because HTTP
> module doesn't know the length of the transmision and the call never
> ends. I've been able to reproduce it and after ignoring the text case
> it worked perfectly.
>
> Here is it my patch proposal:
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..570fa3934 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -130,7 +130,7 @@ parse_line (grub_file_t file, http_data_t data,
> char *ptr, grub_size_t len)
>data->first_line_recv = 1;
>return GRUB_ERR_NONE;
> }
> - if (grub_memcmp (ptr, "Content-Length: ", sizeof ("Content-Length: ") - 1)
> + if (grub_strncasecmp (ptr, "Content-Length: ", grub_strlen
> ("Content-Length: ") )
I don't think there should be a new line here. And why change to
grub_strlen? Now the length is calculated everytime at runtime instead
of once at compile time.
>== 0 && !data->size_recv)
> {
>ptr += sizeof ("Content-Length: ") - 1;
> @@ -138,8 +138,8 @@ parse_line (grub_file_t file, http_data_t data,
> char *ptr, grub_size_t len)
>data->size_recv = 1;
>return GRUB_ERR_NONE;
> }
> - if (grub_memcmp (ptr, "Transfer-Encoding: chunked",
> -sizeof ("Transfer-Encoding: chunked") - 1) == 0)
> + if (grub_strncasecmp (ptr, "Transfer-Encoding: chunked",
> +grub_strlen ("Transfer-Encoding: chunked") ) == 0)
Ditto on the grub_strlen. I also don't like the original indentation of
this line and think that it should align with "ptr".
> {
>data->chunked = 1;
>return GRUB_ERR_NONE;
Otherwise, it seems like good patch.
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk
On Mon, 7 Feb 2022 10:29:44 -0500
James Bottomley wrote:
> This module is designed to provide an efisecret provider which
> interrogates the EFI configuration table to find the location of the
> confidential computing secret and tries to register the secret with
> the cryptodisk.
>
> The secret is stored in a boot allocated area, usually a page in size.
> The layout of the secret injection area is a header
>
> |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|
>
> with entries of the form
>
> |guid|len|data|
>
> the guid corresponding to the disk encryption passphrase is
> GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.
Is the NULL termination requirement something specified in some
specification? Otherwise, its not clear to me why it must be so.
> To get a high entropy string that doesn't need large numbers of
> iterations, use a base64 encoding of 33 bytes of random data.
I won't reiterate the comments by David Gilbert, which I also agree
with.
>
> Signed-off-by: James Bottomley
>
> ---
>
> v2: use callback to print failure message and destroy secret
> v3: change to generic naming to use for TDX and SEV and use new mechanism
> v4: review fixes
> ---
> grub-core/Makefile.core.def| 8 ++
> grub-core/disk/efi/efisecret.c | 129 +
> include/grub/efi/api.h | 15
> 3 files changed, 152 insertions(+)
> create mode 100644 grub-core/disk/efi/efisecret.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c0a..6293ddaa5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -788,6 +788,14 @@ module = {
>enable = efi;
> };
>
> +module = {
> + name = efisecret;
> +
> + common = disk/efi/efisecret.c;
> +
> + enable = efi;
> +};
> +
> module = {
>name = lsefimmap;
>
> diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
> new file mode 100644
> index 0..4cecebbdc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid =
> GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +struct efi_secret {
I find this name confusing. Perhaps a better name would be
"efi_secret_table_location"?
> + grub_uint64_t base;
> + grub_uint64_t size;
> +};
> +
> +struct secret_header {
> + grub_efi_packed_guid_t guid;
> + grub_uint32_t len;
> +};
> +
> +struct secret_entry {
> + grub_efi_packed_guid_t guid;
> + grub_uint32_t len;
> + grub_uint8_t data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> + grub_uint8_t **ptr)
> +{
> + struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct
> secret_entry *)0)->data);
> + int len = e->len;
> +
> + /* destroy the secret */
> + grub_memset (e, 0, len);
> + /* put back the length to make sure the table is still traversable */
> + e->len = len;
> +
> + *ptr = NULL;
> +
> + if (have_it)
> +return GRUB_ERR_NONE;
> +
> + return grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock
> any volumes");
I'm not seeing why the "have_it" argument and returning an error here is
useful. Its seems a bit out of place. Wouldn't it be better to do this
in the caller?
> +}
> +
> +static grub_err_t
> +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t **secret_ptr)
> +{
> + int len;
> + struct secret_header *h;
> + struct secret_entry *e;
> + unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> +
> + /* the area must be big enough for a guid and a u32 length */
> + if (s->size < sizeof (*h))
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too
> small");
> +
> + h = (struct secret_header *)ptr;
> + if (grub_memcmp(>guid, _guid, sizeof (h->guid)))
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not
> start with correct guid\n");
> + if (h->len < sizeof (*h))
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too
> small\n");
> +
> + len = h->len - sizeof (*h);
> + ptr += sizeof (*h);
> +
> + while (len >= (int)sizeof (*e)) {
> +e = (struct secret_entry *)ptr;
> +if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> corrupt\n");
> +
> +if (! grub_memcmp (>guid, _guid, sizeof (e->guid))) {
> + int end = e->len - sizeof(*e);
> +
> + /*
> + * the passphrase must be a zero terminated string because the
> + * password routines call grub_strlen () to find its size
> + */
I'm confused, what password routines are being referenced here?
> + if (end < 2 || e->data[end -
Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support
On Mon, 7 Feb 2022 10:29:43 -0500
James Bottomley wrote:
> Make use of the new OS provided secrets API so that if the new '-s'
> option is passed in we try to extract the secret from the API rather
> than prompting for it.
>
> The primary consumer of this is AMD SEV, which has been programmed to
> provide an injectable secret to the encrypted virtual machine. OVMF
> provides the secret area and passes it into the EFI Configuration
> Tables. The grub EFI layer pulls the secret out and primes the
> secrets API with it. The upshot of all of this is that a SEV
> protected VM can do an encrypted boot with a protected boot secret.
I think I prefer the key protector framework proposed in the first
patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It feels
like a more generic mechanism (though admittedly very similar to yours)
because its not tied to cryptodisk. I'm not sure where we'd want to use
the secrets/protectors framework outside of cryptodisk, but it seems
like it could be useful. The advantage of this patch is that it allows
for the clearing of key data from memory.
I don't think we should have both a secrets and a key protectors
framework, as its seems they are aiming to accomplish basically the
same thing.
The name "secrets" seems a bit more generic than "protectors" because,
as in this case, the secret is not protected. There is something I
don't like about the word "secrets", but I don't have a better
suggestion, so this might be what sticks. One thing going for "secrets"
over "protectors", is that the cryptomount option "-s" works better
than "-p" for protectors because that's already taken by the password
option.
>
> Signed-off-by: James Bottomley
>
> ---
>
> v2: add callback for after secret use
> v3: update to use named module mechanism for secret loading
> v4: update for new secret passing API
> ---
> grub-core/disk/cryptodisk.c | 56 +++--
> include/grub/cryptodisk.h | 14 ++
> 2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..f9c86f958 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> +{"secret", 's', 0, N_("Get secret passphrase from named module and
> optional identifier"), 0, 0},
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> +/* variable to hold the list of secret providers */
> +static struct grub_secret_entry *secret_providers;
> +
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> {
> @@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
>return dev;
> }
>
> +void
> +grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e)
> +{
> + grub_list_push(GRUB_AS_LIST_P (_providers), GRUB_AS_LIST (e));
> +}
> +
> +void
> +grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e)
> +{
> + grub_list_remove (GRUB_AS_LIST (e));
> +}
> +
> #ifdef GRUB_UTIL
> #include
> grub_err_t
> @@ -1160,10 +1176,14 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> {
>struct grub_arg_list *state = ctxt->state;
>struct grub_cryptomount_args cargs = {0};
> + struct grub_secret_entry *se = NULL;
>
> - if (argc < 1 && !state[1].set && !state[2].set)
> + if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> + if (state[3].set && state[4].set)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive
> options");
> +
>if (grub_cryptodisk_list == NULL)
> return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
>
> @@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>cargs.key_data = (grub_uint8_t *) state[3].arg;
>cargs.key_len = grub_strlen (state[3].arg);
> }
> + else if (state[4].set) /* secret module */
> +{
> + grub_err_t rc;
> +
> + if (argc < 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must be
> specified");
> +#ifndef GRUB_UTIL
> + grub_dl_load (args[0]);
> +#endif
> + se = grub_named_list_find (GRUB_AS_NAMED_LIST (secret_providers),
> args[0]);
> + if (se == NULL)
> + return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret provider is
> found");
> +
> + rc = se->get (args[1], _data);
> + if (rc)
> + return rc;
> + cargs.key_len = grub_strlen((char *) cargs.key_data);
It seems better to me to send a pointer to cargs.key_len to se->get()
because it already knows the
[PATCH 7/8] gdb: Conditionally run GDB script logic for dynamically or statically positioned GRUB
There are broadly two classes of targets to consider when loading symbols for GRUB, targets that determine where to load GRUB at runtime (dynamically positioned) and those that do not (statically positioned). For statically poisitioned targets, symbol loading is determined at link time, so nothing more needs to be known to load the symbols. For dynamically positioned targets, such as EFI targets, at runtime symbols should be offset by an amount that depends on where the runtime chose to load GRUB. It is important to not load symbols statically for dynamic targets because then when subsequently loading the symbols correctly one must take care to remove the existing static symbols, otherwise there will be two sets of symbols and GDB seems to prefer the ones loaded first (ie the static ones). Use autoconf variables to generate a gdb_grub for a particular target, which conditionally run startup code depending on if the target uses static or dynamic loading. This is complicated by the fact that a subshell is used to check the autoconf variable and shells return 0 for success, but 0 is false in conditionals in GDB. So an inversion of the status code is needed. Signed-off-by: Glenn Washburn --- grub-core/gdb_grub.in | 22 +++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in index f3c6faf94..4c2b8bf52 100644 --- a/grub-core/gdb_grub.in +++ b/grub-core/gdb_grub.in @@ -145,7 +145,23 @@ end ### set confirm off -file kernel.exec -target remote :1234 -runtime_load_module +# Note: On EFI and other platforms that load GRUB to an address that is +# determined at runtime, the symbols in kernel.exec will be wrong. +# However, we must start by loading some executable file or GDB will +# fail. + +# If this returns success, the exit code is 0, however, GDB's "if" +# considers 0 to be false. So invert the status code. +shell test "@platform@" == "efi" +set $platform_efi = ! $_shell_exitcode + +if $platform_efi + # Only load the executable file, not the symbols + exec-file kernel.exec +else + file kernel.exec + runtime_load_module +end + +target remote :1234 -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 5/8] gdb: Add functions to make loading from dynamically positioned targets easier
Many targets, such as EFI, load GRUB at addresses that are determined at
runtime. So the load addresses in kernel.exec will almost certainly be
wrong. Given the address of the start of the text and data segments, these
functions will tell GDB to load the symbols at the proper locations.
It is left up to the user to determine how to get the test and data
addresses.
Signed-off-by: Glenn Washburn
---
grub-core/gdb_grub.in | 54 +++
1 file changed, 54 insertions(+)
diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in
index dd1e86bf2..f3c6faf94 100644
--- a/grub-core/gdb_grub.in
+++ b/grub-core/gdb_grub.in
@@ -9,6 +9,60 @@
### Lubomir Kundrak
###
+define dynamic_load_kernel_exec_symbols
+ shell rm -f .remove-kernel.exec.symfile.gdb
+
+ # Loading symbols is complicated by the fact that kernel.exec is an ELF
+ # ELF binary, but the UEFI runtime is PE32+. All the data sections of
+ # the ELF binary are concatenated (accounting for ELF section alignment)
+ # and put into one .data section in the PE32+ runtime image. So given
+ # the load address of the .data PE32+ section we can determine the
+ # addresses each ELF data section maps to.
+ shell bash -c ' \
+ ( \
+ export PE_TEXT=$1 PE_DATA=$2; \
+ alignup() { \
+ PAD=1; \
+ if [ "$(($1%$2))" -eq 0 ]; then \
+ PAD=0; \
+ fi; \
+ printf "0x%x\n" $$1/$2)+$PAD)*$2)); \
+ }; \
+ printf "add-symbol-file kernel.exec ${PE_TEXT}"; \
+ objdump -h kernel.exec | tail -n +6 | \
+ while read IDX NAME SIZE _ _ OFFSET ALIGN; do \
+ read FLAGS; \
+ if [ -n "$FLAGS" ] && [ -z "${FLAGS%%*DATA*}" -o "$NAME" = .bss
]; then \
+ OFF=$(alignup ${OFF:-0} $ALIGN); \
+ echo -n " -s $NAME (${PE_DATA}+$(printf "0x%x" $OFF))"; \
+ OFF=$((${OFF} + 0x${SIZE})); \
+ fi; \
+ done \
+ )' script $arg0 $arg1 >.kernel.exec.loadsym.gdb
+ source .kernel.exec.loadsym.gdb
+end
+document dynamic_load_kernel_exec_symbols
+ Load debugging symbols from kernel.exec given the address of the
+ .text and .data segments of the UEFI binary.
+end
+
+define dynamic_load_symbols
+ dynamic_load_kernel_exec_symbols $arg0 $arg1
+
+ # We may have been very late to loading the kernel.exec symbols and
+ # and modules may already be loaded. So load symbols for any already
+ # loaded.
+ load_all_modules
+
+ runtime_load_module
+end
+document dynamic_load_symbols
+ Load debugging symbols from kernel.exec and any loaded modules given
+ the address of the .text and .data segments of the UEFI binary. Also
+ setup session to automatically load module symbols for modules loaded
+ in the future.
+end
+
# Add section numbers and addresses to .segments.tmp
define dump_module_sections
set $mod = $arg0
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 6/8] gdb: If enabled, print line used to load EFI kernel symbols when using gdb_grub script
If the macro PRINT_GDB_SYM_LOAD_CMD is non-zero, compile code which will
print the command needed to load symbols for the GRUB EFI kernel. This is
needed because EFI firmware determines where to load the GRUB EFI
at runtime, and so the relevant addresses are not known ahead of time.
The command is a custom command defined in the gdb_grub GDB script. So GDB
should be started with the script as an argument to the -x option or
sourced into an active GDB session before running the outputted command.
Co-developed-by: Peter Jones
Signed-off-by: Glenn Washburn
---
config.h.in | 3 +++
grub-core/kern/efi/efi.c | 4 ++--
grub-core/kern/efi/init.c | 25 -
include/grub/efi/efi.h| 2 +-
4 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/config.h.in b/config.h.in
index 9e8f9911b..6d0743a78 100644
--- a/config.h.in
+++ b/config.h.in
@@ -9,6 +9,9 @@
#define GCRYPT_NO_DEPRECATED 1
#define HAVE_MEMMOVE 1
+/* Define to 1 to enable printing of gdb command to load module symbols. */
+#define PRINT_GDB_SYM_LOAD_CMD 0
+
/* Define to 1 to enable disk cache statistics. */
#define DISK_CACHE_STATS @DISK_CACHE_STATS@
#define BOOT_TIME_STATS @BOOT_TIME_STATS@
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 18858c327..f492cdd99 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -299,7 +299,7 @@ grub_efi_get_variable (const char *var, const
grub_efi_guid_t *guid,
/* Search the mods section from the PE32/PE32+ image. This code uses
a PE32 header, but should work with PE32+ as well. */
grub_addr_t
-grub_efi_modules_addr (void)
+grub_efi_section_addr (const char *section_name)
{
grub_efi_loaded_image_t *image;
struct grub_pe32_header *header;
@@ -324,7 +324,7 @@ grub_efi_modules_addr (void)
i < coff_header->num_sections;
i++, section++)
{
- if (grub_strcmp (section->name, "mods") == 0)
+ if (grub_strcmp (section->name, section_name) == 0)
break;
}
diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 7facacf09..f640b8192 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -82,10 +82,30 @@ stack_protector_init (void)
grub_addr_t grub_modbase;
+#if PRINT_GDB_SYM_LOAD_CMD
+static void
+grub_efi_print_gdb_info (void)
+{
+ grub_addr_t text;
+ grub_addr_t data;
+
+ text = grub_efi_section_addr (".text");
+ if (!text)
+return;
+
+ data = grub_efi_section_addr (".data");
+ if (data)
+grub_printf ("dynamic_load_symbols %p -s .data %p\n",
+(void *)text, (void *)data);
+ else
+grub_printf ("dynamic_load_symbols %p\n", (void *)text);
+}
+#endif
+
void
grub_efi_init (void)
{
- grub_modbase = grub_efi_modules_addr ();
+ grub_modbase = grub_efi_section_addr ("mods");
/* First of all, initialize the console so that GRUB can display
messages. */
grub_console_init ();
@@ -108,6 +128,9 @@ grub_efi_init (void)
efi_call_4 (grub_efi_system_table->boot_services->set_watchdog_timer,
0, 0, 0, NULL);
+#if PRINT_GDB_SYM_LOAD_CMD
+ grub_efi_print_gdb_info ();
+#endif
grub_efidisk_init ();
}
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index fc723962d..f3ed52dda 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -107,7 +107,7 @@ grub_err_t grub_arch_efi_linux_boot_image(grub_addr_t addr,
grub_size_t size,
char *args);
#endif
-grub_addr_t grub_efi_modules_addr (void);
+grub_addr_t grub_efi_section_addr (const char *section);
void grub_efi_mm_init (void);
void grub_efi_mm_fini (void);
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/8] gdb: If no modules have been loaded, do not try to load module symbols
This prevents load_all_modules from failing when called before any modules have been loaded. Failures in GDB user-defined functions cause any function which called them to also fail. Signed-off-by: Glenn Washburn --- grub-core/gdb_grub.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in index b0e8d1ed1..3a0593610 100644 --- a/grub-core/gdb_grub.in +++ b/grub-core/gdb_grub.in @@ -64,7 +64,9 @@ define load_all_modules dump_module_sections $this set $this = $this->next end - match_and_load_symbols + if (grub_dl_head != 0) + match_and_load_symbols + end end document load_all_modules Load debugging information for all loaded modules. -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/8] GDB script fixes and improvements
This is a collection of fixes and improvements mostly to the GDB script, gdb_grub. I don't think the first 4 need commnt. A major benefit added in patch #5 is the ability to properly load symbols on EFI targets given the load address for the text and data segments. This is written as a shell script in the GDB script primarily because I'm not familiar with the Python API. I think it would probably be better written in Python, though. Daniel what do you think about using Python in the GDB script? First of all, I get the impression that this script isn't used much (but I'd like to know if I'm wrong about that), so I suspect changes won't affect many people one way or the other. And second, I think its reasonable to assume these days that people's GDB is compiled with Python support. Patch #6 has code stolen from the patch by Peter Jones[1]. I've added a Co-developed-by tag. Let me know if there's something different I should do. I've removed the printing of lines for each module as this is unnecessary if the kernel.exec symbols have been loaded correctly (see load_all_modules in gdb_grub). Also, I've made the printed message for loading kernel.exec symbols only happen if enabled by setting PRINT_GDB_SYM_LOAD_CMD to a non- zero value in config.h.in. Ultimately, I think a better solution is to check the EFI variables for a certain value and enable the print using that. This way support would always be compiled in and can still be enabled very early in the GRUB boot process. However, I'm not sure what that variable should be, or if we should just create our own variable. Someone more experienced in EFI than I could help with that or say that using an EFI var would be unadvisable. Testing on QEMU with OVMF firmware, shows that GRUB will be loaded to the same address consistently so long as the binary hasn't changed. This is nice because by the time you get the message with the right parameters needed to load the symbols you may have already passed by code you wanted to break on. So you can use the same parameters to load the symbols before GRUB executes and set break points as early as you want in GRUB code. I'm not sure if this is true of real hardware, but I suspect it is. If this is so, then Daniel's idea of making this a command may make sense as well, so that GRUB doesn't need to be recompiled in "debug mode" to get this address. Of course, if GRUB is failing before module initialization, the module won't work. Patch #7 adds a check if the build was for an EFI platform and runs the dynamic code at startup if so. As noted in the commit message, without this the user must remember to first remove the symbols loaded at the statically positioned offset. Otherwise, symbols will be multiply defined and GDB uses the first ones defined by default. It was suggested by Vladimir that targets with platform as EFI were all the dynamic ones[2], but 10+ years later this may have changed. Other target tests can be added as needed for ones that are dynamic but are not EFI. Patch #8 This is a work around for a strange issue where GDB is stopping before the stack frame has been setup when breaking on grub_dl_add(). But GDB thinks that the stack frame has been setup. I'm not sure what is causing this (not using -fomit-frame-pointer and don't see any obvious culprits in other GCC compile options). I also don't particularly like this solution, but its the best one I've found. I originally was going up a frame in grub_dl_add() and getting the mod variable there, but that assumes knowledge of the caller. I'd like to set a break point on the instruction that I'd get to by issuing a "step" in GDB after the break on grub_dl_add, but I don't see a way to do that. Another option is to set an unused label in grub_dl_add() and do a tbreak on the label, but I was wanting to avoid code changes. Maybe this is acceptable though. Glenn [1] https://lists.gnu.org/archive/html/grub-devel/2021-11/msg8.html [2] https://lists.gnu.org/archive/html/grub-devel/2011-11/msg00069.html Glenn Washburn (8): gdb: Move runtime module loading into runtime_load_module gdb: If no modules have been loaded, do not try to load module symbols gdb: Do not lazy load module symbols gdb: Prevent wrapping when writing to .segments.tmp gdb: Add functions to make loading from dynamically positioned targets easier gdb: If enabled, print line used to load EFI kernel symbols when using gdb_grub script gdb: Conditionally run GDB script logic for dynamically or statically positioned GRUB gdb: Get correct mod variable value config.h.in | 3 ++ grub-core/gdb_grub.in | 109 ++ grub-core/gmodule.pl.in | 2 +- grub-core/kern/efi/efi.c | 4 +- grub-core/kern/efi/init.c | 25 - include/grub/efi/efi.h| 2 +- 6 files changed, 131 insertions(+), 14 deletions(-) -- 2.27.0 ___ Grub-devel mailing lis
[PATCH 1/8] gdb: Move runtime module loading into runtime_load_module
Signed-off-by: Glenn Washburn --- grub-core/gdb_grub.in | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in index e322d3dc1..b0e8d1ed1 100644 --- a/grub-core/gdb_grub.in +++ b/grub-core/gdb_grub.in @@ -70,16 +70,22 @@ document load_all_modules Load debugging information for all loaded modules. end +define runtime_load_module + break grub_dl_add + commands + silent + load_module mod + cont + end +end +document runtime_load_module + Load module symbols at runtime as they are loaded. +end + ### set confirm off file kernel.exec target remote :1234 -# inform when module is loaded -break grub_dl_add -commands - silent - load_module mod - cont -end +runtime_load_module -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/8] gdb: Do not lazy load module symbols
When loading module symbols, tell GDB to load them all right away. GDB by
default will lazy load symbols as needed in the background. However, this
process will output to GDB's stdout some messages. This fixes a bug where
output from the loading process gets printed to the .segments.tmp file
causing gmodule.pl to write a bad .loadsym.gdb that GDB chokes on.
Signed-off-by: Glenn Washburn
---
grub-core/gmodule.pl.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/gmodule.pl.in b/grub-core/gmodule.pl.in
index 78aa1e64e..c74eedf4a 100644
--- a/grub-core/gmodule.pl.in
+++ b/grub-core/gmodule.pl.in
@@ -11,7 +11,7 @@ use strict;
while (<>) {
my ($name, %sections) = split;
- print "add-symbol-file $name.module";
+ print "add-symbol-file -readnow $name.module";
open (READELF, "readelf -S $name.mod |") or die;
while () {
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 4/8] gdb: Prevent wrapping when writing to .segments.tmp
GDB logging is redirected to write .segments.tmp, which means that GDB will wrap lines longer than what it thinks is the screen width (typically 80 characters). When wrapping does occur it causes gmodule.pl to misbehave. So disable line wrapping. Signed-off-by: Glenn Washburn --- grub-core/gdb_grub.in | 4 1 file changed, 4 insertions(+) diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in index 3a0593610..dd1e86bf2 100644 --- a/grub-core/gdb_grub.in +++ b/grub-core/gdb_grub.in @@ -13,6 +13,10 @@ define dump_module_sections set $mod = $arg0 + # Set unlimited width so that lines don't get wrapped writing + # to .segments.tmp + set width 0 + # FIXME: save logging status set logging file .segments.tmp set logging redirect on -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 8/8] gdb: Get correct mod variable value
For some reason, GDB is breaking on grub_dl_add() before the function has setup its stack frame, but GDB thinks it has. So the value of mod is bogus. To get the correct value, create a one-time break on grub_dl_get(), which is the first line of grub_dl_add(). When this break point hits, grub_dl_add() will have finished setting up it stack frame. But at this point we will be in grub_dl_get()'s stack frame. So go one frame up, which will be grub_dl_add(), to get mod's value. Signed-off-by: Glenn Washburn --- grub-core/gdb_grub.in | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/grub-core/gdb_grub.in b/grub-core/gdb_grub.in index 4c2b8bf52..a1031e58d 100644 --- a/grub-core/gdb_grub.in +++ b/grub-core/gdb_grub.in @@ -134,7 +134,16 @@ define runtime_load_module break grub_dl_add commands silent - load_module mod + # GDB has stopped before the call frame is setup, so mod does + # not have the correct value. Create a one-time break on the + # next function call and then go one frame up, back to the + # grub_dl_add frame, to get the correct value for mod. + tbreak grub_dl_get + commands + fr 1 + load_module mod + cont + end cont end end -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/3] Fixes and enhancements for memory debugging
The first patch supercedes a patch sent earlier of the same subject. I've removed the #undef if --mm-debug is not passwed to configure. The idea is that even if --mm-debug is not passed to configure, the user should still be able to enable memory debugging by passing -DMM_DEBUG in CFLAGS. The second patch I found useful in dumping the memory allocation state from a module. And the third patch is a bug fix, without which enabling memory debugging leads to a certain infinite recursion crash (when also enabling grub_mm_debug). As an aside, this leads me to believe that no one has used this feature in a very long time (and is not currently using it). Glenn Glenn Washburn (3): configure: Properly handle MM_DEBUG mm: Export grub_mm_dump and grub_mm_dump_free mm: Temporarily disable grub_mm_debug while calling grub_vprintf in grub_printf config.h.in | 4 configure.ac | 6 -- grub-core/kern/misc.c | 19 +++ include/grub/mm.h | 4 ++-- 4 files changed, 29 insertions(+), 4 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/3] mm: Temporarily disable grub_mm_debug while calling grub_vprintf in grub_printf
To prevent infinite recursion when grub_mm_debug is on, disable it when
calling grub_vprintf. One such call loop is:
grub_vprintf -> parse_printf_args -> parse_printf_arg_fmt
-> grub_debug_calloc -> grub_printf -> grub_vprintf
Signed-off-by: Glenn Washburn
---
grub-core/kern/misc.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index de40f566d6..cd6f460a21 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -113,10 +113,29 @@ grub_printf (const char *fmt, ...)
va_list ap;
int ret;
+#if defined(MM_DEBUG) && !defined(GRUB_UTIL) && !defined (GRUB_MACHINE_EMU)
+ /*
+ * To prevent infinite recursion when grub_mm_debug is on, disable it
+ * when calling grub_vprintf. One such call loop is:
+ * grub_vprintf -> parse_printf_args -> parse_printf_arg_fmt
+ * -> grub_debug_calloc -> grub_printf -> grub_vprintf
+ */
+ int grub_mm_debug_save = 0;
+ if (grub_mm_debug)
+{
+ grub_mm_debug_save = grub_mm_debug;
+ grub_mm_debug = 0;
+}
+#endif
+
va_start (ap, fmt);
ret = grub_vprintf (fmt, ap);
va_end (ap);
+#if defined(MM_DEBUG) && !defined(GRUB_UTIL) && !defined (GRUB_MACHINE_EMU)
+ grub_mm_debug = grub_mm_debug_save;
+#endif
+
return ret;
}
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 0/6] Update gnulib version and drop most gnulib patches
On Fri, 4 Mar 2022 18:01:10 -0600 Glenn Washburn wrote: > On Wed, 2 Mar 2022 14:08:23 -0500 > Robbie Harwood wrote: > > > Changes this version: > > > > - Reorder last two commits so that warning fixes come after the change that > > introduces them. > > - Fix comment formatting to comply with grub2 style. > > Either I missed it before or something changed. But I'm getting this > build error now for x86_64-efi, and I'm not getting it without this patch > series. > > In file included from > /root/grub-tests.update-gnulib/grub/include/grub/disk.h:31 > , > from > /root/grub-tests.update-gnulib/grub/include/grub/file.h:26 > , > from > /root/grub-tests.update-gnulib/grub/include/grub/loader.h: > 23, > from > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/ > bsd.c:19: > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c: In function > ‘gr > ub_freebsd_add_meta_module’: > /root/grub-tests.update-gnulib/grub/include/grub/misc.h:71:10: error: ‘ptr’ > may be used uninitialized in this function [-Werror=maybe-uninitialized] >71 | return grub_memmove (dest, src, n); > | ^~~ > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c:266:9: note: > ‘ptr’ was declared here > 266 | void *ptr; > | ^~~ > > Reviewing the code it doesn't look like ptr can actually be used > uninitialized, so it seems like GCC 10.1.0 isn't smart enough to figure > that out. Initializing to NULL fixes the build issue. For completeness, I just realized that I wasn't using the compiler I thought I was using. Turns out this issue is with x86_64 Debian 11's GCC, which is 10.2.1. I don't think this is relevant to the issue, but may help in reproducing if needed. After some more testing, I've narrowed it down to passing -O2 to gcc, without which the series builds fine. Glenn > > > > > No functional code changes. > > > > Be well, > > --Robbie > > > > Robbie Harwood (6): > > Use visual indentation in config.h.in > > Where present, ensure config-util.h precedes config.h > > Drop gnulib fix-base64.patch > > Drop gnulib no-abort.patch > > Update gnulib version and drop most gnulib patches > > Handle warnings introduced by updated gnulib > > > > INSTALL | 4 +- > > bootstrap | 319 ++ > > bootstrap.conf| 23 +- > > conf/Makefile.extra-dist | 8 - > > config.h.in | 142 ++-- > > configure.ac | 2 +- > > grub-core/Makefile.core.def | 3 + > > grub-core/disk/host.c | 2 +- > > grub-core/disk/luks2.c| 4 +- > > grub-core/gensymlist.sh | 1 + > > grub-core/kern/emu/argp_common.c | 2 +- > > grub-core/kern/emu/main.c | 2 +- > > grub-core/lib/gnulib-patches/fix-base64.patch | 21 -- > > .../lib/gnulib-patches/fix-null-deref.patch | 13 - > > .../gnulib-patches/fix-null-state-deref.patch | 12 - > > .../fix-regcomp-uninit-token.patch| 15 - > > .../fix-regexec-null-deref.patch | 12 - > > .../gnulib-patches/fix-uninit-structure.patch | 11 - > > .../lib/gnulib-patches/fix-unused-value.patch | 14 - > > grub-core/lib/gnulib-patches/no-abort.patch | 26 -- > > grub-core/lib/posix_wrap/limits.h | 6 +- > > grub-core/lib/posix_wrap/sys/types.h | 7 +- > > grub-core/lib/xzembed/xz.h| 5 +- > > grub-core/osdep/aros/config.c | 2 +- > > grub-core/osdep/basic/emunet.c| 2 +- > > grub-core/osdep/basic/init.c | 2 +- > > grub-core/osdep/haiku/getroot.c | 2 +- > > grub-core/osdep/linux/emunet.c| 2 +- > > grub-core/osdep/unix/config.c | 2 +- > > grub-core/osdep/unix/cputime.c| 2 +- > > grub-core/osdep/unix/dl.c | 2 +- > > grub-core/osdep/unix/emuconsole.c | 2 +- > > grub-core/osdep/unix/getroot.c| 2 +- > > grub-core/osdep/windows/config.c | 2 +- > > grub-core/osdep/windows/cputime.c | 2 +- > > grub-core/osdep/windows/dl.c | 2 +- > > grub-core/osdep/windows/emuconsole.c | 2 +- > &
Re: [PATCH v8 4/6] Drop gnulib no-abort.patch
On Thu, 03 Mar 2022 13:47:29 -0500 Robbie Harwood wrote: > Glenn Washburn writes: > > > Robbie Harwood wrote: > > > >> If you have a patch that makes this work, I don't have a problem with > >> it. However, I was unable to make that work in practice. > > > > Can you provide some specifics on what problem you were running in to? > > Was it a link issue at build time? platform specific? Did it build > > fine, but blew up in testing? Did you try using the linker options I > > suggested above? > > No, nor am I about to. This is code that I have, that builds, that I'm > submitting to grub. If you want a different approach, that's fine - > you're welcome to write a patch to do that instead. I have built this > bike shed and I *really* do not care what color it is, nor do I > appreciate being asked to test other people's proposals for them: you're > presumably just as capable of building the code yourself and seeing if > something works or doesn't. This is v8 of the series and I'm pretty > much done caring about it at this point. Sounds like you're frustrated with the process and needed to let off some stream. I can speak from experience on that subject (I got up to v9 on a cryptodisk patch series that was actually fixing bugs). Of course, its fine that you dont appreciate being asked to "test other people's proposals" and its fine for anyone to ask you to do that, which I did not (I asked if you had, there's a difference). You seem capable of setting boundaries, as evidenced here, so no problem there. There are many people on this list, including me, that have on occasion modified a patch series according to someone else's proposal. I don't think its an unreasonable ask, especially when the patch in question is not correct. You're a free human being capable of saying no, as is your perogative. There's no need to take it as a personal affront. The GRUB project seems to me to have a fairly high code quality bar. I've been frustrated in the past about what I considered bike shedding as well and have made my frustrations known on and off this list. And I think this is less trivial than some of those things. Also, I think a more accurate metaphor, in this case, rather than quibbling over color is whether you'll get electrocuted when lightening strikes. This seems like its introducing a misfeature that someone else down the line will get bitten by. So, if we can avoid that now when we know there's an issue, the cost is much lower. I hear you loud and clear saying you're done pushing on this patch series. Would you like me to take it over? I've found what I believe to be an acceptable solution using grub_abort. > > For completeness, here's what happens if one just defines to grub_abort > without further modification: > > $ uname -m > x86_64 > $ ./bootstrap > ... > $ ./configure --enable-grub-mkfont > ... > $ make > ... > gcc -DHAVE_CONFIG_H -I. -I.. -Wall -W -DGRUB_MACHINE_PCBIOS=1 > -DGRUB_MACHINE=I386_PC -m32 -nostdinc -isystem > /usr/lib/gcc/x86_64-linux-gnu/11/include -I../include -I../include > -DGRUB_FILE=\"lib/gnulib/regex.c\" -I. -I. -I.. -I.. -I../include > -I../include -I../grub-core/lib/libgcrypt-grub/src/ > -I../grub-core/lib/posix_wrap -I../grub-core/lib/gnulib > -I../grub-core/lib/gnulib -D_FILE_OFFSET_BITS=64 -std=gnu99 -Os -m32 -Wall > -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment > -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal > -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit > -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces > -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type > -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs > -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label > -Wunused-parameter -Wunused-value -Wunused-variable -Wwrite-strings > -Wnested-externs -Wstrict-prototypes -g -Wredundant-decls > -Wmissing-prototypes -Wmissing-declarations -Wextra -Wattributes > -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch > -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast > -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign > -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 > -march=i386 -mrtd -mregparm=3 -falign-functions=1 -falign-loops=1 > -falign-jumps=1 -freg-struct-return -mno-mmx -mno-sse -mno-sse2 -mno-sse3 > -mno-3dnow -Wa,-mx86-used-note=no -msoft-float -fno-dwarf2-cfi-asm > -mno-stack-arg-probe -fno-asynchronous-unwind-tables -fno-unwind-tables > -fno-ident -fno-PIE -fno-pie -fno-stack-protector -Wtrampolines -Werror > -ffreestanding -fno-builtin -Wno-undef -Wno-sign-compare -Wno-unused > -
Re: [PATCH v8 0/6] Update gnulib version and drop most gnulib patches
On Wed, 2 Mar 2022 14:08:23 -0500 Robbie Harwood wrote: > Changes this version: > > - Reorder last two commits so that warning fixes come after the change that > introduces them. > - Fix comment formatting to comply with grub2 style. Either I missed it before or something changed. But I'm getting this build error now for x86_64-efi, and I'm not getting it without this patch series. In file included from /root/grub-tests.update-gnulib/grub/include/grub/disk.h:31 , from /root/grub-tests.update-gnulib/grub/include/grub/file.h:26 , from /root/grub-tests.update-gnulib/grub/include/grub/loader.h: 23, from /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/ bsd.c:19: /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c: In function ‘gr ub_freebsd_add_meta_module’: /root/grub-tests.update-gnulib/grub/include/grub/misc.h:71:10: error: ‘ptr’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 71 | return grub_memmove (dest, src, n); | ^~~ /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c:266:9: note: ‘ptr’ was declared here 266 | void *ptr; | ^~~ Reviewing the code it doesn't look like ptr can actually be used uninitialized, so it seems like GCC 10.1.0 isn't smart enough to figure that out. Initializing to NULL fixes the build issue. Glenn > > No functional code changes. > > Be well, > --Robbie > > Robbie Harwood (6): > Use visual indentation in config.h.in > Where present, ensure config-util.h precedes config.h > Drop gnulib fix-base64.patch > Drop gnulib no-abort.patch > Update gnulib version and drop most gnulib patches > Handle warnings introduced by updated gnulib > > INSTALL | 4 +- > bootstrap | 319 ++ > bootstrap.conf| 23 +- > conf/Makefile.extra-dist | 8 - > config.h.in | 142 ++-- > configure.ac | 2 +- > grub-core/Makefile.core.def | 3 + > grub-core/disk/host.c | 2 +- > grub-core/disk/luks2.c| 4 +- > grub-core/gensymlist.sh | 1 + > grub-core/kern/emu/argp_common.c | 2 +- > grub-core/kern/emu/main.c | 2 +- > grub-core/lib/gnulib-patches/fix-base64.patch | 21 -- > .../lib/gnulib-patches/fix-null-deref.patch | 13 - > .../gnulib-patches/fix-null-state-deref.patch | 12 - > .../fix-regcomp-uninit-token.patch| 15 - > .../fix-regexec-null-deref.patch | 12 - > .../gnulib-patches/fix-uninit-structure.patch | 11 - > .../lib/gnulib-patches/fix-unused-value.patch | 14 - > grub-core/lib/gnulib-patches/no-abort.patch | 26 -- > grub-core/lib/posix_wrap/limits.h | 6 +- > grub-core/lib/posix_wrap/sys/types.h | 7 +- > grub-core/lib/xzembed/xz.h| 5 +- > grub-core/osdep/aros/config.c | 2 +- > grub-core/osdep/basic/emunet.c| 2 +- > grub-core/osdep/basic/init.c | 2 +- > grub-core/osdep/haiku/getroot.c | 2 +- > grub-core/osdep/linux/emunet.c| 2 +- > grub-core/osdep/unix/config.c | 2 +- > grub-core/osdep/unix/cputime.c| 2 +- > grub-core/osdep/unix/dl.c | 2 +- > grub-core/osdep/unix/emuconsole.c | 2 +- > grub-core/osdep/unix/getroot.c| 2 +- > grub-core/osdep/windows/config.c | 2 +- > grub-core/osdep/windows/cputime.c | 2 +- > grub-core/osdep/windows/dl.c | 2 +- > grub-core/osdep/windows/emuconsole.c | 2 +- > grub-core/osdep/windows/init.c| 2 +- > include/grub/compiler.h | 4 +- > include/grub/list.h | 2 +- > 40 files changed, 347 insertions(+), 343 deletions(-) > delete mode 100644 grub-core/lib/gnulib-patches/fix-base64.patch > delete mode 100644 grub-core/lib/gnulib-patches/fix-null-deref.patch > delete mode 100644 grub-core/lib/gnulib-patches/fix-null-state-deref.patch > delete mode 100644 > grub-core/lib/gnulib-patches/fix-regcomp-uninit-token.patch > delete mode 100644 grub-core/lib/gnulib-patches/fix-regexec-null-deref.patch > delete mode 100644 grub-core/lib/gnulib-patches/fix-uninit-structure.patch > delete mode 100644 grub-core/lib/gnulib-patches/fix-unused-value.patch > delete mode 100644 grub-core/lib/gnulib-patches/no-abort.patch > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] po: Un-transliterate the %zu format code
Commit 45bffae13 uses the %zu format specifier which has not been used in any translated strings yet. So the sed scripts used for tranliterating certain languages need to be updated otherwise creation of the message indexes will fail on an unknown format code. This is essentially the same issue fixed for the %m format code in commit 2e246b6f. Signed-off-by: Glenn Washburn --- po/arabic.sed | 3 ++- po/cyrillic.sed | 3 ++- po/greek.sed| 3 ++- po/hebrew.sed | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/po/arabic.sed b/po/arabic.sed index 21dc8b6db2..3fbee72480 100644 --- a/po/arabic.sed +++ b/po/arabic.sed @@ -74,9 +74,10 @@ s,%\([0-9]*\)للد,%\1lld,g s,%\([0-9\.\*]*\)س,%\1s,g s,%\([0-9\.\*]*\)م,%\1m,g -s,%\([0-9]*\)لو,%\1lu,g s,%\([0-9]*\)و,%\1u,g +s,%\([0-9]*\)لو,%\1lu,g s,%\([0-9]*\)للو,%\1llu,g +s,%\([0-9]*\)زو,%\1zu,g s,%\([0-9]*\)كس,%\1x,g s,%\([0-9]*\)لكس,%\1lx,g s,%\([0-9]*\)للكس,%\1llx,g diff --git a/po/cyrillic.sed b/po/cyrillic.sed index 2e3e6655d6..472f095295 100644 --- a/po/cyrillic.sed +++ b/po/cyrillic.sed @@ -97,9 +97,10 @@ s,%\([0-9]*\)ллд,%\1lld,g s,%\([0-9\.\*]*\)с,%\1s,g s,%\([0-9\.\*]*\)м,%\1m,g -s,%\([0-9]*\)лу,%\1lu,g s,%\([0-9]*\)у,%\1u,g +s,%\([0-9]*\)лу,%\1lu,g s,%\([0-9]*\)ллу,%\1llu,g +s,%\([0-9]*\)зу,%\1zu,g s,%\([0-9]*\)ѯ,%\1x,g s,%\([0-9]*\)лѯ,%\1lx,g s,%\([0-9]*\)ллѯ,%\1llx,g diff --git a/po/greek.sed b/po/greek.sed index 3543f6aac3..0e81625fbb 100644 --- a/po/greek.sed +++ b/po/greek.sed @@ -99,9 +99,10 @@ s,%\([0-9]*\)λλδ,%\1lld,g s,%\([0-9\.\*]*\)σ,%\1s,g s,%\([0-9\.\*]*\)μ,%\1m,g -s,%\([0-9]*\)λυ,%\1lu,g s,%\([0-9]*\)υ,%\1u,g +s,%\([0-9]*\)λυ,%\1lu,g s,%\([0-9]*\)λλυ,%\1llu,g +s,%\([0-9]*\)ζυ,%\1zu,g s,%\([0-9]*\)ξ,%\1x,g s,%\([0-9]*\)λξ,%\1lx,g s,%\([0-9]*\)λλξ,%\1llx,g diff --git a/po/hebrew.sed b/po/hebrew.sed index 9210014bc3..33174bbdcc 100644 --- a/po/hebrew.sed +++ b/po/hebrew.sed @@ -82,9 +82,10 @@ s,%\([0-9]*\)ללד,%\1lld,g s,%\([0-9\.\*]*\)ש,%\1s,g s,%\([0-9\.\*]*\)מ,%\1m,g -s,%\([0-9]*\)לוּ,%\1lu,g s,%\([0-9]*\)וּ,%\1u,g +s,%\([0-9]*\)לוּ,%\1lu,g s,%\([0-9]*\)ללוּ,%\1llu,g +s,%\([0-9]*\)זוּ,%\1zu,g s,%\([0-9]*\)כּס,%\1x,g s,%\([0-9]*\)לכּס,%\1lx,g s,%\([0-9]*\)ללכּס,%\1llx,g -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/6] commands/i386/pc/sendkey: Fix "writing 1 byte into a region of size 0" build error
On Fri, 11 Mar 2022 00:35:57 +0100 Daniel Kiper wrote: > Latest GCC may complain in that way: > > commands/i386/pc/sendkey.c: In function ‘grub_sendkey_postboot’: > commands/i386/pc/sendkey.c:223:21: error: writing 1 byte into a region of > size 0 [-Werror=stringop-overflow=] > 223 | *((char *) 0x41a) = 0x1e; > | ~~^~ I'm curious why I'm not seeing this. It looks like the target was i386-pc, correct? I'm guessing this is on GCC11. What compiler are you using? Any extra CFLAGS? Anything else I might need to reproduce? Glenn > > The volatile keyword addition helps and additionally assures us the > compiler will not optimize out fixed assignments. > > Signed-off-by: Daniel Kiper > --- > grub-core/commands/i386/pc/sendkey.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/grub-core/commands/i386/pc/sendkey.c > b/grub-core/commands/i386/pc/sendkey.c > index 26d9acd3d..ab4bca9e9 100644 > --- a/grub-core/commands/i386/pc/sendkey.c > +++ b/grub-core/commands/i386/pc/sendkey.c > @@ -220,8 +220,8 @@ grub_sendkey_postboot (void) > >*flags = oldflags; > > - *((char *) 0x41a) = 0x1e; > - *((char *) 0x41c) = 0x1e; > + *((volatile char *) 0x41a) = 0x1e; > + *((volatile char *) 0x41c) = 0x1e; > >return GRUB_ERR_NONE; > } > @@ -236,8 +236,8 @@ grub_sendkey_preboot (int noret __attribute__ ((unused))) >oldflags = *flags; > >/* Set the sendkey. */ > - *((char *) 0x41a) = 0x1e; > - *((char *) 0x41c) = keylen + 0x1e; > + *((volatile char *) 0x41a) = 0x1e; > + *((volatile char *) 0x41c) = keylen + 0x1e; >grub_memcpy ((char *) 0x41e, sendkey, 0x20); > >/* Transform "any ctrl" to "right ctrl" flag. */ ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] po: Un-transliterate the %zu format code
Commit 45bffae13 (util/resolve: Bail with error if moddep.lst file line is too long) uses the %zu format specifier which has not been used in any translated strings yet. So the sed scripts used for tranliterating certain languages need to be updated otherwise creation of the message indexes will fail on an unknown format code. This is essentially the same issue fixed for the %m format code in commit 2e246b6f (po: Fix replacement of %m in sed programs). Also reorder transliteration lines to be more lexographically ordered. Signed-off-by: Glenn Washburn --- po/arabic.sed | 3 ++- po/cyrillic.sed | 3 ++- po/greek.sed| 3 ++- po/hebrew.sed | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/po/arabic.sed b/po/arabic.sed index 21dc8b6db2..3fbee72480 100644 --- a/po/arabic.sed +++ b/po/arabic.sed @@ -74,9 +74,10 @@ s,%\([0-9]*\)للد,%\1lld,g s,%\([0-9\.\*]*\)س,%\1s,g s,%\([0-9\.\*]*\)م,%\1m,g -s,%\([0-9]*\)لو,%\1lu,g s,%\([0-9]*\)و,%\1u,g +s,%\([0-9]*\)لو,%\1lu,g s,%\([0-9]*\)للو,%\1llu,g +s,%\([0-9]*\)زو,%\1zu,g s,%\([0-9]*\)كس,%\1x,g s,%\([0-9]*\)لكس,%\1lx,g s,%\([0-9]*\)للكس,%\1llx,g diff --git a/po/cyrillic.sed b/po/cyrillic.sed index 2e3e6655d6..472f095295 100644 --- a/po/cyrillic.sed +++ b/po/cyrillic.sed @@ -97,9 +97,10 @@ s,%\([0-9]*\)ллд,%\1lld,g s,%\([0-9\.\*]*\)с,%\1s,g s,%\([0-9\.\*]*\)м,%\1m,g -s,%\([0-9]*\)лу,%\1lu,g s,%\([0-9]*\)у,%\1u,g +s,%\([0-9]*\)лу,%\1lu,g s,%\([0-9]*\)ллу,%\1llu,g +s,%\([0-9]*\)зу,%\1zu,g s,%\([0-9]*\)ѯ,%\1x,g s,%\([0-9]*\)лѯ,%\1lx,g s,%\([0-9]*\)ллѯ,%\1llx,g diff --git a/po/greek.sed b/po/greek.sed index 3543f6aac3..0e81625fbb 100644 --- a/po/greek.sed +++ b/po/greek.sed @@ -99,9 +99,10 @@ s,%\([0-9]*\)λλδ,%\1lld,g s,%\([0-9\.\*]*\)σ,%\1s,g s,%\([0-9\.\*]*\)μ,%\1m,g -s,%\([0-9]*\)λυ,%\1lu,g s,%\([0-9]*\)υ,%\1u,g +s,%\([0-9]*\)λυ,%\1lu,g s,%\([0-9]*\)λλυ,%\1llu,g +s,%\([0-9]*\)ζυ,%\1zu,g s,%\([0-9]*\)ξ,%\1x,g s,%\([0-9]*\)λξ,%\1lx,g s,%\([0-9]*\)λλξ,%\1llx,g diff --git a/po/hebrew.sed b/po/hebrew.sed index 9210014bc3..33174bbdcc 100644 --- a/po/hebrew.sed +++ b/po/hebrew.sed @@ -82,9 +82,10 @@ s,%\([0-9]*\)ללד,%\1lld,g s,%\([0-9\.\*]*\)ש,%\1s,g s,%\([0-9\.\*]*\)מ,%\1m,g -s,%\([0-9]*\)לוּ,%\1lu,g s,%\([0-9]*\)וּ,%\1u,g +s,%\([0-9]*\)לוּ,%\1lu,g s,%\([0-9]*\)ללוּ,%\1llu,g +s,%\([0-9]*\)זוּ,%\1zu,g s,%\([0-9]*\)כּס,%\1x,g s,%\([0-9]*\)לכּס,%\1lx,g s,%\([0-9]*\)ללכּס,%\1llx,g -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] po: Un-transliterate the %zu format code
On Wed, 9 Mar 2022 17:10:01 +0100 Daniel Kiper wrote: > On Mon, Mar 07, 2022 at 12:15:36PM -0600, Glenn Washburn wrote: > > Commit 45bffae13 uses the %zu format specifier which has not been used in > > If you mention a commit please use the following format: > commit 45bffae13 (util/resolve: Bail with error if moddep.lst file line is > too long) Ok, I'm not clear if this is just for the future, or you want me to resend this patch. I have no problem resending. Glenn > > > any translated strings yet. So the sed scripts used for tranliterating > > certain languages need to be updated otherwise creation of the message > > indexes will fail on an unknown format code. This is essentially the same > > issue fixed for the %m format code in commit 2e246b6f. > > Ditto. > > > Signed-off-by: Glenn Washburn > > --- > > po/arabic.sed | 3 ++- > > po/cyrillic.sed | 3 ++- > > po/greek.sed| 3 ++- > > po/hebrew.sed | 3 ++- > > 4 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/po/arabic.sed b/po/arabic.sed > > index 21dc8b6db2..3fbee72480 100644 > > --- a/po/arabic.sed > > +++ b/po/arabic.sed > > @@ -74,9 +74,10 @@ s,%\([0-9]*\)للد,%\1lld,g > > > > s,%\([0-9\.\*]*\)س,%\1s,g > > s,%\([0-9\.\*]*\)م,%\1m,g > > -s,%\([0-9]*\)لو,%\1lu,g > > s,%\([0-9]*\)و,%\1u,g > > +s,%\([0-9]*\)لو,%\1lu,g > > I am OK with this move but please mention it in the commit message. > > > s,%\([0-9]*\)للو,%\1llu,g > > +s,%\([0-9]*\)زو,%\1zu,g > > s,%\([0-9]*\)كس,%\1x,g > > s,%\([0-9]*\)لكس,%\1lx,g > > s,%\([0-9]*\)للكس,%\1llx,g > > diff --git a/po/cyrillic.sed b/po/cyrillic.sed > > index 2e3e6655d6..472f095295 100644 > > --- a/po/cyrillic.sed > > +++ b/po/cyrillic.sed > > @@ -97,9 +97,10 @@ s,%\([0-9]*\)ллд,%\1lld,g > > > > s,%\([0-9\.\*]*\)с,%\1s,g > > s,%\([0-9\.\*]*\)м,%\1m,g > > -s,%\([0-9]*\)лу,%\1lu,g > > s,%\([0-9]*\)у,%\1u,g > > +s,%\([0-9]*\)лу,%\1lu,g > > Ditto and below... > > Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 0/6] Update gnulib version and drop most gnulib patches
On Wed, 9 Mar 2022 16:42:43 +0100 Daniel Kiper wrote: > On Mon, Mar 07, 2022 at 02:15:49AM -0600, Glenn Washburn wrote: > > On Fri, 4 Mar 2022 18:01:10 -0600 > > Glenn Washburn wrote: > > > > > On Wed, 2 Mar 2022 14:08:23 -0500 > > > Robbie Harwood wrote: > > > > > > > Changes this version: > > > > > > > > - Reorder last two commits so that warning fixes come after the change > > > > that > > > > introduces them. > > > > - Fix comment formatting to comply with grub2 style. > > > > > > Either I missed it before or something changed. But I'm getting this > > > build error now for x86_64-efi, and I'm not getting it without this patch > > > series. > > > > > > In file included from > > > /root/grub-tests.update-gnulib/grub/include/grub/disk.h:31 > > > , > > > from > > > /root/grub-tests.update-gnulib/grub/include/grub/file.h:26 > > > , > > > from > > > /root/grub-tests.update-gnulib/grub/include/grub/loader.h: > > > 23, > > > from > > > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/ > > > bsd.c:19: > > > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c: In > > > function ‘gr > > > ub_freebsd_add_meta_module’: > > > /root/grub-tests.update-gnulib/grub/include/grub/misc.h:71:10: error: > > > ‘ptr’ may be used uninitialized in this function > > > [-Werror=maybe-uninitialized] > > >71 | return grub_memmove (dest, src, n); > > > | ^~~ > > > /root/grub-tests.update-gnulib/grub/grub-core/loader/i386/bsd.c:266:9: > > > note: ‘ptr’ was declared here > > > 266 | void *ptr; > > > | ^~~ > > > > > > Reviewing the code it doesn't look like ptr can actually be used > > > uninitialized, so it seems like GCC 10.1.0 isn't smart enough to figure > > > that out. Initializing to NULL fixes the build issue. > > > > For completeness, I just realized that I wasn't using the compiler I > > thought I was using. Turns out this issue is with x86_64 Debian 11's > > GCC, which is 10.2.1. I don't think this is relevant to the issue, but > > may help in reproducing if needed. > > > > After some more testing, I've narrowed it down to passing -O2 to gcc, > > without which the series builds fine. > > Thanks for the report. I am just running build tests for all architectures > and platforms with GCC 10/11. I have spotted and fixed 2 additional problems. > I hope I will post fixes for all of them tomorrow. They are not, build errors, are they? If so, I'm curious what targets they are for and why I'm not seeing them either. Glenn > > Robbie and I discovered another gnulib build issue for Windows platforms. > I prepared a patch and will share it with Robbie to send it in updated > gnulib v9 patchset. > > Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] gdb: Add malloc and free symbols to kernel.exec to improve gdb functionality
On Wed, 9 Mar 2022 16:49:57 +0100
Daniel Kiper wrote:
> On Wed, Mar 02, 2022 at 06:25:12PM -0600, Glenn Washburn wrote:
> > Add linker flags when linking kernel.exec to have malloc and free point to
> > grub_malloc and grub_free respectively. Some gdb functionality depends on
> > gdb locating the symbols "malloc" and "free", such as dynamically creating
> > strings for arguments to injected function calls. A trivial example would
> > the gdb command 'p strlen("astring")'.
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > This should have been included in the gdb patch series I recently sent,
> > although its not required by nor requires any of those patches.
> >
> > Glenn
> >
> > ---
> > conf/Makefile.common | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/conf/Makefile.common b/conf/Makefile.common
> > index f0bb6e160a..069b428c1a 100644
> > --- a/conf/Makefile.common
> > +++ b/conf/Makefile.common
> > @@ -36,6 +36,7 @@ BUILD_CPPFLAGS += $(CPPFLAGS_DEFAULT)
> >
> > CFLAGS_KERNEL = $(CFLAGS_PLATFORM) -ffreestanding
> > LDFLAGS_KERNEL = $(LDFLAGS_PLATFORM) -nostdlib $(TARGET_LDFLAGS_OLDMAGIC)
> > +LDFLAGS_KERNEL += -Wl,--defsym=malloc=grub_malloc
> > -Wl,--defsym=free=grub_free
>
> Could not we teach gdb somehow to use grub_malloc()/grub_free() instead
> of malloc()/free()?
Considering the tons of options that GDB has, I was hoping the same
thing. Unfortunately, it appears to be hardcoded[1]. So not without
changing the source.
Glenn
[1]
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/valops.c;hb=HEAD#l169
>
> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/3] net: Unset grub_net_poll_cards_idle when net module has been unloaded
Signed-off-by: Glenn Washburn --- grub-core/net/net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/net/net.c b/grub-core/net/net.c index 4d3eb5c1a5..563dea9ec8 100644 --- a/grub-core/net/net.c +++ b/grub-core/net/net.c @@ -1948,5 +1948,5 @@ GRUB_MOD_FINI(net) grub_net_open = NULL; grub_net_fini_hw (0); grub_loader_unregister_preboot_hook (fini_hnd); - grub_net_poll_cards_idle = grub_net_poll_cards_idle_real; + grub_net_poll_cards_idle = NULL; } -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 0/3] Net fix and improvements
The first patch looks like it was a copy/paste error. If the net module is unloaded, grub_net_poll_cards_idle should be NULL so that a function in the net module which now doesn't exist. The second and third patches are for performance and were helpful when debugging GRUB. When the net module is loaded, even if there are no net cards found, grub_net_poll_cards_idle will call grub_net_tcp_retransmit() unconditionally. But there's no need to go through tcp retransmit logic if there aren't any cards that we could be retransmitting on. As for the third patch, if the machine has network cards found, but there are no tcp open sockets (because the user doesn't use the network to boot), then grub_net_tcp_retransmit() should be a noop. Thus is doesn't need to call grub_get_time_ms(), which does a call into firmware on powerpc-ieee1275. So only call grub_get_time_ms() if there are tcp sockets. Calls to grub_net_poll_cards_idle can happen a lot when GRUB is waiting for a keypress (grub_getkey_noblock calls grub_net_poll_cards_idle). I found this annoying when debugging an issue in GRUB with GDB and I found that nearly every time I interrupted GRUB with the GDB I was landing in OpenBIOS's milliseconds call and then I had to step out back into GRUB which could be a hundred or more instructions. This reduces the probability that interrupting GRUB lands in the firmware when GRUB is blocked waiting on a keypress. Glenn Glenn Washburn (3): net: Unset grub_net_poll_cards_idle when net module has been unloaded net: Avoid unnecessary calls to grub_net_tcp_retransmit net/tcp: Only call grub_get_time_ms when there are sockets to potentially retransmit for grub-core/net/net.c | 5 +++-- grub-core/net/tcp.c | 9 +++-- 2 files changed, 10 insertions(+), 4 deletions(-) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/3] net: Avoid unnecessary calls to grub_net_tcp_retransmit
In grub_net_poll_cards_idle_real, only call grub_net_tcp_retransmit if there are network cards found. If there are no network card found, there can be no tcp sockets to transmit on. Signed-off-by: Glenn Washburn --- grub-core/net/net.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/grub-core/net/net.c b/grub-core/net/net.c index 563dea9ec8..e208b84019 100644 --- a/grub-core/net/net.c +++ b/grub-core/net/net.c @@ -1580,7 +1580,8 @@ grub_net_poll_cards_idle_real (void) || ctime >= card->last_poll + card->idle_poll_delay_ms) receive_packets (card, 0); } - grub_net_tcp_retransmit (); + if (grub_net_cards) +grub_net_tcp_retransmit (); } /* Read from the packets list*/ -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/3] net/tcp: Only call grub_get_time_ms when there are sockets to potentially retransmit for
If there are no TCP sockets, this call to grub_get_time_ms is unneeded. This
prevents a call into the firmware on some platforms.
Signed-off-by: Glenn Washburn
---
grub-core/net/tcp.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/grub-core/net/tcp.c b/grub-core/net/tcp.c
index e8ad34b84d..75461eb54e 100644
--- a/grub-core/net/tcp.c
+++ b/grub-core/net/tcp.c
@@ -362,8 +362,13 @@ void
grub_net_tcp_retransmit (void)
{
grub_net_tcp_socket_t sock;
- grub_uint64_t ctime = grub_get_time_ms ();
- grub_uint64_t limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
+ grub_uint64_t ctime = 0, limit_time = 0;
+
+ if (tcp_sockets)
+{
+ ctime = grub_get_time_ms ();
+ limit_time = ctime - TCP_RETRANSMISSION_TIMEOUT;
+}
FOR_TCP_SOCKETS (sock)
{
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free
On Tue, 22 Feb 2022 19:03:39 +0100 Daniel Kiper wrote: > On Tue, Feb 15, 2022 at 12:36:42PM -0600, Glenn Washburn wrote: > > These functions may be useful within modules as well. Export them so that > > modules can use them. > > Though there are no users for these functions today. So, I am not > convinced we should export them. How do you know there are no users for these functions today? As I see it, the point of these functions is only for doing memory debugging. They should not be enabled in general use. So no modules should ever use them unconditionally. Also, here they are only export _if_ configure was passed --mm-debug or MM_DEBUG is defined, which is never don't by default and likey only done when someone would be more likely to use these functions. I'm not quite following what the concern is. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] misc: Fix whitespace formatting
Signed-off-by: Glenn Washburn --- tests/util/grub-fs-tester.in | 246 +-- 1 file changed, 123 insertions(+), 123 deletions(-) diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in index a1f3f299b4..65633c7f81 100644 --- a/tests/util/grub-fs-tester.in +++ b/tests/util/grub-fs-tester.in @@ -53,27 +53,27 @@ case x"$fs" in MAXLOGSECSIZE=12;; xvfat*|xmsdos*) MINLOGSECSIZE=9 - # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 + # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 MAXLOGSECSIZE=12;; xext*) MINLOGSECSIZE=8 MAXLOGSECSIZE=12;; xbtrfs*) MINLOGSECSIZE=12 - # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 + # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 MAXLOGSECSIZE=12;; xxfs) MINLOGSECSIZE=9 - # OS LIMITATION: GNU/Linux doesn't accept > 4096 + # OS LIMITATION: GNU/Linux doesn't accept > 4096 MAXLOGSECSIZE=12;; xxfs_crc) MINLOGSECSIZE=9 - # OS LIMITATION: GNU/Linux doesn't accept > 1024 + # OS LIMITATION: GNU/Linux doesn't accept > 1024 MAXLOGSECSIZE=10;; xzfs*) - # OS LIMITATION: zfs-fuse hangs when creating zpool with sectors <=256B. + # OS LIMITATION: zfs-fuse hangs when creating zpool with sectors <=256B. MINLOGSECSIZE=9 - # OS LIMITATION: zfs-fuse fails with >= 32K sectors. + # OS LIMITATION: zfs-fuse fails with >= 32K sectors. # OS limitation: zfs-fuse always uses ashift=9 with loop devices MAXLOGSECSIZE=9;; esac @@ -97,7 +97,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do ;; xexfat*) MINBLKSIZE=$SECSIZE - # It could go further but it requires more and more space + # It could go further but it requires more and more space MAXBLKSIZE=8286208 ;; xhfs) @@ -134,41 +134,41 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do ;; x"mdraid"*) MINBLKSIZE=4096 - # OS LIMITATION: Linux oopses with >=32768K + # OS LIMITATION: Linux oopses with >=32768K MAXBLKSIZE=$((16384*1024)) ;; x"lvm_raid1"* | x"lvm_raid4" | x"lvm_raid5" | x"lvm_raid6") - # OS LIMITATION: Linux crashes with less than 16384 + # OS LIMITATION: Linux crashes with less than 16384 MINBLKSIZE=16384 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_mirrorall") MINBLKSIZE=2048 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_mirror1") MINBLKSIZE=4096 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_stripe") MINBLKSIZE=4096 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm"*) MINBLKSIZE=1024 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; - xext4_encrypt) - # OS LIMITATION: Linux currently only allows the 'encrypt' feature - # in combination with block_size = PAGE_SIZE (4096 bytes on x86). - MINBLKSIZE=$(getconf PAGE_SIZE) - MAXBLKSIZE=$MINBLKSIZE - ;; + xext4_encrypt) + # OS LIMITATION: Linux currently only allows the 'encrypt' feature + # in combination with block_size = PAGE_SIZE (4096 bytes on x86). + MINBLKSIZE=$(getconf PAGE_SIZE) + MAXBLKSIZE=$MINBLKSIZE + ;; xext*) MINBLKSIZE=1024 if [ $MINBLKSIZE -lt $SECSIZE ]; then @@ -181,7 +181,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do MAXBLKSIZE=1048576;; xxfs|xf2fs) MINBLKSIZE=$SECSIZE - # OS Limitation: GNU/Linux doesn't accept > 4096 + # OS Limitation: GNU/Linux doesn't accept > 4096 MAXBLKSIZE=4096;; xxfs_crc) # OS Limitation: GNU/Linux doesn't accept != 1024 @@ -195,11 +195,11 @@ for LOGSECSIZE in $(r
Re: [PATCH] misc: Fix whitespace formatting
On Tue, 22 Feb 2022 14:49:39 +0100 Daniel Kiper wrote: > On Tue, Feb 15, 2022 at 12:44:44PM -0600, Glenn Washburn wrote: > > Signed-off-by: Glenn Washburn > > --- > > Daniel, > > > > I think I recall you saying you accumulate whitespace fixes. So here's a > > Yeah... Sadly it looks it will not apply to the latest upstream... :-( Hmm, I just did a fetch of master and at commit a9c22577. That's not the latest? The patch does apply albeit with the hunks being offset. > > patch to add to those, if so. Otherwise, the commit message could probably > > be a little better,so feel free to modify as desired. > > For patches like that one I think it is enough to have proper subject. If you want to fix the title, please do. I can fix the title if this needs another version to fix it for current master, but it seems fine to me. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support
Finally getting back to this... On Thu, 17 Feb 2022 17:18:47 -0500 James Bottomley wrote: > On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote: > > On Mon, 7 Feb 2022 10:29:43 -0500 > > James Bottomley wrote: > > > > > Make use of the new OS provided secrets API so that if the new '-s' > > > option is passed in we try to extract the secret from the API > > > rather than prompting for it. > > > > > > The primary consumer of this is AMD SEV, which has been programmed > > > to provide an injectable secret to the encrypted virtual > > > machine. OVMF provides the secret area and passes it into the EFI > > > Configuration Tables. The grub EFI layer pulls the secret out and > > > primes the secrets API with it. The upshot of all of this is that > > > a SEV protected VM can do an encrypted boot with a protected boot > > > secret. > > > > I think I prefer the key protector framework proposed in the first > > patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It > > feels like a more generic mechanism (though admittedly very similar > > to yours) because its not tied to cryptodisk. > > It's not really an either/or, though is it? It looks to me like one > can trivially evolve into the other. Yep, this patch can easily become essentially what the key protector framework is. I make the comparison because (1) its pending so it seems likely that this would be modified soon, (2) I like that the key protectors framework is in its own file as opposed to put in with cryptodisk (though this patch is smaller so makes less sense as a separate file), and (3) I find it aestetically unpleasing to add this patch and then turn around and take it out and put it somewhere else. But yes, it can be done. > > I'm not sure where we'd want to use the secrets/protectors framework > > outside of cryptodisk, but it seems like it could be useful. The > > advantage of this patch is that it allows for the clearing of key > > data from memory. > > So if you evolve one into the other the put API would naturally remain > even if the tpm2 protector didn't want it. Yep, we're on the same page here. > > I don't think we should have both a secrets and a key protectors > > framework, as its seems they are aiming to accomplish basically the > > same thing. > > Well, I don't think we would either. Looking at the protectors API, it > would become an incremental patch to this. Where I come from, this is > how we do API development: you merge an implementation and an API > that's as minimal as possible. You try to make sure the API can be > extended if necessary, but you don't extend it yourself until another > implementation comes along that needs the extension. This way you > don't need to bikeshed about future implementations because the perfect > is the enemy of the good. Yep, agreed. Both patches provide minimal APIs for the feature sets they are introducing, as it should be. Aside from name changes, I'm not criticizing your API (which to be clear I'm meaning the function signatures). So I'm not seeing the above as relevant here. I suspect that the core of your issue is that I was too general in my comments above giving the impression that I'm rejecting this patch without a clear path towards making it acceptable. To be more specific, I liked that the key protectors patch was separated from cryptodisk both in naming and in a separate file. I've since reconsidered the separate file, which also doesn't make as much sense for this patch because its small. I forgot to mention it before, but I was also thinking that the function names could have a grub_secret_ prefix instead of grub_cryptodisk_. I think another issue, in which I've been on your side of the table of, is determining which suggestions or dislikes are deal breakers. In the abcense of clear communication on what's a deal breaker it can seem like all are deal breakers. Me registering a complaint about something doesn't necessarily mean I think it can't be accepted as is. However, as a reviewer I feel it is my duty to list everything I have a problem with. Ideally it should be very clear, but sometimes its hard to pin point how a patch might be changed to be better. And if you want more clarity (eg. on the specifics of a displike or how much of a deal breaker something is) asking never hurts. As for perfect being the enemy of the good, that's only if one forego's the good in hand for an unattainable perfect. Good should just be an iteration toward perfect. Perfect is the north star that provides guidance on what is good. So I'm noting where its not perfect or at least can be better (according to me). That doesn't mean if its not perfect it can't be committed. But if we can make changes to make good better now, then I
Re: [PATCH 2/2] grub-core/loader/i386/multiboot_mbi.c: Remove dead increment
On Mon, 28 Feb 2022 21:48:35 +0100 Elyes Haouas wrote: > The value stored to 'ptrdest' is never read > > Signed-off-by: Elyes Haouas > --- > grub-core/loader/i386/multiboot_mbi.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/grub-core/loader/i386/multiboot_mbi.c > b/grub-core/loader/i386/multiboot_mbi.c > index a67d9d0a8..663b25cc5 100644 > --- a/grub-core/loader/i386/multiboot_mbi.c > +++ b/grub-core/loader/i386/multiboot_mbi.c > @@ -609,7 +609,6 @@ grub_multiboot_make_mbi (grub_uint32_t *target) >ptrorig += sizeof (struct grub_vbe_info_block); >ptrdest += sizeof (struct grub_vbe_info_block); >ptrorig += sizeof (struct grub_vbe_mode_info_block); > - ptrdest += sizeof (struct grub_vbe_mode_info_block); I'm not familiar with this code and just did a cursory glance, but it looks to me like none of these above 4 lines are used nor in the "if" block above them. This begs for more investigation. It looks to me like either there's code missing after this or this apparently useless code wasn't removed during a refactor. Both ptrdest and ptrorig are stack variables, so I don't think there's any magic in writing to them. What am I missing? Until this is clarified, I don't think we should be removing this code. Glenn > #endif > > #ifdef GRUB_MACHINE_EFI ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free
These functions may be useful within modules as well. Export them so that modules can use them. Signed-off-by: Glenn Washburn --- include/grub/mm.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/grub/mm.h b/include/grub/mm.h index 9c38dd3ca5..44fde7cb90 100644 --- a/include/grub/mm.h +++ b/include/grub/mm.h @@ -46,8 +46,8 @@ void grub_mm_check_real (const char *file, int line); /* Set this variable to 1 when you want to trace all memory function calls. */ extern int EXPORT_VAR(grub_mm_debug); -void grub_mm_dump_free (void); -void grub_mm_dump (unsigned lineno); +void EXPORT_FUNC(grub_mm_dump_free) (void); +void EXPORT_FUNC(grub_mm_dump) (unsigned lineno); #define grub_calloc(nmemb, size) \ grub_debug_calloc (GRUB_FILE, __LINE__, nmemb, size) -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] misc: Fix whitespace formatting
Signed-off-by: Glenn Washburn --- Daniel, I think I recall you saying you accumulate whitespace fixes. So here's a patch to add to those, if so. Otherwise, the commit message could probably be a little better,so feel free to modify as desired. Glenn --- tests/util/grub-fs-tester.in | 246 +-- 1 file changed, 123 insertions(+), 123 deletions(-) diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in index a88cec2482..b2a8e3723f 100644 --- a/tests/util/grub-fs-tester.in +++ b/tests/util/grub-fs-tester.in @@ -81,27 +81,27 @@ case x"$fs" in MAXLOGSECSIZE=12;; xvfat*|xmsdos*) MINLOGSECSIZE=9 - # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 + # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 MAXLOGSECSIZE=12;; xext*) MINLOGSECSIZE=8 MAXLOGSECSIZE=12;; xbtrfs*) MINLOGSECSIZE=12 - # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 + # OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096 MAXLOGSECSIZE=12;; xxfs) MINLOGSECSIZE=9 - # OS LIMITATION: GNU/Linux doesn't accept > 4096 + # OS LIMITATION: GNU/Linux doesn't accept > 4096 MAXLOGSECSIZE=12;; xxfs_crc) MINLOGSECSIZE=9 - # OS LIMITATION: GNU/Linux doesn't accept > 1024 + # OS LIMITATION: GNU/Linux doesn't accept > 1024 MAXLOGSECSIZE=10;; xzfs*) - # OS LIMITATION: zfs-fuse hangs when creating zpool with sectors <=256B. + # OS LIMITATION: zfs-fuse hangs when creating zpool with sectors <=256B. MINLOGSECSIZE=9 - # OS LIMITATION: zfs-fuse fails with >= 32K sectors. + # OS LIMITATION: zfs-fuse fails with >= 32K sectors. # OS limitation: zfs-fuse always uses ashift=9 with loop devices MAXLOGSECSIZE=9;; esac @@ -125,7 +125,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do ;; xexfat*) MINBLKSIZE=$SECSIZE - # It could go further but it requires more and more space + # It could go further but it requires more and more space MAXBLKSIZE=8286208 ;; xhfs) @@ -162,41 +162,41 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do ;; x"mdraid"*) MINBLKSIZE=4096 - # OS LIMITATION: Linux oopses with >=32768K + # OS LIMITATION: Linux oopses with >=32768K MAXBLKSIZE=$((16384*1024)) ;; x"lvm_raid1"* | x"lvm_raid4" | x"lvm_raid5" | x"lvm_raid6") - # OS LIMITATION: Linux crashes with less than 16384 + # OS LIMITATION: Linux crashes with less than 16384 MINBLKSIZE=16384 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_mirrorall") MINBLKSIZE=2048 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_mirror1") MINBLKSIZE=4096 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm_stripe") MINBLKSIZE=4096 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; x"lvm"*) MINBLKSIZE=1024 - # Could go further but what's the point? + # Could go further but what's the point? MAXBLKSIZE=$((65536*1024)) ;; - xext4_encrypt) - # OS LIMITATION: Linux currently only allows the 'encrypt' feature - # in combination with block_size = PAGE_SIZE (4096 bytes on x86). - MINBLKSIZE=$(getconf PAGE_SIZE) - MAXBLKSIZE=$MINBLKSIZE - ;; + xext4_encrypt) + # OS LIMITATION: Linux currently only allows the 'encrypt' feature + # in combination with block_size = PAGE_SIZE (4096 bytes on x86). + MINBLKSIZE=$(getconf PAGE_SIZE) + MAXBLKSIZE=$MINBLKSIZE + ;; xext*) MINBLKSIZE=1024 if [ $MINBLKSIZE -lt $SECSIZE ]; then @@ -209,7 +209,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do MAXBLKSIZE=1048576;; xxfs|xf2fs) MINBLKSIZE=$SECSIZE - # OS Limitation: GNU/Linux doesn't a
Re: [PATCH v2 2/2] lsefi: fixed memory leaks
On Tue, 15 Feb 2022 14:07:15 +0100 Renaud Métrich wrote: > Please ignore, deprecated by "efi: new 'connectefi' command" (v3). I'm not sure what was eactly wrong with v3 (haven't looked at the difference). However, v3 was done better in some regards. On this list it is customary to have multi-patch series both threaded and with a cover letter, as was done in v3. v2 is confusing because there are apparently 3 patches, 2 of which are duplicates (or so it seems from the subject). Of the 3 patches two are threaded, but one is on its own thread. For a patch series such as this, it is recommended to use git format-patches --thread and --cover-letter options and have a brief explanation of the series in the cover letter. I also like to use either --range-diff or --interdiff (as appropriate) when not the first version of the patch series. So if there is some problem with the actual changes in v3, I recommend creating a v4 incorporating the above suggestions. Glenn > > Sorry for the mess. > > Le 2/14/22 à 14:25, Renaud Métrich a écrit : > > Signed-off-by: Renaud Métrich > > --- > > grub-core/commands/efi/lsefi.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c > > index 7acba3b39..f0b9201f1 100644 > > --- a/grub-core/commands/efi/lsefi.c > > +++ b/grub-core/commands/efi/lsefi.c > > @@ -186,8 +186,12 @@ grub_cmd_lsefi (grub_command_t cmd __attribute__ > > ((unused)), > > (unsigned) protocols[j]->data4[7]); > > } > > > > + if (protocols) > > + grub_efi_free_pool (protocols); > > } > > > > + grub_free (handles); > > + > > return 0; > > } > > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/3] configure: Properly handle MM_DEBUG
Define MM_DEBUG in config.h when --enable-mm-debug is passed to configure. It was being defined in config-util.h which only gets used when building GRUB utilities for the host side. The enabling of debugging for memory management in include/grub/mm.h explicitly does not happen when compiling for the GRUB utilities. So this debugging code effectively could never be enabled. Note, that MM_DEBUG is defined in an #if directive because the enabling of debugging checks if MM_DEBUG is defined, not what its value is. So even if MM_DEBUG were defined to nothing, the debugging code would still be enabled. Signed-off-by: Glenn Washburn --- config.h.in | 4 configure.ac | 6 -- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/config.h.in b/config.h.in index 9e8f9911b1..c1323a88e3 100644 --- a/config.h.in +++ b/config.h.in @@ -9,6 +9,10 @@ #define GCRYPT_NO_DEPRECATED 1 #define HAVE_MEMMOVE 1 +#if @MM_DEBUG@ +#define MM_DEBUG @MM_DEBUG@ +#endif + /* Define to 1 to enable disk cache statistics. */ #define DISK_CACHE_STATS @DISK_CACHE_STATS@ #define BOOT_TIME_STATS @BOOT_TIME_STATS@ diff --git a/configure.ac b/configure.ac index 5c01af0fab..65d7f8a3be 100644 --- a/configure.ac +++ b/configure.ac @@ -1519,9 +1519,11 @@ AC_ARG_ENABLE([mm-debug], AS_HELP_STRING([--enable-mm-debug], [include memory manager debugging])) if test x$enable_mm_debug = xyes; then -AC_DEFINE([MM_DEBUG], [1], -[Define to 1 if you enable memory manager debugging.]) +MM_DEBUG=1 +else +MM_DEBUG=0 fi +AC_SUBST([MM_DEBUG]) AC_ARG_ENABLE([cache-stats], AS_HELP_STRING([--enable-cache-stats], -- 2.27.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] net: fix null pointer dereference when parsing ICMP6_ROUTER_ADVERTISE messages
On Thu, 17 Feb 2022 21:48:58 +0800
Qiumiao Zhang via Grub-devel wrote:
> During UEFI PXE boot in IPv6 network, if the DHCP server adopts stateful
> automatic
> configuration, when the client receives the ICMP6_ROUTER_ADVERTISE message
> multicast
> from the server, it will cause the problem of dereference null
> pointer and cause the grub2 program to crash.
This commit message could be more clear. Maybe have something like:
During UEFI PXE boot in IPv6 network, if the DHCP server adopts
stateful automatic configuration, then the client receives a
ICMP6_ROUTER_ADVERTISE multicast message from the server. This may be
received without the interfaced having a configured network address,
so orig_inf will be null, which can lead to a null dereference when
creating the default route.
Of course, assuming that the above is in fact correct.
>
> Fixes bug: https://savannah.gnu.org/bugs/index.php?62072
> ---
> grub-core/net/icmp6.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grub-core/net/icmp6.c b/grub-core/net/icmp6.c
> index 2cbd95d..264fc4a 100644
> --- a/grub-core/net/icmp6.c
> +++ b/grub-core/net/icmp6.c
> @@ -477,7 +477,7 @@ grub_net_recv_icmp6_packet (struct grub_net_buff *nb,
>
> /* May not have gotten slaac info, find a global address on this
> card. */
> - if (route_inf == NULL)
> + if (route_inf == NULL && orig_inf != NULL)
So if orig_inf == NULL and route_inf == NULL here, we do not set a
default route. Does this have any implications to be concerned about?
In this case, can we still find a good route interface and setup a
default route?
Glenn
> {
> FOR_NET_NETWORK_LEVEL_INTERFACES (inf)
> {
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 4/5] Drop gnulib no-abort.patch
On Wed, 2 Mar 2022 18:00:11 +0100
Daniel Kiper wrote:
> On Thu, Feb 24, 2022 at 01:37:19PM -0500, Robbie Harwood wrote:
> > Originally added in db7337a3d353a817ffe9eb4a3702120527100be9, this
> > patched out all relevant invocations of abort() in gnulib. While it was
> > not documented why at the time, testing suggests that there's no abort()
> > implementation available for gnulib to use.
> >
> > gnulib's position is that the use of abort() is correct here, since it
> > happens when input violates a "shall" from POSIX. Additionally, the
> > code in question is probably not reachable. Since abort() is more
> > friendly to user-space, they prefer to make no change, so we can just
> > carry a define instead. (Suggested by Paul Eggert.)
> >
> > Signed-off-by: Robbie Harwood
> > ---
> > bootstrap.conf | 2 +-
> > conf/Makefile.extra-dist| 1 -
> > config.h.in | 3 +++
> > grub-core/lib/gnulib-patches/no-abort.patch | 26 -
> > 4 files changed, 4 insertions(+), 28 deletions(-)
> > delete mode 100644 grub-core/lib/gnulib-patches/no-abort.patch
> >
> > diff --git a/bootstrap.conf b/bootstrap.conf
> > index 21a8cf15d..71acbeeb1 100644
> > --- a/bootstrap.conf
> > +++ b/bootstrap.conf
> > @@ -82,7 +82,7 @@ cp -a INSTALL INSTALL.grub
> > bootstrap_post_import_hook () {
> >set -e
> >for patchname in fix-null-deref fix-null-state-deref
> > fix-regcomp-uninit-token \
> > - fix-regexec-null-deref fix-uninit-structure fix-unused-value
> > fix-width no-abort; do
> > + fix-regexec-null-deref fix-uninit-structure fix-unused-value
> > fix-width; do
> > patch -d grub-core/lib/gnulib -p2 \
> >< "grub-core/lib/gnulib-patches/$patchname.patch"
> >done
> > diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist
> > index 15a9b74e9..4ddc3c8f7 100644
> > --- a/conf/Makefile.extra-dist
> > +++ b/conf/Makefile.extra-dist
> > @@ -35,7 +35,6 @@ EXTRA_DIST +=
> > grub-core/lib/gnulib-patches/fix-regexec-null-deref.patch
> > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-uninit-structure.patch
> > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-unused-value.patch
> > EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch
> > -EXTRA_DIST += grub-core/lib/gnulib-patches/no-abort.patch
> >
> > EXTRA_DIST += grub-core/lib/libgcrypt
> > EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic
> > diff --git a/config.h.in b/config.h.in
> > index 965eaffce..a1f2b63e6 100644
> > --- a/config.h.in
> > +++ b/config.h.in
> > @@ -66,6 +66,9 @@
> >
> > # ifndef _GL_INLINE_HEADER_BEGIN
> > #define _GL_ATTRIBUTE_CONST __attribute__ ((const))
> > +
> > +/* We don't have an abort() for gnulib to call in regexp. */
> > +#define abort __builtin_unreachable
>
> There are more abort() calls in the gnulib than just regexp one. So,
> I think this definition is wrong. IMO we should print an error message
> if possible and jump into endless loop or use __builtin_trap() to safely
> reboot the machine.
Is it naive to think we can use grub_abort()?
Glenn
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 4/6] Drop gnulib no-abort.patch
On Thu, 03 Mar 2022 11:58:11 -0500
Robbie Harwood wrote:
> Glenn Washburn writes:
>
> > On Wed, 2 Mar 2022 14:08:27 -0500
> > Robbie Harwood wrote:
> >
> >> Originally added in db7337a3d353a817ffe9eb4a3702120527100be9, this
> >> patched out all relevant invocations of abort() in gnulib. While it was
> >> not documented why at the time, testing suggests that there's no abort()
> >> implementation available for gnulib to use.
> >>
> >> gnulib's position is that the use of abort() is correct here, since it
> >> happens when input violates a "shall" from POSIX. Additionally, the
> >> code in question is probably not reachable. Since abort() is more
> >> friendly to user-space, they prefer to make no change, so we can just
> >> carry a define instead. (Suggested by Paul Eggert.)
> >>
> >> Signed-off-by: Robbie Harwood
> >> ---
> >> bootstrap.conf | 2 +-
> >> conf/Makefile.extra-dist| 1 -
> >> config.h.in | 10
> >> grub-core/lib/gnulib-patches/no-abort.patch | 26 -
> >> 4 files changed, 11 insertions(+), 28 deletions(-)
> >> delete mode 100644 grub-core/lib/gnulib-patches/no-abort.patch
> >>
> >> diff --git a/bootstrap.conf b/bootstrap.conf
> >> index 21a8cf15d..71acbeeb1 100644
> >> --- a/bootstrap.conf
> >> +++ b/bootstrap.conf
> >> @@ -82,7 +82,7 @@ cp -a INSTALL INSTALL.grub
> >> bootstrap_post_import_hook () {
> >>set -e
> >>for patchname in fix-null-deref fix-null-state-deref
> >> fix-regcomp-uninit-token \
> >> - fix-regexec-null-deref fix-uninit-structure fix-unused-value
> >> fix-width no-abort; do
> >> + fix-regexec-null-deref fix-uninit-structure fix-unused-value
> >> fix-width; do
> >> patch -d grub-core/lib/gnulib -p2 \
> >>< "grub-core/lib/gnulib-patches/$patchname.patch"
> >>done
> >> diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist
> >> index 15a9b74e9..4ddc3c8f7 100644
> >> --- a/conf/Makefile.extra-dist
> >> +++ b/conf/Makefile.extra-dist
> >> @@ -35,7 +35,6 @@ EXTRA_DIST +=
> >> grub-core/lib/gnulib-patches/fix-regexec-null-deref.patch
> >> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-uninit-structure.patch
> >> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-unused-value.patch
> >> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch
> >> -EXTRA_DIST += grub-core/lib/gnulib-patches/no-abort.patch
> >>
> >> EXTRA_DIST += grub-core/lib/libgcrypt
> >> EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic
> >> diff --git a/config.h.in b/config.h.in
> >> index 965eaffce..209e27449 100644
> >> --- a/config.h.in
> >> +++ b/config.h.in
> >> @@ -66,6 +66,16 @@
> >>
> >> # ifndef _GL_INLINE_HEADER_BEGIN
> >> #define _GL_ATTRIBUTE_CONST __attribute__ ((const))
> >> +
> >> +/*
> >> + * We don't have an abort() for gnulib to call in regexp. Because gnulib
> >> is
> >> + * built as a separate object that is then linked with, it doesn't have
> >> any
> >> + * of our headers or functions around to use - so we unfortunately can't
> >> + * print anything. Builds of emu include the system's stdlib, which
> >> includes
> >> + * a prototype for abort(), so leave this as a macro that doesn't take
> >> + * arguments.
> >> + */
> >> +#define abort __builtin_trap
> >
> > I asked earlier if we couldn't use grub_abort for gnulib's abort and
> > Daniel referred me to subsequent patch series. I suppose this is the
> > relevant section he was thinking of, however, its still not clear to me
> > why we can't use grub_abort(). And actually looking more into it, its
> > seems to me that using grub_abort() is probably what we should do
> > because it has platform specific cleanup code. It appears that
> > __builtin_trap is target dependent[1], but I do not believe that it is
> > or can be platform dependent.
> >
> > Is the problem with grub_abort() that it provides some exit
> > guarantees[2] and we're looking for the equivalent of a machine crash?
> > That doesn't seem right to me. It is true that grub_abort calls
> > grub_exit which is platform specific. So for instance for x86_64-efi,
> > this will not call EFI boot_services->exit. Not being
Re: [PATCH v8 4/6] Drop gnulib no-abort.patch
On Wed, 2 Mar 2022 14:08:27 -0500
Robbie Harwood wrote:
> Originally added in db7337a3d353a817ffe9eb4a3702120527100be9, this
> patched out all relevant invocations of abort() in gnulib. While it was
> not documented why at the time, testing suggests that there's no abort()
> implementation available for gnulib to use.
>
> gnulib's position is that the use of abort() is correct here, since it
> happens when input violates a "shall" from POSIX. Additionally, the
> code in question is probably not reachable. Since abort() is more
> friendly to user-space, they prefer to make no change, so we can just
> carry a define instead. (Suggested by Paul Eggert.)
>
> Signed-off-by: Robbie Harwood
> ---
> bootstrap.conf | 2 +-
> conf/Makefile.extra-dist| 1 -
> config.h.in | 10
> grub-core/lib/gnulib-patches/no-abort.patch | 26 -
> 4 files changed, 11 insertions(+), 28 deletions(-)
> delete mode 100644 grub-core/lib/gnulib-patches/no-abort.patch
>
> diff --git a/bootstrap.conf b/bootstrap.conf
> index 21a8cf15d..71acbeeb1 100644
> --- a/bootstrap.conf
> +++ b/bootstrap.conf
> @@ -82,7 +82,7 @@ cp -a INSTALL INSTALL.grub
> bootstrap_post_import_hook () {
>set -e
>for patchname in fix-null-deref fix-null-state-deref
> fix-regcomp-uninit-token \
> - fix-regexec-null-deref fix-uninit-structure fix-unused-value fix-width
> no-abort; do
> + fix-regexec-null-deref fix-uninit-structure fix-unused-value
> fix-width; do
> patch -d grub-core/lib/gnulib -p2 \
>< "grub-core/lib/gnulib-patches/$patchname.patch"
>done
> diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist
> index 15a9b74e9..4ddc3c8f7 100644
> --- a/conf/Makefile.extra-dist
> +++ b/conf/Makefile.extra-dist
> @@ -35,7 +35,6 @@ EXTRA_DIST +=
> grub-core/lib/gnulib-patches/fix-regexec-null-deref.patch
> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-uninit-structure.patch
> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-unused-value.patch
> EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch
> -EXTRA_DIST += grub-core/lib/gnulib-patches/no-abort.patch
>
> EXTRA_DIST += grub-core/lib/libgcrypt
> EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic
> diff --git a/config.h.in b/config.h.in
> index 965eaffce..209e27449 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -66,6 +66,16 @@
>
> # ifndef _GL_INLINE_HEADER_BEGIN
> #define _GL_ATTRIBUTE_CONST __attribute__ ((const))
> +
> +/*
> + * We don't have an abort() for gnulib to call in regexp. Because gnulib is
> + * built as a separate object that is then linked with, it doesn't have any
> + * of our headers or functions around to use - so we unfortunately can't
> + * print anything. Builds of emu include the system's stdlib, which includes
> + * a prototype for abort(), so leave this as a macro that doesn't take
> + * arguments.
> + */
> +#define abort __builtin_trap
I asked earlier if we couldn't use grub_abort for gnulib's abort and
Daniel referred me to subsequent patch series. I suppose this is the
relevant section he was thinking of, however, its still not clear to me
why we can't use grub_abort(). And actually looking more into it, its
seems to me that using grub_abort() is probably what we should do
because it has platform specific cleanup code. It appears that
__builtin_trap is target dependent[1], but I do not believe that it is
or can be platform dependent.
Is the problem with grub_abort() that it provides some exit
guarantees[2] and we're looking for the equivalent of a machine crash?
That doesn't seem right to me. It is true that grub_abort calls
grub_exit which is platform specific. So for instance for x86_64-efi,
this will not call EFI boot_services->exit. Not being an EFI expert,
I'm not sure of the implications of that. If my suspicion is correct
and an abort in gnulib with this patch would not properly exit the EFI
app and not allow returning control to another EFI app, then I would
consider this patch undesirable. And I notice that other platforms have
special code run on grub_exit. I suspect that GCC's __builtin_trap is
more geared toward user-space programs and not for our use case.
If the problem is that gnulib, as stated above, is built as a separate
object and then linked to GRUB, which does not have an abort symbol
(thus causing a link failure). Then I think the right solution is to
create a symbol in the GRUB kernel named abort that has the same info as
grub_abort.
I have a patch I've been meaning to send which solves this problem for
GDB which in some cases look for symbols free() and malloc(). When
building the GRUB kernel add "-Wl,--defsym=abort=grub_abort" to the
LDFLAGS_KERNEL variable in conf/Makefile.common.
I haven't tested this, so I'm interested in hearing why this won't work
or isn't a good solution. I believe this should work for user-space
platforms like emu because of the
