[PATCH] Support printenv direct from file

[Silvano Cirujano Cuesta]

The tools bg_setenv and bg_printenv are mostly oriented at running systems

that have the partitions providing BG environments already mounted.

This patch extends bg_printenv so that the environment can be read directly

from whatever valid BG environment file. This usage is very usefull in

development, diagnose, testing,... scenarios.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>

---

 env/env_api_fat.c         |  4 +--

 env/env_config_file.c     | 24 +++++++++------

 include/env_config_file.h |  3 +-

 tools/bg_setenv.c         | 64 ++++++++++++++++++++++++++++++++++-----

 4 files changed, 75 insertions(+), 20 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c

index 2b23e7f..3732050 100644

--- a/env/env_api_fat.c

+++ b/env/env_api_fat.c

@@ -67,7 +67,7 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env)

             part->mountpoint);

     }

     FILE *config;

-    if (!(config = open_config_file(part, "rb"))) {

+    if (!(config = open_config_file_from_part(part, "rb"))) {

         return false;

     }

     bool result = true;

@@ -104,7 +104,7 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)

             part->mountpoint);

     }

     FILE *config;

-    if (!(config = open_config_file(part, "wb"))) {

+    if (!(config = open_config_file_from_part(part, "wb"))) {

         VERBOSE(stderr, "Could not open config file for writing.\n");

         return false;

     }

diff --git a/env/env_config_file.c b/env/env_config_file.c

index 873fe10..4e68b7f 100644

--- a/env/env_config_file.c

+++ b/env/env_config_file.c

@@ -18,7 +18,19 @@

 #include "env_disk_utils.h"

 #include "env_config_file.h"

 

-FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)

+FILE *open_config_file(char *configfilepath, char *mode)

+{

+    if (!configfilepath) {

+        return NULL;

+    }

+

+    VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);

+    FILE *config = fopen(configfilepath, mode);

+    free(configfilepath);

+    return config;

+}

+

+FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)

 {

     char *configfilepath;

 

@@ -27,16 +39,10 @@ FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)

     }

     configfilepath = (char *)malloc(strlen(FAT_ENV_FILENAME) +

                     strlen(cfgpart->mountpoint) + 2);

-    if (!configfilepath) {

-        return NULL;

-    }

     strcpy(configfilepath, cfgpart->mountpoint);

     strcat(configfilepath, "/");

     strcat(configfilepath, FAT_ENV_FILENAME);

-    VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);

-    FILE *config = fopen(configfilepath, mode);

-    free(configfilepath);

-    return config;

+    return open_config_file(configfilepath, mode);

 }

 

 int close_config_file(FILE *config_file_handle)

@@ -74,7 +80,7 @@ bool probe_config_file(CONFIG_PART *cfgpart)

             cfgpart->devpath, cfgpart->mountpoint);

         bool result = false;

         FILE *config;

-        if (!(config = open_config_file(cfgpart, "rb"))) {

+        if (!(config = open_config_file_from_part(cfgpart, "rb"))) {

             printf_debug(

                 "Could not open config file on partition %s.\n",

                 FAT_ENV_FILENAME);

diff --git a/include/env_config_file.h b/include/env_config_file.h

index 8fbe8fa..f333ddb 100644

--- a/include/env_config_file.h

+++ b/include/env_config_file.h

@@ -15,7 +15,8 @@

 #ifndef __ENV_CONFIG_FILE_H__

 #define __ENV_CONFIG_FILE_H__

 

-FILE *open_config_file(CONFIG_PART *cfgpart, char *mode);

+FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);

+FILE *open_config_file(char *configdirpath, char *mode);

 int close_config_file(FILE *config_file_handle);

 bool probe_config_file(CONFIG_PART *cfgpart);

 

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c

index 24b4067..8aace0c 100644

--- a/tools/bg_setenv.c

+++ b/tools/bg_setenv.c

@@ -18,6 +18,7 @@

 #include "ebgenv.h"

 #include "uservars.h"

 #include "version.h"

+#include "env_config_file.h"

 

 static char doc[] =

     "bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";

@@ -48,12 +49,15 @@ static struct argp_option options_setenv[] = {

     {0}};

 

 static struct argp_option options_printenv[] = {

+    {"filepath", 'f', "ENVFILE", 0, "Output environment from file. Expects "

+                    "a valid EFI Boot Guard environment file."},

     {"verbose", 'v', 0, 0, "Be verbose"},

     {"version", 'V', 0, 0, "Print version"},

     {0}};

 

 struct arguments {

-    bool output_to_file;

+    bool target_file;

+    bool printenv;

     int which_part;

 };

 

@@ -351,10 +355,17 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

                        (uint8_t *)arg, strlen(arg) + 1);

         break;

     case 'f':

