[PATCH 0/9] Add in_progress and improve code

[Andreas J. Reichel]

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

This patch series introduces a new variable into the core environment
called `in_progress`. This variable signals, that the corresponding
environment is currently being used by an update process. This way, the
updater can resume in case of unexpected aborts. Additional resume
information can be handled by using user variables.
`ustate` is now only set to `INSTALLED` after an update has been
finalized (read below), whereas `in_progress` is automatically set to
`1` when a new environment is created. `in_progress` is used as a flag
for ebg_env_create_new, to detect if a new environment has already been
created. The flag is removed from the context struct.

The boot loader is adapted to never boot environments which are `in
progress`. The existing selection/update mechanism of the bootloader
is not touched otherwise.

The API gets a new function `ebg_env_finalize_update` which finalizes an
update by setting `in_progress` to 0 and `ustate` to INSTALLED.

Additionally:
* Code duplication is reduced in the API and the tools.
* Readability of the environment dump of the tools is improved.
* Two minor memory leaks in the argument parser are fixed, that
went undetected before.
* The unused user data type for ustate is corrected from 16-bit to 8-bit
for consistency.

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

Andreas Reichel (9):
env: Include update process indicator
env: Simplify code for bgenv_get/set
bootloader: Ignore latest environment if in progress
API: Automatically set in_progress but keep ustate
API: Add update finalize function
tools: bg_setenv: Add support for in_progress variable
Bugfix: Fix datatype returned for ustate
tools: bg_setenv: Improve parser code
tools: bg_setenv: Improve output to user

env/env_api.c | 27 ++++++--
env/env_api_fat.c | 146 ++++++++++++++++++++++--------------------
env/fatvars.c | 6 +-
include/ebgenv.h | 7 +-
include/env_api.h | 1 +
include/envdata.h | 2 +-
tools/bg_setenv.c | 98 ++++++++++++++++++++--------
tools/tests/test_ebgenv_api.c | 51 +++++++++------
8 files changed, 212 insertions(+), 126 deletions(-)

--
2.15.0

[Andreas J. Reichel]

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

Remove duplicate code and refactor functions.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
env/env_api_fat.c | 153 ++++++++++++++++++++++++------------------------------
1 file changed, 67 insertions(+), 86 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 89eb3f0..471084a 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -243,6 +243,9 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
uint32_t maxlen)
{
EBGENVKEY e;
+ unsigned int src;
+ uint64_t t;
+ wchar_t *srcstr;
char buffer[ENV_STRING_LENGTH];

if (!key || maxlen == 0) {
@@ -265,72 +268,70 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
}
switch (e) {
case EBGENV_KERNELFILE:
- str16to8(buffer, env->data->kernelfile);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_STRING_ASCII;
- }
- break;
+ srcstr = env->data->kernelfile;
+ goto bgenv_get_string;
case EBGENV_KERNELPARAMS:
- str16to8(buffer, env->data->kernelparams);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_STRING_ASCII;
- }
- break;
+ srcstr = env->data->kernelparams;
+ goto bgenv_get_string;
case EBGENV_WATCHDOG_TIMEOUT_SEC:
- sprintf(buffer, "%u", env->data->watchdog_timeout_sec);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;
+ t = USERVAR_TYPE_UINT16;
+ src = env->data->watchdog_timeout_sec;
+ goto bgenv_get_uint;
case EBGENV_REVISION:
- sprintf(buffer, "%u", env->data->revision);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;
+ t = USERVAR_TYPE_UINT16;
+ src = env->data->revision;
+ goto bgenv_get_uint;
case EBGENV_USTATE:
- sprintf(buffer, "%u", env->data->ustate);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;
+ t = USERVAR_TYPE_UINT16;
+ src = env->data->ustate;
+ goto bgenv_get_uint;
case EBGENV_IN_PROGRESS:
- sprintf(buffer, "%u", env->data->in_progress);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT8;
- }
- break;
+ t = USERVAR_TYPE_UINT8;
+ src = env->data->in_progress;
+ goto bgenv_get_uint;
default:
if (!data) {
return 0;
}
return -EINVAL;
}
+bgenv_get_uint:
+ sprintf(buffer, "%u", src);
+ if (!data) {
+ return strlen(buffer)+1;
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
+ if (type) {
+ *type = t;
+ }
return 0;
+bgenv_get_string:
+ str16to8(buffer, srcstr);
+ if (!data) {
+ return strlen(buffer)+1;
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
+ if (type) {
+ *type = USERVAR_TYPE_STRING_ASCII;
+ }
+ return 0;
+}
+
+static long bgenv_convert_to_long(char *value)
+{
+ long val;
+ char *p;
+
+ errno = 0;
+ val = strtol(value, &p, 10);
+ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
+ (errno != 0 && val == 0)) {
+ return -errno;
+ }
+ if (p == value) {
+ return -EINVAL;
+ }
+ return val;
}

int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
@@ -355,14 +356,9 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
}
switch (e) {
case EBGENV_REVISION:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->revision = val;
break;
@@ -373,38 +369,23 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
str8to16(env->data->kernelparams, value);
break;
case EBGENV_WATCHDOG_TIMEOUT_SEC:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->watchdog_timeout_sec = val;
break;
case EBGENV_USTATE:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->ustate = val;
break;
case EBGENV_IN_PROGRESS:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->in_progress = val;
break;
--
2.15.0

[Andreas J. Reichel]

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

If in_progress is set in the latest environment, do not boot from it, as
a current update process may have been interrupted and this environment
is currently used by software update. Instead, boot from the 2nd latest
revision.

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

env/fatvars.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 5d391c7..2c84700 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -191,8 +191,12 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
/* Assume we boot with the latest configuration */
current_partition = latest_idx;

+ /* Test if this environment is currently 'in_progress'. If yes,
+ * do not boot from it, instead ignore it */
+ if (env[latest_idx].in_progress == 1) {
+ current_partition = pre_latest_idx;
+ } else if (env[latest_idx].ustate == USTATE_TESTING) {
/* Test if this configuration is in test mode */
- if (env[latest_idx].ustate == USTATE_TESTING) {
/* If it has already been booted, this indicates a failed
* update. In this case, mark it as failed by giving a
* zero-revision */
--
2.15.0

[Andreas J. Reichel]

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

ustate is an 8-bit int, not a 16-bit int.

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

env/env_api_fat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 1790d43..c007eb0 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -282,7 +282,7 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,

src = env->data->revision;

goto bgenv_get_uint;
case EBGENV_USTATE:
- t = USERVAR_TYPE_UINT16;
+ t = USERVAR_TYPE_UINT8;

src = env->data->ustate;

goto bgenv_get_uint;
case EBGENV_IN_PROGRESS:
--
2.15.0

[Andreas J. Reichel]

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

Enable tools to access in_progress variable for advanced environment
manipulation.

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

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

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 741d604..41dfb83 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -39,6 +39,9 @@ static struct argp_option options_setenv[] = {
{"uservar", 'x', "KEY=VAL", 0, "Set user-defined string variable. For "
"setting multiple variables, use this "
"option multiple times."},
+ {"in_progress", 'i', "IN_PROGRESS", 0, "Set in_progress variable to "
+ "simulate a running update "
+ "process."},
{"version", 'V', 0, 0, "Print version"},
{0}};

@@ -289,6 +292,30 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ustate2str(i));
}
break;
+ case 'i':
+ errno = 0;
+ i = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (errno != 0 && i == 0) || (tmp == arg)) {
+ fprintf(stderr, "Invalid value specified.\n");
+ return 1;
+ }
+ if (i < 0 || i > 1) {
+ fprintf(
+ stderr,
+ "Invalid value specified. Possible values: "
+ "0 (no), 1 (yes)\n");
+ return 1;
+ } else {
+ res = asprintf(&tmp, "%u", i);
+ if (res == -1) {
+ return ENOMEM;
+ }
+ e = journal_add_action(ENV_TASK_SET, "in_progress", 0,
+ (uint8_t *)tmp, strlen(tmp) + 1);
+ VERBOSE(stdout, "in_progress set to %d.\n", i);
+ }
+ break;
case 'r':
i = atoi(arg);
VERBOSE(stdout, "Revision is set to %d.\n", i);
@@ -431,6 +458,7 @@ static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];
printf("Values: \n");
+ printf("in_progress: %s\n", env->in_progress ? "yes" : "no");
printf("revision: %u\n", env->revision);
printf("kernel: %s\n", str16to8(buffer, env->kernelfile));
printf("kernelargs: %s\n", str16to8(buffer, env->kernelparams));
--
2.15.0

