[PATCH] bg_setenv: simplify dump_envs

[Michael Adler]

If verbosity is false, then dump_envs is mostly a no-op. In that case,
do not even call the function. In other words, move the if-clause
where it belongs.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 071c9fc..7b9ce79 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -549,10 +549,8 @@ static void update_environment(BGENV *env)
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);
- }
+ 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 "
@@ -560,9 +558,7 @@ static void dump_envs(void)
i);
return;
}
- if (verbosity) {
- dump_env(env->data);
- }
+ dump_env(env->data);
bgenv_close(env);
}
}
@@ -703,13 +699,16 @@ int main(int argc, char **argv)
return 1;
}

- dump_envs();
-
if (!write_mode) {
+ dump_envs();
bgenv_finalize();
return 0;
}

+ if (verbosity) {
+ dump_envs();
+ }
+
BGENV *env_new = NULL;
BGENV *env_current;

--
2.33.0

[Jan Kiszka]

But this will now dump irrespective of "verbosity", no?

> bgenv_finalize();
> return 0;
> }
>
> + if (verbosity) {
> + dump_envs();
> + }
> +

You rather need to pull this up an share it with the !write_mode case.

Jan

> BGENV *env_new = NULL;
> BGENV *env_current;
>
>

--

Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Michael Adler]

> But this will now dump irrespective of "verbosity", no?

No, the if-block is only entered for bg_printenv, and (unfortunately) bg_printenv always implies verbosity [1].

> You rather need to pull this up an share it with the !write_mode case.

That would reduce code duplication, yes, but I think it's the wrong call, since verbosity is conceptually wrong here: if
I run bg_printenv, I *always* want to dump the BGENVs - I mean, that's why I ran bg_printenv in the first place. In
fact, in the case of bg_printenv, verbosity should rather turn on additional output. As it is now, the variable
verbosity is wasted for bg_printetnv, because it's always true [1].

Some background: I'm close to finishing the bg_printenv enhancement which we discussed here last week and it will
further decouple setenv/printenv (for it is necessary to split the argument parser). The whole entanglement makes it
difficult to extend one without the other, and this patch addresses one instance of it.

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

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

[Jan Kiszka]

On 19.10.21 10:57, Michael Adler wrote:
>> But this will now dump irrespective of "verbosity", no?
>
> No, the if-block is only entered for bg_printenv, and (unfortunately) bg_printenv always implies verbosity [1].
>

Fine, but please add such non-obvious things to the commit log then. I
was too lazy to read the whole file, and from that patch loop-hole, that
was not clear.

Jan

>> You rather need to pull this up an share it with the !write_mode case.
>
> That would reduce code duplication, yes, but I think it's the wrong call, since verbosity is conceptually wrong here: if
> I run bg_printenv, I *always* want to dump the BGENVs - I mean, that's why I ran bg_printenv in the first place. In
> fact, in the case of bg_printenv, verbosity should rather turn on additional output. As it is now, the variable
> verbosity is wasted for bg_printetnv, because it's always true [1].
>
> Some background: I'm close to finishing the bg_printenv enhancement which we discussed here last week and it will
> further decouple setenv/printenv (for it is necessary to split the argument parser). The whole entanglement makes it
> difficult to extend one without the other, and this patch addresses one instance of it.
>
> [1] https://github.com/siemens/efibootguard/blob/c01324d0da202727eb0744c0f67a78f9c9b65c46/tools/bg_setenv.c#L670
>


--

[Jan Kiszka]

On 19.10.21 17:40, Jan Kiszka wrote:
> On 19.10.21 10:57, Michael Adler wrote:
>>> But this will now dump irrespective of "verbosity", no?
>>
>> No, the if-block is only entered for bg_printenv, and (unfortunately) bg_printenv always implies verbosity [1].
>>
>
> Fine, but please add such non-obvious things to the commit log then. I
> was too lazy to read the whole file, and from that patch loop-hole, that
> was not clear.
>

And as you now no longer evaluate verbosity in the printenv case, the
verbosity = true in main() can be dropped, I suppose.

Jan

[Michael Adler]

Hi Jan,

> Fine, but please add such non-obvious things to the commit log then.

you are absolutely right. I am currently so deep into the code that I sometimes forget such things. Apologies!

> And as you now no longer evaluate verbosity in the printenv case, the
> verbosity = true in main() can be dropped, I suppose.

You are right again :) Nice catch! I went ahead and removed the line in V2 of the patch (see my next mail).

Kind regards,
Michael

Michael Adler (1):
bg_setenv: simplify dump_envs

tools/bg_setenv.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

--
2.33.0

[Michael Adler]

Previously, bg_printenv *always* implied verbosity (see line 670). The
verbosity flag was subsequently (mis)used to ensure the environments are
actually being dumped in the case of printenv. There is no need for such
a misuse though: if verbosity is false, then dump_envs is (mostly)
a no-op. In that case, do not even call the function or in other words,

move the if-clause where it belongs.

Finally, this allows us to leave verbosity as it is (i.e. unset) in the
case of bg_printenv. Now it is possible to give the verbose flag
a proper meaning in the case of printenv, i.e. the output could actually
be more verbose when the flag is set. This is not in the scope of this
commit, though.

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

tools/bg_setenv.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 071c9fc..e564219 100644

--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -549,10 +549,8 @@ static void update_environment(BGENV *env)
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);
- }
+ 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 "
@@ -560,9 +558,7 @@ static void dump_envs(void)
i);
return;
}
- if (verbosity) {
- dump_env(env->data);
- }
+ dump_env(env->data);
bgenv_close(env);
}
}

@@ -667,7 +663,6 @@ int main(int argc, char **argv)

} else {
argp = &argp_printenv;
- verbosity = true;
}

struct arguments arguments;
@@ -703,13 +698,16 @@ int main(int argc, char **argv)

return 1;
}

- dump_envs();
-
if (!write_mode) {
+ dump_envs();

bgenv_finalize();
return 0;
}

+ if (verbosity) {
+ dump_envs();
+ }
+

BGENV *env_new = NULL;
BGENV *env_current;

--

2.33.0

[Jan Kiszka]

Thanks, applied.