[PATCH 0/6] RfC: Added INI format to bg_setenv and pg_printenv

[Tobias Schmidl]

Hello all,

I've added a first draft for an INI format as alternative to BGENV.DAT.
Both bg_setenv and bg_printenv have been extended for this possibility.

Looking forwards to your comments.

Kind regards,

Tobias


Tobias Schmidl (6):
env_user_config_file: Added read part
bg_printenv: added INI part
gitignore: added libebgenv.pc
envdata: moved magic strings to a preprocessor macro
env_user_config_file: Added writing of INI file
bg_setenv: added INI part

.gitignore | 1 +
Makefile.am | 2 +
env/env_user_config_file.c | 298 +++++++++++++++++++++++++++++++++
include/env_user_config_file.h | 23 +++
include/envdata.h | 9 +
tools/bg_printenv.c | 41 +++--
tools/bg_setenv.c | 10 ++
7 files changed, 368 insertions(+), 16 deletions(-)
create mode 100644 env/env_user_config_file.c
create mode 100644 include/env_user_config_file.h

--
2.36.1

[Tobias Schmidl]

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---
env/env_user_config_file.c | 37 +++++++++++++++++++++++++------------
include/envdata.h | 8 ++++++++
2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/env/env_user_config_file.c b/env/env_user_config_file.c
index 8d20ed3..4b24d9c 100644
--- a/env/env_user_config_file.c
+++ b/env/env_user_config_file.c
@@ -83,31 +83,44 @@ int env_user_config_file_read(const char *file_name, BG_ENVDATA *data,
const char *key = keys[j] + prefix_index + 1;
const char *value =
iniparser_getstring(ini_file, keys[j], NULL);
- if (strcmp(key, "kernelfile") == 0 &&
+ if (strcmp(key, BG_ENV_KERNELFILE_STRING) == 0 &&
strlen(value) < ENV_STRING_LENGTH) {
str8to16(data->kernelfile, value);
- VERBOSE(stderr, "kernelfile: %s\n", value);
- } else if (strcmp(key, "kernelparams") == 0 &&
+ VERBOSE(stderr,
+ BG_ENV_KERNELFILE_STRING ": %s\n",
+ value);
+ } else if (strcmp(key, BG_ENV_KERNELPARAMS_STRING) ==
+ 0 &&
strlen(value) < ENV_STRING_LENGTH) {
str8to16(data->kernelparams, value);
- VERBOSE(stderr, "kernelparams: %s\n", value);
- } else if (strcmp(key, "in_progress") == 0) {
+ VERBOSE(stderr,
+ BG_ENV_KERNELPARAMS_STRING ": %s\n",
+ value);
+ } else if (strcmp(key, BG_ENV_IN_PROGRESS_STRING) ==
+ 0) {
data->in_progress = MIN(
strtoul(value, NULL, 10), UINT8_MAX);
- VERBOSE(stdout, "in_progress: %hhu\n",
+ VERBOSE(stdout,
+ BG_ENV_IN_PROGRESS_STRING ": %hhu\n",
data->in_progress);
- } else if (strcmp(key, "ustate") == 0) {
+ } else if (strcmp(key, BG_ENV_USTATE_STRING) == 0) {
data->ustate = MIN(strtoul(value, NULL, 10),
UINT8_MAX);
- VERBOSE(stdout, "ustate: %hhu\n", data->ustate);
- } else if (strcmp(key, "watchdog_timeout_sec") == 0) {
+ VERBOSE(stdout, BG_ENV_USTATE_STRING ": %hhu\n",
+ data->ustate);
+ } else if (strcmp(key,
+ BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING) ==
+ 0) {
data->watchdog_timeout_sec = MIN(
strtoul(value, NULL, 10), UINT16_MAX);
- VERBOSE(stdout, "watchdog_timeout_sec: %hu\n",
+ VERBOSE(stdout,
+ BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING
+ ": %hu\n",
data->watchdog_timeout_sec);
- } else if (strcmp(key, "revision") == 0) {
+ } else if (strcmp(key, BG_ENV_REVISION_STRING) == 0) {
data->revision = strtoul(value, NULL, 0);
- VERBOSE(stdout, "revision: 0x%08lx\n",
+ VERBOSE(stdout,
+ BG_ENV_REVISION_STRING ": 0x%08lx\n",
data->revision);
} else
VERBOSE(stderr, "<unknown>: %s\n", value);
diff --git a/include/envdata.h b/include/envdata.h
index 7c3cfca..acd2a16 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -45,4 +45,12 @@ struct _BG_ENVDATA {
};
#pragma pack(pop)

+#define BG_ENV_KERNELFILE_STRING "kernelfile"
+#define BG_ENV_KERNELPARAMS_STRING "kernelparams"
+#define BG_ENV_IN_PROGRESS_STRING "in_progress"
+#define BG_ENV_USTATE_STRING "ustate"
+#define BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING "watchdog_timeout_sec"
+#define BG_ENV_REVISION_STRING "revision"
+#define BG_ENV_CRC32_STRING "crc32"
+
typedef struct _BG_ENVDATA BG_ENVDATA;
--
2.36.1

[Tobias Schmidl]

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---

.gitignore | 1 +
1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index ae0d682..ddbbf45 100644
--- a/.gitignore
+++ b/.gitignore
@@ -100,3 +100,4 @@ __pycache__
### Completion ###
completion/bash
completion/zsh
+/libebgenv.pc
--
2.36.1

[Tobias Schmidl]

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---

env/env_user_config_file.c | 120 ++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 1 deletion(-)

diff --git a/env/env_user_config_file.c b/env/env_user_config_file.c
index 4b24d9c..80dbafd 100644
--- a/env/env_user_config_file.c
+++ b/env/env_user_config_file.c
@@ -17,6 +17,9 @@
#include <iniparser/iniparser.h>
#include <sys/param.h>

+#define DEFAULT_DICTIONARY_SIZE 16384
+#define DEFAULT_STRING_SIZE 4096
+
#ifndef ERROR
#define ERROR wprintf
#endif
@@ -136,7 +139,117 @@ cleanup_get:
int env_user_config_file_write(const char *file_name, const BG_ENVDATA *data,
bool verbose)
{
- return 1;
+ bool bgenv_verbosity = verbose;
+ VERBOSE(stderr, "entered %s.\n", __PRETTY_FUNCTION__);
+ int return_value = 0;
+
+ dictionary *ini_dict = dictionary_new(DEFAULT_DICTIONARY_SIZE);
+ dictionary_set(ini_dict, BG_ENVDATA_STRING, NULL);
+ char *key_name = alloca(DEFAULT_STRING_SIZE);
+ char *buffer = alloca(DEFAULT_STRING_SIZE);
+ key_name[DEFAULT_STRING_SIZE - 1] = 0;
+
+ { // kernelfile
+ if (data->kernelfile == NULL ||
+ str16to8(buffer, data->kernelfile) == NULL ||
+ snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING ":" BG_ENV_KERNELFILE_STRING) <=
+ 0) { /* return with error message */
+ ERROR(L"Cannot parse kernelfile.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ { // kernelparams
+ if (data->kernelparams == NULL ||
+ str16to8(buffer, data->kernelparams) == NULL ||
+ snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING
+ ":" BG_ENV_KERNELPARAMS_STRING) <=
+ 0) { // return with error message
+ ERROR(L"Cannot parse kernelparams.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ uint32_t revision;
+
+ { // in_progress
+ if (snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING
+ ":" BG_ENV_IN_PROGRESS_STRING) <= 0 ||
+ snprintf(buffer, DEFAULT_STRING_SIZE - 1, "%hhu",
+ data->in_progress) <=
+ 0) { // return with error message
+ ERROR(L"Cannot parse in_progress.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ { // ustate
+ if (snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING ":" BG_ENV_USTATE_STRING) <= 0 ||
+ snprintf(buffer, DEFAULT_STRING_SIZE - 1, "%hhu",
+ data->ustate) <= 0) { // return with error message
+ ERROR(L"Cannot parse ustate.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ { // watchdog_timeout_sec
+ if (snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING
+ ":" BG_ENV_WATCHDOG_TIMEOUT_SEC_STRING) <= 0 ||
+ snprintf(buffer, DEFAULT_STRING_SIZE - 1, "%hu",
+ data->watchdog_timeout_sec) <=
+ 0) { // return with error message
+ ERROR(L"Cannot parse watchdog_timeout_sec.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ { // revision
+ if (snprintf(key_name, DEFAULT_STRING_SIZE - 1,
+ BG_ENVDATA_STRING
+ ":" BG_ENV_REVISION_STRING) <= 0 ||
+ snprintf(buffer, DEFAULT_STRING_SIZE - 1, "0x%08lx",
+ data->revision) <=
+ 0) { // return with error message
+ ERROR(L"Cannot parse revision.\n");
+ return_value = EINVAL;
+ goto cleanup_set;
+ }
+ VERBOSE(stderr, "%s=%s\n", key_name, buffer);
+ dictionary_set(ini_dict, key_name, buffer);
+ }
+
+ FILE *output_file = NULL;
+ if (NULL == (output_file = fopen(file_name, "w"))) {
+ return_value = errno;
+ ERROR(L"Cannot write output INI file.\n");
+ } else {
+ iniparser_dump_ini(ini_dict, output_file);
+ fclose(output_file);
+ }
+cleanup_set:
+ dictionary_del(ini_dict);
+ VERBOSE(stderr, "left %s\n.", __PRETTY_FUNCTION__);
+ return return_value;
}

#ifdef INI_MAIN
@@ -174,6 +287,11 @@ int main(int argc, char *argv[])
int return_value =
env_user_config_file_read(input_file, &data, verbose);
fprintf(stderr, "env_user_config_file_read: %i\n", return_value);
+ if (return_value != 0) return return_value;
+
+ return_value =
+ env_user_config_file_write("/dev/stdout", &data, verbose);
+ fprintf(stderr, "env_user_config_file_write: %i\n", return_value);
return return_value;
}

--
2.36.1

[Tobias Schmidl]

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---

tools/bg_setenv.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d685412..4d4916a 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -21,6 +21,7 @@
#include "bg_envtools.h"
#include "bg_setenv.h"
#include "bg_printenv.h"
+#include "env_user_config_file.h"

static char tool_doc[] =
"bg_setenv - Environment tool for the EFI Boot Guard";
@@ -351,6 +352,15 @@ static int dumpenv_to_file(char *envfilepath, bool verbosity, bool preserve_env)
if (verbosity) {
dump_env(env.data, &ALL_FIELDS, false);
}
+
+ /* compare the filename. if we operate on bgenv.dat, use the old way,
+ * otherwise try the INI file approach. */
+ const char *file_name = strrchr(envfilepath, '/');
+ file_name = (file_name != NULL) ? file_name + 1 : envfilepath;
+
+ if (strcasecmp(file_name, FAT_ENV_FILENAME) != 0)
+ return env_user_config_file_write(envfilepath, &data, verbosity);
+
FILE *of = fopen(envfilepath, "wb");
if (of) {
if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
--
2.36.1

[Jan Kiszka]

On 09.08.22 15:52, Tobias Schmidl wrote:
> Hello all,
>
> I've added a first draft for an INI format as alternative to BGENV.DAT.
> Both bg_setenv and bg_printenv have been extended for this possibility.
>

Thanks for the patches! Still need to look into the details, just a few
formal remarks upfront:

- please make sure all patches have a commit message providing a
brief (or longer, in complex cases) reasoning for the change
- do not blindly copy file headers from existing files in to new one
but also update them as needed ;)

And where in the patches are the external parser files you mentioned?
Didn't find them yet, but I might have been blind.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

[Schmidl, Tobias]

Hi all,

Am Mittwoch, dem 10.08.2022 um 09:59 +0200 schrieb Jan Kiszka:
>
> - please make sure all patches have a commit message providing a
> brief (or longer, in complex cases) reasoning for the change
>

Thanks, will do.

> - do not blindly copy file headers from existing files in to new one
> but also update them as needed ;)
>

Thanks, fixed.

>
> And where in the patches are the external parser files you mentioned?
> Didn't find them yet, but I might have been blind.
>

No, you're not. They are just implicitly mentioned in Makefile.am [1] by
linking against libiniparser.a. What's missing (and will be fixed in a v2)
is a mention in COMPILE.md.

I went with iniparser [2], a small four-file library that links
statically.

Kind regards,

Tobias

[1]
https://github.com/schtobia/efibootguard/blob/05ce60cb12f41629e3954124ef26c6594a128c81/Makefile.am#L138

[2] https://github.com/ndevilla/iniparser

[Jan Kiszka]

On 10.08.22 11:19, Schmidl, Tobias wrote:
> Hi all,
>
> Am Mittwoch, dem 10.08.2022 um 09:59 +0200 schrieb Jan Kiszka:
>>
>> - please make sure all patches have a commit message providing a
>> brief (or longer, in complex cases) reasoning for the change
>>
>
> Thanks, will do.
>
>> - do not blindly copy file headers from existing files in to new one
>> but also update them as needed ;)
>>
>
> Thanks, fixed.
>
>>
>> And where in the patches are the external parser files you mentioned?
>> Didn't find them yet, but I might have been blind.
>>
>
> No, you're not. They are just implicitly mentioned in Makefile.am [1] by
> linking against libiniparser.a. What's missing (and will be fixed in a v2)
> is a mention in COMPILE.md.
>
> I went with iniparser [2], a small four-file library that links
> statically.
>

As we discussed offline: Given that the files are not packaged by a
distro (specifically as they are shipped as source, not as a lib) and
also due to the possibility of having to modify them, I would suggest to
include those in-tree as copy / fork for now.

[Schmidl, Tobias]

Hi Jan,

Am Mittwoch, dem 10.08.2022 um 13:12 +0200 schrieb Jan Kiszka:

>
> As we discussed offline: Given that the files are not packaged by a
> distro (specifically as they are shipped as source, not as a lib) and
> also due to the possibility of having to modify them, I would suggest to
> include those in-tree as copy / fork for now.
>

there are two options for this:

- preserving the full history as seen in my git repo [1]. This would be
more accurate but less readable
- Squashing the history of the external repo into one commit.

Kind regards,

Tobias

[1]: https://github.com/schtobia/efibootguard/commits/env

[Tobias Schmidl]

This file is created by pkgconfig, so clearly a binary file.

Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
---

[Jan Kiszka]

On 12.08.22 11:28, Tobias Schmidl wrote:
> This file is created by pkgconfig, so clearly a binary file.

Not really a binary file but a generated one, thus out of scope for
version control.

>
> Signed-off-by: Tobias Schmidl <tobias...@siemens.com>
> ---
> .gitignore | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.gitignore b/.gitignore
> index ae0d682..ddbbf45 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -100,3 +100,4 @@ __pycache__
> ### Completion ###
> completion/bash
> completion/zsh
> +/libebgenv.pc

Wrong "section". Should go to autoconf or even an own one (pkg-config).