[PATCH v2 0/3] Bugfix and garbage collector add-on

[Andreas J. Reichel]

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

This patch series fixes a bug regarding user variables.

Furthermore it introduces a garbage collector to
ebg_env_finalize_update, so that the user can register arbitrary user
variables, which automatically get deleted upon finalization.

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

Andreas Reichel (3):
Bugfix: Don't map non-existing user variables
Add registry for garbage collection on update finalization
Tests: Add test for finalization garbage collector

env/env_api.c | 45 ++++++++++++++++++++++++++++-
env/env_api_fat.c | 3 ++
env/uservars.c | 16 +++++++++--
include/ebgenv.h | 8 ++++++
include/env_api.h | 5 ++++
include/uservars.h | 6 ++--
tools/tests/test_ebgenv_api.c | 53 ++++++++++++++++++++++++++++++++++-
7 files changed, 128 insertions(+), 8 deletions(-)

--
2.17.0

[Andreas J. Reichel]

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

Provide possibility to register user variables that are deleted
on finalization of an update.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/ebgenv.h | 8 ++++++++
include/env_api.h | 5 +++++
3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/env/env_api.c b/env/env_api.c
index 75695d1..03cd53c 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
return 0;
}

-int ebg_env_finalize_update(ebgenv_t *e) {
+int ebg_env_gc_register_var(ebgenv_t *e, char *key)
+{
+ GC_ITEM **pgci;
+ pgci = (GC_ITEM **)&e->gc_registry;
+
+ if (!key) {
+ return EINVAL;
+ }
+ while (*pgci) {
+ pgci = &((*pgci)->next);
+ }
+ *pgci = (GC_ITEM *)calloc(1, sizeof(GC_ITEM));
+ if (!*pgci) {
+ return ENOMEM;
+ }
+ if (asprintf(&((*pgci)->key), "%s", key) == -1) {
+ free(*pgci);
+ *pgci = NULL;
+ return ENOMEM;
+ }
+ return 0;
+}
+
+int ebg_env_finalize_update(ebgenv_t *e)
+{
if (!e->bgenv || !((BGENV *)e->bgenv)->data) {
return EIO;
}
+
+ GC_ITEM **pgci, *tmp;
+ uint8_t *udata;
+
+ pgci = (GC_ITEM **)&e->gc_registry;
+ udata = ((BGENV *)e->bgenv)->data->userdata;
+ while (*pgci) {
+ uint8_t *var;
+ var = bgenv_find_uservar(udata, (*pgci)->key);
+ if (var) {
+ bgenv_del_uservar(udata, var);
+ }
+ free((*pgci)->key);
+ tmp = (*pgci)->next;
+ free(*pgci);
+ *pgci = NULL;
+ pgci = &tmp;
+ }
+
((BGENV *)e->bgenv)->data->in_progress = 0;
((BGENV *)e->bgenv)->data->ustate = USTATE_INSTALLED;
return 0;
diff --git a/include/ebgenv.h b/include/ebgenv.h
index 46e692c..8b158b7 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -35,6 +35,7 @@

typedef struct {
void *bgenv;
+ void *gc_registry;
} ebgenv_t;

/** @brief Tell the library to output information for the user.
@@ -128,6 +129,13 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate);
*/
int ebg_env_close(ebgenv_t *e);

+/** @brief Register a variable that will be deleted on finalize
+ * @param e A pointer to an ebgenv_t context.
+ * @param key A string containing the variable key
+ * @return 0 on success, errno on failure
+ */
+int ebg_env_gc_register_var(ebgenv_t *e, char *key);
+
/** @brief Finalizes a currently running update procedure
* @param e A pointer to an ebgenv_t context.
* @return 0 on success, errno on failure
diff --git a/include/env_api.h b/include/env_api.h
index 4d652ff..ec73a64 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -66,6 +66,11 @@ typedef struct {
BG_ENVDATA *data;
} BGENV;

+typedef struct gc_item {
+ char *key;
+ struct gc_item *next;
+} GC_ITEM;
+
extern void bgenv_be_verbose(bool v);

extern char *str16to8(char *buffer, wchar_t *src);
--
2.17.0

[Andreas J. Reichel]

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

Register three user variables, add two of them to the registry
of the garbage collector and call finalize to check if the third
remains and the registered ones are deleted.

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

tools/tests/test_ebgenv_api.c | 53 ++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index 533b50b..d99dc91 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -615,6 +615,56 @@ START_TEST(ebgenv_api_ebg_env_close)
}
END_TEST

+START_TEST(ebgenv_api_ebg_env_gc_register_var)
+{
+ ebgenv_t e;
+ int ret;
+ memset(&e, 0, sizeof(e));
+
+ bgenv_write_fake.return_val = true;
+ bgenv_close_fake.return_val = true;
+
+ bgenv_init_fake.return_val = true;
+
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ envdata[i].revision = i + 1;
+ }
+
+ ret = ebg_env_create_new(&e);
+
+ /* Create three user variables VarA, VarB and VarC */
+ ebg_env_set(&e, "VarA", "TestA");
+ ebg_env_set(&e, "VarB", "TestB");
+ ebg_env_set(&e, "VarC", "TestC");
+
+ /* Check if variables exist */
+ int res;
+ res = ebg_env_get(&e, "VarA", NULL);
+ ck_assert_int_eq(res, strlen("TestA") + 1);
+ res = ebg_env_get(&e, "VarB", NULL);
+ ck_assert_int_eq(res, strlen("TestB") + 1);
+ res = ebg_env_get(&e, "VarC", NULL);
+ ck_assert_int_eq(res, strlen("TestC") + 1);
+
+ /* Register variables for deletion */
+ ebg_env_gc_register_var(&e, "VarA");
+ ebg_env_gc_register_var(&e, "VarC");
+
+ ebg_env_finalize_update(&e);
+
+ /* Check if variables are deleted */
+ res = ebg_env_get(&e, "VarA", NULL);
+ ck_assert_int_eq(res, -EINVAL);
+ res = ebg_env_get(&e, "VarB", NULL);
+ ck_assert_int_eq(res, strlen("TestB") + 1);
+ res = ebg_env_get(&e, "VarC", NULL);
+ ck_assert_int_eq(res, -EINVAL);
+
+ ebg_env_close(&e);
+
+}
+END_TEST
+
Suite *ebg_test_suite(void)
{
Suite *s;
@@ -632,7 +682,8 @@ Suite *ebg_test_suite(void)
ebgenv_api_ebg_env_user_free,
ebgenv_api_ebg_env_getglobalstate,
ebgenv_api_ebg_env_setglobalstate,
- ebgenv_api_ebg_env_close
+ ebgenv_api_ebg_env_close,
+ ebgenv_api_ebg_env_gc_register_var
};

tc_core = tcase_create("Core");
--
2.17.0

[Andreas J. Reichel]

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

If a non-existing user variable is requested and a NULL-buffer is
provided, the getter maps an invalid memory area to retrieve the needed
buffer size.
Fix this by checking if the data pointer is valid first before mapping
anything and furthermore don't call the mapper if the variable is not
found.

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

env/env_api_fat.c | 3 +++
env/uservars.c | 16 +++++++++++++---
include/uservars.h | 6 +++---
3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 1705cb9..a86c05d 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -287,6 +287,9 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
uint8_t *u;
uint32_t size;
u = bgenv_find_uservar(env->data->userdata, key);
+ if (!u) {
+ return -EINVAL;
+ }
bgenv_map_uservar(u, NULL, NULL, NULL, NULL, &size);
return size;
}
diff --git a/env/uservars.c b/env/uservars.c
index eff1cf8..3ad95c3 100644
--- a/env/uservars.c
+++ b/env/uservars.c
@@ -14,8 +14,8 @@
#include "env_api.h"
#include "uservars.h"