[Andreas J. Reichel]

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

For SWupdate, not only the update state, but also the currently
performed action is important. The padding byte in the environment
is used to store, if the environment copy is currently being used to
update the system and thus renamed to 'in_progress'.

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

env/env_api.c | 14 ++++++++++----
env/env_api_fat.c | 25 +++++++++++++++++++++++++
include/ebgenv.h | 1 -

include/env_api.h | 1 +
include/envdata.h | 2 +-

tools/tests/test_ebgenv_api.c | 34 ++++++++++++++--------------------
6 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index abfa49a..81b871a 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -65,9 +65,14 @@ int ebg_env_create_new(ebgenv_t *e)
return EIO;
}

- if (!e->ebg_new_env_created) {
- BGENV *latest_env = bgenv_open_latest();
+ BGENV *latest_env = bgenv_open_latest();
+ BG_ENVDATA *latest_data = ((BGENV *)latest_env)->data;
+
+ if (latest_data->in_progress != 1) {
e->bgenv = (void *)bgenv_create_new();
+ if (!e->bgenv) {
+ goto skip_invalid_env;
+ }
BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
uint32_t new_rev = new_data->revision;
uint8_t new_ustate = new_data->ustate;
@@ -75,10 +80,11 @@ int ebg_env_create_new(ebgenv_t *e)
new_data->revision = new_rev;
new_data->ustate = new_ustate;
bgenv_close(latest_env);
+ } else {
+ e->bgenv = latest_env;
}

- e->ebg_new_env_created = e->bgenv != NULL;
-
+skip_invalid_env:
return e->bgenv == NULL ? errno : 0;
}

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 1b59313..89eb3f0 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -38,6 +38,9 @@ EBGENVKEY bgenv_str2enum(char *key)
if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
return EBGENV_USTATE;
}
+ if (strncmp(key, "in_progress", strlen("in_progress") + 1) == 0) {
+ return EBGENV_IN_PROGRESS;
+ }
return EBGENV_UNKNOWN;
}

@@ -311,6 +314,16 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
*type = USERVAR_TYPE_UINT16;
}
break;
+ case EBGENV_IN_PROGRESS:
+ sprintf(buffer, "%u", env->data->in_progress);

+ if (!data) {
+ return strlen(buffer)+1;
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
+ if (type) {

+ *type = USERVAR_TYPE_UINT8;
+ }
+ break;

default:
if (!data) {
return 0;

@@ -383,6 +396,18 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,

}
env->data->ustate = val;
break;

+ case EBGENV_IN_PROGRESS:
+ errno = 0;

+ val = strtol(value, &p, 10);
+ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
+ (errno != 0 && val == 0)) {
+ return -errno;
+ }
+ if (p == value) {
+ return -EINVAL;
+ }

+ env->data->in_progress = val;
+ break;
default:
return -EINVAL;
}
diff --git a/include/ebgenv.h b/include/ebgenv.h
index 17c4410..d33c0d6 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -35,7 +35,6 @@

typedef struct {
void *bgenv;
- bool ebg_new_env_created;
} ebgenv_t;

/** @brief Tell the library to output information for the user.
diff --git a/include/env_api.h b/include/env_api.h
index d541f66..4d652ff 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -51,6 +51,7 @@ typedef enum {
EBGENV_WATCHDOG_TIMEOUT_SEC,
EBGENV_REVISION,
EBGENV_USTATE,
+ EBGENV_IN_PROGRESS,
EBGENV_UNKNOWN
} EBGENVKEY;

diff --git a/include/envdata.h b/include/envdata.h
index 011053a..42dcd34 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -34,7 +34,7 @@
struct _BG_ENVDATA {
uint16_t kernelfile[ENV_STRING_LENGTH];
uint16_t kernelparams[ENV_STRING_LENGTH];
- uint8_t padding;
+ uint8_t in_progress;
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index f6bebb7..4820bac 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -92,32 +92,19 @@ START_TEST(ebgenv_api_ebg_env_create_new)
char *kernelparams = "param456";

memset(&e, 0, sizeof(e));
+ memset(envdata, 0, sizeof(envdata));
+
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ envdata[i].revision = i + 1;
+ }

/* Test if ebg_env_create_new returns EIO if bgenv_init
* returns false
*/
bgenv_init_fake.return_val = false;
- ret = ebg_env_create_new(&e);
- ck_assert_int_eq(ret, EIO);
-
- /* Test if errno is returned by ebg_env_created, if the bgenv pointer
- * is NULL but ebg_new_env_created is true, which is contradictory.
- * Also, ebg_new_env_created must be reset to false.
- */
- bgenv_init_fake.return_val = true;
bgenv_close_fake.return_val = true;
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++)
- {
- envdata[i].revision = i;
- }
- e.ebg_new_env_created = true;
- e.bgenv = NULL;
- errno = 3044;
-
ret = ebg_env_create_new(&e);
-
- ck_assert_int_eq(ret, 3044);
- ck_assert(e.ebg_new_env_created == false);
+ ck_assert_int_eq(ret, EIO);

