[PATCH 0/4] bg_setenv improvements

67 views
Skip to first unread message

Michael Adler

unread,
Oct 14, 2021, 4:46:11 AM10/14/21
to efibootg...@googlegroups.com, Michael Adler
Hi,

Jan's upcoming release announcement got me startled, so here's my stash.

The following patchset primarily introduces the possibility to dump the configuration of a single partition. A use-case
for this would be to determine the ustate of the system more easily.

Note: Patches 1-3 should be independent of each other. Patch 4 depends on patch 2.

Kind regards,
Michael

Michael Adler (4):
gitignore: added clangd dirs, .libs
bg_setenv: use ENV_NUM_CONFIG_PARTS instead of hard-coded value
bg_setenv: simplify nested if-statements
bg_printenv: possibility to dump a single partition

.gitignore | 5 +++++
tools/bg_setenv.c | 50 ++++++++++++++++++++++++++++++-----------------
2 files changed, 37 insertions(+), 18 deletions(-)

--
2.33.0

Michael Adler

unread,
Oct 14, 2021, 4:46:19 AM10/14/21
to efibootg...@googlegroups.com, Michael Adler
Signed-off-by: Michael Adler <michae...@siemens.com>
---
.gitignore | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/.gitignore b/.gitignore
index c8cb496..c2002c7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -84,3 +84,8 @@ m4/libtool.m4
m4/lt*
version.h
libtool
+.libs
+
+# clangd (lsp)
+.cache
+compile_commands.json
--
2.33.0

Michael Adler

unread,
Oct 14, 2021, 4:46:19 AM10/14/21
to efibootg...@googlegroups.com, Michael Adler
Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index bfedcd7..c5def8d 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -275,14 +275,14 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
fprintf(stderr, "Invalid number specified for -p.\n");
return 1;
}
- if (i == 0 || i == 1) {
+ if (i >= 0 && i < ENV_NUM_CONFIG_PARTS) {
fprintf(stdout, "Updating config partition #%d\n", i);
arguments->which_part = i;
part_specified = true;
} else {
fprintf(stderr,
"Selected partition out of range. Valid range: "
- "0..1.\n");
+ "0..%d.\n", ENV_NUM_CONFIG_PARTS - 1);
return 1;
}
break;
--
2.33.0

Michael Adler

unread,
Oct 14, 2021, 4:46:20 AM10/14/21
to efibootg...@googlegroups.com, Michael Adler
Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index c5def8d..0429c01 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -554,16 +554,15 @@ static void dump_envs(void)
fprintf(stdout, " Config Partition #%d ", i);
}
BGENV *env = bgenv_open_by_index(i);
- if (env) {
- if (verbosity) {
- dump_env(env->data);
- }
- } else {
- fprintf(stderr, "Error, could not read environment "
+ if (!env) {
+ fprintf(stderr, "Error, could not read environment "
"for index %d\n",
i);
return;
}
+ if (verbosity) {
+ dump_env(env->data);
+ }
bgenv_close(env);
}
}
--
2.33.0

Michael Adler

unread,
Oct 14, 2021, 4:46:22 AM10/14/21
to efibootg...@googlegroups.com, Michael Adler
Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 45 ++++++++++++++++++++++++++++++---------------
1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 0429c01..0a2ebce 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -53,6 +53,9 @@ static struct argp_option options_printenv[] = {
{"filepath", 'f', "ENVFILE", 0,
"Read environment from file. Expects a valid EFI Boot Guard "
"environment file."},
+ {"part", 'p', "ENV_PART", 0,
+ "Set environment partition to read from. If no partition is specified, "
+ "all config partitions will be read."},
{"verbose", 'v', 0, 0, "Be verbose"},
{"version", 'V', 0, 0, "Print version"},
{}
@@ -276,7 +279,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
return 1;
}
if (i >= 0 && i < ENV_NUM_CONFIG_PARTS) {
- fprintf(stdout, "Updating config partition #%d\n", i);
+ fprintf(stdout, "Using config partition #%d\n", i);
arguments->which_part = i;
part_specified = true;
} else {
@@ -546,24 +549,32 @@ static void update_environment(BGENV *env)

}