-        arguments->output_to_file = true;

-        res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);

-        if (res == -1) {

-            return ENOMEM;

+        arguments->target_file = true;

+        if (arguments->printenv) {

+            res = asprintf(&envfilepath, "%s", arg);

+            if (res == -1) {

+                return ENOMEM;

+            }

+        } else {

+            res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);

+            if (res == -1) {

+                return ENOMEM;

+            }

         }

         break;

     case 'c':

@@ -533,6 +544,32 @@ static void dump_envs(void)

     }

 }

 

+static bool get_env(char *configfilepath, BG_ENVDATA *env)

+{

+    FILE *config;

+    char buffer[ENV_STRING_LENGTH];

+    bool result = true;

+

+    if (!(config = open_config_file(configfilepath, "rb"))) {

+        return false;

+    }

+

+    if (!(fread(env, sizeof(BG_ENVDATA), 1, config) == 1)) {

+        VERBOSE(stderr, "Error reading environment data from %s\n",

+            configfilepath);

+        if (feof(config)) {

+            VERBOSE(stderr, "End of file encountered.\n");

+        }

+        result = false;

+    }

+

+    if (close_config_file(config)) {

+        VERBOSE(stderr,

+            "Error closing environment file after reading.\n");

+    };

+    return result;

+}

+

 int main(int argc, char **argv)

 {

     static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};

@@ -557,7 +594,8 @@ int main(int argc, char **argv)

     }

 

     struct arguments arguments;

-    arguments.output_to_file = false;

+    arguments.target_file = false;

+    arguments.printenv = !write_mode;

     arguments.which_part = 0;

 

     STAILQ_INIT(&head);

@@ -572,8 +610,8 @@ int main(int argc, char **argv)

 

     /* arguments are parsed, journal is filled */

 

-    /* is output to file ? */

-    if (arguments.output_to_file) {

+    /* is output to file or input from file ? */

+    if (arguments.target_file && ! arguments.printenv) {

         /* execute journal and write to file */

         BGENV env;

         BG_ENVDATA data;

@@ -606,6 +644,16 @@ int main(int argc, char **argv)

         }

         free(envfilepath);

 

+        return 0;

+    } else if (arguments.target_file && arguments.printenv) {

+        BG_ENVDATA env;

+        if (!get_env(envfilepath, &env)) {

+            fprintf(stderr, "Error reading environment file.\n");

+            return 1;

+        }

+

+        dump_env(&env);

+

         return 0;

     }

 

--

2.29.2

[Jan Kiszka]

Got mangled like you can see above and on the archive. Could you resend
as plaintext? When in doubt about your mail client, use git send-email.

Jan

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

[Silvano Cirujano Cuesta]

--
2.29.2

[Silvano Cirujano Cuesta]

Bloody Thunderbird. I've had to reconfigure it after a new installation and I thought I had it all ready to go...

I've resent it with "git send-email" and Google Groups identifies it as being a reply to my first try, although they have completely different Message-IDs :-/

Silvano

[Jan Kiszka]

This is a strange API. The caller should be responsible for freeing, not
the callee.

> + return config;
> +}
> +
> +FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
> {
> char *configfilepath;
>
> @@ -27,16 +39,10 @@ FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)
> }
> configfilepath = (char *)malloc(strlen(FAT_ENV_FILENAME) +
> strlen(cfgpart->mountpoint) + 2);
> - if (!configfilepath) {
> - return NULL;
> - }

Bug!

You call it "configfilepath" in the implementation.

> int close_config_file(FILE *config_file_handle);
> bool probe_config_file(CONFIG_PART *cfgpart);
>
> diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
> index 24b4067..8aace0c 100644
> --- a/tools/bg_setenv.c
> +++ b/tools/bg_setenv.c
> @@ -18,6 +18,7 @@
> #include "ebgenv.h"
> #include "uservars.h"
> #include "version.h"
> +#include "env_config_file.h"
>
> static char doc[] =
> "bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";
> @@ -48,12 +49,15 @@ static struct argp_option options_setenv[] = {
> {0}};
>
> static struct argp_option options_printenv[] = {
> + {"filepath", 'f', "ENVFILE", 0, "Output environment from file. Expects "

"Read environment from file."

> + "a valid EFI Boot Guard environment file."},
> {"verbose", 'v', 0, 0, "Be verbose"},
> {"version", 'V', 0, 0, "Print version"},
> {0}};
>
> struct arguments {
> - bool output_to_file;
> + bool target_file;

It's not the target file. It means "use a file", either as source or sink.

We likely have cases already, but lets avoid adding more:
Please split assignment and result evaluation into two statements.

!arguments...

> /* execute journal and write to file */
> BGENV env;
> BG_ENVDATA data;
> @@ -606,6 +644,16 @@ int main(int argc, char **argv)
> }
> free(envfilepath);
>
> + return 0;
> + } else if (arguments.target_file && arguments.printenv) {
> + BG_ENVDATA env;
> + if (!get_env(envfilepath, &env)) {
> + fprintf(stderr, "Error reading environment file.\n");
> + return 1;
> + }
> +
> + dump_env(&env);
> +
> return 0;
> }
>
>