/* Check if values of the latest environment are copied if a new
* environment is created. The new environment must overwrite the
@@ -132,13 +119,20 @@ START_TEST(ebgenv_api_ebg_env_create_new)
strlen(kernelparams) * 2 + 2);
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);
ck_assert_int_eq(ret, 0);
+
ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);
+
+
+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 0);
ck_assert_int_eq(
- ((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS);
+ ((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);
+
ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_INSTALLED);
ck_assert_int_eq(((BGENV *)e.bgenv)->data->watchdog_timeout_sec, 44);
(void)str16to8(buffer, ((BGENV *)e.bgenv)->data->kernelfile);
--
2.15.0

[Andreas J. Reichel]

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

* Better string formatting
* Don't print verbose messages if not in verbose mode
* Always inform after successful update

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

tools/bg_setenv.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index f865fce..df63265 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -470,13 +470,13 @@ static void dump_uservars(uint8_t *udata)

static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];

- printf("Values: \n");
- printf("in_progress: %s\n", env->in_progress ? "yes" : "no");
- printf("revision: %u\n", env->revision);
- printf("kernel: %s\n", str16to8(buffer, env->kernelfile));
- printf("kernelargs: %s\n", str16to8(buffer, env->kernelparams));
+ printf("Values:\n");

+ printf("in_progress: %s\n", env->in_progress ? "yes" : "no");

+ printf("revision: %u\n", env->revision);
+ printf("kernel: %s\n", str16to8(buffer, env->kernelfile));
+ printf("kernelargs: %s\n", str16to8(buffer, env->kernelparams));
printf("watchdog timeout: %u seconds\n", env->watchdog_timeout_sec);
- printf("ustate: %u (%s)\n", (uint8_t)env->ustate,
+ printf("ustate: %u (%s)\n", (uint8_t)env->ustate,
ustate2str(env->ustate));
printf("\n");
printf("user variables:\n");
@@ -486,7 +486,9 @@ static void dump_env(BG_ENVDATA *env)

static void update_environment(BGENV *env)
{
- printf("Processing journal...\n");
+ if (verbosity) {
+ printf("Processing journal...\n");
+ }

while (!STAILQ_EMPTY(&head)) {
struct env_action *action = STAILQ_FIRST(&head);
@@ -580,7 +582,7 @@ int main(int argc, char **argv)
fprintf(stderr, "Error closing output file.\n");
result = errno;
};
- printf("Output written to %s.\n", envfilepath);
+ fprintf(stdout, "Output written to %s.\n", envfilepath);
} else {
fprintf(stderr, "Error opening output file %s (%s).\n",
envfilepath, strerror(errno));
@@ -668,5 +670,8 @@ int main(int argc, char **argv)
fprintf(stderr, "Error closing environment.\n");
return 1;
}
+
+ fprintf(stdout, "Environment update was successful.\n");
+
return result;
}
--
2.15.0

[Andreas J. Reichel]

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

Add a helper function to the API which should be called in the end of
update processes. The function sets in_progress to 0 and ustate to
INSTALLED.

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

env/env_api.c | 9 +++++++++
include/ebgenv.h | 6 ++++++
tools/tests/test_ebgenv_api.c | 6 ++++++
3 files changed, 21 insertions(+)

diff --git a/env/env_api.c b/env/env_api.c
index c23b25c..84fd4f4 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -235,3 +235,12 @@ int ebg_env_close(ebgenv_t *e)
e->bgenv = NULL;
return 0;
}
+
+int ebg_env_finalize_update(ebgenv_t *e) {
+ if (!e->bgenv || !((BGENV *)e->bgenv)->data) {
+ return EIO;
+ }
+ ((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 d33c0d6..12b778e 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -128,4 +128,10 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate);
*/
int ebg_env_close(ebgenv_t *e);

+/** @brief Finalizes a currently running update procedure
+ * @param e A pointer to an ebgenv_t context.
+ * @return 0 on success, errno on failure
+ */
+int ebg_env_finalize_update(ebgenv_t *e);
+
#endif //__EBGENV_H__
diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index f9ab55c..533b50b 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -150,6 +150,12 @@ START_TEST(ebgenv_api_ebg_env_create_new)
ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);
ck_assert_int_eq(

((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);
+

+ ret = ebg_env_finalize_update(&e);
+
+ ck_assert_int_eq(ret, 0);

+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_INSTALLED);

+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 0);
}

END_TEST

--
2.15.0

[Andreas J. Reichel]

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

On creation of a new environment automatically set in_progress,
but do not set ustate to INSTALLED as that will be done in the
end when finalizing the update process.

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

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

diff --git a/env/env_api.c b/env/env_api.c
index 81b871a..c23b25c 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -75,10 +75,10 @@ int ebg_env_create_new(ebgenv_t *e)

}
BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
uint32_t new_rev = new_data->revision;

- uint8_t new_ustate = new_data->ustate;
+ uint8_t new_in_progress = new_data->in_progress;
memcpy(new_data, latest_env->data, sizeof(BG_ENVDATA));
new_data->revision = new_rev;
- new_data->ustate = new_ustate;
+ new_data->in_progress = new_in_progress;
bgenv_close(latest_env);
} else {
e->bgenv = latest_env;
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 471084a..1790d43 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -420,7 +420,7 @@ BGENV *bgenv_create_new(void)
memset(env_new->data, 0, sizeof(BG_ENVDATA));
/* update revision field and testing mode */
env_new->data->revision = new_rev;
- env_new->data->ustate = 1;
+ env_new->data->in_progress = 1;
/* set default watchdog timeout */
env_new->data->watchdog_timeout_sec = 30;

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index 4820bac..f9ab55c 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -128,12 +128,11 @@ START_TEST(ebgenv_api_ebg_env_create_new)

ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);

-
- ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 0);
+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 1);

ck_assert_int_eq(
((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);

- ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_INSTALLED);
+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);

ck_assert_int_eq(((BGENV *)e.bgenv)->data->watchdog_timeout_sec, 44);
(void)str16to8(buffer, ((BGENV *)e.bgenv)->data->kernelfile);

ck_assert_int_eq(
@@ -141,6 +140,16 @@ START_TEST(ebgenv_api_ebg_env_create_new)
(void)str16to8(buffer, ((BGENV *)e.bgenv)->data->kernelparams);
ck_assert_int_eq(
strncmp(buffer, kernelparams, strlen(kernelparams) + 1), 0);
+
+ /* Test that a new creation of environment does keep the current
+ * values if an update is already in progress
+ */
+ ret = ebg_env_create_new(&e);
+

+ ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);

+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);
+ ck_assert_int_eq(
+ ((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);
}
END_TEST

--
2.15.0

[Andreas J. Reichel]

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

* Unify parsing of int arguments
* Fix memory leaks in argument parser

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

tools/bg_setenv.c | 67 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 41dfb83..f865fce 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -212,6 +212,21 @@ static error_t set_uservars(char *arg)
(uint8_t *)value, strlen(value) + 1);
}

+static int parse_int(char *arg)
+{
+ char *tmp;
+ int i;
+
+ errno = 0;

+ i = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (errno != 0 && i == 0) || (tmp == arg)) {

+ errno = EINVAL;
+ return -1;
+ }
+ return i;
+}
+

