[PATCH 1/2] Drop pointless error check from bgenv_close

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

When passed NULL, just do nothing (which is exactly what free() does).
That allows to clean up even more pointless error checks on the call
sites.

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---
env/env_api.c | 14 +++++--------
env/env_api_fat.c | 12 +++--------
include/env_api.h | 2 +-
tools/bg_setenv.c | 9 ++-------
tools/tests/test_ebgenv_api.c | 28 +-------------------------
tools/tests/test_ebgenv_api_internal.c | 2 +-
6 files changed, 13 insertions(+), 54 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index d6f1795..e6051e3 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -160,7 +160,7 @@ uint16_t ebg_env_getglobalstate(ebgenv_t *e)
env->data->ustate == USTATE_FAILED) {
res = 3;
}
- (void)bgenv_close(env);
+ bgenv_close(env);
if (res == 3) {
return res;
}
@@ -205,13 +205,11 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate)
env->data->crc32 = crc32(0, (Bytef *)env->data,
sizeof(BG_ENVDATA) - sizeof(env->data->crc32));
if (!bgenv_write(env)) {
- (void)bgenv_close(env);
+ bgenv_close(env);
return -EIO;
}
}
- if (!bgenv_close(env)) {
- return -EIO;
- }
+ bgenv_close(env);
}
return 0;
}
@@ -232,12 +230,10 @@ int ebg_env_close(ebgenv_t *e)
sizeof(BG_ENVDATA) - sizeof(env_current->data->crc32));
/* save */
if (!bgenv_write(env_current)) {
- (void)bgenv_close(env_current);
- return EIO;
- }
- if (!bgenv_close(env_current)) {
+ bgenv_close(env_current);
return EIO;
}
+ bgenv_close(env_current);
e->bgenv = NULL;
return 0;
}
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 9f2a5cd..ac408f6 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -232,13 +232,9 @@ BG_ENVDATA *bgenv_read(BGENV *env)
* are redesigned.
*/
__attribute((noinline))
-bool bgenv_close(BGENV *env)
+void bgenv_close(BGENV *env)
{
- if (env) {
- free(env);
- return true;
- }
- return false;
+ free(env);
}

