On 2017-11-22 12:59, Andreas J. Reichel wrote:
> From: Andreas Reichel <andreas.reichel....@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.reichel....@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;
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.
> + }
> + 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);
>
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups "EFI
Boot Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to efibootguard-dev+unsubscr...@googlegroups.com.
To post to this group, send email to efibootguard-dev@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/fe03161d-011c-8baa-c472-6a82a34634f5%40siemens.com.
For more options, visit https://groups.google.com/d/optout.