static error_t parse_opt(int key, char *arg, struct argp_state *state)

{
struct arguments *arguments = state->input;
@@ -243,10 +258,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
(uint8_t *)arg, strlen(arg) + 1);
break;
case 'p':
- errno = 0;
- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {
fprintf(stderr, "Invalid number specified for -p.\n");
return 1;
}
@@ -262,10 +275,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
}
break;
case 's':
- errno = 0;
- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {
// maybe user specified an enum string
i = str2ustate(arg);
if (i == USTATE_UNKNOWN) {
@@ -288,23 +299,21 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
}
e = journal_add_action(ENV_TASK_SET, "ustate", 0,

(uint8_t *)tmp, strlen(tmp) + 1);

+ free(tmp);
VERBOSE(stdout, "Ustate set to %d (%s).\n", i,
ustate2str(i));
}
break;
case 'i':
- errno = 0;
- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {

fprintf(stderr, "Invalid value specified.\n");

return 1;

}
if (i < 0 || i > 1) {

- fprintf(
- stderr,
- "Invalid value specified. Possible values: "
- "0 (no), 1 (yes)\n");
+ fprintf(stderr,

+ "Invalid value specified. Possible values: "
+ "0 (no), 1 (yes)\n");

return 1;
} else {

res = asprintf(&tmp, "%u", i);

@@ -313,27 +322,31 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)

}
e = journal_add_action(ENV_TASK_SET, "in_progress", 0,

(uint8_t *)tmp, strlen(tmp) + 1);

+ free(tmp);

VERBOSE(stdout, "in_progress set to %d.\n", i);
}

break;
case 'r':
- i = atoi(arg);
- VERBOSE(stdout, "Revision is set to %d.\n", i);
+ i = parse_int(arg);
+ if (errno) {
+ fprintf(stderr, "Invalid revision specified.\n");
+ return 1;
+ }
+ VERBOSE(stdout, "Revision is set to %u.\n", (unsigned int) i);
e = journal_add_action(ENV_TASK_SET, "revision", 0,
(uint8_t *)arg, strlen(arg) + 1);
break;
case 'w':
- i = atoi(arg);
- if (i != 0) {
- VERBOSE(stdout,
- "Setting watchdog timeout to %d seconds.\n", i);
- e = journal_add_action(ENV_TASK_SET,
- "watchdog_timeout_sec", 0,
- (uint8_t *)arg, strlen(arg) + 1);
- } else {
- fprintf(stderr, "Watchdog timeout must be non-zero.\n");
+ i = parse_int(arg);
+ if (errno || i == 0) {
+ fprintf(stderr, "Invalid watchdog timeout specified.\n");
return 1;
}
+ VERBOSE(stdout,
+ "Setting watchdog timeout to %d seconds.\n", i);
+ e = journal_add_action(ENV_TASK_SET,
+ "watchdog_timeout_sec", 0,
+ (uint8_t *)arg, strlen(arg) + 1);
break;
case 'f':
arguments->output_to_file = true;
--
2.15.0

[Jan Kiszka]

return errno? No need for a jump and for rechecking bgenv there.

But then we seem to have some inconsistency whether errors codes should
be negative on return. Here they aren't, elsewhere they are. What is the
current convention?

> + }
> BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
> uint32_t new_rev = new_data->revision;
> uint8_t new_ustate = new_data->ustate;
> @@ -75,10 +80,11 @@ int ebg_env_create_new(ebgenv_t *e)
> new_data->revision = new_rev;
> new_data->ustate = new_ustate;
> bgenv_close(latest_env);
> + } else {
> + e->bgenv = latest_env;
> }
>
> - e->ebg_new_env_created = e->bgenv != NULL;
> -
> +skip_invalid_env:
> return e->bgenv == NULL ? errno : 0;

When we get here, we must have been successful, no? Then return 0. If
not, errno seems rather unreliable to return.

> }
>
> diff --git a/env/env_api_fat.c b/env/env_api_fat.c
> index 1b59313..89eb3f0 100644
> --- a/env/env_api_fat.c
> +++ b/env/env_api_fat.c
> @@ -38,6 +38,9 @@ EBGENVKEY bgenv_str2enum(char *key)
> if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
> return EBGENV_USTATE;
> }
> + if (strncmp(key, "in_progress", strlen("in_progress") + 1) == 0) {
> + return EBGENV_IN_PROGRESS;
> + }
> return EBGENV_UNKNOWN;
> }
>
> @@ -311,6 +314,16 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
> *type = USERVAR_TYPE_UINT16;
> }
> break;
> + case EBGENV_IN_PROGRESS:
> + sprintf(buffer, "%u", env->data->in_progress);
> + if (!data) {
> + return strlen(buffer)+1;

Minor, but sprintf returns that length already. You could keep it and
reuse it here.

Jan

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

[Jan Kiszka]

On 2017-11-22 12:59, Andreas J. Reichel wrote:

goto is fine for some cases, but this one screams for using helper
functions instead. Just like you do below.

Jan

[Jan Kiszka]

On 2017-11-22 12:59, Andreas J. Reichel wrote:

> From: Andreas Reichel <andreas.r...@siemens.com>
>
> If in_progress is set in the latest environment, do not boot from it, as
> a current update process may have been interrupted and this environment
> is currently used by software update. Instead, boot from the 2nd latest
> revision.
>
> Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
> ---
> env/fatvars.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/env/fatvars.c b/env/fatvars.c
> index 5d391c7..2c84700 100644
> --- a/env/fatvars.c
> +++ b/env/fatvars.c
> @@ -191,8 +191,12 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
> /* Assume we boot with the latest configuration */
> current_partition = latest_idx;
>
> + /* Test if this environment is currently 'in_progress'. If yes,
> + * do not boot from it, instead ignore it */
> + if (env[latest_idx].in_progress == 1) {
> + current_partition = pre_latest_idx;
> + } else if (env[latest_idx].ustate == USTATE_TESTING) {
> /* Test if this configuration is in test mode */

Comment is now misplaced. But, actually it already was of the kind "set
i to 1" before, so just kill it.

> - if (env[latest_idx].ustate == USTATE_TESTING) {
> /* If it has already been booted, this indicates a failed
> * update. In this case, mark it as failed by giving a
> * zero-revision */
>

Jan

[Jan Kiszka]

On 2017-11-22 12:59, Andreas J. Reichel wrote:

What's the pattern in this file for choosing printf() vs. fprintf(stdout)?

Jan

> +
> return result;

[Andreas Reichel]

True, thanks.

>
> But then we seem to have some inconsistency whether errors codes should
> be negative on return. Here they aren't, elsewhere they are. What is the
> current convention?

Please see next patch series: 11/11

>
> > + }
> > BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
> > uint32_t new_rev = new_data->revision;
> > uint8_t new_ustate = new_data->ustate;
> > @@ -75,10 +80,11 @@ int ebg_env_create_new(ebgenv_t *e)
> > new_data->revision = new_rev;
> > new_data->ustate = new_ustate;
> > bgenv_close(latest_env);
> > + } else {
> > + e->bgenv = latest_env;
> > }
> >
> > - e->ebg_new_env_created = e->bgenv != NULL;
> > -
> > +skip_invalid_env:
> > return e->bgenv == NULL ? errno : 0;
>
> When we get here, we must have been successful, no? Then return 0. If
> not, errno seems rather unreliable to return.

True, thanks.

>
> > }
> >
> > diff --git a/env/env_api_fat.c b/env/env_api_fat.c
> > index 1b59313..89eb3f0 100644
> > --- a/env/env_api_fat.c
> > +++ b/env/env_api_fat.c
> > @@ -38,6 +38,9 @@ EBGENVKEY bgenv_str2enum(char *key)
> > if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
> > return EBGENV_USTATE;
> > }
> > + if (strncmp(key, "in_progress", strlen("in_progress") + 1) == 0) {
> > + return EBGENV_IN_PROGRESS;
> > + }
> > return EBGENV_UNKNOWN;
> > }
> >
> > @@ -311,6 +314,16 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
> > *type = USERVAR_TYPE_UINT16;
> > }
> > break;
> > + case EBGENV_IN_PROGRESS:
> > + sprintf(buffer, "%u", env->data->in_progress);
> > + if (!data) {
> > + return strlen(buffer)+1;
>
> Minor, but sprintf returns that length already. You could keep it and
> reuse it here.

Please see next patch series, 2/11

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

Killed :)