+static bool dump_env_by_index(uint32_t index)
+{
+ if (verbosity) {
+ fprintf(stdout, "\n----------------------------\n");
+ fprintf(stdout, " Config Partition #%d ", index);
+ }
+ BGENV *env = bgenv_open_by_index(index);
+ if (!env) {
+ fprintf(stderr, "Error, could not read environment "
+ "for index %d\n",
+ index);
+ return false;
+ }
+ if (verbosity) {
+ dump_env(env->data);
+ }
+ bgenv_close(env);
+ return true;
+}
+
static void dump_envs(void)
{
for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (verbosity) {
- fprintf(stdout, "\n----------------------------\n");
- fprintf(stdout, " Config Partition #%d ", i);
- }
- BGENV *env = bgenv_open_by_index(i);
- if (!env) {
- fprintf(stderr, "Error, could not read environment "
- "for index %d\n",
- i);
+ if (!dump_env_by_index(i)) {
return;
}
- if (verbosity) {
- dump_env(env->data);
- }
- bgenv_close(env);
}
}

@@ -703,7 +714,11 @@ int main(int argc, char **argv)
return 1;
}

- dump_envs();
+ if (part_specified) {
+ dump_env_by_index(arguments.which_part);
+ } else {
+ dump_envs();
+ }

if (!write_mode) {
bgenv_finalize();
--
2.33.0

Jan Kiszka

unread,
Oct 14, 2021, 5:07:56 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
What of this is truly specific to EBG? .libs looks to me like that. But
the clang stuff is probably better maintained in ~/.gitignore.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Jan Kiszka

unread,
Oct 14, 2021, 5:14:07 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
"This broke when configuring more than 2 partitions via
--with-num-config-parts." - added this.

An empty commit log is often a sign that some information is missing...

On 14.10.21 10:44, Michael Adler wrote:
Thanks, applied to next.

Did you also check if there are other cases that ignore
ENV_NUM_CONFIG_PARTS?

Jan Kiszka

unread,
Oct 14, 2021, 5:20:55 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
On 14.10.21 10:44, Michael Adler wrote:
Thanks, applied, fixing up the whitespace error git reported.

Jan Kiszka

unread,
Oct 14, 2021, 5:24:19 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
Could you explain the use case here? I'm not against the change per se,
but I'm against zero verbosity ;).

On 14.10.21 10:44, Michael Adler wrote:
Looks good otherwise. May require rebasing over next.

Michael Adler

unread,
Oct 14, 2021, 5:59:37 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
> What of this is truly specific to EBG? .libs looks to me like that.

Yes, I think that was introduced with libebgenv.so (c8ad20f36).

> But the clang stuff is probably better maintained in ~/.gitignore.

That's also fine with me.

--
Michael Adler

Siemens AG
T RDA IOT SES-DE
Otto-Hahn-Ring 6
81739 München, Deutschland

Siemens Aktiengesellschaft: Vorsitzender des Aufsichtsrats: Jim Hagemann Snabe; Vorstand: Roland Busch, Vorsitzender; Klaus Helmrich, Cedrik Neike, Matthias Rebellius, Ralf P. Thomas, Judith Wiese; Sitz der Gesellschaft: Berlin und München, Deutschland; Registergericht: Berlin-Charlottenburg, HRB 12300, München, HRB 6684; WEEE-Reg.-Nr. DE 23691322

Michael Adler

unread,
Oct 14, 2021, 5:59:38 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
Hi Jan,

> Could you explain the use case here? I'm not against the change per se,
> but I'm against zero verbosity ;).

yes, sure. One use-case would be to determine the ustate of the overall system (and based on this, do something else,
e.g. perform a migration). There are (at least) two ways to accomplish that:

1) Parse the output of bg_printenv.

There are typically two ustate values in the output, so you have to `grep` for all the ustates and then check
symmetrically (e.g. part 0/1 is in ustate 0/1 or 1/0, which is considered to be the same overall ustate).

