Currently, the implementations of bg_setenv and bg_printenv are tightly coupled with each other, making it (unnecessarily) difficult to grasp, modify and extend the existing control flow. This is a refactoring and does not introduce any new functionality.
Details: * move bg_printenv code into its own method * move cli arguments into struct, thereby keeping the global namespace clean * split bg_setenv/bg_printenv parsers * do not rely on parsing order for arg validation, i.e. first parse and then perform validation check Signed-off-by: Michael Adler <[email protected]> --- tools/bg_setenv.c | 311 ++++++++++++++++++++++++++++------------------ 1 file changed, 189 insertions(+), 122 deletions(-) diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c index e564219..3f1620c 100644 --- a/tools/bg_setenv.c +++ b/tools/bg_setenv.c @@ -24,7 +24,17 @@ static char doc[] = "bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard"; +// clang-format off +#define BG_CLI_OPTIONS_COMMON \ + {"filepath", 'f', "ENVFILE", 0, \ + "Environment to use. Expects a file name, " \ + "usually called BGENV.DAT."}, \ + {"verbose", 'v', 0, 0, "Be verbose"}, \ + {"version", 'V', 0, 0, "Print version"} +// clang-format on + static struct argp_option options_setenv[] = { + BG_CLI_OPTIONS_COMMON, {"preserve", 'P', 0, 0, "Preserve existing entries"}, {"kernel", 'k', "KERNEL", 0, "Set kernel to load"}, {"args", 'a', "KERNEL_ARGS", 0, "Set kernel arguments"}, @@ -33,34 +43,45 @@ static struct argp_option options_setenv[] = { "the one with the smallest revision value above zero is updated."}, {"revision", 'r', "REVISION", 0, "Set revision value"}, {"ustate", 's', "USTATE", 0, "Set update status for environment"}, - {"filepath", 'f', "ENVFILE", 0, - "Output environment to file. Expects an output file name, " - "usually called BGENV.DAT."}, {"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"}, {"confirm", 'c', 0, 0, "Confirm working environment"}, {"update", 'u', 0, 0, "Automatically update oldest revision"}, - {"verbose", 'v', 0, 0, "Be verbose"}, {"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"}, - {} + {}, }; static struct argp_option options_printenv[] = { - {"filepath", 'f', "ENVFILE", 0, - "Read environment from file. Expects a valid EFI Boot Guard " - "environment file."}, - {"verbose", 'v', 0, 0, "Be verbose"}, - {"version", 'V', 0, 0, "Print version"}, - {} + BG_CLI_OPTIONS_COMMON, + {}, +}; + +/* Common arguments used by both bg_setenv and bg_printenv. */ +struct arguments_common { + char *envfilepath; + bool verbosity; }; -struct arguments { - bool printenv; +/* Arguments used by bg_setenv. */ +struct arguments_setenv { + struct arguments_common common; int which_part; + /* auto update feature automatically updates partition with + * oldest environment revision (lowest value) + */ + bool auto_update; + bool part_specified; + /* whether to keep existing entries in BGENV before applying new + * settings */ + bool preserve_env; +}; + +/* Arguments used by bg_printenv. */ +struct arguments_printenv { + struct arguments_common common; }; typedef enum { ENV_TASK_SET, ENV_TASK_DEL } BGENV_TASK; @@ -163,20 +184,6 @@ static void journal_process_action(BGENV *env, struct env_action *action) } } -/* auto update feature automatically updates partition with - * oldest environment revision (lowest value) - */ -static bool auto_update = false; - -static bool part_specified = false; - -static bool verbosity = false; - -static char *envfilepath = NULL; - -/* whether to keep existing entries in BGENV before applying the new settings */ -static bool preserve_env = false; - static char *ustatemap[] = {"OK", "INSTALLED", "TESTING", "FAILED", "UNKNOWN"}; static uint8_t str2ustate(char *str) @@ -239,9 +246,61 @@ static int parse_int(char *arg) return (int) i; } -static error_t parse_opt(int key, char *arg, struct argp_state *state) +static error_t parse_common_opt(int key, char *arg, bool compat_mode, + struct arguments_common *arguments) { - struct arguments *arguments = state->input; + bool found = false; + switch (key) { + case 'f': + found = true; + free(arguments->envfilepath); + arguments->envfilepath = NULL; + + if (compat_mode) { + /* compat mode, permitting "bg_setenv -f <dir>" */ + struct stat sb; + + int res = stat(arg, &sb); + if (res == 0 && S_ISDIR(sb.st_mode)) { + fprintf(stderr, + "WARNING: Using -f to specify only the " + "ouptut directory is deprecated.\n"); + res = asprintf(&arguments->envfilepath, "%s/%s", + arg, FAT_ENV_FILENAME); + if (res == -1) { + return ENOMEM; + } + } + } + + if (!arguments->envfilepath) { + arguments->envfilepath = strdup(arg); + if (!arguments->envfilepath) { + return ENOMEM; + } + } + break; + case 'v': + found = true; + /* Set verbosity in this program */ + arguments->verbosity = true; + /* Set verbosity in the library */ + bgenv_be_verbose(true); + break; + case 'V': + found = true; + fprintf(stdout, "EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION); + exit(0); + } + if (!found) { + return ARGP_ERR_UNKNOWN; + } + return 0; +} + +static error_t parse_setenv_opt(int key, char *arg, struct argp_state *state) +{ + struct arguments_setenv *arguments = state->input; int i, res; char *tmp; error_t e = 0; @@ -278,7 +337,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) if (i >= 0 && i < ENV_NUM_CONFIG_PARTS) { fprintf(stdout, "Updating config partition #%d\n", i); arguments->which_part = i; - part_specified = true; + arguments->part_specified = true; } else { fprintf(stderr, "Selected partition out of range. Valid range: " @@ -361,34 +420,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) "watchdog_timeout_sec", 0, (uint8_t *)arg, strlen(arg) + 1); break; - case 'f': - free(envfilepath); - envfilepath = NULL; - - /* compat mode, permitting "bg_setenv -f <dir>" */ - if (!arguments->printenv) { - struct stat sb; - - res = stat(arg, &sb); - if (res == 0 && S_ISDIR(sb.st_mode)) { - fprintf(stderr, - "WARNING: Using -f to specify only the " - "ouptut directory is deprecated.\n"); - res = asprintf(&envfilepath, "%s/%s", arg, - FAT_ENV_FILENAME); - if (res == -1) { - return ENOMEM; - } - } - } - - if (!envfilepath) { - envfilepath = strdup(arg); - if (!envfilepath) { - return ENOMEM; - } - } - break; case 'c': VERBOSE(stdout, "Confirming environment to work. Removing boot-once " @@ -397,38 +428,22 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) (uint8_t *)"0", 2); break; case 'u': - if (part_specified) { - fprintf(stderr, - "Error, both automatic and manual partition " - "selection. Cannot use -p and -u " - "simultaneously.\n"); - return 1; - } - auto_update = true; - break; - case 'v': - /* Set verbosity in this program */ - verbosity = true; - /* Set verbosity in the library */ - bgenv_be_verbose(true); + arguments->auto_update = true; break; case 'x': /* Set user-defined variable(s) */ e = set_uservars(arg); break; case 'P': - preserve_env = true; + arguments->preserve_env = true; break; - case 'V': - fprintf(stdout, "EFI Boot Guard %s\n", EFIBOOTGUARD_VERSION); - exit(0); case ARGP_KEY_ARG: /* too many arguments - program terminates with call to * argp_usage with non-zero return code */ argp_usage(state); break; default: - return ARGP_ERR_UNKNOWN; + return parse_common_opt(key, arg, true, &arguments->common); } if (e) { @@ -437,6 +452,23 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state) return e; } +static error_t parse_printenv_opt(int key, char *arg, struct argp_state *state) +{ + struct arguments_printenv *arguments = state->input; + + switch (key) { + case ARGP_KEY_ARG: + /* too many arguments - program terminates with call to + * argp_usage with non-zero return code */ + argp_usage(state); + break; + default: + return parse_common_opt(key, arg, false, &arguments->common); + } + + return 0; +} + static void dump_uservars(uint8_t *udata) { char *key, *value; @@ -527,7 +559,7 @@ static void dump_env(BG_ENVDATA *env) fprintf(stdout, "\n\n"); } -static void update_environment(BGENV *env) +static void update_environment(BGENV *env, bool verbosity) { if (verbosity) { fprintf(stdout, "Processing journal...\n"); @@ -602,7 +634,8 @@ static int printenv_from_file(char *envfilepath) { } } -static int dumpenv_to_file(char *envfilepath) { +static int dumpenv_to_file(char *envfilepath, bool verbosity, bool preserve_env) +{ /* execute journal and write to file */ int result = 0; BGENV env; @@ -616,7 +649,7 @@ static int dumpenv_to_file(char *envfilepath) { return 1; } - update_environment(&env); + update_environment(&env, verbosity); if (verbosity) { dump_env(env.data); } @@ -643,52 +676,83 @@ static int dumpenv_to_file(char *envfilepath) { return result; } -int main(int argc, char **argv) +/* This is the entrypoint for the command bg_printenv. */ +static error_t bg_printenv(int argc, char **argv) { - static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc}; - static struct argp argp_printenv = {options_printenv, parse_opt, NULL, - doc}; - static struct argp *argp; - - bool write_mode = (bool)strstr(argv[0], "bg_setenv"); - if (write_mode) { - argp = &argp_setenv; - - if (argc < 2) { - printf("No task to perform. Please specify at least one" - " optional argument. See --help for further" - " information.\n"); - return 1; - } + struct argp argp_printenv = { + .options = options_printenv, + .parser = parse_printenv_opt, + .doc = doc, + }; - } else { - argp = &argp_printenv; + struct arguments_setenv arguments; + memset(&arguments, 0, sizeof(struct arguments_setenv)); + + error_t e = argp_parse(&argp_printenv, argc, argv, 0, 0, &arguments); + if (e) { + return e; + } + if (arguments.common.envfilepath) { + int result = printenv_from_file(arguments.common.envfilepath); + free(arguments.common.envfilepath); + return result; + } + + /* not in file mode */ + if (!bgenv_init()) { + fprintf(stderr, "Error initializing FAT environment.\n"); + return 1; } - struct arguments arguments; - arguments.printenv = !write_mode; - arguments.which_part = 0; + dump_envs(); + bgenv_finalize(); + return 0; +} + +/* This is the entrypoint for the command bg_setenv. */ +static error_t bg_setenv(int argc, char **argv) +{ + if (argc < 2) { + printf("No task to perform. Please specify at least one" + " optional argument. See --help for further" + " information.\n"); + return 1; + } + + struct argp argp_setenv = { + .options = options_setenv, + .parser = parse_setenv_opt, + .doc = doc, + }; + + struct arguments_setenv arguments; + memset(&arguments, 0, sizeof(struct arguments_setenv)); STAILQ_INIT(&head); error_t e; - e = argp_parse(argp, argc, argv, 0, 0, &arguments); + e = argp_parse(&argp_setenv, argc, argv, 0, 0, &arguments); if (e) { return e; } + if (arguments.auto_update && arguments.part_specified) { + fprintf(stderr, "Error, both automatic and manual partition " + "selection. Cannot use -p and -u " + "simultaneously.\n"); + return 1; + } + int result = 0; /* arguments are parsed, journal is filled */ /* is output to file or input from file ? */ - if (envfilepath) { - if (write_mode) { - result = dumpenv_to_file(envfilepath); - } else { - result = printenv_from_file(envfilepath); - } - free(envfilepath); + if (arguments.common.envfilepath) { + result = dumpenv_to_file(arguments.common.envfilepath, + arguments.common.verbosity, + arguments.preserve_env); + free(arguments.common.envfilepath); return result; } @@ -698,20 +762,14 @@ int main(int argc, char **argv) return 1; } - if (!write_mode) { - dump_envs(); - bgenv_finalize(); - return 0; - } - - if (verbosity) { + if (arguments.common.verbosity) { dump_envs(); } BGENV *env_new = NULL; BGENV *env_current; - if (auto_update) { + if (arguments.auto_update) { /* clone latest environment */ env_current = bgenv_open_latest(); @@ -729,7 +787,7 @@ int main(int argc, char **argv) result = 1; goto cleanup; } - if (verbosity) { + if (arguments.common.verbosity) { fprintf(stdout, "Updating environment with revision %u\n", env_new->data->revision); @@ -748,7 +806,7 @@ int main(int argc, char **argv) bgenv_close(env_current); } else { - if (part_specified) { + if (arguments.part_specified) { env_new = bgenv_open_by_index(arguments.which_part); } else { env_new = bgenv_open_latest(); @@ -761,9 +819,9 @@ int main(int argc, char **argv) } } - update_environment(env_new); + update_environment(env_new, arguments.common.verbosity); - if (verbosity) { + if (arguments.common.verbosity) { fprintf(stdout, "New environment data:\n"); fprintf(stdout, "---------------------\n"); dump_env(env_new->data); @@ -780,3 +838,12 @@ cleanup: bgenv_finalize(); return result; } + +int main(int argc, char **argv) +{ + if (strstr(argv[0], "bg_setenv")) { + return bg_setenv(argc, argv); + } else { + return bg_printenv(argc, argv); + } +} -- 2.33.0 -- 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 view this discussion on the web visit https://groups.google.com/d/msgid/efibootguard-dev/20211103084039.274022-4-michael.adler%40siemens.com.