>
> > - if (env[latest_idx].ustate == USTATE_TESTING) {
> > /* If it has already been booted, this indicates a failed
> > * update. In this case, mark it as failed by giving a
> > * zero-revision */
> >
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RDA ITP SES-DE
> Corporate Competence Center Embedded Linux

[Andreas Reichel]

Done.

[Andreas Reichel]

Thanks. See next patch series 10/11

>
> Jan
>
> > +
> > return result;
> > }
> >
>
> --
> Siemens AG, Corporate Technology, CT RDA ITP SES-DE
> Corporate Competence Center Embedded Linux

[Andreas J. Reichel]

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

For SWupdate, not only the update state, but also the currently
performed action is important. The padding byte in the environment
is used to store, if the environment copy is currently being used to
update the system and thus renamed to 'in_progress'.

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

env/env_api.c | 20 +++++++++++++++-----

env/env_api_fat.c | 25 +++++++++++++++++++++++++
include/ebgenv.h | 1 -
include/env_api.h | 1 +
include/envdata.h | 2 +-
tools/tests/test_ebgenv_api.c | 34 ++++++++++++++--------------------

6 files changed, 56 insertions(+), 27 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index abfa49a..2961b77 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -65,9 +65,19 @@ int ebg_env_create_new(ebgenv_t *e)

return EIO;
}

- if (!e->ebg_new_env_created) {
- BGENV *latest_env = bgenv_open_latest();
+ BGENV *latest_env = bgenv_open_latest();

+ if (!latest_env) {

+ return EIO;
+ }
+

+ BG_ENVDATA *latest_data = ((BGENV *)latest_env)->data;
+
+ if (latest_data->in_progress != 1) {
e->bgenv = (void *)bgenv_create_new();
+ if (!e->bgenv) {

+ bgenv_close(latest_env);
+ return errno;

+ }
BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
uint32_t new_rev = new_data->revision;
uint8_t new_ustate = new_data->ustate;

@@ -75,11 +85,11 @@ int ebg_env_create_new(ebgenv_t *e)

new_data->revision = new_rev;
new_data->ustate = new_ustate;
bgenv_close(latest_env);
+ } else {
+ e->bgenv = latest_env;
}

- e->ebg_new_env_created = e->bgenv != NULL;
-

- return e->bgenv == NULL ? errno : 0;
+ return 0;
}

int ebg_env_open_current(ebgenv_t *e)
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 1b59313..89eb3f0 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c

@@ -38,6 +38,9 @@ EBGENVKEY bgenv_str2enum(char *key)
if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
return EBGENV_USTATE;
}
+ if (strncmp(key, "in_progress", strlen("in_progress") + 1) == 0) {
+ return EBGENV_IN_PROGRESS;
+ }
return EBGENV_UNKNOWN;
}

@@ -311,6 +314,16 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
*type = USERVAR_TYPE_UINT16;
}
break;
+ case EBGENV_IN_PROGRESS:

+ sprintf(buffer, "%u", env->data->in_progress);

+ if (!data) {
+ return strlen(buffer)+1;
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
+ if (type) {

+ *type = USERVAR_TYPE_UINT8;
+ }
+ break;

default:
if (!data) {
return 0;

@@ -383,6 +396,18 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,

}
env->data->ustate = val;
break;

+ case EBGENV_IN_PROGRESS:

+ errno = 0;
+ val = strtol(value, &p, 10);
+ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
+ (errno != 0 && val == 0)) {
+ return -errno;
+ }
+ if (p == value) {
+ return -EINVAL;
+ }

--
2.15.0

[Andreas J. Reichel]

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

Remove duplicate code and refactor functions.

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

env/env_api_fat.c | 162 +++++++++++++++++++++++++-----------------------------
1 file changed, 75 insertions(+), 87 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 89eb3f0..8636922 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -239,6 +239,36 @@ bool bgenv_close(BGENV *env)
return false;
}

+static int bgenv_get_uint(char *buffer, uint64_t *type, void *data,
+ unsigned int src, uint64_t t)
+{
+ int res;
+
+ res = sprintf(buffer, "%u", src);
+ if (!data) {
+ return res+1;
+ }
+ strncpy(data, buffer, res+1);
+ if (type) {
+ *type = t;

+ }
+ return 0;
+}
+

+static int bgenv_get_string(char *buffer, uint64_t *type, void *data,
+ wchar_t *srcstr)
+{
+ str16to8(buffer, srcstr);

+ if (!data) {
+ return strlen(buffer)+1;
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
+ if (type) {

+ *type = USERVAR_TYPE_STRING_ASCII;
+ }
+ return 0;

+}
+

int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,

uint32_t maxlen)
{
@@ -265,72 +295,50 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,

}
switch (e) {
case EBGENV_KERNELFILE:
- str16to8(buffer, env->data->kernelfile);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_STRING_ASCII;
- }
- break;

+ return bgenv_get_string(buffer, type, data,
+ env->data->kernelfile);

case EBGENV_KERNELPARAMS:
- str16to8(buffer, env->data->kernelparams);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_STRING_ASCII;
- }
- break;

+ return bgenv_get_string(buffer, type, data,
+ env->data->kernelparams);

case EBGENV_WATCHDOG_TIMEOUT_SEC:
- sprintf(buffer, "%u", env->data->watchdog_timeout_sec);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;

+ return bgenv_get_uint(buffer, type, data,
+ env->data->watchdog_timeout_sec,
+ USERVAR_TYPE_UINT16);

case EBGENV_REVISION:
- sprintf(buffer, "%u", env->data->revision);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;

