[PATCH 0/2] Remove global variable support for release

[Andreas J. Reichel]

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

Global variables are removed to avoid a regression regarding the
robustness of the update mechanism.

Furthermore, an improvement to the 'setglobalstate' function of the API
is added to reduce unnecessary environment modifications and to add
further support to recover from usually non-occuring update states.

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

Andreas Reichel (2):
Remove global user variables
ustate: Enhance API function to reset ustate

docs/API.md | 23 ++++----------------
docs/TOOLS.md | 2 +-
env/env_api.c | 12 +++++++----
env/env_api_fat.c | 39 ----------------------------------
include/ebgenv.h | 3 +--
include/env_api.h | 2 --
tools/tests/test_ebgenv_api_internal.c | 26 +----------------------
7 files changed, 15 insertions(+), 92 deletions(-)

--
2.15.0

[Andreas J. Reichel]

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

The current implementation of user variables introduce a regression
regarding update safety. By storing user variables globally into all
environments, corruption of otherwhise recoverable environments is
risked, which would prevent proper rollback.

For the upcoming release, user variables are local per default to
provide maximum robustness of the update process.

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

docs/API.md | 23 ++++----------------
docs/TOOLS.md | 2 +-

env/env_api_fat.c | 39 ----------------------------------
include/ebgenv.h | 3 +--
include/env_api.h | 2 --
tools/tests/test_ebgenv_api_internal.c | 26 +----------------------

6 files changed, 7 insertions(+), 88 deletions(-)

diff --git a/docs/API.md b/docs/API.md
index e198b21..42e2963 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -38,13 +38,6 @@ of the environment managed by the API.
The structure of an entry is explained in the [source code](../env/uservars.c).
Also see the example program below.

-There are currently two types of user variables: Those with the flag
-USERVAR_TYPE_GLOBAL set in their data field and those without.
-
-Global variables are immune to the update process, i.e. they are stored into all
-environments. If not specifying flags manually with ebg_env_set_ex, the global
-flag is set per default.
-
## Example programs ##

The following example program creates a new environment with the latest revision
@@ -103,23 +96,15 @@ int main(void)

ebg_env_open_current(&e);

- /* This automatically creates a global user variable, stored in all
- * environments */
+ /* This automatically creates a local user variable, stored in the
+ * current environment */
ebg_env_set(&e, "myvar", "myvalue");

- /* This sets the global variable to an empty string */
+ /* This sets the variable to an empty string */
ebg_env_set(&e, "myvar", "");

- /* This creates a user variable in the current environment only */
- ebg_env_set_ex(&e, "currentvar", USERVAR_TYPE_STRING_ASCII, "abc",
- strlen("abc") + 1);
-
- /* This deletes the global myvar key from all environments.
- * Note: value must not be NULL and datalen must be greater than 0 */
- ebg_env_set_ex(&e, "myvar", USERVAR_TYPE_DELETED | USERVAR_TYPE_GLOBAL, "", 1);
-
/* This deletes the variable from the current environment */
- ebg_env_set_ex(&e, "currentvar", USERVAR_TYPE_DELETED, "", 1);
+ ebg_env_set_ex(&e, "myvar", USERVAR_TYPE_DELETED, "", 1);