static int bgenv_get_uint(char *buffer, uint64_t *type, void *data,
@@ -418,9 +414,7 @@ BGENV *bgenv_create_new(void)

int new_rev = env_latest->data->revision + 1;

- if (!bgenv_close(env_latest)) {
- goto create_new_io_error;
- }
+ bgenv_close(env_latest);

env_new = bgenv_open_oldest();
if (!env_new) {
diff --git a/include/env_api.h b/include/env_api.h
index 45bf4b7..8051964 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -84,7 +84,7 @@ extern BGENV *bgenv_open_oldest(void);
extern BGENV *bgenv_open_latest(void);
extern bool bgenv_write(BGENV *env);
extern BG_ENVDATA *bgenv_read(BGENV *env);
-extern bool bgenv_close(BGENV *env);
+extern void bgenv_close(BGENV *env);

extern BGENV *bgenv_create_new(void);
extern int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 9979306..0f34917 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -656,9 +656,7 @@ int main(int argc, char **argv)
sizeof(BG_ENVDATA));
env_new->data->revision = env_current->data->revision + 1;

- if (!bgenv_close(env_current)) {
- fprintf(stderr, "Error closing environment.\n");
- }
+ bgenv_close(env_current);
} else {
if (part_specified) {
env_new = bgenv_open_by_index(arguments.which_part);
@@ -683,10 +681,7 @@ int main(int argc, char **argv)
fprintf(stderr, "Error storing environment.\n");
return 1;
}
- if (!bgenv_close(env_new)) {
- fprintf(stderr, "Error closing environment.\n");
- return 1;
- }
+ bgenv_close(env_new);

fprintf(stdout, "Environment update was successful.\n");

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index f7a3410..e18c055 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -29,12 +29,11 @@ Suite *ebg_test_suite(void);
extern bool write_env(CONFIG_PART *part, BG_ENVDATA *env);
extern bool bgenv_write(BGENV *);
extern bool bgenv_init(void);
-extern bool bgenv_close(BGENV *);
+extern void bgenv_close(BGENV *);
extern BGENV *bgenv_create_new(void);

FAKE_VALUE_FUNC(bool, bgenv_init);
FAKE_VALUE_FUNC(bool, bgenv_write, BGENV *);
-FAKE_VALUE_FUNC(bool, bgenv_close, BGENV *);

int __real_bgenv_set(BGENV *, char *, uint64_t, void *, uint32_t);
int __wrap_bgenv_set(BGENV *, char *, uint64_t, void *, uint32_t);
@@ -104,7 +103,6 @@ START_TEST(ebgenv_api_ebg_env_create_new)
* returns false
*/
bgenv_init_fake.return_val = false;
- bgenv_close_fake.return_val = true;
ret = ebg_env_create_new(&e);
ck_assert_int_eq(ret, EIO);

@@ -122,7 +120,6 @@ START_TEST(ebgenv_api_ebg_env_create_new)
errno = 0;

bgenv_init_fake.return_val = true;
- bgenv_close_fake.return_val = true;
ret = ebg_env_create_new(&e);

ck_assert_int_eq(errno, 0);
@@ -527,7 +524,6 @@ START_TEST(ebgenv_api_ebg_env_setglobalstate)
envdata[1].ustate = USTATE_FAILED;

bgenv_write_fake.return_val = true;
- bgenv_close_fake.return_val = true;

ret = ebg_env_setglobalstate(&e, USTATE_OK);

@@ -550,17 +546,6 @@ START_TEST(ebgenv_api_ebg_env_setglobalstate)
* fails
*/
bgenv_write_fake.return_val = false;
- bgenv_close_fake.return_val = true;
-
- ret = ebg_env_setglobalstate(&e, USTATE_OK);
-
- ck_assert_int_eq(ret, -EIO);
-
- /* Test if ebg_env_setglobalstate fails and returns EIO if bgenv_close
- * fails
- */
- bgenv_write_fake.return_val = true;
- bgenv_close_fake.return_val = false;

ret = ebg_env_setglobalstate(&e, USTATE_OK);

@@ -589,15 +574,6 @@ START_TEST(ebgenv_api_ebg_env_close)

((BGENV *)e.bgenv)->data = calloc(1, sizeof(BG_ENVDATA));
bgenv_write_fake.return_val = false;
- bgenv_close_fake.return_val = true;
- ret = ebg_env_close(&e);
-
- ck_assert_int_eq(ret, EIO);
-
- /* Test if ebg_env_close fails and returns EIO if bgenv_close fails.
- */
- bgenv_write_fake.return_val = true;
- bgenv_close_fake.return_val = false;
ret = ebg_env_close(&e);

ck_assert_int_eq(ret, EIO);
@@ -605,7 +581,6 @@ START_TEST(ebgenv_api_ebg_env_close)
/* Test if ebg_env_close is successful if all prerequisites are met
*/
bgenv_write_fake.return_val = true;
- bgenv_close_fake.return_val = true;
BGENV *save_ptr = e.bgenv;
ret = ebg_env_close(&e);

@@ -624,7 +599,6 @@ START_TEST(ebgenv_api_ebg_env_register_gc_var)
memset(&e, 0, sizeof(e));

bgenv_write_fake.return_val = true;
- bgenv_close_fake.return_val = true;

bgenv_init_fake.return_val = true;

diff --git a/tools/tests/test_ebgenv_api_internal.c b/tools/tests/test_ebgenv_api_internal.c
index 2011bab..1a5dfa0 100644
--- a/tools/tests/test_ebgenv_api_internal.c
+++ b/tools/tests/test_ebgenv_api_internal.c
@@ -419,7 +419,7 @@ START_TEST(ebgenv_api_internal_uservars)

write_env_fake.call_count = 0;

- (void)bgenv_close(handle);
+ bgenv_close(handle);
}
END_TEST

--
2.26.2

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

When done with the with using it, bgenv should be finalized in order to
release resources allocated during bgenv_init and the later usage of
config_parts. Adjust the users accordingly.

Furthermore, bgenv_init should be made robust against multiple
succeeding invocations. They shall simply be ignored so that we do not
leak resources allocated on first init.

