[PATCH 0/1] bg_setenv: preserve existing env

[Michael Adler]

Hi all,

I'm trying to selectively update key/value pairs in an existing BGENV.DAT file while preserving all other settings.
However, all other settings are lost in the update process since the whole BGENV struct is memset to zero before the new
k/v pairs are applied to it.

An obvious workaround is to use bg_printenv to query the current settings and then to pass along the current settings to
bg_setenv. While this is possible, it's also error-prone and somewhat counterintuitive, e.g. compared to U-Boot's
fw_setenv.

I have a simple patch which resolves the above issues which I'd like to contribute upstream, hence this e-mail.

Kind regards,
Michael

Michael Adler (1):
bg_setenv: preserve existing env

tools/bg_setenv.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

--
2.33.0

[Michael Adler]

Previously, the command

bg_setenv -f files/BGENV.DAT --args "root=/dev/sdb"

would correctly update the kernelargs, but all other key/value pairs are
lost (actually, memset to zero). In particular, BGENV.DAT no longer
stores a reference to the kernel file and thus, the device is unable to
boot from the updated BGENV.

The new behavior is as follows:

* if BGENV.DAT already exists, load it before applying the k/v updates
* otherwise fall back to the previous behavior, i.e. create a new
BGENV.DAT and apply the k/v updates to it

Signed-off-by: Michael Adler <michae...@siemens.com>
---

tools/bg_setenv.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d26eeed..522fd83 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -601,13 +601,18 @@ static int printenv_from_file(char *envfilepath) {
static int dumpenv_to_file(char *envfilepath) {
/* execute journal and write to file */
int result = 0;
+ int success = 0;
BGENV env;
BG_ENVDATA data;

memset(&env, 0, sizeof(BGENV));
- memset(&data, 0, sizeof(BG_ENVDATA));
env.data = &data;

+ success = get_env(envfilepath, &data);
+ if (!success) {
+ memset(&data, 0, sizeof(BG_ENVDATA));
+ }
+
update_environment(&env);
if (verbosity) {
dump_env(env.data);
--
2.33.0

[Jan Kiszka]

As this changes the existing behaviour of the bg_setenv command in a
possibly breaking way, how about requesting this explicitly via an
update mode (-u|--update)?

Jan

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

[Jan Kiszka]

...except that --update already exists and has a different semantic.
Better name needed, the key is the concept.

[Michael Adler]

> Better name needed, the key is the concept.

There are 2 hard problems in computer science... How about

1) {"preserve", 'P', 0, 0, "Preserve existing entries"}
2) {"keep", 'K', 0, 0, "Keep existing entries"}
3) {"upsert", 'U', 0, 0, "Update or insert new entries, keep old ones"}

Any preferences?

--
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]

Hi Jan,

> ...except that --update already exists and has a different semantic.

this got me thinking: -u is a no-op if -f is also specified [1].
Thus, it would be possible to re-use this parameter for the purposes of my patch. Of course, if people rely on this
no-op (without knowing), they might be surprised.

Kind regards,
Michael

[1] https://github.com/siemens/efibootguard/blob/c8ad20f36145bb4cd5a61549fd96bb7cbde0f144/tools/bg_setenv.c#L684

[Jan Kiszka]

On 06.10.21 10:40, Michael Adler wrote:
> Hi Jan,
>
>> ...except that --update already exists and has a different semantic.
>
> this got me thinking: -u is a no-op if -f is also specified [1].
> Thus, it would be possible to re-use this parameter for the purposes of my patch. Of course, if people rely on this
> no-op (without knowing), they might be surprised.
>

Worse would be that the switch will gain different semantics, depending
on whether -f is used or not.

I would rather go for --preserve.

Jan

> Kind regards,
> Michael
>
> [1] https://github.com/siemens/efibootguard/blob/c8ad20f36145bb4cd5a61549fd96bb7cbde0f144/tools/bg_setenv.c#L684
>

--