-void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
- uint32_t *record_size, uint32_t *data_size)
+int bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
+ uint32_t *record_size, uint32_t *data_size)
{
/* Each user variable is encoded as follows:
* |------------|--------------|---------------|----------------|
@@ -38,6 +38,10 @@ void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val
uint64_t *var_type;
uint8_t *data;

+ if (!udata) {
+ return -EINVAL;
+ }
+
/* Get the key */
var_key = (char *)udata;
if (key) {
@@ -68,6 +72,8 @@ void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val
if (val) {
*val = data;
}
+
+ return 0;
}

void bgenv_serialize_uservar(uint8_t *p, char *key, uint64_t type, void *data,
@@ -100,6 +106,7 @@ int bgenv_get_uservar(uint8_t *udata, char *key, uint64_t *type, void *data,
char *lkey;
uint32_t dsize;
uint64_t ltype;
+ int res;

uservar = bgenv_find_uservar(udata, key);

@@ -107,7 +114,10 @@ int bgenv_get_uservar(uint8_t *udata, char *key, uint64_t *type, void *data,
return -EINVAL;
}

- bgenv_map_uservar(uservar, &lkey, &ltype, &value, NULL, &dsize);
+ res = bgenv_map_uservar(uservar, &lkey, &ltype, &value, NULL, &dsize);
+ if (res) {
+ return res;
+ }

if (dsize > maxlen) {
dsize = maxlen;
diff --git a/include/uservars.h b/include/uservars.h
index b1d1d7c..012240c 100644
--- a/include/uservars.h
+++ b/include/uservars.h
@@ -15,9 +15,9 @@

#include <stdint.h>

-void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type,
- uint8_t **val, uint32_t *record_size,
- uint32_t *data_size);
+int bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type,
+ uint8_t **val, uint32_t *record_size,
+ uint32_t *data_size);
void bgenv_serialize_uservar(uint8_t *p, char *key, uint64_t type, void *data,
uint32_t record_size);

--
2.17.0

[Jan Kiszka]

Here, you do not check the new return code. OK, u can't be NULL anymore,
but then it has to be clear (comment) that we do not expect
bgenv_map_uservar to fail otherwise.

> return size;
> }
> diff --git a/env/uservars.c b/env/uservars.c
> index eff1cf8..3ad95c3 100644
> --- a/env/uservars.c
> +++ b/env/uservars.c
> @@ -14,8 +14,8 @@
> #include "env_api.h"
> #include "uservars.h"
>
> -void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
> - uint32_t *record_size, uint32_t *data_size)
> +int bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
> + uint32_t *record_size, uint32_t *data_size)
> {
> /* Each user variable is encoded as follows:
> * |------------|--------------|---------------|----------------|
> @@ -38,6 +38,10 @@ void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val
> uint64_t *var_type;
> uint8_t *data;
>
> + if (!udata) {
> + return -EINVAL;
> + }
> +

In the case above, this check is not needed. Why can't we make that
consistent: either do the NULL check on caller side in all cases of let
bgenv_map_uservar do it and evaluate its error in all cases?

Jan

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

[Jan Kiszka]

On 2018-04-27 14:44, Andreas J. Reichel wrote:

> From: Andreas Reichel <andreas.r...@siemens.com>
>
> Provide possibility to register user variables that are deleted
> on finalization of an update.
>
> Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
> ---
> env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> include/ebgenv.h | 8 ++++++++
> include/env_api.h | 5 +++++
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/env/env_api.c b/env/env_api.c
> index 75695d1..03cd53c 100644
> --- a/env/env_api.c
> +++ b/env/env_api.c
> @@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
> return 0;
> }
>
> -int ebg_env_finalize_update(ebgenv_t *e) {
> +int ebg_env_gc_register_var(ebgenv_t *e, char *key)

"Register garbage-collected variable" - ebg_env_register_gc_var, I would
say.

Looks good otherwise.

[Jan Kiszka]

On 2018-04-27 14:44, Andreas J. Reichel wrote:

BTW, shouldn't we rather return -ENOENT if nothing is found?

> + res = ebg_env_get(&e, "VarB", NULL);
> + ck_assert_int_eq(res, strlen("TestB") + 1);
> + res = ebg_env_get(&e, "VarC", NULL);
> + ck_assert_int_eq(res, -EINVAL);
> +
> + ebg_env_close(&e);
> +
> +}
> +END_TEST
> +
> Suite *ebg_test_suite(void)
> {
> Suite *s;
> @@ -632,7 +682,8 @@ Suite *ebg_test_suite(void)
> ebgenv_api_ebg_env_user_free,
> ebgenv_api_ebg_env_getglobalstate,
> ebgenv_api_ebg_env_setglobalstate,
> - ebgenv_api_ebg_env_close
> + ebgenv_api_ebg_env_close,
> + ebgenv_api_ebg_env_gc_register_var
> };
>
> tc_core = tcase_create("Core");
>

Test itself is fine.

[Andreas J. Reichel]

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

If a non-existing user variable is requested and a NULL-buffer is
provided, the getter maps an invalid memory area to retrieve the needed
buffer size.
Fix this by checking if the data pointer is valid first before mapping
anything and furthermore don't call the mapper if the variable is not
found.

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 1705cb9..a86c05d 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -287,6 +287,9 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
uint8_t *u;
uint32_t size;
u = bgenv_find_uservar(env->data->userdata, key);
+ if (!u) {
+ return -EINVAL;
+ }
bgenv_map_uservar(u, NULL, NULL, NULL, NULL, &size);

return size;
}
--
2.17.0

[Andreas J. Reichel]

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

This patch series fixes a bug regarding user variables.

Furthermore it introduces a garbage collector to
ebg_env_finalize_update, so that the user can register arbitrary user
variables, which automatically get deleted upon finalization.

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

Andreas Reichel (3):
Bugfix: Don't map non-existing user variables
Add registry for garbage collection on update finalization
Tests: Add test for finalization garbage collector

env/env_api.c | 45 ++++++++++++++++++++++++++++-
env/env_api_fat.c | 3 ++

include/ebgenv.h | 8 ++++++
include/env_api.h | 5 ++++

tools/tests/test_ebgenv_api.c | 53 ++++++++++++++++++++++++++++++++++-
5 files changed, 112 insertions(+), 2 deletions(-)

--
2.17.0

[Andreas J. Reichel]

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

Provide possibility to register user variables that are deleted
on finalization of an update.

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

env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/ebgenv.h | 8 ++++++++
include/env_api.h | 5 +++++
3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/env/env_api.c b/env/env_api.c
index 75695d1..03cd53c 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
return 0;
}

-int ebg_env_finalize_update(ebgenv_t *e) {
+int ebg_env_gc_register_var(ebgenv_t *e, char *key)

--
2.17.0

[Andreas J. Reichel]

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

Register three user variables, add two of them to the registry
of the garbage collector and call finalize to check if the third
remains and the registered ones are deleted.

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

+ res = ebg_env_get(&e, "VarB", NULL);
+ ck_assert_int_eq(res, strlen("TestB") + 1);
+ res = ebg_env_get(&e, "VarC", NULL);
+ ck_assert_int_eq(res, -EINVAL);
+
+ ebg_env_close(&e);
+
+}
+END_TEST
+
Suite *ebg_test_suite(void)
{
Suite *s;
@@ -632,7 +682,8 @@ Suite *ebg_test_suite(void)
ebgenv_api_ebg_env_user_free,
ebgenv_api_ebg_env_getglobalstate,
ebgenv_api_ebg_env_setglobalstate,
- ebgenv_api_ebg_env_close
+ ebgenv_api_ebg_env_close,
+ ebgenv_api_ebg_env_gc_register_var
};

tc_core = tcase_create("Core");

--
2.17.0

[Andreas Reichel]

On Wed, May 02, 2018 at 01:27:27PM +0200, Jan Kiszka wrote:
> On 2018-04-27 14:44, Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >
> > Provide possibility to register user variables that are deleted
> > on finalization of an update.
> >
> > Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
> > ---
> > env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> > include/ebgenv.h | 8 ++++++++
> > include/env_api.h | 5 +++++
> > 3 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/env/env_api.c b/env/env_api.c
> > index 75695d1..03cd53c 100644
> > --- a/env/env_api.c
> > +++ b/env/env_api.c
> > @@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
> > return 0;
> > }
> >
> > -int ebg_env_finalize_update(ebgenv_t *e) {
> > +int ebg_env_gc_register_var(ebgenv_t *e, char *key)
>
> "Register garbage-collected variable" - ebg_env_register_gc_var, I would
> say.
>

We already discussed that on the last mail. gc is the superior name
here. Other functions could follow.

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

[Andreas Reichel]

Good idea so then please wait for patch v4... I was too fast with v3.

[Andreas Reichel]

I simplified according to your suggestion that the caller has to check
and bgenv_map_uservar is type void.

> > return size;
> > }
> > diff --git a/env/uservars.c b/env/uservars.c
> > index eff1cf8..3ad95c3 100644
> > --- a/env/uservars.c
> > +++ b/env/uservars.c
> > @@ -14,8 +14,8 @@
> > #include "env_api.h"
> > #include "uservars.h"
> >
> > -void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
> > - uint32_t *record_size, uint32_t *data_size)
> > +int bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val,
> > + uint32_t *record_size, uint32_t *data_size)
> > {
> > /* Each user variable is encoded as follows:
> > * |------------|--------------|---------------|----------------|
> > @@ -38,6 +38,10 @@ void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val
> > uint64_t *var_type;
> > uint8_t *data;
> >
> > + if (!udata) {
> > + return -EINVAL;
> > + }
> > +
>
> In the case above, this check is not needed. Why can't we make that
> consistent: either do the NULL check on caller side in all cases of let
> bgenv_map_uservar do it and evaluate its error in all cases?
>

caller now checks... see v3

[Andreas Reichel]

On Wed, May 02, 2018 at 01:40:08PM +0200, Andreas J. Reichel wrote:
> From: Andreas Reichel <andreas.r...@siemens.com>
>
> This patch series fixes a bug regarding user variables.
>
> Furthermore it introduces a garbage collector to
> ebg_env_finalize_update, so that the user can register arbitrary user
> variables, which automatically get deleted upon finalization.
>
> Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
>
> Andreas Reichel (3):
> Bugfix: Don't map non-existing user variables
> Add registry for garbage collection on update finalization
> Tests: Add test for finalization garbage collector
>

Please wait for v4 to change EINVAL to ENOENT when variable
is not found.

> env/env_api.c | 45 ++++++++++++++++++++++++++++-
> env/env_api_fat.c | 3 ++
> include/ebgenv.h | 8 ++++++
> include/env_api.h | 5 ++++
> tools/tests/test_ebgenv_api.c | 53 ++++++++++++++++++++++++++++++++++-
> 5 files changed, 112 insertions(+), 2 deletions(-)
>
> --
> 2.17.0
>

[Jan Kiszka]

On 2018-05-02 13:44, Andreas Reichel wrote:
> On Wed, May 02, 2018 at 01:27:27PM +0200, Jan Kiszka wrote:
>> On 2018-04-27 14:44, Andreas J. Reichel wrote:
>>> From: Andreas Reichel <andreas.r...@siemens.com>
>>>
>>> Provide possibility to register user variables that are deleted
>>> on finalization of an update.
>>>
>>> Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
>>> ---
>>> env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>> include/ebgenv.h | 8 ++++++++
>>> include/env_api.h | 5 +++++
>>> 3 files changed, 57 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/env/env_api.c b/env/env_api.c
>>> index 75695d1..03cd53c 100644
>>> --- a/env/env_api.c
>>> +++ b/env/env_api.c
>>> @@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
>>> return 0;
>>> }
>>>
>>> -int ebg_env_finalize_update(ebgenv_t *e) {
>>> +int ebg_env_gc_register_var(ebgenv_t *e, char *key)
>>
>> "Register garbage-collected variable" - ebg_env_register_gc_var, I would
>> say.
>>
> We already discussed that on the last mail. gc is the superior name
> here. Other functions could follow.
>

See, my memory is getting weak, but my intuition still tells me we are
using a wrong pattern here.

[Andreas J. Reichel]

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

If a user variable is not found, return -ENOENT instead of -EINVAL.

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

env/env_api_fat.c | 2 +-
env/uservars.c | 2 +-
tools/tests/test_ebgenv_api.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index a86c05d..94bd7b6 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -288,7 +288,7 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,

uint32_t size;
u = bgenv_find_uservar(env->data->userdata, key);

if (!u) {
- return -EINVAL;
+ return -ENOENT;

}
bgenv_map_uservar(u, NULL, NULL, NULL, NULL, &size);

return size;
diff --git a/env/uservars.c b/env/uservars.c

index eff1cf8..013a208 100644
--- a/env/uservars.c
+++ b/env/uservars.c
@@ -104,7 +104,7 @@ int bgenv_get_uservar(uint8_t *udata, char *key, uint64_t *type, void *data,
uservar = bgenv_find_uservar(udata, key);

if (!uservar) {
- return -EINVAL;
+ return -ENOENT;

}

bgenv_map_uservar(uservar, &lkey, &ltype, &value, NULL, &dsize);

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index fb6ca44..47abdeb 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -654,11 +654,11 @@ START_TEST(ebgenv_api_ebg_env_register_gc_var)

/* Check if variables are deleted */

res = ebg_env_get(&e, "VarA", NULL);

- ck_assert_int_eq(res, -EINVAL);
+ ck_assert_int_eq(res, -ENOENT);

res = ebg_env_get(&e, "VarB", NULL);

ck_assert_int_eq(res, strlen("TestB") + 1);

res = ebg_env_get(&e, "VarC", NULL);

- ck_assert_int_eq(res, -EINVAL);
+ ck_assert_int_eq(res, -ENOENT);

ebg_env_close(&e);
}
--
2.17.0