It may look better to do

if (arguments.use_file) {
if (arguments.printenv) {
ret = printenv();
} else {
ret = writeenv();
}
return ret;
}

i.e. moving the nested bodies into local functions.

Jan

[Silvano Cirujano Cuesta]

+1 on my last refactoring I left a couple of allocations and corresponding free spread over wrong places

>
>> + return config;
>> +}
>> +
>> +FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
>> {
>> char *configfilepath;
>>
>> @@ -27,16 +39,10 @@ FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)
>> }
>> configfilepath = (char *)malloc(strlen(FAT_ENV_FILENAME) +
>> strlen(cfgpart->mountpoint) + 2);
>> - if (!configfilepath) {
>> - return NULL;
>> - }
> Bug!

+1 Good catch! No excuse for this :-)

+1 Good catch! Left over of last refactoring.

>
>> int close_config_file(FILE *config_file_handle);
>> bool probe_config_file(CONFIG_PART *cfgpart);
>>
>> diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
>> index 24b4067..8aace0c 100644
>> --- a/tools/bg_setenv.c
>> +++ b/tools/bg_setenv.c
>> @@ -18,6 +18,7 @@
>> #include "ebgenv.h"
>> #include "uservars.h"
>> #include "version.h"
>> +#include "env_config_file.h"
>>
>> static char doc[] =
>> "bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";
>> @@ -48,12 +49,15 @@ static struct argp_option options_setenv[] = {
>> {0}};
>>
>> static struct argp_option options_printenv[] = {
>> + {"filepath", 'f', "ENVFILE", 0, "Output environment from file. Expects "
> "Read environment from file."

I don't have a strong opinion on this, so your proposal is fine for me.

>
>> + "a valid EFI Boot Guard environment file."},
>> {"verbose", 'v', 0, 0, "Be verbose"},
>> {"version", 'V', 0, 0, "Print version"},
>> {0}};
>>
>> struct arguments {
>> - bool output_to_file;
>> + bool target_file;
> It's not the target file. It means "use a file", either as source or sink.

Originally I had two separate variables (input_file and output_file) and I thought that merging them would make the code easier to understand. I didn't come up to a better naming.

What's your proposal? Splitting them again? Another name? Which one?

You're right, we have more like this [1]. In fact I've simply reused the existing code to keep the style, although I'd rather split also assignment and comparison.

I'd then fix the other one right away to keep them consistent, if it's fine for you.

[1] https://github.com/siemens/efibootguard/blob/d86e13439c94c4db0641f18e025bb2055cf582e8/env/env_api_fat.c#L70

What? Sorry, I don't understand what you mean here.

>
>> /* execute journal and write to file */
>> BGENV env;
>> BG_ENVDATA data;
>> @@ -606,6 +644,16 @@ int main(int argc, char **argv)
>> }
>> free(envfilepath);
>>
>> + return 0;
>> + } else if (arguments.target_file && arguments.printenv) {
>> + BG_ENVDATA env;
>> + if (!get_env(envfilepath, &env)) {
>> + fprintf(stderr, "Error reading environment file.\n");
>> + return 1;
>> + }
>> +
>> + dump_env(&env);
>> +
>> return 0;
>> }
>>
>>
> It may look better to do
>
> if (arguments.use_file) {
> if (arguments.printenv) {
> ret = printenv();
> } else {
> ret = writeenv();
> }
> return ret;
> }
>
> i.e. moving the nested bodies into local functions.

I used originally the nested conditionals, that you're using. But the resulting diff was much harder to review than the resulting from this patch. I'd foreseen a 2ND version and was already planned to do it there with nested conditional.

Silvano

>
> Jan
>

[Jan Kiszka]

Sure, just provide separate patches.

Style: No space between "!" and "arguments.printenv".

Just push the refactoring into a separate patch that comes before the
feature addition.

[Silvano Cirujano Cuesta]

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>

---
tools/bg_setenv.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 24b4067..3d7329f 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -23,29 +23,29 @@ static char doc[] =

"bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";

static struct argp_option options_setenv[] = {

- {"kernel", 'k', "KERNEL", 0, "Set kernel to load"},
- {"args", 'a', "KERNEL_ARGS", 0, "Set kernel arguments"},
- {"part", 'p', "ENV_PART", 0, "Set environment partition to update. "
+ {"kernel", 'k', "KERNEL", 0, "Set kernel to load"},
+ {"args", 'a', "KERNEL_ARGS", 0, "Set kernel arguments"},
+ {"part", 'p', "ENV_PART", 0, "Set environment partition to update. "
"If no partition is specified, the one "
"with the smallest revision value "
"above zero is updated."},
- {"revision", 'r', "REVISION", 0, "Set revision value"},
- {"ustate", 's', "USTATE", 0, "Set update status for environment"},
- {"filepath", 'f', "ENVFILE_DIR", 0, "Output environment to file. Expects "
+ {"revision", 'r', "REVISION", 0, "Set revision value"},
+ {"ustate", 's', "USTATE", 0, "Set update status for environment"},
+ {"filepath", 'f', "ENVFILE_DIR", 0, "Output environment to file. Expects "
"an output path where the file name "
"is automatically appended."},
- {"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
- {"confirm", 'c', 0, 0, "Confirm working environment"},
- {"update", 'u', 0, 0, "Automatically update oldest revision"},
- {"verbose", 'v', 0, 0, "Be verbose"},
- {"uservar", 'x', "KEY=VAL", 0, "Set user-defined string variable. For "
+ {"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
+ {"confirm", 'c', 0, 0, "Confirm working environment"},
+ {"update", 'u', 0, 0, "Automatically update oldest revision"},
+ {"verbose", 'v', 0, 0, "Be verbose"},
+ {"uservar", 'x', "KEY=VAL", 0, "Set user-defined string variable. For "
"setting multiple variables, use this "
"option multiple times."},
- {"in_progress", 'i', "IN_PROGRESS", 0, "Set in_progress variable to "
+ {"in_progress", 'i', "IN_PROGRESS", 0, "Set in_progress variable to "
"simulate a running update "
"process."},
- {"version", 'V', 0, 0, "Print version"},
- {0}};
+ {"version", 'V', 0, 0, "Print version"},
+ {0}};

static struct argp_option options_printenv[] = {

{"verbose", 'v', 0, 0, "Be verbose"},

@@ -134,7 +134,7 @@ static void journal_process_action(BGENV *env, struct env_action *action)
t = strtol(arg, &tmp, 10);
if ((errno == ERANGE && (t == LONG_MAX ||
t == LONG_MIN)) ||
- (errno != 0 && t == 0) || (tmp == arg)) {
+ (errno != 0 && t == 0) || (tmp == arg)) {
fprintf(stderr, "Invalid value for ustate: %s",
(char *)action->data);
return;
--
2.29.2

[Silvano Cirujano Cuesta]

This patch series is mostly focused on adding support from reading the
environment from a file which path is given to bg_printenv.

It also provides some small style fixes.

Silvano Cirujano Cuesta (5):
style: fix mixed usage of tabs and whitespaces
style: split variable assignment and comparison
style: simplify removing unneeded code
Support printenv direct from file
refactor: move code to functions

env/env_api_fat.c | 6 +-
env/env_config_file.c | 18 ++++-
include/env_config_file.h | 3 +-
tools/bg_setenv.c | 165 ++++++++++++++++++++++++++------------
4 files changed, 132 insertions(+), 60 deletions(-)

--
2.29.2

[Silvano Cirujano Cuesta]

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>
---

tools/bg_setenv.c | 98 +++++++++++++++++++++++++++--------------------
1 file changed, 56 insertions(+), 42 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 1023395..262ccde 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -568,6 +568,56 @@ static bool get_env(char *configfilepath, BG_ENVDATA *data)
return result;
}

+static int printenv_from_file(char *envfilepath) {
+ int success = 0;
+ BG_ENVDATA data;
+
+ success = get_env(envfilepath, &data);
+ if (success) {
+ dump_env(&data);
+ return 0;
+ } else {

+ fprintf(stderr, "Error reading environment file.\n");
+ return 1;
+ }
+}
+

+static int dumpenv_to_file(char *envfilepath) {
+ /* execute journal and write to file */
+ int result = 0;
+ BGENV env;
+ BG_ENVDATA data;
+
+ memset(&env, 0, sizeof(BGENV));
+ memset(&data, 0, sizeof(BG_ENVDATA));
+ env.data = &data;
+
+ update_environment(&env);
+ if (verbosity) {
+ dump_env(env.data);
+ }
+ FILE *of = fopen(envfilepath, "wb");
+ if (of) {
+ if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
+ fprintf(stderr,
+ "Error writing to output file: %s\n",
+ strerror(errno));
+ result = errno;
+ }
+ if (fclose(of)) {
+ fprintf(stderr, "Error closing output file.\n");
+ result = errno;
+ };
+ fprintf(stdout, "Output written to %s.\n", envfilepath);
+ } else {
+ fprintf(stderr, "Error opening output file %s (%s).\n",
+ envfilepath, strerror(errno));
+ result = 1;
+ }

+
+ return result;
+}
+
int main(int argc, char **argv)
{
static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};

@@ -607,51 +657,15 @@ int main(int argc, char **argv)

/* arguments are parsed, journal is filled */

- /* is output to file ? */

- if (envfilepath && write_mode) {
- /* execute journal and write to file */
- BGENV env;
- BG_ENVDATA data;
-
- memset(&env, 0, sizeof(BGENV));
- memset(&data, 0, sizeof(BG_ENVDATA));
- env.data = &data;
-
- update_environment(&env);
- if (verbosity) {
- dump_env(env.data);
- }
- FILE *of = fopen(envfilepath, "wb");
- if (of) {
- if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
- fprintf(stderr,
- "Error writing to output file: %s\n",
- strerror(errno));
- result = errno;
- }
- if (fclose(of)) {
- fprintf(stderr, "Error closing output file.\n");
- result = errno;
- };
- fprintf(stdout, "Output written to %s.\n", envfilepath);

+ /* is output to file or input from file ? */

+ if (envfilepath) {
+ if (write_mode) {
+ result = dumpenv_to_file(envfilepath);
} else {
- fprintf(stderr, "Error opening output file %s (%s).\n",
- envfilepath, strerror(errno));
- result = 1;
+ result = printenv_from_file(envfilepath);
}
free(envfilepath);
-
- return 0;
- } else if (envfilepath && !write_mode) {
- BG_ENVDATA env;
- if (!get_env(envfilepath, &env)) {
- fprintf(stderr, "Error reading environment file.\n");
- return 1;
- }
-
- dump_env(&env);
-
- return 0;
+ return result;
}

/* not in file mode */
--
2.29.2

[Silvano Cirujano Cuesta]

A variable shouldn't get assigned directly in an if statement. It's a
discouraged code style because it's error prone (e.g. using assignment
'=' where comparison '==' was desired) and difficult to detect.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>
---

env/env_api_fat.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 2b23e7f..c443408 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -67,7 +67,8 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env)

part->mountpoint);
}
FILE *config;
- if (!(config = open_config_file(part, "rb"))) {

+ config = open_config_file(part, "rb");
+ if (!config) {

return false;
}
bool result = true;

@@ -104,7 +105,8 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)

part->mountpoint);
}
FILE *config;
- if (!(config = open_config_file(part, "wb"))) {

+ config = open_config_file(part, "wb");
+ if (!config) {

VERBOSE(stderr, "Could not open config file for writing.\n");
return false;
}

--
2.29.2

[Silvano Cirujano Cuesta]

The tools bg_setenv and bg_printenv are mostly oriented at running systems
that have the partitions providing BG environments already mounted.

This patch extends bg_printenv so that the environment can be read directly
from whatever valid BG environment file. This usage is very usefull in
development, diagnose, testing,... scenarios.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
env/env_api_fat.c | 4 +--

env/env_config_file.c | 18 ++++++++++---
include/env_config_file.h | 3 ++-
tools/bg_setenv.c | 56 ++++++++++++++++++++++++++++++++++++---
4 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index c443408..130c161 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -67,7 +67,7 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env)
part->mountpoint);
}
FILE *config;
- config = open_config_file(part, "rb");
+ config = open_config_file_from_part(part, "rb");
if (!config) {
return false;
}
@@ -105,7 +105,7 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
part->mountpoint);
}
FILE *config;
- config = open_config_file(part, "wb");
+ config = open_config_file_from_part(part, "wb");