Usually the output processing happens in some sort of shell script and so everyone tends to cook up his own hacks (which may not
work in all shells).

With this patch, I can run something like `bg_printenv -p 0 | grep "^ustate"` for each partition and then perform the
comparisons (the correct usage of `head` and `tail` will accomplish the same).

Eventually though, I would like to further extend bg_printenv to get rid of the `grep` as well. In that sense, the patch is
a first step to reach that goal.

2) (UGLY!) Mount each boot partition and use something like bg_printenv -f BGENV.dat | grep ustate.

Slightly off-topic: When I do AB firmware updates with U-Boot, I only use a single ustate to track the update state.
This makes things a lot easier (e.g. certain ustate combinations such as 1,1 are impossible by design).

Kind regards,
Michael

Jan Kiszka

unread,
Oct 14, 2021, 6:05:54 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
On 14.10.21 11:30, Michael Adler wrote:
>> What of this is truly specific to EBG? .libs looks to me like that.
>
> Yes, I think that was introduced with libebgenv.so (c8ad20f36).
>

Right. Then please provide that as separate patch.

>> But the clang stuff is probably better maintained in ~/.gitignore.
>
> That's also fine with me.
>

We could create infinitely long .gitignore files, for each project, if
we added all the artifacts that all the favorite editors tend to
generate, just to name another example. Therefore, I generally refuse
such changes in the projects I maintain.

Jan Kiszka

unread,
Oct 14, 2021, 6:14:58 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
On 14.10.21 11:58, Michael Adler wrote:
> Hi Jan,
>
>> Could you explain the use case here? I'm not against the change per se,
>> but I'm against zero verbosity ;).
>
> yes, sure. One use-case would be to determine the ustate of the overall system (and based on this, do something else,
> e.g. perform a migration). There are (at least) two ways to accomplish that:
>
> 1) Parse the output of bg_printenv.
>
> There are typically two ustate values in the output, so you have to `grep` for all the ustates and then check
> symmetrically (e.g. part 0/1 is in ustate 0/1 or 1/0, which is considered to be the same overall ustate).
>
> Usually the output processing happens in some sort of shell script and so everyone tends to cook up his own hacks (which may not
> work in all shells).
>
> With this patch, I can run something like `bg_printenv -p 0 | grep "^ustate"` for each partition and then perform the
> comparisons (the correct usage of `head` and `tail` will accomplish the same).
>
> Eventually though, I would like to further extend bg_printenv to get rid of the `grep` as well. In that sense, the patch is
> a first step to reach that goal.

You mean you want the effective ustate, considering what is the latest
env in use etc. - would be fine with me and likely has an even higher value.

>
> 2) (UGLY!) Mount each boot partition and use something like bg_printenv -f BGENV.dat | grep ustate.
>
> Slightly off-topic: When I do AB firmware updates with U-Boot, I only use a single ustate to track the update state.
> This makes things a lot easier (e.g. certain ustate combinations such as 1,1 are impossible by design).

I assume you are referring to using classic U-Boot A/B selection via
scripts, not via UEFI and EBG: U-Boot has a different strategy to make
that single state persistent and (hopefully) robust against corruptions
via power-cuts during updates. EBG has the strategy to never touch the
data that is sufficient to boot a working system, only update the one
that will eventually replace that. That said, nothing speaks against
presenting a virtual state that takes that into account.

Michael Adler

unread,
Oct 14, 2021, 8:55:35 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
Hi Jan,

> You mean you want the effective ustate, considering what is the latest
> env in use etc. - would be fine with me and likely has an even higher value.

yes, I agree that this would have a higher value, at least for all my use-cases.
So I'd suggest to drop the current patch in favor of that new functionality.

How about the following interface?

$ bg_printenv --current --ustate
ustate: 0 (OK)

$ bg_printenv --current --ustate --raw
0

Jan Kiszka