[Andreas J. Reichel]

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

This patch series fixes a bug regarding user variables: The caller of
map_uservar must be sure that the data pointer is not NULL.

Furthermore it introduces a garbage collector to
ebg_env_finalize_update, so that the user can register arbitrary user
variables, which automatically get deleted upon finalization.

diff to v3:

* ebg_env_get now returns -ENOENT if a user variable is not found
instead of -EINVAL.

* gc_register_var is now renamed to register_gc_var

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

Andreas Reichel (4):

Bugfix: Don't map non-existing user variables
Add registry for garbage collection on update finalization
Tests: Add test for finalization garbage collector

env: uservars: return -ENOENT if not found

env/env_api.c | 45 +++++++++++++++++++++++++++++-
env/env_api_fat.c | 3 ++
env/uservars.c | 2 +-

include/ebgenv.h | 8 ++++++
include/env_api.h | 5 ++++

tools/tests/test_ebgenv_api.c | 52 ++++++++++++++++++++++++++++++++++-
6 files changed, 112 insertions(+), 3 deletions(-)

--
2.17.0

[Andreas J. Reichel]

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

Register three user variables, add two of them to the registry
of the garbage collector and call finalize to check if the third
remains and the registered ones are deleted.

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

tools/tests/test_ebgenv_api.c | 52 ++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index 533b50b..fb6ca44 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -615,6 +615,55 @@ START_TEST(ebgenv_api_ebg_env_close)
}
END_TEST

