[PATCH] Support check-valgrind

[Earl Chew]

Use AX_VALGRIND_CHECK in configure.ac to enable support
for check-valgrind target that uses valgrind when running
unit tests.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---
.github/workflows/main.yaml | 3 ++-
Makefile.am | 2 +-
configure.ac | 6 ++++++
tools/tests/Makefile.am | 2 ++
tools/tests/test_ebgenv_api_internal.c | 24 +++++++++++++++---------
5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml
index 2cce035..5c589d8 100644
--- a/.github/workflows/main.yaml
+++ b/.github/workflows/main.yaml
@@ -44,7 +44,7 @@ jobs:
sudo apt-get update
sudo apt-get install --no-install-recommends \
autoconf-archive gcc-multilib gnu-efi libpci-dev check \
- bats libarchive-zip-perl
+ bats libarchive-zip-perl valgrind
- name: Install i386 dependencies
if: ${{ matrix.target == 'i386' }}
run: |
@@ -124,6 +124,7 @@ jobs:
pushd build >/dev/null
../configure
make check -j $(nproc)
+ make check-valgrind -j $(nproc)
sudo make install
time bats --tap ../tests
popd >/dev/null
diff --git a/Makefile.am b/Makefile.am
index 3b05e7c..68c0f59 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -15,7 +15,7 @@
# Copyright (C) 2013 Karel Zak <kz...@redhat.com>
#

-ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}
+ACLOCAL_AMFLAGS = -I m4 --install ${ACLOCAL_FLAGS}
AM_MAKEFLAGS = --no-print-directory

efibootguarddir = $(libdir)/efibootguard
diff --git a/configure.ac b/configure.ac
index 1738968..afff539 100644
--- a/configure.ac
+++ b/configure.ac
@@ -235,6 +235,12 @@ AS_IF([test "x$enable_completion" != "xno"],
])
AM_CONDITIONAL([COMPLETION], [test "x$enable_completion" != "xno"])