if (!config) {
VERBOSE(stderr, "Could not open config file for writing.\n");
return false;

diff --git a/env/env_config_file.c b/env/env_config_file.c
index 873fe10..e270fbb 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -18,7 +18,18 @@

#include "env_disk_utils.h"
#include "env_config_file.h"

-FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)
+FILE *open_config_file(char *configfilepath, char *mode)
+{

+ VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
+ FILE *config = fopen(configfilepath, mode);

+ if (config) {
+ return config;
+ } else {

+ return NULL;
+ }
+}
+

+FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
{
char *configfilepath;

@@ -33,8 +44,7 @@ FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)

strcpy(configfilepath, cfgpart->mountpoint);
strcat(configfilepath, "/");
strcat(configfilepath, FAT_ENV_FILENAME);
- VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
- FILE *config = fopen(configfilepath, mode);

+ FILE *config = open_config_file(configfilepath, mode);
free(configfilepath);
return config;
}
@@ -74,7 +84,7 @@ bool probe_config_file(CONFIG_PART *cfgpart)

cfgpart->devpath, cfgpart->mountpoint);
bool result = false;

FILE *config;

- if (!(config = open_config_file(cfgpart, "rb"))) {
+ if (!(config = open_config_file_from_part(cfgpart, "rb"))) {
printf_debug(
"Could not open config file on partition %s.\n",
FAT_ENV_FILENAME);
diff --git a/include/env_config_file.h b/include/env_config_file.h

index 8fbe8fa..2679174 100644

--- a/include/env_config_file.h
+++ b/include/env_config_file.h
@@ -15,7 +15,8 @@
#ifndef __ENV_CONFIG_FILE_H__
#define __ENV_CONFIG_FILE_H__

-FILE *open_config_file(CONFIG_PART *cfgpart, char *mode);
+FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);

+FILE *open_config_file(char *configfilepath, char *mode);

int close_config_file(FILE *config_file_handle);
bool probe_config_file(CONFIG_PART *cfgpart);

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 0395751..1023395 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c

@@ -18,6 +18,7 @@
#include "ebgenv.h"
#include "uservars.h"
#include "version.h"
+#include "env_config_file.h"

static char doc[] =
"bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";

@@ -48,11 +49,14 @@ static struct argp_option options_setenv[] = {

{0}};

static struct argp_option options_printenv[] = {

+ {"filepath", 'f', "ENVFILE", 0, "Read environment from file. Expects "

+ "a valid EFI Boot Guard environment file."},

{"verbose", 'v', 0, 0, "Be verbose"},

{"version", 'V', 0, 0, "Print version"},

{0}};

struct arguments {

+ bool printenv;
int which_part;
};

@@ -350,9 +354,16 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

(uint8_t *)arg, strlen(arg) + 1);
break;
case 'f':