While at it, drop pointless zeroing of global var config_parts.

Reported-by: Tolga Hosgor <tolga....@siemens.com>

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

env/env_api.c | 1 +
env/env_api_fat.c | 24 +++++++++++++++++++++---
include/env_api.h | 1 +
tools/bg_setenv.c | 25 ++++++++++++++++---------
4 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index e6051e3..ebc61a0 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -235,6 +235,7 @@ int ebg_env_close(ebgenv_t *e)

}
bgenv_close(env_current);
e->bgenv = NULL;

+ bgenv_finalize();

return 0;
}

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

index ac408f6..2b23e7f 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -128,10 +128,13 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
BG_ENVDATA envdata[ENV_NUM_CONFIG_PARTS];

-bool bgenv_init()
+static bool initialized;
+
+bool bgenv_init(void)
{
- memset((void *)&config_parts, 0,
- sizeof(CONFIG_PART) * ENV_NUM_CONFIG_PARTS);
+ if (initialized) {
+ return true;
+ }
/* enumerate all config partitions */
if (!probe_config_partitions(config_parts)) {
VERBOSE(stderr, "Error finding config partitions.\n");
@@ -149,9 +152,24 @@ bool bgenv_init()
sizeof(BG_ENVDATA) - sizeof(envdata[i].crc32));
}
}
+ initialized = true;
return true;
}

+void bgenv_finalize(void)
+{
+ if (!initialized) {
+ return;
+ }
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ free(config_parts[i].devpath);
+ config_parts[i].devpath = NULL;
+ free(config_parts[i].mountpoint);
+ config_parts[i].mountpoint = NULL;
+ }
+ initialized = false;
+}
+
BGENV *bgenv_open_by_index(uint32_t index)
{
BGENV *handle;
diff --git a/include/env_api.h b/include/env_api.h
index 8051964..9a5c2a8 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -79,6 +79,7 @@ extern char *str16to8(char *buffer, wchar_t *src);
extern wchar_t *str8to16(wchar_t *buffer, char *src);

extern bool bgenv_init(void);
+extern void bgenv_finalize(void);
extern BGENV *bgenv_open_by_index(uint32_t index);

extern BGENV *bgenv_open_oldest(void);
extern BGENV *bgenv_open_latest(void);

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 0f34917..24b4067 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -618,6 +618,7 @@ int main(int argc, char **argv)
dump_envs();

if (!write_mode) {
+ bgenv_finalize();
return 0;
}

@@ -631,13 +632,16 @@ int main(int argc, char **argv)
if (!env_current) {
fprintf(stderr, "Failed to retrieve latest environment."

"\n");
- return 1;

+ result = 1;
+ goto cleanup;

}
env_new = bgenv_open_oldest();
if (!env_new) {

fprintf(stderr, "Failed to retrieve oldest environment."

"\n");
- return 1;

+ bgenv_close(env_current);
+ result = 1;
+ goto cleanup;
}
if (verbosity) {
fprintf(stdout,
@@ -647,9 +651,9 @@ int main(int argc, char **argv)

if (!env_current->data || !env_new->data) {
fprintf(stderr, "Invalid environment data pointer.\n");
- bgenv_close(env_new);
bgenv_close(env_current);
- return 1;
+ result = 1;
+ goto cleanup;
}

memcpy((char *)env_new->data, (char *)env_current->data,
@@ -666,7 +670,8 @@ int main(int argc, char **argv)
if (!env_new) {
fprintf(stderr, "Failed to retrieve environment by "
"index.\n");
- return 1;
+ result = 1;
+ goto cleanup;
}
}

@@ -679,11 +684,13 @@ int main(int argc, char **argv)
}
if (!bgenv_write(env_new)) {

fprintf(stderr, "Error storing environment.\n");

- return 1;
+ result = 1;
+ } else {
+ fprintf(stdout, "Environment update was successful.\n");
}
- bgenv_close(env_new);
-
- fprintf(stdout, "Environment update was successful.\n");

+cleanup:
+ bgenv_close(env_new);
+ bgenv_finalize();
return result;
}
--
2.26.2