On 30/11/2020 19:38, Jan Kawasaki wrote:
> On 30.11.20 17:31, Silvano Cirujano Cuesta wrote:
>> The tools bg_setenv and bg_printenv are mostly oriented at running systems
>> that have the partitions providing BG environments already mounted.
>>
>> This patch extends bg_printenv so that the environment can be read directly
>> from whatever valid BG environment file. This usage is very usefull in
>> development, diagnose, testing,... scenarios.
>>
>> Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
>> Signed-off-by: Jan Kiszka <[email protected]>
>> ---
>> env/env_api_fat.c | 4 +--
>> env/env_config_file.c | 24 +++++++++------
>> include/env_config_file.h | 3 +-
>> tools/bg_setenv.c | 64 ++++++++++++++++++++++++++++++++++-----
>> 4 files changed, 75 insertions(+), 20 deletions(-)
>>
>> diff --git a/env/env_api_fat.c b/env/env_api_fat.c
>> index 2b23e7f..3732050 100644
>> --- a/env/env_api_fat.c
>> +++ b/env/env_api_fat.c
>> @@ -67,7 +67,7 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env)
>> part->mountpoint);
>> }
>> FILE *config;
>> - if (!(config = open_config_file(part, "rb"))) {
>> + if (!(config = open_config_file_from_part(part, "rb"))) {
>> return false;
>> }
>> bool result = true;
>> @@ -104,7 +104,7 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
>> part->mountpoint);
>> }
>> FILE *config;
>> - if (!(config = open_config_file(part, "wb"))) {
>> + if (!(config = open_config_file_from_part(part, "wb"))) {
>> VERBOSE(stderr, "Could not open config file for writing.\n");
>> return false;
>> }
>> diff --git a/env/env_config_file.c b/env/env_config_file.c
>> index 873fe10..4e68b7f 100644
>> --- a/env/env_config_file.c
>> +++ b/env/env_config_file.c
>> @@ -18,7 +18,19 @@
>> #include "env_disk_utils.h"
>> #include "env_config_file.h"
>>
>> -FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)
>> +FILE *open_config_file(char *configfilepath, char *mode)
>> +{
>> + if (!configfilepath) {
>> + return NULL;
>> + }
>> +
>> + VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
>> + FILE *config = fopen(configfilepath, mode);
>> + free(configfilepath);
> This is a strange API. The caller should be responsible for freeing, not
> the callee.
+1 on my last refactoring I left a couple of allocations and corresponding free
spread over wrong places
>
>> + return config;
>> +}
>> +
>> +FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
>> {
>> char *configfilepath;
>>
>> @@ -27,16 +39,10 @@ FILE *open_config_file(CONFIG_PART *cfgpart, char *mode)
>> }
>> configfilepath = (char *)malloc(strlen(FAT_ENV_FILENAME) +
>> strlen(cfgpart->mountpoint) + 2);
>> - if (!configfilepath) {
>> - return NULL;
>> - }
> Bug!
+1 Good catch! No excuse for this :-)
>
>> strcpy(configfilepath, cfgpart->mountpoint);
>> strcat(configfilepath, "/");
>> strcat(configfilepath, FAT_ENV_FILENAME);
>> - VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
>> - FILE *config = fopen(configfilepath, mode);
>> - free(configfilepath);
>> - return config;
>> + return open_config_file(configfilepath, mode);
>> }
>>
>> int close_config_file(FILE *config_file_handle)
>> @@ -74,7 +80,7 @@ bool probe_config_file(CONFIG_PART *cfgpart)
>> cfgpart->devpath, cfgpart->mountpoint);
>> bool result = false;
>> FILE *config;
>> - if (!(config = open_config_file(cfgpart, "rb"))) {
>> + if (!(config = open_config_file_from_part(cfgpart, "rb"))) {
>> printf_debug(
>> "Could not open config file on partition %s.\n",
>> FAT_ENV_FILENAME);
>> diff --git a/include/env_config_file.h b/include/env_config_file.h
>> index 8fbe8fa..f333ddb 100644
>> --- a/include/env_config_file.h
>> +++ b/include/env_config_file.h
>> @@ -15,7 +15,8 @@
>> #ifndef __ENV_CONFIG_FILE_H__
>> #define __ENV_CONFIG_FILE_H__
>>
>> -FILE *open_config_file(CONFIG_PART *cfgpart, char *mode);
>> +FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);
>> +FILE *open_config_file(char *configdirpath, char *mode);
> You call it "configfilepath" in the implementation.
+1 Good catch! Left over of last refactoring.
>
>> int close_config_file(FILE *config_file_handle);
>> bool probe_config_file(CONFIG_PART *cfgpart);
>>
>> diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
>> index 24b4067..8aace0c 100644
>> --- a/tools/bg_setenv.c
>> +++ b/tools/bg_setenv.c
>> @@ -18,6 +18,7 @@
>> #include "ebgenv.h"
>> #include "uservars.h"
>> #include "version.h"
>> +#include "env_config_file.h"
>>
>> static char doc[] =
>> "bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";
>> @@ -48,12 +49,15 @@ static struct argp_option options_setenv[] = {
>> {0}};
>>
>> static struct argp_option options_printenv[] = {
>> + {"filepath", 'f', "ENVFILE", 0, "Output environment from file. Expects "
> "Read environment from file."
I don't have a strong opinion on this, so your proposal is fine for me.
>
>> + "a valid EFI Boot Guard environment
>> file."},
>> {"verbose", 'v', 0, 0, "Be verbose"},
>> {"version", 'V', 0, 0, "Print version"},
>> {0}};
>>
>> struct arguments {
>> - bool output_to_file;
>> + bool target_file;
> It's not the target file. It means "use a file", either as source or sink.
Originally I had two separate variables (input_file and output_file) and I
thought that merging them would make the code easier to understand. I didn't
come up to a better naming.
What's your proposal? Splitting them again? Another name? Which one?
>
>> + bool printenv;
>> int which_part;
>> };
>>
>> @@ -351,10 +355,17 @@ static error_t parse_opt(int key, char *arg, struct
>> argp_state *state)
>> (uint8_t *)arg, strlen(arg) + 1);
>> break;
>> case 'f':
>> - arguments->output_to_file = true;
>> - res = asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
>> - if (res == -1) {
>> - return ENOMEM;
>> + arguments->target_file = true;
>> + if (arguments->printenv) {
>> + res = asprintf(&envfilepath, "%s", arg);
>> + if (res == -1) {
>> + return ENOMEM;
>> + }
>> + } else {
>> + res = asprintf(&envfilepath, "%s/%s", arg,
>> FAT_ENV_FILENAME);
>> + if (res == -1) {
>> + return ENOMEM;
>> + }
>> }
>> break;
>> case 'c':
>> @@ -533,6 +544,32 @@ static void dump_envs(void)
>> }
>> }
>>
>> +static bool get_env(char *configfilepath, BG_ENVDATA *env)
>> +{
>> + FILE *config;
>> + char buffer[ENV_STRING_LENGTH];
>> + bool result = true;
>> +
>> + if (!(config = open_config_file(configfilepath, "rb"))) {
> We likely have cases already, but lets avoid adding more:
> Please split assignment and result evaluation into two statements.
You're right, we have more like this [1]. In fact I've simply reused the
existing code to keep the style, although I'd rather split also assignment and
comparison.
I'd then fix the other one right away to keep them consistent, if it's fine for
you.
[1]
https://github.com/siemens/efibootguard/blob/d86e13439c94c4db0641f18e025bb2055cf582e8/env/env_api_fat.c#L70
>
>> + return false;
>> + }
>> +
>> + if (!(fread(env, sizeof(BG_ENVDATA), 1, config) == 1)) {
>> + VERBOSE(stderr, "Error reading environment data from %s\n",
>> + configfilepath);
>> + if (feof(config)) {
>> + VERBOSE(stderr, "End of file encountered.\n");
>> + }
>> + result = false;
>> + }
>> +
>> + if (close_config_file(config)) {
>> + VERBOSE(stderr,
>> + "Error closing environment file after reading.\n");
>> + };
>> + return result;
>> +}
>> +
>> int main(int argc, char **argv)
>> {
>> static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};
>> @@ -557,7 +594,8 @@ int main(int argc, char **argv)
>> }
>>
>> struct arguments arguments;
>> - arguments.output_to_file = false;
>> + arguments.target_file = false;
>> + arguments.printenv = !write_mode;
>> arguments.which_part = 0;
>>
>> STAILQ_INIT(&head);
>> @@ -572,8 +610,8 @@ int main(int argc, char **argv)
>>
>> /* arguments are parsed, journal is filled */
>>
>> - /* is output to file ? */
>> - if (arguments.output_to_file) {
>> + /* is output to file or input from file ? */
>> + if (arguments.target_file && ! arguments.printenv) {
> !arguments...
What? Sorry, I don't understand what you mean here.
>
>> /* execute journal and write to file */
>> BGENV env;
>> BG_ENVDATA data;
>> @@ -606,6 +644,16 @@ int main(int argc, char **argv)
>> }
>> free(envfilepath);
>>
>> + return 0;
>> + } else if (arguments.target_file && arguments.printenv) {
>> + BG_ENVDATA env;
>> + if (!get_env(envfilepath, &env)) {
>> + fprintf(stderr, "Error reading environment file.\n");
>> + return 1;
>> + }
>> +
>> + dump_env(&env);
>> +
>> return 0;
>> }
>>
>>
> It may look better to do
>
> if (arguments.use_file) {
> if (arguments.printenv) {
> ret = printenv();
> } else {
> ret = writeenv();
> }
> return ret;
> }
>
> i.e. moving the nested bodies into local functions.
I used originally the nested conditionals, that you're using. But the resulting
diff was much harder to review than the resulting from this patch. I'd foreseen
a 2ND version and was already planned to do it there with nested conditional.
Silvano
>
> Jan
>
--
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/0dc61704-81b4-4026-9b19-b56d38d05323%40siemens.com.