- res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
- if (res == -1) {
- return ENOMEM;

+ if (arguments->printenv) {
+ res = asprintf(&envfilepath, "%s", arg);
+ if (res == -1) {
+ return ENOMEM;
+ }
+ } else {
+ res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ if (res == -1) {
+ return ENOMEM;
+ }
}
break;
case 'c':

@@ -531,6 +542,32 @@ static void dump_envs(void)
}
}

+static bool get_env(char *configfilepath, BG_ENVDATA *data)

+{
+ FILE *config;
+ char buffer[ENV_STRING_LENGTH];
+ bool result = true;
+
+ if (!(config = open_config_file(configfilepath, "rb"))) {

+ return false;
+ }
+

+ if (!(fread(data, sizeof(BG_ENVDATA), 1, config) == 1)) {

+ VERBOSE(stderr, "Error reading environment data from %s\n",
+ configfilepath);
+ if (feof(config)) {
+ VERBOSE(stderr, "End of file encountered.\n");
+ }
+ result = false;
+ }
+
+ if (close_config_file(config)) {
+ VERBOSE(stderr,
+ "Error closing environment file after reading.\n");

+ };
+ return result;
+}
+
int main(int argc, char **argv)
{
static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};

@@ -555,6 +592,7 @@ int main(int argc, char **argv)
}

struct arguments arguments;

+ arguments.printenv = !write_mode;
arguments.which_part = 0;

STAILQ_INIT(&head);

@@ -570,7 +608,7 @@ int main(int argc, char **argv)

/* arguments are parsed, journal is filled */

/* is output to file ? */