+START_TEST(ebgenv_api_ebg_env_register_gc_var)

+ ebg_env_register_gc_var(&e, "VarA");
+ ebg_env_register_gc_var(&e, "VarC");
+
+ ebg_env_finalize_update(&e);
+
+ /* Check if variables are deleted */
+ res = ebg_env_get(&e, "VarA", NULL);
+ ck_assert_int_eq(res, -EINVAL);

+ res = ebg_env_get(&e, "VarB", NULL);
+ ck_assert_int_eq(res, strlen("TestB") + 1);
+ res = ebg_env_get(&e, "VarC", NULL);
+ ck_assert_int_eq(res, -EINVAL);
+
+ ebg_env_close(&e);
+}

+END_TEST
+
Suite *ebg_test_suite(void)
{
Suite *s;

@@ -632,7 +681,8 @@ Suite *ebg_test_suite(void)

ebgenv_api_ebg_env_user_free,
ebgenv_api_ebg_env_getglobalstate,
ebgenv_api_ebg_env_setglobalstate,
- ebgenv_api_ebg_env_close
+ ebgenv_api_ebg_env_close,

+ ebgenv_api_ebg_env_register_gc_var

};

tc_core = tcase_create("Core");

--
2.17.0

[Andreas J. Reichel]

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

If a non-existing user variable is requested and a NULL-buffer is
provided, the getter maps an invalid memory area to retrieve the needed
buffer size.
Fix this by checking if the data pointer is valid first before mapping
anything and furthermore don't call the mapper if the variable is not
found.

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 1705cb9..a86c05d 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -287,6 +287,9 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
uint8_t *u;

