From: Andreas Reichel <[email protected]> From: Reichel Andreas <[email protected]>
Combine testing and boot_once flag to `ustate`: * boot_once = 0, testing = 0: ustate = 0 * boot_once = 0, testing = 1: ustate = 1 * boot_once = 1, testing = 1: ustate = 2 * boot_once = 1, testing = 1, revision = 0: ustate = 3 Signed-off-by: Andreas Reichel <[email protected]> --- docs/TODO.md | 9 ------- docs/TOOLS.md | 6 ++--- docs/UPDATE.md | 35 +++++++++++--------------- env/fatvars.c | 37 +++++++++++++--------------- include/envdata.h | 8 ++++-- swupdate-adapter/ebgenv.c | 63 +++++++++++------------------------------------ tools/bg_setenv.c | 39 +++++++++-------------------- tools/tests/test_api.c | 24 ++++++------------ 8 files changed, 75 insertions(+), 146 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index 36c5dfb..e5f4b3e 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -13,15 +13,6 @@ method must be defined and implemented to account for generic key-value pairs. -* State refactoring - * Currently, there are three variables 'revision', 'testing', - 'boot_once', where the latter two are mapped onto a variable called - 'ustate'. The 'ustate' variable in turn equals an enum type variable - within swupdate, so that for the swupdate adapter, a complex mapping - must be implemented. To resolve this issue, the two variables - 'boot_once' and 'testing' will be unified to the 'ustate' variable, - which will have the same enum type as used in 'swupdate'. - * API refactoring * Currently, there are two APIs, a lower API 'bg_utils.c', and an adapter-API 'ebgenv.c'. After refactoring the state variable, the API diff --git a/docs/TOOLS.md b/docs/TOOLS.md index c79e5a8..ff0179f 100644 --- a/docs/TOOLS.md +++ b/docs/TOOLS.md @@ -28,7 +28,7 @@ In most cases, the user wants to update to a new environment configuration, which can be done with: ``` -./bg_setenv --update --kernel="XXXX" --args="YYYY" --watchdog=25 --testing=1 +./bg_setenv --update --kernel="XXXX" --args="YYYY" --watchdog=25 ``` The `--update` parameter tells `bg_setenv` to automatically overwrite the @@ -71,12 +71,12 @@ To simulate a failed update, with its environment data stored in config partitio issue: ``` -bg_setenv --partition=1 --bootonce --testing=1 --revision=0 +bg_setenv --partition=1 --ustate=3 --revision=0 ``` To simulate a reboot of a recently updated configuration stored in config partition 1, issue: ``` -bg_setenv --partition=1 --boot_once +bg_setenv --partition=1 --ustate=2 ``` diff --git a/docs/UPDATE.md b/docs/UPDATE.md index 169fb9d..a00de25 100644 --- a/docs/UPDATE.md +++ b/docs/UPDATE.md @@ -11,8 +11,7 @@ The structure of the environment data is as follows: struct _BG_ENVDATA { uint16_t kernelfile[ENV_STRING_LENGTH]; uint16_t kernelparams[ENV_STRING_LENGTH]; - uint8_t testing; - uint8_t boot_once; + uint16_t ustate; uint16_t watchdog_timeout_sec; uint32_t revision; uint32_t crc32; @@ -22,8 +21,8 @@ struct _BG_ENVDATA { The fields have the following meaning: * `kernelfile`: Path to the kernel image, utf-16 encoded * `kernelparams`: Arguments to the kernel, utf-16 encoded -* `testing`: A flag that specifies if the configuration is in test mode -* `boot_once`: Set by `efibootguard` if it first boots a test configuration +* `ustate`: Update status (`0` no action, `1` update installed, `2` testing, + `3`: update failed) * `watchdog_timeout_sec`: Number of seconds, the watchdog times out after * `revision`: The revision number explained above * `crc32`: A crc32 checksum @@ -35,25 +34,25 @@ struct members. Assume the following for the next examples: The system has been booted with a configuration that has a `revision` of `4`. The user can set a new -configuration with a `revision` of `5`, with `testing` flag set. On the next +configuration with a `revision` of `5`, with `ustate` set to `1`. On the next reboot, `efibootguard` loads the configuration with the highest `revision` -number. If it detects, that `testing` is set, it will enable the `boot_once` -flag and boot the system with this configuration. +number. If it detects, that `ustate` is `1`, it will update the value to +`2` and boot the system with this configuration. ### Example scenario 1 - Successful update ### -Once booted, the user disables both `testing` and `boot_once` to confirm the +Once booted, the user resets `ustate` to a value of `0` to confirm the update using the [tools](TOOLS.md). ### Example scenario 2 - System crash during first boot after update ### If the system freezes during boot, the watchdog will reset the system. On the next boot `efibootguard` detects that this configuration had already been -tested before, because `boot_once` is already set. Thus, it will load the 2nd -latest configuration instead. The failed update is indicated by the revision -of the failed configuration set to `0` with both `boot_once` and `testing` -enabled. A revision of 0 is the lowest possible number and avoids that the -corresponding configuration is booted again in future. +tested before, because `ustate` is `2`. Thus, it will load the 2nd latest +configuration instead. The failed update is indicated by the revision of the +failed configuration set to `0` with `ustate` set to `3` . A revision of `0` is +the lowest possible number and avoids that the corresponding configuration is +booted again in future. ## Visual explanation of the update process ## @@ -62,7 +61,6 @@ corresponding configuration is booted again in future. | || | | Rev: latest || Rev: oldest | +--------> | (working) || | - | | || | | +--------------++--------------+ | | | | update @@ -70,8 +68,7 @@ corresponding configuration is booted again in future. | +---------------++--------------+ | | || | | | Rev: latest-1 || Rev: latest | - | | (working) || testing: 1 | - | | || boot_once: 0 | + | | (working) || ustate: 1 | | +---------------++--------------+ | | | | reboot @@ -79,8 +76,7 @@ corresponding configuration is booted again in future. | +---------------++--------------+ | | || | | | Rev: latest-1 || Rev: latest | - | | (working) || testing: 1 | - | | || boot_once: 1 | + | | (working) || ustate: 2 | | +---------------++--------------+ | | no | success? ---------------------+ @@ -90,8 +86,7 @@ corresponding configuration is booted again in future. | +----------++--------------+ +----------++--------------+ | | || | | || | | | Rev: || Rev: latest | | Rev: || Rev: 0 | - | | latest-1 || testing: 0 | | latest-1 || testing: 1 | - | | || boot_once: 0 | | || boot_once: 1 | + | | latest-1 || ustate: 0 | | latest-1 || ustate: 3 | | +----------++--------------+ +----------++--------------+ | boots | +----- latest' = latest-1 +---------------------------+ diff --git a/env/fatvars.c b/env/fatvars.c index 406bdf9..8f871b9 100644 --- a/env/fatvars.c +++ b/env/fatvars.c @@ -193,24 +193,21 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp) current_partition = latest_idx; /* Test if this configuration is in test mode */ - if (env[latest_idx].testing) { - if (env[latest_idx].boot_once) { - /* If it has already been booted, this indicates a - * failed update. In this case, mark it as failed by - * giving a zero-revision */ - env[latest_idx].revision = REVISION_FAILED; - save_current_config(); - /* We must boot with the configuration that was active - * before - */ - current_partition = pre_latest_idx; - } else { - /* If this configuration has never been booted with, set - * boot_once flag to indicate that this configuration is - * now being tested */ - env[latest_idx].boot_once = TRUE; - save_current_config(); - } + 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 */ + env[latest_idx].ustate = USTATE_FAILED; + env[latest_idx].revision = REVISION_FAILED; + save_current_config(); + /* We must boot with the configuration that was active before + */ + current_partition = pre_latest_idx; + } else if (env[latest_idx].ustate == USTATE_INSTALLED) { + /* If this configuration has never been booted with, set ustate + * to indicate that this configuration is now being tested */ + env[latest_idx].ustate = USTATE_TESTING; + save_current_config(); } bglp->payload_path = StrDuplicate(env[current_partition].kernelfile); @@ -219,8 +216,8 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp) bglp->timeout = env[current_partition].watchdog_timeout_sec; Print(L"Config Revision: %d:\n", latest_rev); - Print(L" testing: %s\n", - env[current_partition].testing ? L"yes" : L"no"); + Print(L" ustate: %d\n", + env[current_partition].ustate); Print(L" kernel: %s\n", bglp->payload_path); Print(L" args: %s\n", bglp->payload_options); Print(L" timeout: %d seconds\n", bglp->timeout); diff --git a/include/envdata.h b/include/envdata.h index f5b8070..75e2b9f 100644 --- a/include/envdata.h +++ b/include/envdata.h @@ -18,13 +18,17 @@ #define CONFIG_PARTITION_MAXCOUNT 64 +#define USTATE_OK 0 +#define USTATE_INSTALLED 1 +#define USTATE_TESTING 2 +#define USTATE_FAILED 3 + #pragma pack(push) #pragma pack(1) struct _BG_ENVDATA { uint16_t kernelfile[ENV_STRING_LENGTH]; uint16_t kernelparams[ENV_STRING_LENGTH]; - uint8_t testing; - uint8_t boot_once; + uint16_t ustate; uint16_t watchdog_timeout_sec; uint32_t revision; uint32_t crc32; diff --git a/swupdate-adapter/ebgenv.c b/swupdate-adapter/ebgenv.c index d5f7080..951bdc7 100644 --- a/swupdate-adapter/ebgenv.c +++ b/swupdate-adapter/ebgenv.c @@ -19,8 +19,7 @@ typedef enum { EBGENV_KERNELPARAMS, EBGENV_WATCHDOG_TIMEOUT_SEC, EBGENV_REVISION, - EBGENV_BOOT_ONCE, - EBGENV_TESTING, + EBGENV_USTATE, EBGENV_UNKNOWN } EBGENVKEY; @@ -84,11 +83,8 @@ static EBGENVKEY ebg_env_str2enum(char *key) if (strncmp(key, "revision", strlen("revision") + 1) == 0) { return EBGENV_REVISION; } - if (strncmp(key, "boot_once", strlen("boot_once") + 1) == 0) { - return EBGENV_BOOT_ONCE; - } - if (strncmp(key, "testing", strlen("testing") + 1) == 0) { - return EBGENV_TESTING; + if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) { + return EBGENV_USTATE; } return EBGENV_UNKNOWN; } @@ -131,7 +127,7 @@ int ebg_env_create_new(void) memset(env_current->data, 0, sizeof(BG_ENVDATA)); /* update revision field and testing mode */ env_current->data->revision = new_rev; - env_current->data->testing = 1; + env_current->data->ustate = 1; /* set default watchdog timeout */ env_current->data->watchdog_timeout_sec = 30; ebg_new_env_created = true; @@ -218,19 +214,8 @@ char *ebg_env_get(char *key) return NULL; } return buffer; - case EBGENV_BOOT_ONCE: - if (asprintf(&buffer, "%lu", env_current->data->boot_once) < - 0) { - errno = ENOMEM; - return NULL; - } - if (!ebg_gc_addpointer(buffer)) { - errno = ENOMEM; - return NULL; - } - return buffer; - case EBGENV_TESTING: - if (asprintf(&buffer, "%lu", env_current->data->testing) < 0) { + case EBGENV_USTATE: + if (asprintf(&buffer, "%u", env_current->data->ustate) < 0) { errno = ENOMEM; return NULL; } @@ -294,7 +279,7 @@ int ebg_env_set(char *key, char *value) } env_current->data->watchdog_timeout_sec = val; break; - case EBGENV_BOOT_ONCE: + case EBGENV_USTATE: errno = 0; val = strtol(value, &p, 10); if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) || @@ -304,19 +289,7 @@ int ebg_env_set(char *key, char *value) if (p == value) { return EINVAL; } - env_current->data->boot_once = val; - break; - case EBGENV_TESTING: - errno = 0; - val = strtol(value, &p, 10); - env_current->data->testing = val; - if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) || - (errno != 0 && val == 0)) { - return errno; - } - if (p == value) { - return EINVAL; - } + env_current->data->ustate = val; break; default: return EINVAL; @@ -337,7 +310,7 @@ bool ebg_env_isupdatesuccessful(void) * with * testing and boot_once set */ if (env->data->revision == REVISION_FAILED && - env->data->testing == 1 && env->data->boot_once == 1) { + env->data->ustate == 3) { (void)bgenv_close(env); return false; } @@ -355,9 +328,8 @@ int ebg_env_clearerrorstate(void) continue; } if (env->data->revision == REVISION_FAILED && - env->data->testing == 1 && env->data->boot_once == 1) { - env->data->testing = 0; - env->data->boot_once = 0; + env->data->ustate == 3) { + env->data->ustate = 0; if (!bgenv_write(env)) { (void)bgenv_close(env); return EIO; @@ -372,12 +344,7 @@ int ebg_env_clearerrorstate(void) int ebg_env_confirmupdate(void) { - int ret = ebg_env_set("testing", "0"); - - if (ret) { - return ret; - } - return ebg_env_set("boot_once", "0"); + return ebg_env_set("ustate", "0"); } bool ebg_env_isokay(void) @@ -390,7 +357,7 @@ bool ebg_env_isokay(void) errno = EIO; return res; } - if (env->data->testing == 0) { + if (env->data->ustate == 0) { res = true; } bgenv_close(env); @@ -407,7 +374,7 @@ bool ebg_env_isinstalled(void) errno = EIO; return res; } - if (env->data->testing == 1 && env->data->boot_once == 0) { + if (env->data->ustate == 1) { res = true; } bgenv_close(env); @@ -424,7 +391,7 @@ bool ebg_env_istesting(void) errno = EIO; return res; } - if (env->data->testing == 1 && env->data->boot_once == 1) { + if (env->data->ustate == 2) { res = true; } bgenv_close(env); diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c index 525a96e..ad11138 100644 --- a/tools/bg_setenv.c +++ b/tools/bg_setenv.c @@ -23,7 +23,7 @@ static struct argp_option options_setenv[] = { "with the smallest revision value " "above zero is updated."}, {"revision", 'r', "REVISION", 0, "Set revision value"}, - {"testing", 't', "TESTING", 0, "Set test mode for environment"}, + {"ustate", 's', "USTATE", 0, "Set update status for environment"}, {"filepath", 'f', "ENVFILE_DIR", 0, "Output environment to file. Please " "only provide an output path. The " "file name is automatically appended."}, @@ -31,7 +31,6 @@ static struct argp_option options_setenv[] = { {"confirm", 'c', 0, 0, "Confirm working environment"}, {"update", 'u', 0, 0, "Automatically update oldest revision"}, {"verbose", 'v', 0, 0, "Be verbose"}, - {"bootonce", 'b', 0, 0, "Simulate boot with update installed"}, {0}}; static struct argp_option options_printenv[] = { @@ -110,17 +109,16 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) return 1; } break; - case 't': + case 's': i = atoi(arg); - if (i == 0 || i == 1) { - if (arguments->tmpdata.testing = (bool)i) { - VERBOSE(stdout, "Testing mode enabled.\n"); - } + if (i >= 0 && i <= 3) { + arguments->tmpdata.ustate = i; + VERBOSE(stdout, "Ustate set to %u.\n", i); } else { fprintf( stderr, - "Invalid testing flag specified. Possible values: " - "0 (disabled), 1 (enabled)\n"); + "Invalid ustate value specified. Possible values: " + "0 (ok), 1 (installed), 2 (testing), 3 (failed)\n"); return 1; } break; @@ -148,8 +146,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) VERBOSE(stdout, "Confirming environment to work. Removing boot-once " "and testing flag.\n"); - arguments->tmpdata.boot_once = false; - arguments->tmpdata.testing = false; + arguments->tmpdata.ustate = 0; break; case 'u': if (part_specified) { @@ -167,13 +164,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) /* Set verbosity in the library */ be_verbose(true); break; - case 'b': - /* Simulate a reboot with testing=1 */ - VERBOSE(stdout, - "Simulating reboot by setting boot_once to 1.\n"); - arguments->tmpdata.testing = true; - arguments->tmpdata.boot_once = true; - break; case ARGP_KEY_ARG: /* too many arguments - program terminates with call to * argp_usage with non-zero return code */ @@ -198,9 +188,9 @@ static void update_environment(BG_ENVDATA *dest, BG_ENVDATA *src) memcpy((void *)dest->kernelparams, (void *)src->kernelparams, sizeof(src->kernelparams)); } - if ((uint8_t)src->testing != IGNORE_MARKER_BYTE) { - memcpy((void *)&dest->testing, (void *)&src->testing, - sizeof(src->testing)); + if ((uint8_t)src->ustate != IGNORE_MARKER_BYTE) { + memcpy((void *)&dest->ustate, (void *)&src->ustate, + sizeof(src->ustate)); } if ((uint8_t)src->revision != IGNORE_MARKER_BYTE) { memcpy((void *)&dest->revision, (void *)&src->revision, @@ -211,10 +201,6 @@ static void update_environment(BG_ENVDATA *dest, BG_ENVDATA *src) (void *)&src->watchdog_timeout_sec, sizeof(src->watchdog_timeout_sec)); } - if ((uint8_t)src->boot_once != IGNORE_MARKER_BYTE) { - memcpy((void *)&dest->boot_once, (void *)&src->boot_once, - sizeof(src->boot_once)); - } dest->crc32 = crc32(0, (Bytef *)dest, sizeof(BG_ENVDATA) - sizeof(dest->crc32)); } @@ -227,8 +213,7 @@ static void dump_env(BG_ENVDATA *env) 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("test flag: %s\n", env->testing ? "enabled" : "disabled"); - printf("boot once flag: %s\n", env->boot_once ? "set" : "not set"); + printf("ustate: %u\n", env->ustate); printf("\n\n"); } diff --git a/tools/tests/test_api.c b/tools/tests/test_api.c index cca4573..53e573d 100644 --- a/tools/tests/test_api.c +++ b/tools/tests/test_api.c @@ -115,8 +115,7 @@ static void test_api_accesscurrent(void **state) assert_int_equal(ebg_env_set("kernelparams", "root=/dev/sda"), 0); assert_int_equal(ebg_env_set("watchdog_timeout_sec", "abc"), EINVAL); assert_int_equal(ebg_env_set("watchdog_timeout_sec", "0013"), 0); - assert_int_equal(ebg_env_set("testing", "1"), 0); - assert_int_equal(ebg_env_set("boot_once", "1"), 0); + assert_int_equal(ebg_env_set("ustate", "1"), 0); will_return(bgenv_write, true); ret = ebg_env_close(); @@ -134,8 +133,7 @@ static void test_api_accesscurrent(void **state) assert_string_equal(ebg_env_get("kernelfile"), "vmlinuz"); assert_string_equal(ebg_env_get("kernelparams"), "root=/dev/sda"); assert_string_equal(ebg_env_get("watchdog_timeout_sec"), "13"); - assert_string_equal(ebg_env_get("testing"), "1"); - assert_string_equal(ebg_env_get("boot_once"), "1"); + assert_string_equal(ebg_env_get("ustate"), "1"); assert_string_equal(ebg_env_get("revision"), test_env_revision_str); will_return(bgenv_write, true); @@ -154,29 +152,21 @@ static void test_api_update(void **state) assert_int_equal(envupdate.data->revision, test_env_revision + 1); assert_int_equal(envupdate.data->watchdog_timeout_sec, DEFAULT_WATCHDOG_TIMEOUT_SEC); + assert_int_equal(envupdate.data->ustate, 1); - assert_int_equal(ebg_env_set("testing", "1"), 0); - assert_int_equal(ebg_env_set("boot_once", "1"), 0); + assert_int_equal(ebg_env_set("ustate", "2"), 0); assert_int_equal(ebg_env_confirmupdate(), 0); assert_int_equal(ebg_env_set("revision", "0"), 0); - assert_int_equal(ebg_env_set("testing", "1"), 0); - assert_int_equal(ebg_env_set("boot_once", "1"), 0); + assert_int_equal(ebg_env_set("ustate", "3"), 0); assert_false(ebg_env_isupdatesuccessful()); assert_int_equal(ebg_env_set("revision", "0"), 0); - assert_int_equal(ebg_env_set("testing", "1"), 0); - assert_int_equal(ebg_env_set("boot_once", "0"), 0); + assert_int_equal(ebg_env_set("ustate", "0"), 0); assert_true(ebg_env_isupdatesuccessful()); assert_int_equal(ebg_env_set("revision", "0"), 0); - assert_int_equal(ebg_env_set("testing", "0"), 0); - assert_int_equal(ebg_env_set("boot_once", "0"), 0); - assert_true(ebg_env_isupdatesuccessful()); - - assert_int_equal(ebg_env_set("revision", "0"), 0); - assert_int_equal(ebg_env_set("testing", "1"), 0); - assert_int_equal(ebg_env_set("boot_once", "1"), 0); + assert_int_equal(ebg_env_set("ustate", "3"), 0); will_return(bgenv_write, true); assert_int_equal(ebg_env_clearerrorstate(), 0); assert_true(ebg_env_isupdatesuccessful()); -- 2.13.3 -- 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 [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/20170822161551.12662-8-andreas.reichel.ext%40siemens.com. For more options, visit https://groups.google.com/d/optout.