- if (envfilepath) {
+ if (envfilepath && write_mode) {

/* execute journal and write to file */

BGENV env;
BG_ENVDATA data;
@@ -603,6 +641,16 @@ int main(int argc, char **argv)
}
free(envfilepath);

+ return 0;
+ } else if (envfilepath && !write_mode) {

+ BG_ENVDATA env;
+ if (!get_env(envfilepath, &env)) {

+ fprintf(stderr, "Error reading environment file.\n");
+ return 1;
+ }
+

[Jan Kiszka]

That was almost correctly indented, but now it's misleading. Dropped
that hunk while merging, will fix up separately on top (there are more
inconsistencies in this file).

> fprintf(stderr, "Invalid value for ustate: %s",
> (char *)action->data);
> return;
>

[Silvano Cirujano Cuesta]

Changes V3: rebased on next

Silvano Cirujano Cuesta (2):

Support printenv direct from file
refactor: move code to functions

env/env_api_fat.c | 4 +-
env/env_config_file.c | 18 ++++--
include/env_config_file.h | 3 +-
tools/bg_setenv.c | 133 +++++++++++++++++++++++++++-----------
4 files changed, 115 insertions(+), 43 deletions(-)

--
2.29.2

[Silvano Cirujano Cuesta]

The tools bg_setenv and bg_printenv are mostly oriented at running systems
that have the partitions providing BG environments already mounted.

This patch extends bg_printenv so that the environment can be read directly
from whatever valid BG environment file. This usage is very usefull in
development, diagnose, testing,... scenarios.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
env/env_api_fat.c | 4 +--

env/env_config_file.c | 18 +++++++++---
include/env_config_file.h | 3 +-
tools/bg_setenv.c | 59 +++++++++++++++++++++++++++++++++++----
4 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 57bbc0d..d035c54 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c

@@ -18,6 +18,7 @@
#include "ebgenv.h"
#include "uservars.h"
#include "version.h"
+#include "env_config_file.h"

static char doc[] =
"bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";

@@ -47,12 +48,14 @@ static struct argp_option options_setenv[] = {

};

static struct argp_option options_printenv[] = {

+ {"filepath", 'f', "ENVFILE", 0, "Read environment from file. Expects "
+ "a valid EFI Boot Guard environment file."},

{"verbose", 'v', 0, 0, "Be verbose"},

{"version", 'V', 0, 0, "Print version"},
- {}

-};
+ {0}};

struct arguments {
+ bool printenv;
int which_part;
};

@@ -350,9 +353,16 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

(uint8_t *)arg, strlen(arg) + 1);
break;
case 'f':
- res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
- if (res == -1) {
- return ENOMEM;
+ if (arguments->printenv) {
+ res = asprintf(&envfilepath, "%s", arg);
+ if (res == -1) {
+ return ENOMEM;
+ }
+ } else {
+ res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
+ if (res == -1) {
+ return ENOMEM;
+ }
}
break;
case 'c':

@@ -531,6 +541,32 @@ static void dump_envs(void)

@@ -555,6 +591,7 @@ int main(int argc, char **argv)

}

struct arguments arguments;
+ arguments.printenv = !write_mode;
arguments.which_part = 0;

STAILQ_INIT(&head);

@@ -570,7 +607,7 @@ int main(int argc, char **argv)

/* arguments are parsed, journal is filled */

/* is output to file ? */
- if (envfilepath) {
+ if (envfilepath && write_mode) {
/* execute journal and write to file */
BGENV env;
BG_ENVDATA data;

@@ -603,6 +640,16 @@ int main(int argc, char **argv)

[Silvano Cirujano Cuesta]

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>

---
tools/bg_setenv.c | 98 +++++++++++++++++++++++++++--------------------
1 file changed, 56 insertions(+), 42 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d035c54..53c004a 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -567,6 +567,56 @@ static bool get_env(char *configfilepath, BG_ENVDATA *data)

return result;
}

+static int printenv_from_file(char *envfilepath) {
+ int success = 0;
+ BG_ENVDATA data;
+
+ success = get_env(envfilepath, &data);
+ if (success) {
+ dump_env(&data);

+ return 0;
+ } else {

+ fprintf(stderr, "Error reading environment file.\n");
+ return 1;
+ }
+}
+

+
+ return result;
+}
+
int main(int argc, char **argv)
{
static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};

@@ -606,51 +656,15 @@ int main(int argc, char **argv)

/* arguments are parsed, journal is filled */

[Jan Kiszka]

On 01.12.20 15:06, [ext] Silvano Cirujano Cuesta wrote:
> Changes V3: rebased on next
>
> Silvano Cirujano Cuesta (2):
> Support printenv direct from file
> refactor: move code to functions
>

...but not reordered. Sorry, but this is a common pattern: You want to
add something that requires some factoring, do that first.

Jan

> env/env_api_fat.c | 4 +-
> env/env_config_file.c | 18 ++++--
> include/env_config_file.h | 3 +-
> tools/bg_setenv.c | 133 +++++++++++++++++++++++++++-----------
> 4 files changed, 115 insertions(+), 43 deletions(-)
>

--

[Silvano Cirujano Cuesta]

On 01/12/2020 15:14, Jan Kiszka wrote:
> On 01.12.20 15:06, [ext] Silvano Cirujano Cuesta wrote:
>> Changes V3: rebased on next
>>
>> Silvano Cirujano Cuesta (2):
>> Support printenv direct from file
>> refactor: move code to functions
>>
> ...but not reordered. Sorry, but this is a common pattern: You want to
> add something that requires some factoring, do that first.
>
> Jan