unread,
Oct 14, 2021, 9:00:19 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
On 14.10.21 14:55, Michael Adler wrote:
> Hi Jan,
>
>> You mean you want the effective ustate, considering what is the latest
>> env in use etc. - would be fine with me and likely has an even higher value.
>
> yes, I agree that this would have a higher value, at least for all my use-cases.
> So I'd suggest to drop the current patch in favor of that new functionality.
>
> How about the following interface?
>
> $ bg_printenv --current --ustate
> ustate: 0 (OK)
>
> $ bg_printenv --current --ustate --raw
> 0

--raw to make it script-friendly?

Sounds good to me.

I just realized that we have a unified help for print/setenv, and that
is now getting longer and longer. Maybe time to split this.

Michael Adler

unread,
Oct 14, 2021, 10:26:19 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
> --raw to make it script-friendly?

Exactly.

> Sounds good to me.

Alright then, I'll try to cook something up before the release window closes.

> I just realized that we have a unified help for print/setenv, and that
> is now getting longer and longer. Maybe time to split this.

I rather think you stumbled over the same bug as I did :) After running `make`, ./bg_printenv incorrectly believes that
it is bg_setenv, i.e. the argv[0] detection mechanism does not work. Indeed, if you run `./bg_printenv -f BGENV.dat`, it
will even create an empty BGENV.dat. I wasn't sure if this only happens on dev env (NixOS), but looks like others are
affected too. Interestingly, when I installed bg_printenv via a .deb package (ISAR) for testing purposes, it worked.
I haven't investigated it further but I suspect it might have something to do with c8ad20f. git bisect should do the
trick.

Michael Adler

unread,
Oct 14, 2021, 10:49:26 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
> I haven't investigated it further but I suspect it might have something to do with c8ad20f

Turns out it's fairly simple:

$ cat ./bg_printenv

[...]
# The bg_setenv program cannot be directly executed until all the libtool
# libraries that it depends on are installed.
#
# This wrapper script should never be moved out of the build directory.
# If it is, it will not operate correctly.

Indeed, running bg_printenv results in argv[0] being /home/.../.libs/lt-bg_setenv and so the argv[0]
detection mechanism does not work.

Michael Adler

unread,
Oct 14, 2021, 11:21:58 AM10/14/21
to Jan Kiszka, efibootg...@googlegroups.com
> Did you also check if there are other cases that ignore ENV_NUM_CONFIG_PARTS?

Not thoroughly, but so far I haven't seen one. I'm staying alert though.

Jan Kiszka

unread,
Oct 14, 2021, 11:26:31 AM10/14/21
to Michael Adler, efibootg...@googlegroups.com
On 14.10.21 17:10, Michael Adler wrote:
>> Did you also check if there are other cases that ignore ENV_NUM_CONFIG_PARTS?
>
> Not thoroughly, but so far I haven't seen one. I'm staying alert though.
>

Were you testing ENV_NUM > 2, or was this review fall-out?

Thanks in any case,

Christian Storm

unread,
Oct 14, 2021, 11:58:21 AM10/14/21
to efibootg...@googlegroups.com

> > I haven't investigated it further but I suspect it might have
> > something to do with c8ad20f
>
> Turns out it's fairly simple:
>
> $ cat ./bg_printenv
>
> [...]
> # The bg_setenv program cannot be directly executed until all the libtool
> # libraries that it depends on are installed.
> #
> # This wrapper script should never be moved out of the build directory.
> # If it is, it will not operate correctly.
>
> Indeed, running bg_printenv results in argv[0] being
> /home/.../.libs/lt-bg_setenv and so the argv[0] detection mechanism
> does not work.


Hm, I'm on c8ad20f and there I do see two different help texts, just
as it should be. Can you share the exact build steps you're doing to
reproduce it?


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Jan Kiszka