ebg_env_close(&e);
}
diff --git a/docs/TOOLS.md b/docs/TOOLS.md
index 814e6ba..2b66aeb 100644
--- a/docs/TOOLS.md
+++ b/docs/TOOLS.md
@@ -92,7 +92,7 @@ bg_setenv --partition=1 --ustate=TESTING
bg_setenv -x key=value
```

-This will set the variable named `key` to `value` in all environments.
+This will set the variable named `key` to `value` in the current environment.

If The user wants to delete such a variable, the value after the `=` must be omitted, e.g.

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index a2e4700..1b59313 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -320,41 +320,6 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
return 0;
}

-int bgenv_set_uservar_global(char *key, uint64_t type, void *data,
- uint32_t datalen)
-{
- int ret = 0;
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- BGENV *env = bgenv_open_by_index(i);
- if (!env) {
- ret = -EIO;
- VERBOSE(stderr, "bgenv_set_uservar_global: Could not "
- "open config partition #%d: (%s)\n",
- i, strerror(-ret));
- continue;
- }
- int r = bgenv_set_uservar(env->data->userdata, key, type, data,
- datalen);
- if (r) {
- VERBOSE(stderr, "bgenv_set_uservar_global: Could not "
- "set uservar on config partition "
- " #%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 "
- " store environment to partition "
- " #%d: (%s)\n", i, strerror(-ret));
- }
- (void)bgenv_close(env);
- }
- return ret;
-}
-
int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
uint32_t datalen)
{
@@ -372,10 +337,6 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
return -EPERM;
}
if (e == EBGENV_UNKNOWN) {
- if (type & USERVAR_TYPE_GLOBAL) {
- return bgenv_set_uservar_global(key, type, data,
- datalen);
- }
return bgenv_set_uservar(env->data->userdata, key, type, data,
datalen);
}
diff --git a/include/ebgenv.h b/include/ebgenv.h
index 3f03012..17c4410 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -28,9 +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_DEFAULT USERVAR_TYPE_GLOBAL
+#define USERVAR_TYPE_DEFAULT 0

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

diff --git a/include/env_api.h b/include/env_api.h
index 3433ac5..d541f66 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -83,8 +83,6 @@ extern int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
uint32_t maxlen);
extern int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
uint32_t datalen);
-extern int bgenv_set_uservar_global(char *key, uint64_t type,
- void *data, uint32_t datalen);
extern uint8_t *bgenv_find_uservar(uint8_t *userdata, char *key);

#endif // __ENV_API_H__
diff --git a/tools/tests/test_ebgenv_api_internal.c b/tools/tests/test_ebgenv_api_internal.c
index 868218d..17b866b 100644
--- a/tools/tests/test_ebgenv_api_internal.c
+++ b/tools/tests/test_ebgenv_api_internal.c
@@ -404,43 +404,19 @@ START_TEST(ebgenv_api_internal_uservars)
}
}

- res = bgenv_set(handle, "myvar", USERVAR_TYPE_GLOBAL, "mydata", 5);
- ck_assert_int_eq(write_env_fake.call_count, ENV_NUM_CONFIG_PARTS);
- ck_assert_int_eq(res, 0);
-
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- data = bgenv_find_uservar((uint8_t *)&(envdata[i].userdata),
- "myvar");
- ck_assert(data != NULL);
- }
-
res = bgenv_set(handle, "myvar", USERVAR_TYPE_DELETED, "mydata", 5);
ck_assert_int_eq(res, 0);

for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
data = bgenv_find_uservar((uint8_t *)&(envdata[i].userdata),
"myvar");
- if (handle->data != &envdata[i]) {
- ck_assert(data != NULL);
- } else
- {
+ if (handle->data == &envdata[i]) {
ck_assert(data == NULL);
}
}

write_env_fake.call_count = 0;

- res = bgenv_set(handle, "myvar", USERVAR_TYPE_DELETED | USERVAR_TYPE_GLOBAL,
- "mydata", 5);
- ck_assert_int_eq(write_env_fake.call_count, ENV_NUM_CONFIG_PARTS);
- ck_assert_int_eq(res, 0);
-
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- data = bgenv_find_uservar((uint8_t *)&(envdata[i].userdata),
- "myvar");
- ck_assert(data == NULL);
- }
-
(void)bgenv_close(handle);
}
END_TEST
--
2.15.0

[Andreas J. Reichel]

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

Only write environment if ustate differs from the value it is set to.
This reduces the risk of environment corruption.

Add missing CRC calculation to enable resetting invalid configurations
without making those invalid, i.e. two configurations with both ustate
== INSTALLED can now be properly reset to ustate == OK, without
rendering the inactive config invalid.

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

env/env_api.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index 29808ff..abfa49a 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -188,10 +188,14 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate)
if (!env) {
continue;
}
- env->data->ustate = ustate;
- if (!bgenv_write(env)) {
- (void)bgenv_close(env);
- return -EIO;
+ if (env->data->ustate != ustate) {
+ env->data->ustate = ustate;
+ env->data->crc32 = crc32(0, (Bytef *)env->data,
+ sizeof(BG_ENVDATA) - sizeof(env->data->crc32));
+ if (!bgenv_write(env)) {
+ (void)bgenv_close(env);
+ return -EIO;
+ }
}
if (!bgenv_close(env)) {
return -EIO;
--
2.15.0

[Jan Kiszka]

On 2017-11-17 17:32, Andreas J. Reichel wrote:
> From: Andreas Reichel <andreas.r...@siemens.com>
>
> Global variables are removed to avoid a regression regarding the
> robustness of the update mechanism.
>
> Furthermore, an improvement to the 'setglobalstate' function of the API
> is added to reduce unnecessary environment modifications and to add
> further support to recover from usually non-occuring update states.

Err, you need to help me: Why do we still have a setglobalstate API when
global vars are removed? Shouldn't that one go as well?

Jan

[Andreas Reichel]

On Mon, Nov 20, 2017 at 12:28:18PM +0100, Jan Kiszka wrote:
> On 2017-11-17 17:32, Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >
> > Global variables are removed to avoid a regression regarding the
> > robustness of the update mechanism.
> >
> > Furthermore, an improvement to the 'setglobalstate' function of the API
> > is added to reduce unnecessary environment modifications and to add
> > further support to recover from usually non-occuring update states.
>
> Err, you need to help me: Why do we still have a setglobalstate API when
> global vars are removed? Shouldn't that one go as well?
>

That is okay :) The name 'globalstate' refers to ustate and was chosen
as such, because in case of failure, ALL environments are searched for
ustate == FAILED and revision == 0. That's why.

The setter only modifies environments that are marked as FAILED if
setting the state to OK. If setting the state to another value, only the
current environment is accessed.

Andreas

> Jan
>
> >
> > Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
> >
> > Andreas Reichel (2):
> > Remove global user variables
> > ustate: Enhance API function to reset ustate
> >
> > docs/API.md | 23 ++++----------------
> > docs/TOOLS.md | 2 +-
> > env/env_api.c | 12 +++++++----
> > env/env_api_fat.c | 39 ----------------------------------
> > include/ebgenv.h | 3 +--
> > include/env_api.h | 2 --
> > tools/tests/test_ebgenv_api_internal.c | 26 +----------------------
> > 7 files changed, 15 insertions(+), 92 deletions(-)
> >
>

--
Andreas Reichel
Dipl.-Phys. (Univ.)
Software Consultant

Andreas...@tngtech.com, +49-174-3180074
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring
Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller
Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082

[Jan Kiszka]

On 2017-11-17 17:32, Andreas J. Reichel wrote:

Strictly spoken, this qualifies for two commits. Please do so in the
future, I'm relaxing it in this case.

Jan

--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[Jan Kiszka]

On 2017-11-20 13:25, Andreas Reichel wrote:
> On Mon, Nov 20, 2017 at 12:28:18PM +0100, Jan Kiszka wrote:
>> On 2017-11-17 17:32, Andreas J. Reichel wrote:
>>> From: Andreas Reichel <andreas.r...@siemens.com>
>>>
>>> Global variables are removed to avoid a regression regarding the
>>> robustness of the update mechanism.
>>>
>>> Furthermore, an improvement to the 'setglobalstate' function of the API
>>> is added to reduce unnecessary environment modifications and to add
>>> further support to recover from usually non-occuring update states.
>>
>> Err, you need to help me: Why do we still have a setglobalstate API when
>> global vars are removed? Shouldn't that one go as well?
>>
> That is okay :) The name 'globalstate' refers to ustate and was chosen
> as such, because in case of failure, ALL environments are searched for
> ustate == FAILED and revision == 0. That's why.
>
> The setter only modifies environments that are marked as FAILED if
> setting the state to OK. If setting the state to another value, only the
> current environment is accessed.

OK, that resolves my concerns. Merged, also to master.

Thanks,
Jan