I also do refactoring before changing. Except if, like in this case, the refactoring is only motivated by my changes.

But I can reorder it, not a big deal.

Silvano

[Silvano Cirujano Cuesta]

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>
---

tools/bg_setenv.c | 70 +++++++++++++++++++++++++----------------------
1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 57bbc0d..d068a2b 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -531,6 +531,42 @@ static void dump_envs(void)

@@ -571,39 +607,9 @@ int main(int argc, char **argv)

/* is output to file ? */

if (envfilepath) {

- } else {

- fprintf(stderr, "Error opening output file %s (%s).\n",
- envfilepath, strerror(errno));
- result = 1;

- }
+ result = dumpenv_to_file(envfilepath);

free(envfilepath);
-
- return 0;

[Silvano Cirujano Cuesta]

The tools bg_setenv and bg_printenv are mostly oriented at running systems
that have the partitions providing BG environments already mounted.

This patch extends bg_printenv so that the environment can be read directly
from whatever valid BG environment file. This usage is very usefull in
development, diagnose, testing,... scenarios.

Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>
---

env/env_api_fat.c | 4 +--
env/env_config_file.c | 18 +++++++---
include/env_config_file.h | 3 +-
tools/bg_setenv.c | 69 +++++++++++++++++++++++++++++++++++----
4 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d068a2b..53c004a 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c

@@ -531,6 +541,46 @@ static void dump_envs(void)

}
}

+static bool get_env(char *configfilepath, BG_ENVDATA *data)
+{
+ FILE *config;
+ char buffer[ENV_STRING_LENGTH];
+ bool result = true;
+
+ if (!(config = open_config_file(configfilepath, "rb"))) {
+ return false;
+ }
+
+ if (!(fread(data, sizeof(BG_ENVDATA), 1, config) == 1)) {
+ VERBOSE(stderr, "Error reading environment data from %s\n",
+ configfilepath);
+ if (feof(config)) {
+ VERBOSE(stderr, "End of file encountered.\n");
+ }
+ result = false;
+ }
+
+ if (close_config_file(config)) {
+ VERBOSE(stderr,
+ "Error closing environment file after reading.\n");

+ };
+ return result;
+}
+

+static int printenv_from_file(char *envfilepath) {
+ int success = 0;

+ BG_ENVDATA data;
+

+ success = get_env(envfilepath, &data);
+ if (success) {
+ dump_env(&data);
+ return 0;

+ } else {

+ fprintf(stderr, "Error reading environment file.\n");
+ return 1;
+ }
+}
+

static int dumpenv_to_file(char *envfilepath) {

/* execute journal and write to file */

int result = 0;
@@ -591,6 +641,7 @@ int main(int argc, char **argv)

}

struct arguments arguments;
+ arguments.printenv = !write_mode;
arguments.which_part = 0;

STAILQ_INIT(&head);

@@ -605,9 +656,13 @@ int main(int argc, char **argv)

/* arguments are parsed, journal is filled */

- /* is output to file ? */
+ /* is output to file or input from file ? */
if (envfilepath) {
- result = dumpenv_to_file(envfilepath);

+ if (write_mode) {
+ result = dumpenv_to_file(envfilepath);

+ } else {
+ result = printenv_from_file(envfilepath);
+ }
free(envfilepath);
return result;
}
--
2.29.2

[Silvano Cirujano Cuesta]

Changes V4: patches reordered

First refactoring and then functionality addition.

Silvano Cirujano Cuesta (2):

refactor: move code to functions
Support printenv direct from file

env/env_api_fat.c | 4 +-
env/env_config_file.c | 18 ++++--
include/env_config_file.h | 3 +-
tools/bg_setenv.c | 133 +++++++++++++++++++++++++++-----------
4 files changed, 115 insertions(+), 43 deletions(-)

--
2.29.2

[Jan Kiszka]

Applied both - just dropping the bottom half of the hunk and aligned the
style of the top half.

Thanks,
Jan

[Jan Kiszka]

On 01.12.20 16:01, [ext] Silvano Cirujano Cuesta wrote:

cppcheck found this as unused - forgotten or hidden issue?

Jan

[Silvano Cirujano Cuesta]

Left over of a first draft where I was reading and dumping the fields individually instead of using "dump_env". Can be safely removed.

Silvano

[Jan Kiszka]

Only noticed now: This is inconsistent with bg_setenv. The latter uses
filepath to take a directory where BGENV.DAT is placed. This here
expects the filename under the very same argument. We must fix this, one
way or the other.

Jan