unread,
Oct 14, 2021, 12:01:00 PM10/14/21
to efibootg...@googlegroups.com
On 14.10.21 17:59, Christian Storm wrote:
>
>>> I haven't investigated it further but I suspect it might have
>>> something to do with c8ad20f
>>
>> Turns out it's fairly simple:
>>
>> $ cat ./bg_printenv
>>
>> [...]
>> # The bg_setenv program cannot be directly executed until all the libtool
>> # libraries that it depends on are installed.
>> #
>> # This wrapper script should never be moved out of the build directory.
>> # If it is, it will not operate correctly.
>>
>> Indeed, running bg_printenv results in argv[0] being
>> /home/.../.libs/lt-bg_setenv and so the argv[0] detection mechanism
>> does not work.
>
>
> Hm, I'm on c8ad20f and there I do see two different help texts, just
> as it should be. Can you share the exact build steps you're doing to
> reproduce it?
>

On my side:

$ mkdir build
$ cd build
$ ../configure
$ make
$ ./bg_printenv --help
Usage: bg_setenv [OPTION...]
bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard
...

Jan Kiszka

unread,
Oct 14, 2021, 12:04:08 PM10/14/21
to efibootg...@googlegroups.com
And I can confirm that the issue is gone when properly installing
things. But that's what I rarely do when testing commits...

Michael Adler

unread,
Oct 15, 2021, 2:21:44 AM10/15/21
to Jan Kiszka, efibootg...@googlegroups.com
> Were you testing ENV_NUM > 2, or was this review fall-out?

Just review fallout.

Christian Storm

unread,
Oct 18, 2021, 3:51:26 AM10/18/21
to efibootg...@googlegroups.com
Hm, I just did exactly this and I get the correct help texts for
bg_printenv and bg_setenv. Strange...


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Jan Kiszka

unread,
Oct 18, 2021, 5:20:42 AM10/18/21
to efibootg...@googlegroups.com
Maybe my libtool version is older than yours and still has issues in
this regard. IIRC, libtool is generating that wrapper script.

Anyway, not a real issue.

Michael Adler

unread,
Oct 18, 2021, 5:38:36 AM10/18/21
to Jan Kiszka, efibootg...@googlegroups.com
> Maybe my libtool version is older than yours and still has issues in
> this regard. IIRC, libtool is generating that wrapper script.

Indeed, it does work on Arch Linux. They use libtool 2.4.6+42+gb88cebd5-16, which is not an officially released version.
This is uncommon (even for Arch), but I suspect they had their reasons.

> Anyway, not a real issue.

Agreed, slightly annoying though. I'll try to upgrade libtool and see if that helps.

Kind regards,
Michael

--
Michael Adler

Siemens AG
T RDA IOT SES-DE

Jan Kiszka

unread,
Oct 18, 2021, 5:52:18 AM10/18/21
to Michael Adler, efibootg...@googlegroups.com
On 18.10.21 11:38, Michael Adler wrote:
>> Maybe my libtool version is older than yours and still has issues in
>> this regard. IIRC, libtool is generating that wrapper script.
>
> Indeed, it does work on Arch Linux. They use libtool 2.4.6+42+gb88cebd5-16, which is not an officially released version.
> This is uncommon (even for Arch), but I suspect they had their reasons.
>

I'm on libtool-2.4.6-3.4.1 with SUSE 15.3 - same upstream version at least.

Jan

>> Anyway, not a real issue.
>
> Agreed, slightly annoying though. I'll try to upgrade libtool and see if that helps.
>
> Kind regards,
> Michael
>
--

Michael Adler

unread,
Oct 18, 2021, 7:08:48 AM10/18/21
to Jan Kiszka, efibootg...@googlegroups.com
> I'm on libtool-2.4.6-3.4.1 with SUSE 15.3 - same upstream version at least.

I don't think so: 2.4.6+42+gb88cebd5-16 means there are 42 commits _on top_ of 2.4.6. It is built from git rev b88cebd5.

I have just upgraded my libtool to git revision b88cebd5 and indeed, the issue is gone!

So, problem solved, at least for me ;)

Michael Adler

unread,
Oct 19, 2021, 7:43:58 AM10/19/21
to Jan Kiszka, efibootg...@googlegroups.com, Christian Storm
> So, problem solved, at least for me ;)

I was too quick with my conclusion. Incremental compilation does not work as it should, even on Arch Linux.