+ return bgenv_get_uint(buffer, type, data,
+ env->data->revision,
+ USERVAR_TYPE_UINT16);

case EBGENV_USTATE:
- sprintf(buffer, "%u", env->data->ustate);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT16;
- }
- break;

+ return bgenv_get_uint(buffer, type, data,
+ env->data->ustate,
+ USERVAR_TYPE_UINT16);

case EBGENV_IN_PROGRESS:
- sprintf(buffer, "%u", env->data->in_progress);
- if (!data) {
- return strlen(buffer)+1;
- }
- strncpy(data, buffer, strlen(buffer)+1);
- if (type) {
- *type = USERVAR_TYPE_UINT8;
- }
- break;

+ return bgenv_get_uint(buffer, type, data,
+ env->data->in_progress,
+ USERVAR_TYPE_UINT8);

default:
if (!data) {
return 0;
}

return -EINVAL;
}
- return 0;

+}
+
+static long bgenv_convert_to_long(char *value)
+{
+ long val;
+ char *p;
+

+ errno = 0;
+ val = strtol(value, &p, 10);
+ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
+ (errno != 0 && val == 0)) {
+ return -errno;
+ }
+ if (p == value) {
+ return -EINVAL;
+ }

+ return val;

}

int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,

@@ -355,14 +363,9 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,

}
switch (e) {
case EBGENV_REVISION:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->revision = val;
break;

@@ -373,38 +376,23 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,

str8to16(env->data->kernelparams, value);
break;
case EBGENV_WATCHDOG_TIMEOUT_SEC:
- errno = 0;
- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}
env->data->watchdog_timeout_sec = val;
break;
case EBGENV_USTATE:
- errno = 0;

- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}

env->data->ustate = val;
break;

case EBGENV_IN_PROGRESS:
- errno = 0;

- val = strtol(value, &p, 10);
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) ||
- (errno != 0 && val == 0)) {
- return -errno;
- }
- if (p == value) {
- return -EINVAL;
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
}

env->data->in_progress = val;

break;
--
2.15.0

[Andreas J. Reichel]

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

This patch series introduces a new variable into the core environment
called `in_progress`. This variable signals, that the corresponding
environment is currently being used by an update process. This way, the
updater can resume in case of unexpected aborts. Additional resume
information can be handled by using user variables.
`ustate` is now only set to `INSTALLED` after an update has been
finalized (read below), whereas `in_progress` is automatically set to
`1` when a new environment is created. `in_progress` is used as a flag
for ebg_env_create_new, to detect if a new environment has already been
created. The flag is removed from the context struct.

The boot loader is adapted to never boot environments which are `in
progress`. The existing selection/update mechanism of the bootloader
is not touched otherwise.

The API gets a new function `ebg_env_finalize_update` which finalizes an
update by setting `in_progress` to 0 and `ustate` to INSTALLED.

Additionally:
* Code duplication is reduced in the API and the tools.
* Readability of the environment dump of the tools is improved.
* Two minor memory leaks in the argument parser are fixed, that
went undetected before.
* The unused user data type for ustate is corrected from 16-bit to 8-bit
for consistency.

* Fix API header comments regarding errno / -errno
* Fix printf/fprintf inconsistency in tools/bg_setenv.c

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

Andreas Reichel (11):

env: Include update process indicator
env: Simplify code for bgenv_get/set
bootloader: Ignore latest environment if in progress
API: Automatically set in_progress but keep ustate
API: Add update finalize function
tools: bg_setenv: Add support for in_progress variable
Bugfix: Fix datatype returned for ustate
tools: bg_setenv: Improve parser code
tools: bg_setenv: Improve output to user

tools: Fix printf/fprintf inconsistency
API: Correct prototype comments on return value

env/env_api.c | 33 +++++++--
env/env_api_fat.c | 155 +++++++++++++++++++++++-------------------
env/fatvars.c | 7 +-
include/ebgenv.h | 11 ++-

include/env_api.h | 1 +
include/envdata.h | 2 +-

tools/bg_setenv.c | 144 +++++++++++++++++++++++++++------------
tools/tests/test_ebgenv_api.c | 51 ++++++++------
8 files changed, 254 insertions(+), 150 deletions(-)

--
2.15.0

[Andreas J. Reichel]

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

On creation of a new environment automatically set in_progress,
but do not set ustate to INSTALLED as that will be done in the
end when finalizing the update process.

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

env/env_api.c | 4 ++--
env/env_api_fat.c | 2 +-
tools/tests/test_ebgenv_api.c | 15 ++++++++++++---

3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index 2961b77..6b18646 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -80,10 +80,10 @@ int ebg_env_create_new(ebgenv_t *e)

}
BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
uint32_t new_rev = new_data->revision;

- uint8_t new_ustate = new_data->ustate;
+ uint8_t new_in_progress = new_data->in_progress;
memcpy(new_data, latest_env->data, sizeof(BG_ENVDATA));
new_data->revision = new_rev;
- new_data->ustate = new_ustate;
+ new_data->in_progress = new_in_progress;
bgenv_close(latest_env);
} else {
e->bgenv = latest_env;

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 8636922..60a5ff2 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -427,7 +427,7 @@ BGENV *bgenv_create_new(void)

memset(env_new->data, 0, sizeof(BG_ENVDATA));
/* update revision field and testing mode */
env_new->data->revision = new_rev;
- env_new->data->ustate = 1;
+ env_new->data->in_progress = 1;
/* set default watchdog timeout */
env_new->data->watchdog_timeout_sec = 30;

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index 4820bac..f9ab55c 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -128,12 +128,11 @@ START_TEST(ebgenv_api_ebg_env_create_new)

ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);

-
- ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 0);
+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 1);
ck_assert_int_eq(

((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);

- ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_INSTALLED);
+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);

ck_assert_int_eq(((BGENV *)e.bgenv)->data->watchdog_timeout_sec, 44);
(void)str16to8(buffer, ((BGENV *)e.bgenv)->data->kernelfile);

ck_assert_int_eq(
@@ -141,6 +140,16 @@ START_TEST(ebgenv_api_ebg_env_create_new)
(void)str16to8(buffer, ((BGENV *)e.bgenv)->data->kernelparams);
ck_assert_int_eq(
strncmp(buffer, kernelparams, strlen(kernelparams) + 1), 0);
+
+ /* Test that a new creation of environment does keep the current
+ * values if an update is already in progress
+ */
+ ret = ebg_env_create_new(&e);
+

+ ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);

+ ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);
+ ck_assert_int_eq(

+ ((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);
}

END_TEST

--
2.15.0

[Andreas J. Reichel]

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

If in_progress is set in the latest environment, do not boot from it, as
a current update process may have been interrupted and this environment
is currently used by software update. Instead, boot from the 2nd latest
revision.

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

env/fatvars.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 5d391c7..42574ea 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -191,8 +191,11 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

/* Assume we boot with the latest configuration */
current_partition = latest_idx;

- /* Test if this configuration is in test mode */

- if (env[latest_idx].ustate == USTATE_TESTING) {

+ /* Test if this environment is currently 'in_progress'. If yes,
+ * do not boot from it, instead ignore it */
+ if (env[latest_idx].in_progress == 1) {
+ current_partition = pre_latest_idx;
+ } else if (env[latest_idx].ustate == USTATE_TESTING) {

/* If it has already been booted, this indicates a failed
* update. In this case, mark it as failed by giving a
* zero-revision */

--
2.15.0

[Andreas J. Reichel]

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

* Better string formatting
* Don't print verbose messages if not in verbose mode
* Always inform after successful update

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

[Andreas J. Reichel]

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

Enable tools to access in_progress variable for advanced environment
manipulation.

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

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

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 741d604..41dfb83 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c

@@ -39,6 +39,9 @@ static struct argp_option options_setenv[] = {
{"uservar", 'x', "KEY=VAL", 0, "Set user-defined string variable. For "
"setting multiple variables, use this "
"option multiple times."},
+ {"in_progress", 'i', "IN_PROGRESS", 0, "Set in_progress variable to "
+ "simulate a running update "
+ "process."},
{"version", 'V', 0, 0, "Print version"},
{0}};

@@ -289,6 +292,30 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ustate2str(i));
}
break;
+ case 'i':

+ errno = 0;

+ i = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (errno != 0 && i == 0) || (tmp == arg)) {

+ fprintf(stderr, "Invalid value specified.\n");
+ return 1;
+ }
+ if (i < 0 || i > 1) {
+ fprintf(
+ stderr,

+ "Invalid value specified. Possible values: "
+ "0 (no), 1 (yes)\n");

+ return 1;
+ } else {
+ res = asprintf(&tmp, "%u", i);
+ if (res == -1) {
+ return ENOMEM;
+ }
+ e = journal_add_action(ENV_TASK_SET, "in_progress", 0,
+ (uint8_t *)tmp, strlen(tmp) + 1);
+ VERBOSE(stdout, "in_progress set to %d.\n", i);
+ }
+ break;
case 'r':
i = atoi(arg);

VERBOSE(stdout, "Revision is set to %d.\n", i);

@@ -431,6 +458,7 @@ static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];

printf("Values: \n");
+ printf("in_progress: %s\n", env->in_progress ? "yes" : "no");

printf("revision: %u\n", env->revision);

printf("kernel: %s\n", str16to8(buffer, env->kernelfile));

printf("kernelargs: %s\n", str16to8(buffer, env->kernelparams));

--
2.15.0

[Andreas J. Reichel]

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

Add a helper function to the API which should be called in the end of

update processes. The function sets in_progress to 0 and ustate to
INSTALLED.

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

env/env_api.c | 9 +++++++++
include/ebgenv.h | 6 ++++++
tools/tests/test_ebgenv_api.c | 6 ++++++
3 files changed, 21 insertions(+)

diff --git a/env/env_api.c b/env/env_api.c
index 6b18646..75695d1 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -239,3 +239,12 @@ int ebg_env_close(ebgenv_t *e)