uint32_t size;
u = bgenv_find_uservar(env->data->userdata, key);

+ if (!u) {
+ return -EINVAL;
+ }

bgenv_map_uservar(u, NULL, NULL, NULL, NULL, &size);
return size;
}

--
2.17.0

[Andreas J. Reichel]

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

Provide possibility to register user variables that are deleted
on finalization of an update.

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

env/env_api.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
include/ebgenv.h | 8 ++++++++
include/env_api.h | 5 +++++

3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/env/env_api.c b/env/env_api.c
index 75695d1..784f6ce 100644

--- a/env/env_api.c
+++ b/env/env_api.c
@@ -240,10 +240,53 @@ int ebg_env_close(ebgenv_t *e)
return 0;
}

-int ebg_env_finalize_update(ebgenv_t *e) {

+int ebg_env_register_gc_var(ebgenv_t *e, char *key)

index 46e692c..6f99266 100644

--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -35,6 +35,7 @@

typedef struct {
void *bgenv;
+ void *gc_registry;
} ebgenv_t;

/** @brief Tell the library to output information for the user.
@@ -128,6 +129,13 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate);
*/
int ebg_env_close(ebgenv_t *e);

+/** @brief Register a variable that will be deleted on finalize
+ * @param e A pointer to an ebgenv_t context.
+ * @param key A string containing the variable key
+ * @return 0 on success, errno on failure
+ */

+int ebg_env_register_gc_var(ebgenv_t *e, char *key);

+
/** @brief Finalizes a currently running update procedure
* @param e A pointer to an ebgenv_t context.
* @return 0 on success, errno on failure
diff --git a/include/env_api.h b/include/env_api.h
index 4d652ff..ec73a64 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -66,6 +66,11 @@ typedef struct {
BG_ENVDATA *data;
} BGENV;

+typedef struct gc_item {
+ char *key;
+ struct gc_item *next;
+} GC_ITEM;
+
extern void bgenv_be_verbose(bool v);

extern char *str16to8(char *buffer, wchar_t *src);

--
2.17.0

[Jan Kiszka]

Thanks, merged to next.

[Jan Kiszka]

On 2018-05-02 14:39, Andreas J. Reichel wrote:

Missing "ck_assert_int_eq(ret, 0);" here, right? Then I will just fix up
the patch.

Jan

[Jan Kiszka]

Done, and that made cppcheck happy again.

Jan