git revision: c8ad20f36145bb4cd5a61549fd96bb7cbde0f144

Steps (clean checkout):

```
$ autoreconf -fi
$ ./configure
$ make
# first time
$ ./bg_printenv --help
Usage: bg_printenv [OPTION...]
# ...

# edit tools/bg_setenv.c
$ make
# second time
$ ./bg_printenv --help
Usage: lt-bg_setenv [OPTION...]
# ...
```

1) The first time, ./bg_printenv is an ELF file and everything is fine, i.e. bg_printenv thinks that it is bg_printenv.
2) The second time, ./bg_printenv is a shell script (generated by libtool) and hands over control to .libs/lt-bg_setenv.
Thus bg_printenv believes that it is bg_setenv, which is wrong.

Christian Storm

unread,
Oct 20, 2021, 4:50:46 PM10/20/21
to efibootg...@googlegroups.com
Hi Michael, Jan,

> > So, problem solved, at least for me ;)
>
> I was too quick with my conclusion. Incremental compilation does not
> work as it should, even on Arch Linux.
>
> git revision: c8ad20f36145bb4cd5a61549fd96bb7cbde0f144
>
> Steps (clean checkout):
>
> ```
> $ autoreconf -fi
> $ ./configure
> $ make
> # first time
> $ ./bg_printenv --help
> Usage: bg_printenv [OPTION...]
> # ...
>
> # edit tools/bg_setenv.c
> $ make
> # second time
> $ ./bg_printenv --help
> Usage: lt-bg_setenv [OPTION...]
> # ...
> ```
>
> 1) The first time, ./bg_printenv is an ELF file and everything is
> fine, i.e. bg_printenv thinks that it is bg_printenv.
> 2) The second time, ./bg_printenv is a shell script (generated by
> libtool) and hands over control to .libs/lt-bg_setenv. Thus
> bg_printenv believes that it is bg_setenv, which is wrong.

FWIW, I took a deeper look into the whole libtool stuff and I can
only recommend not to do the same for your mental sanity... So:

First time you build it, libtool produces a proper statically linked
bg_setenv which by argv[0] decides what to do. So far so good.

The next time, i.e., incremental build, libtool in all its wisdom
decides not to do the same but instead builds bg_setenv *dynamically*
linked against libebgenv. Then, you have opened the can of worms
which libtool tries to escape by wrapping executables/scripts, see,
e.g., [1,2] for the reasoning. This is exactly what we're seeing here.

Luckily, we're after a statically linked bg_setenv anyway, so we don't
need this "feature". Hence, can you please test whether the following
patch solves the issue? It did for me in all the scenarios I tested.

--- a/Makefile.am
+++ b/Makefile.am
@@ -107,7 +107,7 @@ bg_setenv_SOURCES = \
tools/bg_setenv.c

bg_setenv_CFLAGS = \
- $(AM_CFLAGS)
+ $(AM_CFLAGS) -static

bg_setenv_LDADD = \
-lebgenv \


If it does solve the issue, I'll prepare a proper patch and sent it out.

Thanks!


Kind regards,
Christian

[1] https://www.gnu.org/software/libtool/manual/html_node/Linking-executables.html#Linking-executables
[2] https://autotools.io/libtool/wrappers.html

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Jan Kiszka

unread,
Oct 21, 2021, 2:24:25 AM10/21/21
to efibootg...@googlegroups.com
It resolves the build-env weirdness - but why are we after a statically
linked version, installing libebgenv.so twice this way?

Jan

Michael Adler

unread,
Oct 21, 2021, 2:32:12 AM10/21/21
to efibootg...@googlegroups.com, Christian Storm
> FWIW, I took a deeper look into the whole libtool stuff and I can
> only recommend not to do the same for your mental sanity...

Yes, I figured that much after taking a glimpse at their wrapper script ;)

> Hence, can you please test whether the following patch solves the issue?

Indeed, this resolves *that* issue, but something is still funny: in the case of incremental compilation, bg_setenv is
linked against libebgenv-0.9.0.so. This is not the case if you run 'make clean' and build it from scratch. So I guess
the following statement is not always true:

> Luckily, we're after a statically linked bg_setenv anyway

Just run ldd against ./bg_setenv and you will see yourself.

Kind regards,
Michael

--
Michael Adler

Siemens AG
T RDA IOT SES-DE

Christian Storm

unread,
Oct 21, 2021, 3:50:47 AM10/21/21
to efibootg...@googlegroups.com
Quoting the original patch:
"Note that the bg_setenv utility deliberately remains
statically linked against libebgenv.a. for easy
inclusion in initramfs or rescue media and to not
break existing deployments. Alike, libebgenv.a is
still installed via make install (though distributions
may decide to not package it).
This may be deprecated and removed in future versions."

I think this still holds true and it has some value to have
bg_setenv statically linked, in particular for rescue scenarios.

Then, if you have further tools wanting to modify the bootloader
environment, you install the library contents twice: statically
inside bg_setenv and as an .so shared among those tools. So you
"wasted" ~40k disk space.
If you don't have such tools, you "waste" nothing, i.e., when
bg_setenv is your "interface" to the bootloader.


IMO, the hassle with a shared library libebgenv.so in rescue/
emergency scenarios is well worth the disk space "waste".
But as already noted in the patch, we may deprecate this with a
proper phase-out period...



Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Christian Storm

unread,
Oct 21, 2021, 3:53:48 AM10/21/21
to efibootg...@googlegroups.com
> > Hence, can you please test whether the following patch solves the issue?
>
> Indeed, this resolves *that* issue, but something is still funny: in
> the case of incremental compilation, bg_setenv is linked against
> libebgenv-0.9.0.so. This is not the case if you run 'make clean' and
> build it from scratch. So I guess the following statement is not
> always true:
>
> > Luckily, we're after a statically linked bg_setenv anyway
>
> Just run ldd against ./bg_setenv and you will see yourself.

Hmm, I did and it worked. Can you give me the repro steps?


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Michael Adler

unread,
Oct 21, 2021, 4:55:14 AM10/21/21
to efibootg...@googlegroups.com, Christian Storm
> Hmm, I did and it worked. Can you give me the repro steps?

Hm, it works on Bullseye and Arch, so I can only reproduce it on NixOS. libtool is a strange beast.

It's not an issue for me, since the binary does what it's supposed to do, so I would say go ahead and submit a patch.

--
Michael Adler

Siemens AG
T RDA IOT SES-DE

Christian Storm

unread,
Oct 21, 2021, 5:12:46 AM10/21/21
to efibootg...@googlegroups.com
> > Hmm, I did and it worked. Can you give me the repro steps?
>
> Hm, it works on Bullseye and Arch, so I can only reproduce it on
> NixOS. libtool is a strange beast.

Indeed. This and the whole auto stuff is overly picky version-dependent,
breaks, and behaves strange each and every time I have touched it in
the past for this project. Maybe I just don't fully get it or it's
over-engineered beyond good. Either way, I'd like to look into NixOS
issues as well, maybe there's a hidden bomb in there we just don't see
now on the other distros?


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

Michael Adler

unread,
Oct 21, 2021, 5:53:23 AM10/21/21
to efibootg...@googlegroups.com, Christian Storm
> Either way, I'd like to look into NixOS > issues as well

Ok, here are the steps to reproduce it in a container using podman (alias podman=docker should work too):

git clone --branch repro/dynamic https://github.com/michaeladler/efibootguard.git
cd efibootguard

podman run --rm -it -v $(pwd):/src -w /src nixos/nix:latest /bin/sh

Inside the container:

$ nix-shell
$ make
$ ldd ./bg_setenv
# ok, no libebgenv
$ echo "" >> ./tools/bg_setenv.c
$ make
$ ldd ./bg_setenv
[...]
libebgenv-0.9.0.so => /src/.libs/libebgenv-0.9.0.so (0x00007f6f10442000)

--
Michael Adler

Siemens AG
T RDA IOT SES-DE
Reply all
Reply to author
Forward
0 new messages