+AX_VALGRIND_DFLT(memcheck, on)
+AX_VALGRIND_DFLT(helgrind, off)
+AX_VALGRIND_DFLT(drd, off)
+AX_VALGRIND_DFLT(sgcheck, off)
+AX_VALGRIND_CHECK
+
# ------------------------------------------------------------------------------
AC_CONFIG_FILES([
Makefile
diff --git a/tools/tests/Makefile.am b/tools/tests/Makefile.am
index 58fd26f..97d4367 100644
--- a/tools/tests/Makefile.am
+++ b/tools/tests/Makefile.am
@@ -108,3 +108,5 @@ test_fat_SOURCES = test_fat.c $(SRC_TEST_COMMON)
test_fat_LDADD = $(FAT_TESTLIB) $(LIBCHECK_LIBS)

TESTS = $(check_PROGRAMS)
+
+@VALGRIND_CHECK_RULES@
diff --git a/tools/tests/test_ebgenv_api_internal.c b/tools/tests/test_ebgenv_api_internal.c
index 6a3d816..08d690d 100644
--- a/tools/tests/test_ebgenv_api_internal.c
+++ b/tools/tests/test_ebgenv_api_internal.c
@@ -165,12 +165,14 @@ END_TEST

START_TEST(ebgenv_api_internal_bgenv_write)
{
+ bool err = true;
+
bool res;
BGENV *dummy_env;

dummy_env = calloc(1, sizeof(BGENV));
if (!dummy_env) {
- goto bgew_error;
+ goto finally;
}

RESET_FAKE(write_env);
@@ -194,25 +196,29 @@ START_TEST(ebgenv_api_internal_bgenv_write)
*/
dummy_env->desc = calloc(1, sizeof(CONFIG_PART));
if (!dummy_env->desc) {
- goto bgew_error;
+ goto finally;
}

dummy_env->data = calloc(1, sizeof(BG_ENVDATA));
if (!dummy_env->data) {
- goto bgew_error;
+ goto finally;
}

res = bgenv_write(dummy_env);
ck_assert(write_env_fake.call_count == 1);
ck_assert(res == true);

- return;
+ err = false;
+
+finally:
+ if (dummy_env) {
+ free(dummy_env->data);
+ free(dummy_env->desc);
+ free(dummy_env);
+ }

-bgew_error:
- free(dummy_env->data);
- free(dummy_env->desc);
- free(dummy_env);
- exit(errno);
+ if (err)
+ ck_abort();
}
END_TEST

--
2.39.1

[Jan Kiszka]

Interesting - does that report anything? And will it fail on errors only
or also on warnings?

Jan

--
Siemens AG, Technology
Linux Expert Center

[Earl Chew]

Jan,

On 2024-02-06 08:27, Jan Kiszka wrote:
> Interesting - does that report anything? And will it fail on errors only
> or also on warnings?

I've left it configured with the defaults:

VALGRIND_memcheck_FLAGS ?= --leak-check=full --show-reachable=no

VALGRIND_LOG_COMPILER = \
$(valgrind_lt) \
$(VALGRIND) $(VALGRIND_SUPPRESSIONS) --error-exitcode=1 $(VALGRIND_FLAGS)


Artificially forcing a leak:

% make check-valgrind
Making check-valgrind in tools/tests
USE memcheck:
CCLD test_ebgenv_api_internal
CCLD test_ebgenv_api
CCLD test_uservars
CCLD test_fat
CC ../../env/test_fatvars-fatvars.o
CCLD test_fatvars
PASS: test_bgenv_init_retval
PASS: test_probe_config_partitions
PASS: test_probe_config_file
FAIL: test_ebgenv_api_internal
FAIL: test_ebgenv_api
PASS: test_uservars
PASS: test_fat
PASS: test_fatvars
...


Looking in tools/tests/test-suite-memcheck.log -


FAIL: test_ebgenv_api_internal
==============================
...
==1180619== 16 bytes in 1 blocks are definitely lost in loss record 1 of 2
==1180619== at 0x483AB65: calloc (vg_replace_malloc.c:760)
==1180619== by 0x10D545: bgenv_open_by_index (env_api_fat.c:210)
==1180619== by 0x10D545: bgenv_open_latest (env_api_fat.c:243)
==1180619== by 0x10D545: bgenv_create_new (env_api_fat.c:457)
==1180619== by 0x10B67F: ebgenv_api_internal_bgenv_create_new_fn (test_ebgenv_api_internal.c:252)
==1180619== by 0x112050: tcase_run_tfun_nofork (check_run.c:420)
==1180619== by 0x1125F9: srunner_iterate_tcase_tfuns (check_run.c:263)
==1180619== by 0x1125F9: srunner_run_tcase (check_run.c:402)
==1180619== by 0x1125F9: srunner_iterate_suites (check_run.c:222)
==1180619== by 0x1125F9: srunner_run_tagged (check_run.c:814)
==1180619== by 0x10A5AF: main (test_main.c:38)


Earl

[Jan Kiszka]

On 06.02.24 16:47, 'Earl Chew' via EFI Boot Guard wrote:

This target is not generated in my setup. Configure reports

...
checking for valgrind... valgrind
checking for Valgrind tool memcheck... yes
...

What am I missing?

Jan

[Earl Chew]

On 2024-02-12 00:17, Jan Kiszka wrote:
> On 06.02.24 16:47, 'Earl Chew' via EFI Boot Guard wrote:
>> make check -j $(nproc)
>> + make check-valgrind -j $(nproc)
>
> This target is not generated in my setup. Configure reports
>
> ..

> checking for valgrind... valgrind
> checking for Valgrind tool memcheck... yes
> ..
>

> What am I missing?

I confirmed that this passes on github.com:

https://github.com/earlchew/efibootguard/actions/runs/7787040735/workflow#L127

Apart from checking the versions of autoconf-archive,
autoconf, etc, the other part I found important is:

diff --git a/Makefile.am b/Makefile.am
index 3b05e7c..68c0f59 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -15,7 +15,7 @@
# Copyright (C) 2013 Karel Zak <kz...@redhat.com>
#

-ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}
+ACLOCAL_AMFLAGS = -I m4 --install ${ACLOCAL_FLAGS}
AM_MAKEFLAGS = --no-print-directory

efibootguarddir = $(libdir)/efibootguard

Earl

[Jan Kiszka]

Simpler: Forgot to run autoreconf locally.

But check-valgrind is missing some deps:

Making check-valgrind in tools/tests
USE memcheck:

CC test_bgenv_init_retval-test_bgenv_init_retval.o
CC test_bgenv_init_retval-test_main.o
CC ../../env/env_api.o
CC ../../env/env_api_fat.o
CC ../../env/env_api_crc32.o
CC ../../tools/ebgpart.o
CC ../../env/env_config_file.o
CC ../../env/env_config_partitions.o
CC ../../env/env_disk_utils.o
CC ../../env/uservars.o
CC ../../tools/bg_envtools.o
../../../tools/tests/../../tools/bg_envtools.c:18:10: fatal error:
version.h: No such file or directory
#include "version.h"
^~~~~~~~~~~
compilation terminated.


Jan

[Earl Chew]

Use AX_VALGRIND_CHECK in configure.ac to enable support
for check-valgrind target that uses valgrind when running
unit tests.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---
.github/workflows/main.yaml | 3 ++-

Makefile.am | 5 ++++-

configure.ac | 6 ++++++
tools/tests/Makefile.am | 2 ++
tools/tests/test_ebgenv_api_internal.c | 24 +++++++++++++++---------

5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/.github/workflows/main.yaml b/.github/workflows/main.yaml
index 2cce035..5c589d8 100644
--- a/.github/workflows/main.yaml
+++ b/.github/workflows/main.yaml
@@ -44,7 +44,7 @@ jobs:
sudo apt-get update
sudo apt-get install --no-install-recommends \
autoconf-archive gcc-multilib gnu-efi libpci-dev check \
- bats libarchive-zip-perl
+ bats libarchive-zip-perl valgrind
- name: Install i386 dependencies
if: ${{ matrix.target == 'i386' }}
run: |
@@ -124,6 +124,7 @@ jobs:
pushd build >/dev/null
../configure

make check -j $(nproc)
+ make check-valgrind -j $(nproc)

sudo make install
time bats --tap ../tests
popd >/dev/null

diff --git a/Makefile.am b/Makefile.am
index 3b05e7c..f59ce31 100644

--- a/Makefile.am
+++ b/Makefile.am
@@ -15,7 +15,7 @@
# Copyright (C) 2013 Karel Zak <kz...@redhat.com>
#

-ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}
+ACLOCAL_AMFLAGS = -I m4 --install ${ACLOCAL_FLAGS}
AM_MAKEFLAGS = --no-print-directory