e->bgenv = NULL;
return 0;
}
+
+int ebg_env_finalize_update(ebgenv_t *e) {
+ if (!e->bgenv || !((BGENV *)e->bgenv)->data) {

+ return EIO;
+ }

+ ((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 d33c0d6..12b778e 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h

@@ -128,4 +128,10 @@ int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate);
*/
int ebg_env_close(ebgenv_t *e);

+/** @brief Finalizes a currently running update procedure
+ * @param e A pointer to an ebgenv_t context.
+ * @return 0 on success, errno on failure
+ */
+int ebg_env_finalize_update(ebgenv_t *e);
+
#endif //__EBGENV_H__

diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index f9ab55c..533b50b 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -150,6 +150,12 @@ START_TEST(ebgenv_api_ebg_env_create_new)

ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_OK);

ck_assert_int_eq(
((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);

[Andreas J. Reichel]

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

Console outputs now all use fprintf to stdout or stderr.

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

tools/bg_setenv.c | 62 +++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index df63265..b397aca 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -264,7 +264,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
return 1;
}
if (i == 0 || i == 1) {
- printf("Updating config partition #%d\n", i);
+ fprintf(stdout, "Updating config partition #%d\n", i);
arguments->which_part = i;
part_specified = true;
} else {
@@ -339,7 +339,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
case 'w':
i = parse_int(arg);

if (errno || i == 0) {

- fprintf(stderr, "Invalid watchdog timeout specified.\n");
+ fprintf(stderr,
+ "Invalid watchdog timeout specified.\n");
return 1;
}
VERBOSE(stdout,
@@ -383,7 +384,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
e = set_uservars(arg);
break;
case 'V':
- printf("EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION);
+ fprintf(stdout, "EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION);
exit(0);
case ARGP_KEY_ARG:
/* too many arguments - program terminates with call to
@@ -411,10 +412,10 @@ static void dump_uservars(uint8_t *udata)
while (*udata) {
bgenv_map_uservar(udata, &key, &type, (uint8_t **)&value,
&rsize, &dsize);
- printf("%s ", key);
+ fprintf(stdout, "%s ", key);
type &= USERVAR_STANDARD_TYPE_MASK;
if (type == USERVAR_TYPE_STRING_ASCII) {
- printf("= %s\n", value);
+ fprintf(stdout, "= %s\n", value);
} else if (type >= USERVAR_TYPE_UINT8 &&
type <= USERVAR_TYPE_UINT64) {
switch(type) {
@@ -431,7 +432,8 @@ static void dump_uservars(uint8_t *udata)
val_unum = *((uint64_t *) value);
break;
}
- printf("= %llu\n", (long long unsigned int) val_unum);
+ fprintf(stdout, "= %llu\n",
+ (long long unsigned int) val_unum);
} else if (type >= USERVAR_TYPE_SINT8 &&
type <= USERVAR_TYPE_SINT64) {
switch(type) {
@@ -448,18 +450,19 @@ static void dump_uservars(uint8_t *udata)
val_snum = *((int64_t *) value);
break;
}
- printf("= %lld\n", (long long signed int) val_snum);
+ fprintf(stdout, "= %lld\n",
+ (long long signed int) val_snum);
} else {
switch(type) {
case USERVAR_TYPE_CHAR:
- printf("= %c\n", (char) *value);
+ fprintf(stdout, "= %c\n", (char) *value);
break;
case USERVAR_TYPE_BOOL:
- printf("= %s\n",
+ fprintf(stdout, "= %s\n",
(bool) *value ? "true" : "false");
break;
default:
- printf("( Type is not printable )\n");
+ fprintf(stdout, "( Type is not printable )\n");
}
}

@@ -470,24 +473,28 @@ static void dump_uservars(uint8_t *udata)

static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];

- printf("Values:\n");

- printf("in_progress: %s\n", env->in_progress ? "yes" : "no");
- printf("revision: %u\n", env->revision);
- printf("kernel: %s\n", str16to8(buffer, env->kernelfile));
- printf("kernelargs: %s\n", str16to8(buffer, env->kernelparams));
- printf("watchdog timeout: %u seconds\n", env->watchdog_timeout_sec);

- printf("ustate: %u (%s)\n", (uint8_t)env->ustate,

+ fprintf(stdout, "Values:\n");
+ fprintf(stdout,
+ "in_progress: %s\n",env->in_progress ? "yes" : "no");
+ fprintf(stdout, "revision: %u\n", env->revision);
+ fprintf(stdout,
+ "kernel: %s\n", str16to8(buffer, env->kernelfile));
+ fprintf(stdout,
+ "kernelargs: %s\n", str16to8(buffer, env->kernelparams));
+ fprintf(stdout,
+ "watchdog timeout: %u seconds\n", env->watchdog_timeout_sec);
+ fprintf(stdout, "ustate: %u (%s)\n", (uint8_t)env->ustate,
ustate2str(env->ustate));
- printf("\n");
- printf("user variables:\n");
+ fprintf(stdout, "\n");
+ fprintf(stdout, "user variables:\n");
dump_uservars(env->userdata);
- printf("\n\n");
+ fprintf(stdout, "\n\n");
}

static void update_environment(BGENV *env)
{
if (verbosity) {

- printf("Processing journal...\n");

+ fprintf(stdout, "Processing journal...\n");
}

while (!STAILQ_EMPTY(&head)) {
@@ -507,8 +514,8 @@ static void dump_envs(void)
{

for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {

if (verbosity) {
- printf("\n----------------------------\n");
- printf(" Config Partition #%d ", i);
+ fprintf(stdout, "\n----------------------------\n");
+ fprintf(stdout, " Config Partition #%d ", i);
}
BGENV *env = bgenv_open_by_index(i);
if (env) {
@@ -624,8 +631,9 @@ int main(int argc, char **argv)
return 1;
}
if (verbosity) {
- printf("Updating environment with revision %u\n",
- env_new->data->revision);
+ fprintf(stdout,
+ "Updating environment with revision %u\n",
+ env_new->data->revision);
}

if (!env_current->data || !env_new->data) {
@@ -658,8 +666,8 @@ int main(int argc, char **argv)
update_environment(env_new);

if (verbosity) {
- printf("New environment data:\n");
- printf("---------------------\n");
+ fprintf(stdout, "New environment data:\n");
+ fprintf(stdout, "---------------------\n");
dump_env(env_new->data);
}
if (!bgenv_write(env_new)) {
--
2.15.0

[Andreas J. Reichel]

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

Convention for error return values is:

* return `-errno` for all functions accessing environment variables,
because their return value meaning depend on the parameters.

* return `errno` or `NULL` or `false` for all other functions.

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 12b778e..46e692c 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -63,7 +63,7 @@ int ebg_env_open_current(ebgenv_t *e);
* @param key an enum constant to specify the variable
* @param buffer pointer to buffer containing requested value.
* If buffer is NULL, return needed buffer size.
- * @return If buffer != NULL: 0 on success, errno on failure
+ * @return If buffer != NULL: 0 on success, -errno on failure
* If buffer == NULL: needed buffer size, 0 if variable
* is not found.
*/
@@ -95,7 +95,7 @@ int ebg_env_set_ex(ebgenv_t *e, char *key, uint64_t datatype, uint8_t *value,
* @param buffer to store the datatype of the value
* @param buffer destination for data to be stored into the variable
* @param maxlen size of provided buffer
- * @return 0 on success, errno on failure
+ * @return 0 on success, -errno on failure
*/
int ebg_env_get_ex(ebgenv_t *e, char *key, uint64_t *datatype, uint8_t *buffer,
uint32_t maxlen);
--
2.15.0

[Andreas J. Reichel]

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

ustate is an 8-bit int, not a 16-bit int.

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

env/env_api_fat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 60a5ff2..1705cb9 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -311,7 +311,7 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
case EBGENV_USTATE:
return bgenv_get_uint(buffer, type, data,
env->data->ustate,
- USERVAR_TYPE_UINT16);
+ USERVAR_TYPE_UINT8);
case EBGENV_IN_PROGRESS:
return bgenv_get_uint(buffer, type, data,
env->data->in_progress,
--
2.15.0

[Andreas J. Reichel]

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

* Unify parsing of int arguments
* Fix memory leaks in argument parser

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

tools/bg_setenv.c | 67 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 41dfb83..f865fce 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c

@@ -212,6 +212,21 @@ static error_t set_uservars(char *arg)
(uint8_t *)value, strlen(value) + 1);
}

+static int parse_int(char *arg)
+{
+ char *tmp;
+ int i;
+

+ errno = 0;
+ i = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (errno != 0 && i == 0) || (tmp == arg)) {

+ errno = EINVAL;
+ return -1;
+ }
+ return i;
+}
+

static error_t parse_opt(int key, char *arg, struct argp_state *state)

{
struct arguments *arguments = state->input;
@@ -243,10 +258,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
(uint8_t *)arg, strlen(arg) + 1);
break;
case 'p':

- errno = 0;

- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {
fprintf(stderr, "Invalid number specified for -p.\n");
return 1;
}

@@ -262,10 +275,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
}
break;
case 's':
- errno = 0;

- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {
// maybe user specified an enum string
i = str2ustate(arg);
if (i == USTATE_UNKNOWN) {

@@ -288,23 +299,21 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
}
e = journal_add_action(ENV_TASK_SET, "ustate", 0,

(uint8_t *)tmp, strlen(tmp) + 1);

+ free(tmp);
VERBOSE(stdout, "Ustate set to %d (%s).\n", i,
ustate2str(i));
}
break;
case 'i':

- errno = 0;

- i = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
- (errno != 0 && i == 0) || (tmp == arg)) {
+ i = parse_int(arg);
+ if (errno) {

fprintf(stderr, "Invalid value specified.\n");
return 1;
}

if (i < 0 || i > 1) {

- fprintf(
- stderr,
- "Invalid value specified. Possible values: "

- "0 (no), 1 (yes)\n");
+ fprintf(stderr,

+ "Invalid value specified. Possible values: "
+ "0 (no), 1 (yes)\n");

return 1;
} else {

res = asprintf(&tmp, "%u", i);

@@ -313,27 +322,31 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
}
e = journal_add_action(ENV_TASK_SET, "in_progress", 0,

(uint8_t *)tmp, strlen(tmp) + 1);

+ free(tmp);

VERBOSE(stdout, "in_progress set to %d.\n", i);
}

break;
case 'r':
- i = atoi(arg);
- VERBOSE(stdout, "Revision is set to %d.\n", i);
+ i = parse_int(arg);
+ if (errno) {

+ fprintf(stderr, "Invalid revision specified.\n");
+ return 1;
+ }

[Jan Kiszka]

Thanks, merged to next. Unless something unexpected happens, I'll do a
release based on this in a couple of days.

Jan