[PATCH v3 0/3] Fix global variables with tools

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

NOTE: This patch series replaces all currently pending
patches on the mailing list.

Integration tests with swupdate showed a problem with
global variables, which led to corrupted environments.

Problem was a missing crc calculation in the handler
for global user variables.

On the other hand, the tools could easily be upgraded
to handle global user variables now.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>

Andreas Reichel (3):
Tools: Fix global variable management in tools.
ebgenv.h: Use brackets with numerical definitions
Add missing crc32 calculation to set_uservar_global

docs/TOOLS.md | 10 +++++++++-
env/env_api.c | 5 +++--
env/env_api_fat.c | 3 +++
include/ebgenv.h | 4 ++--
tools/bg_setenv.c | 18 ++++--------------
5 files changed, 21 insertions(+), 19 deletions(-)

--
2.15.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Correct default data type in API function and fix impractical function
calls to fully support global user variables from tools.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

docs/TOOLS.md | 10 +++++++++-
env/env_api.c | 5 +++--

tools/bg_setenv.c | 18 ++++--------------
3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/docs/TOOLS.md b/docs/TOOLS.md
index 7dd1f83..814e6ba 100644
--- a/docs/TOOLS.md
+++ b/docs/TOOLS.md
@@ -92,4 +92,12 @@ bg_setenv --partition=1 --ustate=TESTING
bg_setenv -x key=value
```

-This will set the variable named `key` to `value` in the current environment.
+This will set the variable named `key` to `value` in all environments.
+
+If The user wants to delete such a variable, the value after the `=` must be omitted, e.g.
+
+```
+bg_setenv -x key=
+```
+will delete the variable with key `key`.
+
diff --git a/env/env_api.c b/env/env_api.c
index ea55bad..29808ff 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -107,8 +107,9 @@ int ebg_env_get_ex(ebgenv_t *e, char *key, uint64_t *usertype, uint8_t *buffer,

int ebg_env_set(ebgenv_t *e, char *key, char *value)
{
- return bgenv_set((BGENV *)e->bgenv, key, USERVAR_TYPE_DEFAULT, value,
- strlen(value) + 1);
+ return bgenv_set((BGENV *)e->bgenv, key, USERVAR_TYPE_DEFAULT |
+ USERVAR_TYPE_STRING_ASCII, value,
+ strlen(value) + 1);
}

int ebg_env_set_ex(ebgenv_t *e, char *key, uint64_t usertype, uint8_t *value,
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index e9156cd..741d604 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -65,8 +65,6 @@ struct env_action {

STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);

-static bool inhibit_global_store = false;
-
static void journal_free_action(struct env_action *action)
{
if (!action) {
@@ -116,12 +114,6 @@ static void journal_process_action(BGENV *env, struct env_action *action)
ebgenv_t e;
char *tmp;

- if (inhibit_global_store) {
- /* If the environment is written to a file, we don't want a
- * global variable storage, but only into the output file */
- action->type &= ~USERVAR_TYPE_GLOBAL;
- }
-
switch (action->task) {
case ENV_TASK_SET:
VERBOSE(stdout, "Task = SET, key = %s, type = %llu, val = %s\n",
@@ -156,10 +148,7 @@ static void journal_process_action(BGENV *env, struct env_action *action)
break;
case ENV_TASK_DEL:
VERBOSE(stdout, "Task = DEL, key = %s\n", action->key);
- var = bgenv_find_uservar(env->data->userdata, action->key);
- if (var) {
- bgenv_del_uservar(env->data->userdata, var);
- }
+ bgenv_set(env, action->key, action->type, "", 1);
break;
}
}
@@ -211,7 +200,9 @@ static error_t set_uservars(char *arg)

value = strtok(NULL, "=");
if (value == NULL) {
- return journal_add_action(ENV_TASK_DEL, key, 0, NULL, 0);
+ return journal_add_action(ENV_TASK_DEL, key,
+ USERVAR_TYPE_DEFAULT |
+ USERVAR_TYPE_DELETED, NULL, 0);
}
return journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT |
USERVAR_TYPE_STRING_ASCII,
@@ -319,7 +310,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'f':
arguments->output_to_file = true;
- inhibit_global_store = true;
res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
if (res == -1) {
return ENOMEM;
--
2.15.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Brackets are important for definition of flag bits,
otherwise logical operations like 'not' lead to wrong
results.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

include/ebgenv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/ebgenv.h b/include/ebgenv.h
index 1257141..3f03012 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -28,8 +28,8 @@
#define USERVAR_TYPE_SINT64 9
#define USERVAR_TYPE_STRING_ASCII 32
#define USERVAR_TYPE_BOOL 64
-#define USERVAR_TYPE_GLOBAL 1ULL << 62
-#define USERVAR_TYPE_DELETED 1ULL << 63
+#define USERVAR_TYPE_GLOBAL (1ULL << 62)
+#define USERVAR_TYPE_DELETED (1ULL << 63)
#define USERVAR_TYPE_DEFAULT USERVAR_TYPE_GLOBAL

#define USERVAR_STANDARD_TYPE_MASK ((1ULL << 32) - 1)
--
2.15.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

set_uservar_global missed crc32 calculation so that
all environments were rendered invalid.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---

env/env_api_fat.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 0351330..a2e4700 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -341,6 +341,9 @@ int bgenv_set_uservar_global(char *key, uint64_t type, void *data,
" #%d: (%s)\n", i, strerror(-r));
ret = r;
}
+ uint32_t sum = crc32(0, (Bytef *)env->data,
+ sizeof(BG_ENVDATA) - sizeof(env->data->crc32));
+ env->data->crc32 = sum;
if (!bgenv_write(env)) {
ret = -EIO;
VERBOSE(stderr, "bgenv_set_uservar_global: Could not "
--
2.15.0

[Jan Kiszka]

All look good, applied to next. But my conceptual concern regarding
global variables should be resolved (or we need at least a strategy to
fix a potential issue on top) prior to making next master.

Jan