efibootguarddir = $(libdir)/efibootguard

@@ -362,6 +362,9 @@ clean-local-completion-pycache:
rm -rf $(top_builddir)/completion/bg_printenv/__pycache__
rm -rf $(top_builddir)/completion/bg_setenv/__pycache__

+.PHONY: check-valgrind-local
+check-valgrind-local: $(GEN_VERSION_H)
+
# Tests depend on libraries being built - start with "."
SUBDIRS = . tools/tests

diff --git a/configure.ac b/configure.ac
index de7ef3e..20fcf32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -237,6 +237,12 @@ AS_IF([test "x$enable_completion" != "xno"],

2.39.1

[Jan Kiszka]

This is already provided by automake, dropping it.

> +check-valgrind-local: $(GEN_VERSION_H)
> +

Not very nice, but apparently the best option, given that GEN_VERSION_H
is not available in tools/test/Makefile.am.

Thanks, applied.

[Earl Chew]

Support a configuration option to insert a delay before
starting the image to give the operator a chance to capture
diagnostic messages.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---

configure.ac | 17 +++++++++++++++++
main.c | 2 ++
2 files changed, 19 insertions(+)

diff --git a/configure.ac b/configure.ac
index b000603..de7ef3e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,6 +156,22 @@ AC_ARG_WITH([env-backend],

AC_SUBST([env_api_file], [${ENV_API_FILE}])

+AC_ARG_WITH([boot-delay],
+ AS_HELP_STRING([--with-boot-delay=INT],
+ [specify the additional boot delay in seconds, defaults to 3]),
+ [
+ # Limit the boot delay to 1 hour to avoid overflowing
+ # BS->Stall() on 32 bit hosts.
+ ENV_BOOT_DELAY=${withval:-3}
+ AS_IF([test "${ENV_BOOT_DELAY}" -lt "0" -o "${ENV_BOOT_DELAY}" -gt "3600"],
+ [
+ AC_MSG_ERROR([Invalid boot delay.])
+ ])
+ ],
+ [ ENV_BOOT_DELAY=0 ])
+
+AC_DEFINE_UNQUOTED([ENV_BOOT_DELAY], [${ENV_BOOT_DELAY}], [Additional boot delay])
+
AC_ARG_WITH([num-config-parts],
AS_HELP_STRING([--with-num-config-parts=INT],
[specify the number of used config partitions, defaults to 2]),
@@ -248,4 +264,5 @@ AC_MSG_RESULT([
environment backend: ${ENV_API_FILE}.c
number of config parts: ${ENV_NUM_CONFIG_PARTS}
reserved for uservars: ${ENV_MEM_USERVARS} bytes
+ boot delay: ${ENV_BOOT_DELAY} seconds
])
diff --git a/main.c b/main.c
index 0ff121a..a0b1b60 100644
--- a/main.c
+++ b/main.c
@@ -214,5 +214,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
INFO(L"Starting %s with watchdog set to %d seconds ...\n",
bg_loader_params.payload_path, bg_loader_params.timeout);

+ BS->Stall(1000 * 1000 * ENV_BOOT_DELAY);
+
return BS->StartImage(payload_handle, NULL, NULL);
}
--
2.39.1

[Earl Chew]

Apologies, this update was attached to the wrong email thread.

[Jan Kiszka]

On 24.02.24 18:49, 'Earl Chew' via EFI Boot Guard wrote:
> Apologies, this update was attached to the wrong email thread.
>

No problem, I found it.

In general it is better anyway to start a new thread when sending a new
version of a patch or patch series.

Jan

> On 2024-02-24 09:42, Earl Chew wrote:
>> Support a configuration option to insert a delay before
>> starting the image to give the operator a chance to capture
>> diagnostic messages.
>