[PATCH v4 03/13] bg_setenv: Specify output path with --filepath

3 views
Skip to first unread message

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

For better usability, `bg_setenv`'s parameter -f is changed from
`--file` to `--filepath` and now accepts a path, where `BGENV.DAT`
is created so that it can directly be applied to the real environ-
ment location.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/TODO.md | 1 -
docs/USAGE.md | 12 ++++++------
tools/bg_setenv.c | 20 ++++++++++++++------
3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/docs/TODO.md b/docs/TODO.md
index 40d5ba8..832f76b 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -6,7 +6,6 @@
the current working environment to the (latest-1) environment, so
that if the current environment breaks, there is a backup with the
latest values.
- * Make `bg_setenv -f` take a path to where the `BGENV.DAT` is stored.

* Application specific variables
* applications may need to store their own variables into the
diff --git a/docs/USAGE.md b/docs/USAGE.md
index 7dd89ce..97a25cb 100644
--- a/docs/USAGE.md
+++ b/docs/USAGE.md
@@ -98,13 +98,13 @@ This step first creates a custom label contained in `EFILABEL`, which is later
used to specify the kernel location.

```
-# mount /dev/sdX2 /mnt && cd /mnt
-# echo -n "KERNEL1" | iconv -f ascii -t UTF-16LE > EFILABEL
-# bg_setenv -f -r 1 --kernel="C:KERNEL1:vmlinuz-linux" --args="root=/dev/sdX4 noinitrd"
+# mount /dev/sdX2 /mnt
+# echo -n "KERNEL1" | iconv -f ascii -t UTF-16LE > /mnt/EFILABEL
+# bg_setenv -f /mnt -r 1 --kernel="C:KERNEL1:vmlinuz-linux" --args="root=/dev/sdX4 noinitrd"
# umount /mnt
-# mount /dev/sdX3 /mnt && cd /mnt
-# echo -n "KERNEL2" | iconv -f ascii -t UTF-16LE > EFILABEL
-# bg_setenv -f -r 2 --kernel="C:KERNEL2:vmlinuz-linux" --args="root=/dev/sdX5 noinitrd"
+# mount /dev/sdX3 /mnt
+# echo -n "KERNEL2" | iconv -f ascii -t UTF-16LE > /mnt/EFILABEL
+# bg_setenv -f /mnt -r 2 --kernel="C:KERNEL2:vmlinuz-linux" --args="root=/dev/sdX5 noinitrd"
# umount /mnt
```

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 5c313cf..c006bb6 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -24,7 +24,9 @@ static struct argp_option options_setenv[] = {
"above zero is updated."},
{"revision", 'r', "REVISION", 0, "Set revision value"},
{"testing", 't', "TESTING", 0, "Set test mode for environment"},
- {"file", 'f', 0, 0, "Output environment to file"},
+ {"filepath", 'f', "ENVFILE_DIR", 0, "Output environment to file. Please "
+ "only provide an output path. The "
+ "file name is automatically appended."},
{"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
{"confirm", 'c', 0, 0, "Confirm working environment"},
{"update", 'u', 0, 0, "Automatically update oldest revision"},
@@ -58,6 +60,8 @@ static bool part_specified = false;

static bool verbosity = false;

+static char *envfilepath = NULL;
+
static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
@@ -138,6 +142,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'f':
arguments->output_to_file = true;
+ asprintf(&envfilepath, "%s/%s", arg, FAT_ENV_FILENAME);
break;
case 'c':
VERBOSE(stdout,
@@ -363,22 +368,25 @@ int main(int argc, char **argv)
if (verbosity) {
dump_env(&data);
}
- FILE *of = fopen(FAT_ENV_FILENAME, "wb");
+ FILE *of = fopen(envfilepath, "wb");
if (of) {
if (fwrite(&data, sizeof(BG_ENVDATA), 1, of) != 1) {
fprintf(stderr,
- "Error writing to output file.\n");
- result = 1;
+ "Error writing to output file: %s\n",
+ strerror(errno));
+ result = errno;
}
if (fclose(of)) {
fprintf(stderr, "Error closing output file.\n");
result = 1;
};
- printf("Output written to %s.\n", FAT_ENV_FILENAME);
+ printf("Output written to %s.\n", envfilepath);
} else {
- fprintf(stderr, "Error opening output file.\n");
+ fprintf(stderr, "Error opening output file %s (%s).\n",
+ envfilepath, strerror(errno));
result = 1;
}
+ free(envfilepath);
}

return result;
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

Per default, tests are now always compiled.

To run tests, with the main Makefile, issue
* 'make check'.

The Makefile in `tools/tests` is changed so that:
* `make all` and `make build-tests`: builds the tests
* `make run-tests`: runs the tests

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 12 +++++++++++-
docs/COMPILE.md | 13 ++++++++++---
tools/tests/Makefile | 18 ++++++++++++++----
3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index c6ebec4..cb8e238 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -190,6 +190,16 @@ bg_printenvdir = $(top_srcdir)
bg_printenv: $(bg_setenv)
$(LN_S) -f bg_setenv bg_printenv

-all-local: bg_printenv
+all-local: bg_printenv build_tests
+
+build_tests:
+ make -C tools/tests build-tests
+
+.PHONY: check
+check:
+ make -C tools/tests run-tests
+
+clean-local:
+ make -C tools/tests clean

CLEANFILES += bg_printenv
diff --git a/docs/COMPILE.md b/docs/COMPILE.md
index 51c2008..33d5604 100644
--- a/docs/COMPILE.md
+++ b/docs/COMPILE.md
@@ -5,13 +5,13 @@
### Arch Linux ###

```
-pacman -S gnu-efi-libs pciutils
+pacman -S gnu-efi-libs pciutils cmocka
```

### Debian 8 ###

```
-apt-get install gnu-efi libpci-dev
+apt-get install gnu-efi libpci-dev libcmocka-dev
```

## Compilation ##
@@ -28,6 +28,13 @@ autoreconf -fi
make
```

+This will also automatically compile internal tests for the environment API.
+
+To run the tests, call
+```
+make check
+```
+
To cross-compile, the environment variables must be set accordingly, i.e.
`CXX=<compiler-to-use>`. The following example shows how to specify needed
paths for an out-of-tree build, where cross-compilation environment variables
@@ -41,7 +48,7 @@ autoreconf -fi ..
--with-gnuefi-sys-dir=<sys-root-dir> \
--with-gnuefi-include-dir=<sys-root-dir>/usr/include/efi \
--with-gnuefi-lds-dir=<sys-root-dir>/usr/lib \
- --with-gnuefi-lib-dir=<sys-root-dir>/usr/lib
+ --with-gnuefi-lib-dir=<sys-root-dir>/usr/lib \
make
```

diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index d52deaa..da12a61 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -78,18 +78,28 @@ define WEAKEN_SYMBOLS =
$(foreach symbol,$(MOCKOBJS_SYMBOLS_$(1)-$(2)),$(call WEAKEN_SYMBOL,$(symbol),$(1).o))
endef

-define TEST_TARGET_TEMPLATE =
+define TEST_TARGET_COMPILE_TEMPLATE =
$(1): $$(OBJS_$(1:.target=))
$(foreach mockobj,$(MOCKOBJS_$(1:.target=)),$(call WEAKEN_SYMBOLS,$(mockobj),$(1:.target=)))
$(CC) $$(OBJS_$(1:.target=):O=o) -o $(1:.target=) $(LIBS)
+endef
+
+define TEST_TARGET_RUN_TEMPLATE =
+$(1:.target=.run): $$(OBJS_$(1:.target=))
./$(1:.target=)
endef

-.PHONY: clean all $(TEST_TARGETS)
+.PHONY: clean all $(TEST_TARGETS) build-tests run-tests
+
+all: build-tests
+
+build-tests: $(TEST_TARGETS)
+
+$(foreach test,$(TEST_TARGETS),$(eval $(call TEST_TARGET_COMPILE_TEMPLATE,$(test))))

-all: $(TEST_TARGETS)
+run-tests: $(TEST_TARGETS:.target=.run)

-$(foreach test,$(TEST_TARGETS),$(eval $(call TEST_TARGET_TEMPLATE,$(test))))
+$(foreach test,$(TEST_TARGETS),$(eval $(call TEST_TARGET_RUN_TEMPLATE,$(test))))

# Search for source files in current and parent directory
%.O: %.c
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

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

The value can be specified as number or as string when
using the `bg_setenv` utility. Also, an enum is added for
the API code.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/TODO.md | 9 --
docs/TOOLS.md | 9 +-
docs/UPDATE.md | 65 +++++-----
env/fatvars.c | 37 +++---
include/envdata.h | 13 +-
swupdate-adapter/ebgenv.c | 63 +++-------
swupdate-adapter/swupdate.md | 290 -------------------------------------------
tools/bg_setenv.c | 78 +++++++-----
tools/tests/test_api.c | 24 ++--
9 files changed, 138 insertions(+), 450 deletions(-)
delete mode 100644 swupdate-adapter/swupdate.md

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..9883589 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,15 @@ 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=FAILED --revision=0
```

+*NOTE*: The user can either specify a number after `--ustate=` or a string resembling
+the value.
+
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=TESTING
```
diff --git a/docs/UPDATE.md b/docs/UPDATE.md
index 169fb9d..3c84ce4 100644
--- a/docs/UPDATE.md
+++ b/docs/UPDATE.md
@@ -11,8 +11,8 @@ 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;
+ uint8_t padding;
+ uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
uint32_t crc32;
@@ -22,8 +22,10 @@ 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
+* `padding`: Padding byte to stay compatible with the offsets of the previous
+ version.
+* `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 +37,26 @@ 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
-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.
+configuration with a `revision` of `5`, with `ustate` set to `"INSTALLED"`
+(`1`). On the next reboot, `efibootguard` loads the configuration with the
+highest `revision` number. If it detects, that `ustate` is `"INSTALLED"` `(1)`,
+it will update the value to `"TESTING"` `(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 `"OK"` (`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 `"TESTING"` (`2`). Thus, it will load the
+2nd latest configuration instead. The failed update is indicated by the
+revision of the failed configuration set to `"OK"` (`0`) with `ustate` set to
+`"FAILED"` (`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,37 +65,33 @@ corresponding configuration is booted again in future.
| || |
| Rev: latest || Rev: oldest |
+--------> | (working) || |
- | | || |
| +--------------++--------------+
| |
| | update
| v
- | +---------------++--------------+
- | | || |
- | | Rev: latest-1 || Rev: latest |
- | | (working) || testing: 1 |
- | | || boot_once: 0 |
- | +---------------++--------------+
+ | +---------------++-----------------------+
+ | | || |
+ | | Rev: latest-1 || Rev: latest |
+ | | (working) || ustate: INSTALLED (1) |
+ | +---------------++-----------------------+
| |
| | reboot
| v
- | +---------------++--------------+
- | | || |
- | | Rev: latest-1 || Rev: latest |
- | | (working) || testing: 1 |
- | | || boot_once: 1 |
- | +---------------++--------------+
+ | +---------------++---------------------+
+ | | || |
+ | | Rev: latest-1 || Rev: latest |
+ | | (working) || ustate: TESTING (2) |
+ | +---------------++---------------------+
| | no
| success? ---------------------+
| | watchdog reboot |
| yes, confirm | |
| v v
- | +----------++--------------+ +----------++--------------+
- | | || | | || |
- | | Rev: || Rev: latest | | Rev: || Rev: 0 |
- | | latest-1 || testing: 0 | | latest-1 || testing: 1 |
- | | || boot_once: 0 | | || boot_once: 1 |
- | +----------++--------------+ +----------++--------------+
+ | +----------++---------------+ +----------++--------------------+
+ | | || | | || |
+ | | Rev: || Rev: latest | | Rev: || Rev: 0 |
+ | | latest-1 || ustate: OK (0)| | latest-1 || ustate: FAILED (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..78dda9b 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -18,13 +18,22 @@

#define CONFIG_PARTITION_MAXCOUNT 64

+#define USTATE_OK 0
+#define USTATE_INSTALLED 1
+#define USTATE_TESTING 2
+#define USTATE_FAILED 3
+#define USTATE_UNKNOWN 4
+
+#define USTATE_MIN 0
+#define USTATE_MAX 4
+
#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;
+ uint8_t padding;
+ uint8_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..115e0a9 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 = USTATE_INSTALLED;
/* 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 == USTATE_FAILED) {
(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 == USTATE_FAILED) {
+ env->data->ustate = USTATE_OK;
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 == USTATE_OK) {
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 == USTATE_INSTALLED) {
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 == USTATE_TESTING) {
res = true;
}
bgenv_close(env);
diff --git a/swupdate-adapter/swupdate.md b/swupdate-adapter/swupdate.md
deleted file mode 100644
index 7df0c43..0000000
--- a/swupdate-adapter/swupdate.md
+++ /dev/null
@@ -1,290 +0,0 @@
-# Update process with swupdate #
-
-
-## Update state mapping ##
-
-Swupdate-Suricatta works with an internal state variable, which is called `ustate`
-per default.
-
-The values of common interest are:
-
-ustate | meaning
---------|-----------------------------------
-0 | nothing to do
-1 | update installed (reboot pending)
-2 | update testing (after reboot)
-3 | update failed
-4 | state not available
-
-`efibootguard` works with three internal variables regarding the update mechanism:
-
-* `revision`
-* `testing`
-* `boot_once`
-
-The values of these variables are mapped onto ustate according to the following matrix:
-
-*Note*: A failed revision exists, if its `revision` is `0` and at the same time,
-both `boot_once` and `testing` are set to `1`. If such a revision exists in any of
-the stored environment partitions, this is marked as [FAILED] in the matrix below.
-
-`efibootguard` | `suricatta`
----------------------------------------------------------------------------|------------
- (current env.)<br />testing = 0<br />boot_once = 0<br /><br />NOT [FAILED]| ustate = 0
- (current env.)<br />testing = 1<br />boot_once = 0<br /><br />NOT [FAILED]| ustate = 1
- (current env.)<br />testing = 1<br />boot_once = 1<br /><br />NOT [FAILED]| ustate = 2
- [FAILED] | ustate = 3
- Environment<br />Error | ustate = 4
-
-
-## Update state mapping with API functions ##
-
-1. Call `ebg_env_open_current`, which will initialize the configuration environment.
-
-2. Use the following logic
-
-```
-ebg_env_isupdatesuccessful() is false?
- ustate = 3
-else
- ebg_env_isokay() is true?
- ustate = 0
-
- ebg_env_isinstalled() is true?
- ustate = 1
-
- ebg_env_istesting() is true?
- ustate = 2
-
-```
-
-3. call `ebg_env_close()`
-
-
-## Detailed example ##
-
-Test environment: 2 config partitions, FAT16, GPT
-
-**Initial suricatta state: OK**
-
-raw data:
-
-
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1Values:
-revision: 14
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-```
-
-### Installation of Update ###
-
-Used sw-description:
-
-```
-software =
-{
- version = "0.1.0";
- bootenv: (
- {
- name = "testing";
- value = "1";
- },
- {
- name = "kernelfile";
- value = "L:CONFIG1:vmlinuz-linux";
- },
- {
- name = "kernelparams";
- value = "root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset";
- }
- );
-}
-```
-
-Command to generate swupdate cpio payload:
-
-```
-echo -n "sw-description" | cpio --format=crc -o > test.swu
-```
-
-
-Command with block dev access to update efibootguard environment:
-
-```
-swupdate -v -i test.swu
-```
-
-
-#### Resulting environment ####
-
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1 Values:
-revision: 16
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: enabled
-boot once flag: not set
-```
-
-**suricatta state: INSTALLED**
-
-Test conditions:
-* ebg_env_isupdatesuccessful == TRUE (since no revision is 0)
-* testing == 1 && boot_once == 0
-
-Function to retrieve state: `ebg_env_isinstalled()`
-
-### Rebooting ###
-
-efibootguard will detect the `testing` flag and set `boot_once`. This can be
-simulated by
-
-```
-bg_setenv -p X -b
-```
-
-where `X` is the 0-based index of the config partition to be updated. This sets
-the `boot_once` flag.
-
-#### Resulting Environment ####
-
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1 Values:
-revision: 16
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: enabled
-boot once flag: set
-```
-
-**suricatta state: TESTING**
-
-Test conditions:
-* ebg_env_isupdatesuccessful == TRUE (since no revision is 0)
-* testing == 1 && boot_once == 1
-
-Function to retrieve state: `ebg_env_istesting()`
-
-### Confirming working update ###
-
-```
-bg_setenv -c
-```
-
-#### Resulting environment ####
-
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1 Values:
-revision: 16
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-```
-
-**suricatta state: OK**
-
-Test conditions:
-* ebg_env_isupdatesuccessful == TRUE (since no revision is 0)
-* testing == 0
-
-Function to retrieve state: `ebg_env_isokay()`
-
-### Not confirming and rebooting ###
-
-After rebooting with state == INSTALLED and not confirming,
-the resulting environment is:
-
-#### Resulting environment ####
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1 Values:
-revision: 0
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: enabled
-boot once flag: set
-```
-
-**suricatta state: FAILED**
-
-Test conditions:
-* ebg_env_isupdatesuccessful == FALSE (since a revision is 0 and both flags set
- in this config)
-
-### Manually resetting failure state ###
-
-```
-bg_setenv -u -t 0
-```
-*NOTE: The -u option takes all values from the current environment,
-replaces the oldest environment with these and then updates the
-newly set values (-t 0).*
-
-#### Resulting environment ####
-
-```
- Config Partition #0 Values:
-revision: 15
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-
- Config Partition #1 Values:
-revision: 16
-kernel: L:CONFIG1:vmlinuz-linux
-kernelargs: root=/dev/sda4 rw initrd=initramfs-linux.img nomodeset
-watchdog timeout: 30 seconds
-test flag: disabled
-boot once flag: not set
-```
-
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index c1975f6..e678188 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[] = {
@@ -62,6 +61,30 @@ static bool verbosity = false;

static char *envfilepath = NULL;

+static char *ustatemap[] = {"OK", "INSTALLED", "TESTING", "FAILED", "UNKNOWN"};
+
+static uint8_t str2ustate(char *str)
+{
+ uint8_t i;
+
+ if (!str) {
+ return USTATE_UNKNOWN;
+ }
+ for (i = USTATE_MIN; i < USTATE_MAX; i++) {
+ if (strncasecmp(str, ustatemap[i], strlen(ustatemap[i])) == 0) {
+ return i;
+ }
+ }
+ return USTATE_UNKNOWN;
+}
+
+static char *ustate2str(uint8_t ustate)
+{
+ if (ustate >= USTATE_MIN && ustate <= USTATE_MAX) {
+ return ustatemap[ustate];
+ }
+}
+
static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
@@ -110,18 +133,29 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
return 1;
}
break;
- case 't':
- i = atoi(arg);
- if (i == 0 || i == 1) {
- if (arguments->tmpdata.testing = (bool)i) {
- VERBOSE(stdout, "Testing mode enabled.\n");
+ case 's':
+ i = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (i == LONG_MAX || i == LONG_MIN)) ||
+ (errno != 0 && i == 0) || (tmp == arg)) {
+ // maybe user specified an enum string
+ i = str2ustate(arg);
+ if (i == USTATE_UNKNOWN) {
+ fprintf(stderr, "Invalid state specified.\n");
+ return 1;
}
- } else {
+ }
+ if (i < 0 || i > 3) {
fprintf(
stderr,
- "Invalid testing flag specified. Possible values: "
- "0 (disabled), 1 (enabled)\n");
+ "Invalid ustate value specified. Possible values: "
+ "0 (%s), 1 (%s), 2 (%s), 3 (%s)\n",
+ ustatemap[0], ustatemap[1], ustatemap[2],
+ ustatemap[3]);
return 1;
+ } else {
+ arguments->tmpdata.ustate = i;
+ VERBOSE(stdout, "Ustate set to %u (%s).\n", i,
+ ustate2str(i));
}
break;
case 'r':
@@ -148,8 +182,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 +200,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 +224,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 +237,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 +249,8 @@ 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 (%s)\n", (uint8_t)env->ustate,
+ ustate2str(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.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

For API refactoring, storage backend should be configurable.
Add --with-env-backend configure option to specify the c-file
with the backend implementation.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 4 ++--
configure.ac | 11 ++++++++++-
env/env_api_fat.c | 1 +
include/env_api.h | 4 +---
tools/tests/Makefile | 1 +
tools/tests/test_partitions.c | 1 +
6 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index c3908c0..a740b70 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,7 +42,7 @@ CLEANFILES =
lib_LIBRARIES = libebgenv.a libenv_api.a

libebgenv_a_SOURCES = \
- env/env_api_fat.c \
+ env/@env_api_file@.c \
tools/ebgpart.c \
swupdate-adapter/ebgenv.c

@@ -55,7 +55,7 @@ libebgenv_a_CFLAGS = \

libenv_api_a_SOURCES = \
tools/ebgpart.c \
- env/env_api_fat.c
+ env/@env_api_file@.c

libenv_api_a_CFLAGS = \
$(AM_CFLAGS)
diff --git a/configure.ac b/configure.ac
index 80dd365..aa2d423 100644
--- a/configure.ac
+++ b/configure.ac
@@ -134,6 +134,13 @@ AC_TYPE_UINT16_T
AC_TYPE_UINT32_T
AC_TYPE_UINT8_T

+AC_ARG_WITH([env-backend],
+ AS_HELP_STRING([--with-env-backend=STRING],
+ [define the environment backend, defaults to "env_api_fat"]),
+ [ ENV_API_FILE="$withval" ],
+ [ ENV_API_FILE="env_api_fat" ])
+
+AC_SUBST([env_api_file], [${ENV_API_FILE}])
# ------------------------------------------------------------------------------
AC_CONFIG_FILES([
Makefile
@@ -147,11 +154,13 @@ AC_MSG_RESULT([
machine type: $MACHINE_TYPE_NAME

prefix: ${prefix}
- exec_prefix: ${exec_prefix}
+ exec_prefix: ${exec_prefix}
libexecdir: ${libexecdir}
libdir: ${libdir}

efi libs: ${GNUEFI_LIBS}
efi lds: ${GNUEFI_LDS_DIR}
+
+ environment backend: ${ENV_API_FILE}.c
])

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index f282a3f..93b85a1 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -11,6 +11,7 @@
*/

#include "env_api.h"
+#include "ebgpart.h"
#include "test-interface.h"

const char *tmp_mnt_dir = "/tmp/mnt-XXXXXX";
diff --git a/include/env_api.h b/include/env_api.h
index 70727e9..83c7123 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -26,9 +26,7 @@
#include <fcntl.h>
#include <sys/file.h>
#include <sys/mount.h>
-
-#include "ebgpart.h"
-
+#include "config.h"
#include <zlib.h>
#include "envdata.h"

diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index f1fda2e..994dd59 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -22,6 +22,7 @@ INCLUDE ?= /usr/include
CFLAGS = \
-I$(PROJECTDIR) \
-I$(PROJECTDIR)/.. \
+ -I$(PROJECTDIR)/../.. \
-I$(PROJECTDIR)/../../include \
-I$(PROJECTDIR)/../../swupdate-adapter \
-std=gnu99 \
diff --git a/tools/tests/test_partitions.c b/tools/tests/test_partitions.c
index c6865e4..4387ecd 100644
--- a/tools/tests/test_partitions.c
+++ b/tools/tests/test_partitions.c
@@ -17,6 +17,7 @@
#include <setjmp.h>
#include <cmocka.h>
#include "env_api.h"
+#include "ebgpart.h"
#include "test-interface.h"

static PedDevice ped_devices[32] = {0};
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

In this patch series the following is adressed:

* Some Makefile changes / fixes

* Improve `bg_setenv`'s usability and internal workflow

* Improve API structure & code (file names, avoid global variables,
remove unneeded lib, increase extensibility, maintainability and
usability, add functionality for user variables)
State mapping is removed and the API works with `ustate` itself,
as does the environment.

* Tests are updated

* Update documentation

Further comments: docs/TODO is removed and content should be
put into github issues.
Another patch regarding code security bugs and type-casting
issues will follow after this series is applied.

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

Andreas Reichel (1):
Delete docs/TODO.md and use github issues instead

Reichel Andreas (12):
Makefile.am: Bugfix for bg_printenv symlink
tests: Build tests always, run tests on 'make check'
bg_setenv: Specify output path with --filepath
env: Rename environment API files
env: Make backend for environment storage configurable
env: Make number of config partitions configurable
ustate: Refactor state variable
swupdate-adapter: Refactor API
Combine libraries to one API
env_api_fat: Fix problem with crc32 testing
env: Add user variables
bg_setenv: Use task list, refactor code

Makefile.am | 40 +--
configure.ac | 43 ++-
docs/API.md | 22 +-
docs/COMPILE.md | 13 +-
docs/TODO.md | 37 ---
docs/TOOLS.md | 19 +-
docs/UPDATE.md | 65 +++--
docs/USAGE.md | 12 +-
env/env_api.c | 220 +++++++++++++++
tools/bg_utils.c => env/env_api_fat.c | 380 +++++++++++++++++--------
env/fatvars.c | 54 ++--
env/uservars.c | 266 +++++++++++++++++
include/ebgdefs.h | 20 --
include/ebgenv.h | 116 ++++++++
{tools => include}/ebgpart.h | 0
tools/bg_utils.h => include/env_api.h | 43 ++-
include/envdata.h | 17 +-
{tools => include}/test-interface.h | 2 +-
include/uservars.h | 37 +++
swupdate-adapter/ebgenv.c | 458 ------------------------------
swupdate-adapter/ebgenv.h | 90 ------
swupdate-adapter/swupdate.md | 290 -------------------
tools/bg_setenv.c | 518 ++++++++++++++++++++++------------
tools/tests/Makefile | 43 +--
tools/tests/test_api.c | 262 ++++++++++++-----
tools/tests/test_environment.c | 7 +-
tools/tests/test_partitions.c | 42 +--
27 files changed, 1683 insertions(+), 1433 deletions(-)
delete mode 100644 docs/TODO.md
create mode 100644 env/env_api.c
rename tools/bg_utils.c => env/env_api_fat.c (54%)
create mode 100644 env/uservars.c
delete mode 100644 include/ebgdefs.h
create mode 100644 include/ebgenv.h
rename {tools => include}/ebgpart.h (100%)
rename tools/bg_utils.h => include/env_api.h (60%)
rename {tools => include}/test-interface.h (96%)
create mode 100644 include/uservars.h
delete mode 100644 swupdate-adapter/ebgenv.c
delete mode 100644 swupdate-adapter/ebgenv.h
delete mode 100644 swupdate-adapter/swupdate.md

--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

Add --with-num-config-parts=XX to define number of config partitions
at configuration.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
configure.ac | 15 +++++++++++++++
docs/TODO.md | 4 ----
env/env_api_fat.c | 22 +++++++++++-----------
env/fatvars.c | 16 +++++++---------
include/envdata.h | 1 -
swupdate-adapter/ebgenv.c | 4 ++--
tools/bg_setenv.c | 2 +-
tools/tests/test_environment.c | 4 ++--
tools/tests/test_partitions.c | 38 +++++++++++---------------------------
9 files changed, 49 insertions(+), 57 deletions(-)

diff --git a/configure.ac b/configure.ac
index aa2d423..219c419 100644
--- a/configure.ac
+++ b/configure.ac
@@ -141,6 +141,20 @@ AC_ARG_WITH([env-backend],
[ ENV_API_FILE="env_api_fat" ])

AC_SUBST([env_api_file], [${ENV_API_FILE}])
+
+AC_ARG_WITH([num-config-parts],
+ AS_HELP_STRING([--with-num-config-parts=INT],
+ [specify the number of used config partitions, defaults to 2]),
+ [
+ ENV_NUM_CONFIG_PARTS=${withval:-0}
+ AS_IF([test "${ENV_NUM_CONFIG_PARTS}" -lt "1"],
+ [
+ AC_MSG_ERROR([Invalid number of config partitions.])
+ ])
+ ],
+ [ ENV_NUM_CONFIG_PARTS=2 ])
+
+AC_DEFINE_UNQUOTED([ENV_NUM_CONFIG_PARTS], [${ENV_NUM_CONFIG_PARTS}], [Number of config partitions])
# ------------------------------------------------------------------------------
AC_CONFIG_FILES([
Makefile
@@ -162,5 +176,6 @@ AC_MSG_RESULT([
efi lds: ${GNUEFI_LDS_DIR}

environment backend: ${ENV_API_FILE}.c
+ number of config parts: $ENV_NUM_CONFIG_PARTS
])

diff --git a/docs/TODO.md b/docs/TODO.md
index 832f76b..36c5dfb 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -29,8 +29,4 @@
needed then.
* Function / Datatype / Variable names remind of Parted and should be
renamed if code developes independent of libparted.
- * The number of valid config partitions expected by the bootloader and
- the tools is currently fixed to the number defined by
- `CONFIG_PARTITION_COUNT` in `include/envdata.h`. This value should be
- made configurable by a config flag.

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 93b85a1..bed74d9 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -254,11 +254,11 @@ bool probe_config_partitions(CONFIG_PART *cfgpart)
strlen(devpath) + 1);
if (probe_config_file(&cfgpart[count])) {
printf_debug("%s", "Environment file found.\n");
- if (count >= CONFIG_PARTITION_COUNT) {
+ if (count >= ENV_NUM_CONFIG_PARTS) {
VERBOSE(stderr, "Error, there are "
"more than %d config "
"partitions.\n",
- CONFIG_PARTITION_COUNT);
+ ENV_NUM_CONFIG_PARTS);
return false;
}
count++;
@@ -266,10 +266,10 @@ bool probe_config_partitions(CONFIG_PART *cfgpart)
part = ped_disk_next_partition(pd, part);
}
}
- if (count < CONFIG_PARTITION_COUNT) {
+ if (count < ENV_NUM_CONFIG_PARTS) {
VERBOSE(stderr,
"Error, less than %d config partitions exist.\n",
- CONFIG_PARTITION_COUNT);
+ ENV_NUM_CONFIG_PARTS);
return false;
}
return true;
@@ -348,21 +348,21 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
return result;
}

-CONFIG_PART config_parts[CONFIG_PARTITION_COUNT];
-BG_ENVDATA oldenvs[CONFIG_PARTITION_COUNT];
+CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
+BG_ENVDATA oldenvs[ENV_NUM_CONFIG_PARTS];

bool bgenv_init(BGENVTYPE type)
{
switch (type) {
case BGENVTYPE_FAT:
memset((void *)&config_parts, 0,
- sizeof(CONFIG_PART) * CONFIG_PARTITION_COUNT);
+ sizeof(CONFIG_PART) * ENV_NUM_CONFIG_PARTS);
/* enumerate all config partitions */
if (!probe_config_partitions(config_parts)) {
VERBOSE(stderr, "Error finding config partitions.\n");
return false;
}
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
read_env(&config_parts[i], &oldenvs[i]);
uint32_t sum = crc32(0, (Bytef *)&oldenvs[i],
sizeof(BG_ENVDATA) - sizeof(oldenvs[i].crc32));
@@ -383,7 +383,7 @@ BGENV *bgenv_get_by_index(BGENVTYPE type, uint32_t index)
switch (type) {
case BGENVTYPE_FAT:
/* get config partition by index and allocate handle */
- if (index >= CONFIG_PARTITION_COUNT) {
+ if (index >= ENV_NUM_CONFIG_PARTS) {
return NULL;
}
if (!(handle = calloc(1, sizeof(BGENV)))) {
@@ -404,7 +404,7 @@ BGENV *bgenv_get_oldest(BGENVTYPE type)

switch (type) {
case BGENVTYPE_FAT:
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
if (oldenvs[i].revision < minrev) {
minrev = oldenvs[i].revision;
min_idx = i;
@@ -422,7 +422,7 @@ BGENV *bgenv_get_latest(BGENVTYPE type)

switch (type) {
case BGENVTYPE_FAT:
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
if (oldenvs[i].revision > maxrev) {
maxrev = oldenvs[i].revision;
max_idx = i;
diff --git a/env/fatvars.c b/env/fatvars.c
index 9d63091..406bdf9 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -15,10 +15,8 @@
#include <syspart.h>
#include <envdata.h>

-#define CONFIG_PARTITION_COUNT 2
-
static int current_partition = 0;
-static BG_ENVDATA env[CONFIG_PARTITION_COUNT];
+static BG_ENVDATA env[ENV_NUM_CONFIG_PARTS];

BG_STATUS save_current_config(void)
{
@@ -41,10 +39,10 @@ BG_STATUS save_current_config(void)
return BG_CONFIG_ERROR;
}

- if (numHandles != CONFIG_PARTITION_COUNT) {
+ if (numHandles != ENV_NUM_CONFIG_PARTS) {
Print(L"Error, unexpected number of config partitions: found "
L"%d, but expected %d.\n",
- numHandles, CONFIG_PARTITION_COUNT);
+ numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */
mfree(roots);
@@ -90,7 +88,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
EFI_FILE_HANDLE *roots;
UINTN i;
- int env_invalid[CONFIG_PARTITION_COUNT] = {0};
+ int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
CONFIG_PARTITION_MAXCOUNT);
@@ -106,10 +104,10 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
return BG_CONFIG_ERROR;
}

- if (numHandles != CONFIG_PARTITION_COUNT) {
+ if (numHandles != ENV_NUM_CONFIG_PARTS) {
Print(L"Warning, unexpected number of config partitions: found "
L"%d, but expected %d.\n",
- numHandles, CONFIG_PARTITION_COUNT);
+ numHandles, ENV_NUM_CONFIG_PARTS);
/* Don't treat this as error because we may still be able to
* find a
* valid config */
@@ -174,7 +172,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
* configuration. */
UINTN latest_rev = 0, latest_idx = 0;
UINTN pre_latest_rev = 0, pre_latest_idx = 0;
- for (i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
if (!env_invalid[i]) {
if (env[i].revision > latest_rev) {
pre_latest_rev = latest_rev;
diff --git a/include/envdata.h b/include/envdata.h
index f62ce35..f5b8070 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -16,7 +16,6 @@
#define FAT_ENV_FILENAME "BGENV.DAT"
#define ENV_STRING_LENGTH 255

-#define CONFIG_PARTITION_COUNT 2
#define CONFIG_PARTITION_MAXCOUNT 64

#pragma pack(push)
diff --git a/swupdate-adapter/ebgenv.c b/swupdate-adapter/ebgenv.c
index 9ac42fc..d5f7080 100644
--- a/swupdate-adapter/ebgenv.c
+++ b/swupdate-adapter/ebgenv.c
@@ -327,7 +327,7 @@ int ebg_env_set(char *key, char *value)
bool ebg_env_isupdatesuccessful(void)
{
/* find all environments with revision 0 */
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
BGENV *env = bgenv_get_by_index(BGENVTYPE_FAT, i);

if (!env) {
@@ -348,7 +348,7 @@ bool ebg_env_isupdatesuccessful(void)

int ebg_env_clearerrorstate(void)
{
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
BGENV *env = bgenv_get_by_index(BGENVTYPE_FAT, i);

if (!env) {
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 817d832..c1975f6 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -264,7 +264,7 @@ int main(int argc, char **argv)
"Error initializing FAT environment.\n");
return 1;
}
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
if (verbosity) {
printf("\n----------------------------\n");
printf(" Config Partition #%d ", i);
diff --git a/tools/tests/test_environment.c b/tools/tests/test_environment.c
index 63df653..7402121 100644
--- a/tools/tests/test_environment.c
+++ b/tools/tests/test_environment.c
@@ -22,8 +22,8 @@

/* Mock functions from libparted */

-CONFIG_PART config_parts[CONFIG_PARTITION_COUNT];
-BG_ENVDATA oldenvs[CONFIG_PARTITION_COUNT];
+CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
+BG_ENVDATA oldenvs[ENV_NUM_CONFIG_PARTS];

FILE test_file;

diff --git a/tools/tests/test_partitions.c b/tools/tests/test_partitions.c
index 4387ecd..237b01e 100644
--- a/tools/tests/test_partitions.c
+++ b/tools/tests/test_partitions.c
@@ -96,37 +96,22 @@ static void test_partition_count(void **state)
bool ret;

num_simulated_devices = 1;
- num_simulated_partitions_per_disk = CONFIG_PARTITION_COUNT;
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ num_simulated_partitions_per_disk = ENV_NUM_CONFIG_PARTS;
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
will_return(probe_config_file, true);
}
ret = probe_config_partitions(cfgparts);
assert_true(ret);

- num_simulated_devices = CONFIG_PARTITION_COUNT;
+ num_simulated_devices = ENV_NUM_CONFIG_PARTS;
num_simulated_partitions_per_disk = 1;
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS - 1; i++) {
will_return(probe_config_file, true);
}
+ will_return(probe_config_file, true);
ret = probe_config_partitions(cfgparts);
assert_true(ret);

- num_simulated_devices = 1;
- num_simulated_partitions_per_disk = CONFIG_PARTITION_COUNT - 1;
- for (int i = 0; i < CONFIG_PARTITION_COUNT - 1; i++) {
- will_return(probe_config_file, true);
- }
- ret = probe_config_partitions(cfgparts);
- assert_false(ret);
-
- num_simulated_devices = 1;
- num_simulated_partitions_per_disk = CONFIG_PARTITION_COUNT + 1;
- for (int i = 0; i < CONFIG_PARTITION_COUNT + 1; i++) {
- will_return(probe_config_file, true);
- }
- ret = probe_config_partitions(cfgparts);
- assert_false(ret);
-
(void)state;
}

@@ -136,19 +121,18 @@ static void test_config_file_existence(void **state)
bool ret;

num_simulated_devices = 1;
- num_simulated_partitions_per_disk = CONFIG_PARTITION_COUNT;
- for (int i = 0; i < CONFIG_PARTITION_COUNT; i++) {
+ num_simulated_partitions_per_disk = ENV_NUM_CONFIG_PARTS;
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
will_return(probe_config_file, false);
}
ret = probe_config_partitions(cfgparts);
assert_false(ret);

- if (CONFIG_PARTITION_COUNT > 1) {
- for (int i = 0; i < CONFIG_PARTITION_COUNT - 1; i++) {
- will_return(probe_config_file, true);
- }
- will_return(probe_config_file, false);
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS - 1; i++) {
+ will_return(probe_config_file, true);
}
+ will_return(probe_config_file, false);
+
ret = probe_config_partitions(cfgparts);
assert_false(ret);

--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

Create symlink with -f option to avoid error if it already
exists.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 6038f30..c6ebec4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -84,7 +84,7 @@ bg_setenv_DEPENDENCIES = \
libbg_utils.a

install-exec-hook:
- $(LN_S) bg_setenv$(EXEEXT) \
+ $(LN_S) -f bg_setenv$(EXEEXT) \
$(DESTDIR)$(bindir)/bg_printenv$(EXEEXT)

#
@@ -185,10 +185,11 @@ $(efi_loadername): $(efi_solib)
-j .dynsym -j .rel -j .rela -j .reloc -j .init_array \
--target=efi-app-$(ARCH) $< $@

-bg_printenv_DATA = bg_printenv
bg_printenvdir = $(top_srcdir)

bg_printenv: $(bg_setenv)
- $(LN_S) bg_setenv bg_printenv
+ $(LN_S) -f bg_setenv bg_printenv
+
+all-local: bg_printenv

CLEANFILES += bg_printenv
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

There is no need for two different static libraries, hence these
two are combined to one `libebgenv.a`.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 20 +++++---------
docs/API.md | 2 +-
docs/TODO.md | 4 ---
swupdate-adapter/ebgenv.c => env/env_api.c | 42 +++++++++++++++++++++++++++++-
env/env_api_fat.c | 42 +-----------------------------
{swupdate-adapter => include}/ebgenv.h | 0
include/env_api.h | 2 +-
tools/bg_setenv.c | 2 +-
tools/tests/Makefile | 7 +----
tools/tests/test_environment.c | 1 +
tools/tests/test_partitions.c | 1 +
11 files changed, 54 insertions(+), 69 deletions(-)
rename swupdate-adapter/ebgenv.c => env/env_api.c (76%)
rename {swupdate-adapter => include}/ebgenv.h (100%)

diff --git a/Makefile.am b/Makefile.am
index a740b70..bbef557 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -39,12 +39,12 @@ CLEANFILES =
#
# Static libraries
#
-lib_LIBRARIES = libebgenv.a libenv_api.a
+lib_LIBRARIES = libebgenv.a

libebgenv_a_SOURCES = \
env/@env_api_file@.c \
- tools/ebgpart.c \
- swupdate-adapter/ebgenv.c
+ env/env_api.c \
+ tools/ebgpart.c

libebgenv_a_CPPFLAGS = \
$(AM_CPPFLAGS) \
@@ -53,16 +53,8 @@ libebgenv_a_CPPFLAGS = \
libebgenv_a_CFLAGS = \
$(AM_CFLAGS)

-libenv_api_a_SOURCES = \
- tools/ebgpart.c \
- env/@env_api_file@.c
-
-libenv_api_a_CFLAGS = \
- $(AM_CFLAGS)
-
pkginclude_HEADERS = \
- swupdate-adapter/ebgenv.h \
- include/env_api.h
+ include/ebgenv.h

#
# bg_setenv binary
@@ -77,11 +69,11 @@ bg_setenv_CFLAGS = \
$(AM_CFLAGS)

bg_setenv_LDADD = \
- -lenv_api \
+ -lebgenv \
-lz

bg_setenv_DEPENDENCIES = \
- libenv_api.a
+ libebgenv.a

install-exec-hook:
$(LN_S) -f bg_setenv$(EXEEXT) \
diff --git a/docs/API.md b/docs/API.md
index 3da00b3..1793581 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -6,7 +6,7 @@ The library `libebgenv.a` provides an API to access the environment from a
user space program.

The header file with the interface definitions is
-[/swupdate-adapter/ebgenv.h](../swupdate-adapter/ebgenv.h).
+[/include/ebgenv.h](../include/ebgenv.h).

The interface provides functions to:
* enable/disable for output to stdout and stderr
diff --git a/docs/TODO.md b/docs/TODO.md
index e5f4b3e..29b7fe0 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -14,10 +14,6 @@
key-value pairs.

* 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
- will be simplified as well. It is possible, that only one API is
- needed then.
* Function / Datatype / Variable names remind of Parted and should be
renamed if code developes independent of libparted.

diff --git a/swupdate-adapter/ebgenv.c b/env/env_api.c
similarity index 76%
rename from swupdate-adapter/ebgenv.c
rename to env/env_api.c
index 9b4e0c4..26d823d 100644
--- a/swupdate-adapter/ebgenv.c
+++ b/env/env_api.c
@@ -13,9 +13,49 @@
#include "env_api.h"
#include "ebgenv.h"

+/* UEFI uses 16-bit wide unicode strings.
+ * However, wchar_t support functions are fixed to 32-bit wide
+ * characters in glibc. This code is compiled with
+ * -fshort-wchar
+ * which enables 16-bit wide wchar_t support. However,
+ * glibc functions do not work with 16-bit wchar_t input, except
+ * it was specifically compiled for that, which is unusual.
+ * Thus, the needed conversion by truncation function is
+ * reimplemented here.
+ */
+char *str16to8(char *buffer, wchar_t *src)
+{
+ if (!src || !buffer) {
+ return NULL;
+ }
+ char *tmp = buffer;
+ while (*src) {
+ *buffer = (char)*src;
+ src++;
+ buffer++;
+ }
+ *buffer = 0;
+ return tmp;
+}
+
+wchar_t *str8to16(wchar_t *buffer, char *src)
+{
+ if (!src || !buffer) {
+ return NULL;
+ }
+ wchar_t *tmp = buffer;
+ while (*src) {
+ *buffer = (wchar_t)*src;
+ src++;
+ buffer++;
+ }
+ *buffer = 0;
+ return tmp;
+}
+
void ebg_beverbose(ebgenv_t *e, bool v)
{
- be_verbose(v);
+ bgenv_be_verbose(v);
}

int ebg_env_create_new(ebgenv_t *e)
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 24235c0..4376b8a 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -39,52 +39,12 @@ static EBGENVKEY bgenv_str2enum(char *key)
return EBGENV_UNKNOWN;
}

-void be_verbose(bool v)
+void bgenv_be_verbose(bool v)
{
verbosity = v;
ebgpart_beverbose(v);
}

-/* UEFI uses 16-bit wide unicode strings.
- * However, wchar_t support functions are fixed to 32-bit wide
- * characters in glibc. This code is compiled with
- * -fshort-wchar
- * which enables 16-bit wide wchar_t support. However,
- * glibc functions do not work with 16-bit wchar_t input, except
- * it was specifically compiled for that, which is unusual.
- * Thus, the needed conversion by truncation function is
- * reimplemented here.
- */
-char *str16to8(char *buffer, wchar_t *src)
-{
- if (!src || !buffer) {
- return NULL;
- }
- char *tmp = buffer;
- while (*src) {
- *buffer = (char)*src;
- src++;
- buffer++;
- }
- *buffer = 0;
- return tmp;
-}
-
-wchar_t *str8to16(wchar_t *buffer, char *src)
-{
- if (!src || !buffer) {
- return NULL;
- }
- wchar_t *tmp = buffer;
- while (*src) {
- *buffer = (wchar_t)*src;
- src++;
- buffer++;
- }
- *buffer = 0;
- return tmp;
-}
-
static char *get_mountpoint(char *devpath)
{
struct mntent *part = NULL;
diff --git a/swupdate-adapter/ebgenv.h b/include/ebgenv.h
similarity index 100%
rename from swupdate-adapter/ebgenv.h
rename to include/ebgenv.h
diff --git a/include/env_api.h b/include/env_api.h
index dc53ae4..8fe5454 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -62,7 +62,7 @@ typedef struct {
BG_ENVDATA *data;
} BGENV;

-extern void be_verbose(bool v);
+extern void bgenv_be_verbose(bool v);

extern char *str16to8(char *buffer, wchar_t *src);
extern wchar_t *str8to16(wchar_t *buffer, char *src);
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index fc3628f..3911ca3 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -198,7 +198,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
/* Set verbosity in this program */
verbosity = true;
/* Set verbosity in the library */
- be_verbose(true);
+ bgenv_be_verbose(true);
break;
case ARGP_KEY_ARG:
/* too many arguments - program terminates with call to
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 417e449..c6a8bb2 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -24,7 +24,6 @@ CFLAGS = \
-I$(PROJECTDIR)/.. \
-I$(PROJECTDIR)/../.. \
-I$(PROJECTDIR)/../../include \
- -I$(PROJECTDIR)/../../swupdate-adapter \
-std=gnu99 \
-g

@@ -40,7 +39,6 @@ CFLAGS += \
-fshort-wchar

LIBS = -L../.. \
- -L../../swupdate-adapter \
-lcmocka \
-lebgenv \
-lz
@@ -56,7 +54,7 @@ ENV_API ?= env_api_fat
#
OBJS_test_partitions = test_partitions.O $(ENV_API).O ebgpart.O
OBJS_test_environment = test_environment.O $(ENV_API).O ebgpart.O
-OBJS_test_api = test_api.O $(ENV_API).O ebgenv.O
+OBJS_test_api = test_api.O $(ENV_API).O

MOCKOBJS_test_partitions = $(ENV_API) ebgpart
MOCKOBJS_test_environment = $(ENV_API)
@@ -114,8 +112,5 @@ $(foreach test,$(TEST_TARGETS),$(eval $(call TEST_TARGET_RUN_TEMPLATE,$(test))))
%.O: ../../env/%.c
$(CC) $(CFLAGS) $(DEFINES) -c $< -o $(@:O=o)

-%.O: ../../swupdate-adapter/%.c
- $(CC) $(CFLAGS) $(DEFINES) -c $< -o $(@:O=o)
-
clean:
@rm -rf *.o $(TEST_TARGETS:.target=)
diff --git a/tools/tests/test_environment.c b/tools/tests/test_environment.c
index 7402121..a15606c 100644
--- a/tools/tests/test_environment.c
+++ b/tools/tests/test_environment.c
@@ -18,6 +18,7 @@
#include <cmocka.h>
#include <string.h>
#include "env_api.h"
+#include "ebgenv.h"
#include "test-interface.h"

/* Mock functions from libparted */
diff --git a/tools/tests/test_partitions.c b/tools/tests/test_partitions.c
index 237b01e..e043ff7 100644
--- a/tools/tests/test_partitions.c
+++ b/tools/tests/test_partitions.c
@@ -18,6 +18,7 @@
#include <cmocka.h>
#include "env_api.h"
#include "ebgpart.h"
+#include "ebgenv.h"

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:42 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

* bg_utils is actually a core API, thus rename it to env_api
* move include files to include directory
* adapt Makefiles

This patch is a preparation step for a later patch which will
simplify the API structure.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 18 +++++++++---------
tools/bg_utils.c => env/env_api_fat.c | 2 +-
{tools => include}/ebgpart.h | 0
tools/bg_utils.h => include/env_api.h | 0
{tools => include}/test-interface.h | 2 +-
swupdate-adapter/ebgenv.c | 2 +-
tools/bg_setenv.c | 2 +-
tools/tests/Makefile | 23 ++++++++++++++---------
tools/tests/test_api.c | 2 +-
tools/tests/test_environment.c | 2 +-
tools/tests/test_partitions.c | 2 +-
11 files changed, 30 insertions(+), 25 deletions(-)
rename tools/bg_utils.c => env/env_api_fat.c (99%)
rename {tools => include}/ebgpart.h (100%)
rename tools/bg_utils.h => include/env_api.h (100%)
rename {tools => include}/test-interface.h (96%)

diff --git a/Makefile.am b/Makefile.am
index cb8e238..c3908c0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -39,10 +39,10 @@ CLEANFILES =
#
# Static libraries
#
-lib_LIBRARIES = libebgenv.a libbg_utils.a
+lib_LIBRARIES = libebgenv.a libenv_api.a

libebgenv_a_SOURCES = \
- tools/bg_utils.c \
+ env/env_api_fat.c \
tools/ebgpart.c \
swupdate-adapter/ebgenv.c

@@ -53,16 +53,16 @@ libebgenv_a_CPPFLAGS = \
libebgenv_a_CFLAGS = \
$(AM_CFLAGS)

-libbg_utils_a_SOURCES = \
+libenv_api_a_SOURCES = \
tools/ebgpart.c \
- tools/bg_utils.c
+ env/env_api_fat.c

-libbg_utils_a_CFLAGS = \
+libenv_api_a_CFLAGS = \
$(AM_CFLAGS)

pkginclude_HEADERS = \
swupdate-adapter/ebgenv.h \
- tools/bg_utils.h
+ include/env_api.h

#
# bg_setenv binary
@@ -77,11 +77,11 @@ bg_setenv_CFLAGS = \
$(AM_CFLAGS)

bg_setenv_LDADD = \
- -lbg_utils \
+ -lenv_api \
-lz

bg_setenv_DEPENDENCIES = \
- libbg_utils.a
+ libenv_api.a

install-exec-hook:
$(LN_S) -f bg_setenv$(EXEEXT) \
@@ -197,7 +197,7 @@ build_tests:

.PHONY: check
check:
- make -C tools/tests run-tests
+ make -C tools/tests run-tests ENV_API=env_api_fat

clean-local:
make -C tools/tests clean
diff --git a/tools/bg_utils.c b/env/env_api_fat.c
similarity index 99%
rename from tools/bg_utils.c
rename to env/env_api_fat.c
index 496e2c8..f282a3f 100644
--- a/tools/bg_utils.c
+++ b/env/env_api_fat.c
@@ -10,7 +10,7 @@
* the COPYING file in the top-level directory.
*/

-#include "bg_utils.h"
+#include "env_api.h"
#include "test-interface.h"

const char *tmp_mnt_dir = "/tmp/mnt-XXXXXX";
diff --git a/tools/ebgpart.h b/include/ebgpart.h
similarity index 100%
rename from tools/ebgpart.h
rename to include/ebgpart.h
diff --git a/tools/bg_utils.h b/include/env_api.h
similarity index 100%
rename from tools/bg_utils.h
rename to include/env_api.h
diff --git a/tools/test-interface.h b/include/test-interface.h
similarity index 96%
rename from tools/test-interface.h
rename to include/test-interface.h
index d0bf6d4..cceb1cb 100644
--- a/tools/test-interface.h
+++ b/include/test-interface.h
@@ -13,7 +13,7 @@
#ifndef __TEST_INTERFACE_H__
#define __TEST_INTERFACE_H__

-#include "bg_utils.h"
+#include "env_api.h"

bool read_env(CONFIG_PART *part, BG_ENVDATA *env);
bool write_env(CONFIG_PART *part, BG_ENVDATA *env);
diff --git a/swupdate-adapter/ebgenv.c b/swupdate-adapter/ebgenv.c
index 4c767ee..9ac42fc 100644
--- a/swupdate-adapter/ebgenv.c
+++ b/swupdate-adapter/ebgenv.c
@@ -10,7 +10,7 @@
* the COPYING file in the top-level directory.
*/

-#include "bg_utils.h"
+#include "env_api.h"
#include "ebgdefs.h"
#include "ebgenv.h"

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index c006bb6..817d832 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -10,7 +10,7 @@
* the COPYING file in the top-level directory.
*/

-#include "bg_utils.h"
+#include "env_api.h"

static char doc[] =
"bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index da12a61..f1fda2e 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -44,6 +44,8 @@ LIBS = -L../.. \
-lebgenv \
-lz

+ENV_API ?= env_api_fat
+
# Test recipes shall run everytime independent of already built files.
# A simple way to achieve this is to depend on files that don't exist
# by changing their extension with Makefile's string functions.
@@ -51,20 +53,20 @@ LIBS = -L../.. \
# dependency recipes.
# All targets' '.target' extensions get removed within the target recipes.
#
-OBJS_test_partitions = test_partitions.O bg_utils.O ebgpart.O
-OBJS_test_environment = test_environment.O bg_utils.O ebgpart.O
-OBJS_test_api = test_api.O bg_utils.O ebgenv.O
+OBJS_test_partitions = test_partitions.O $(ENV_API).O ebgpart.O
+OBJS_test_environment = test_environment.O $(ENV_API).O ebgpart.O
+OBJS_test_api = test_api.O $(ENV_API).O ebgenv.O

-MOCKOBJS_test_partitions = bg_utils ebgpart
-MOCKOBJS_test_environment = bg_utils
-MOCKOBJS_test_api = bg_utils
+MOCKOBJS_test_partitions = $(ENV_API) ebgpart
+MOCKOBJS_test_environment = $(ENV_API)
+MOCKOBJS_test_api = $(ENV_API)

# Define symbols to be stripped dependent on target and object file name
# MOCKOBJS_SYMBOLS_objectname-targetname = symbolname1 symbolname2 ...

-MOCKOBJS_SYMBOLS_bg_utils-test_partitions = probe_config_file
-MOCKOBJS_SYMBOLS_bg_utils-test_environment = oldenvs configparts fopen fclose fread fwrite feof mount_partition
-MOCKOBJS_SYMBOLS_bg_utils-test_api = bgenv_init bgenv_write bgenv_close bgenv_get_latest bgenv_get_by_index bgenv_get_oldest
+MOCKOBJS_SYMBOLS_$(ENV_API)-test_partitions = probe_config_file
+MOCKOBJS_SYMBOLS_$(ENV_API)-test_environment = oldenvs configparts fopen fclose fread fwrite feof mount_partition
+MOCKOBJS_SYMBOLS_$(ENV_API)-test_api = bgenv_init bgenv_write bgenv_close bgenv_get_latest bgenv_get_by_index bgenv_get_oldest
MOCKOBJS_SYMBOLS_ebgpart-test_partitions = ped_device_probe_all ped_device_get_next ped_disk_next_partition

TEST_TARGETS = test_partitions.target test_environment.target test_api.target
@@ -108,6 +110,9 @@ $(foreach test,$(TEST_TARGETS),$(eval $(call TEST_TARGET_RUN_TEMPLATE,$(test))))
%.O: ../%.c
$(CC) $(CFLAGS) $(DEFINES) -c $< -o $(@:O=o)

+%.O: ../../env/%.c
+ $(CC) $(CFLAGS) $(DEFINES) -c $< -o $(@:O=o)
+
%.O: ../../swupdate-adapter/%.c
$(CC) $(CFLAGS) $(DEFINES) -c $< -o $(@:O=o)

diff --git a/tools/tests/test_api.c b/tools/tests/test_api.c
index fb6e658..cca4573 100644
--- a/tools/tests/test_api.c
+++ b/tools/tests/test_api.c
@@ -18,7 +18,7 @@
#include <cmocka.h>
#include <string.h>
#include <error.h>
-#include "bg_utils.h"
+#include "env_api.h"
#include "ebgenv.h"

static BGENV env = {0};
diff --git a/tools/tests/test_environment.c b/tools/tests/test_environment.c
index fe9d50e..63df653 100644
--- a/tools/tests/test_environment.c
+++ b/tools/tests/test_environment.c
@@ -17,7 +17,7 @@
#include <setjmp.h>
#include <cmocka.h>
#include <string.h>
-#include "bg_utils.h"
+#include "env_api.h"
#include "test-interface.h"

/* Mock functions from libparted */
diff --git a/tools/tests/test_partitions.c b/tools/tests/test_partitions.c
index 6781b79..c6865e4 100644
--- a/tools/tests/test_partitions.c
+++ b/tools/tests/test_partitions.c
@@ -16,7 +16,7 @@
#include <stdbool.h>
#include <setjmp.h>
#include <cmocka.h>
-#include "bg_utils.h"
+#include "env_api.h"

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:43 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

If the crc was found to be wrong, the environment was used
normally despite, which caused problems in user tools.
Now, if an environment with an invalid crc is found, its working
copy is cleared. This way it can be automatically overwritten
by updates.

Also, the variable name 'oldenvs' is misleading, as it is an
array containing all current environments.

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

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 4376b8a..10fe37b 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -330,7 +330,7 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
}

static CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
-static BG_ENVDATA oldenvs[ENV_NUM_CONFIG_PARTS];
+static BG_ENVDATA envdata[ENV_NUM_CONFIG_PARTS];

bool bgenv_init()
{
@@ -342,12 +342,15 @@ bool bgenv_init()
return false;
}
for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- read_env(&config_parts[i], &oldenvs[i]);
- uint32_t sum = crc32(0, (Bytef *)&oldenvs[i],
- sizeof(BG_ENVDATA) - sizeof(oldenvs[i].crc32));
- if (oldenvs[i].crc32 != sum) {
+ read_env(&config_parts[i], &envdata[i]);
+ uint32_t sum = crc32(0, (Bytef *)&envdata[i],
+ sizeof(BG_ENVDATA) - sizeof(envdata[i].crc32));
+ if (envdata[i].crc32 != sum) {
VERBOSE(stderr, "Invalid CRC32!\n");
- continue;
+ /* clear invalid environment */
+ memset(&envdata[i], 0, sizeof(BG_ENVDATA));
+ envdata[i].crc32 = crc32(0, (Bytef *)&envdata[i],
+ sizeof(BG_ENVDATA) - sizeof(envdata[i].crc32));
}
}
return true;
@@ -365,7 +368,7 @@ BGENV *bgenv_open_by_index(uint32_t index)
return NULL;
}
handle->desc = (void *)&config_parts[index];
- handle->data = &oldenvs[index];
+ handle->data = &envdata[index];
return handle;
}

@@ -375,8 +378,8 @@ BGENV *bgenv_open_oldest()
uint32_t min_idx = 0;

for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (oldenvs[i].revision < minrev) {
- minrev = oldenvs[i].revision;
+ if (envdata[i].revision < minrev) {
+ minrev = envdata[i].revision;
min_idx = i;
}
}
@@ -389,8 +392,8 @@ BGENV *bgenv_open_latest()
uint32_t max_idx = 0;

for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (oldenvs[i].revision > maxrev) {
- maxrev = oldenvs[i].revision;
+ if (envdata[i].revision > maxrev) {
+ maxrev = envdata[i].revision;
max_idx = i;
}
}
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:43 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

* Refactor API

Provide one function to set and one to get the global update state
instead of six different.

Improve code quality and use a context for API functions instead of
hidden static global variables.

* env: Remove BGENVTYPE from code

Since different storage backends come with different .c files, there
is no need for switch cases.

* move some functions to simplify API dependencies

* remove ebgdefs.h, since it only included one define, which better
fits into the envdata.h

* Add a ebgenv_t structure as a context for all ebg_ API functions.
This provides better maintainability and future extensibility.
It helps avoiding ugly global variables.
One might argue that a context implicitly suggests multi-threading
capability and needs an implementation for access scheduling for
multiple environment accesses at the same time, however, the
introduction of a better coding style per se is independent of this
idea and all functions remain unsafe regarding threads.

* Update tests

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/API.md | 20 ++-
env/env_api_fat.c | 294 +++++++++++++++++++++++++---------
env/fatvars.c | 1 -
include/ebgdefs.h | 20 ---
include/env_api.h | 30 ++--
include/envdata.h | 2 +
swupdate-adapter/ebgenv.c | 398 ++++++++--------------------------------------
swupdate-adapter/ebgenv.h | 78 +++++----
tools/bg_setenv.c | 14 +-
tools/tests/Makefile | 2 +-
tools/tests/test_api.c | 128 ++++++++-------
11 files changed, 431 insertions(+), 556 deletions(-)
delete mode 100644 include/ebgdefs.h

diff --git a/docs/API.md b/docs/API.md
index 8778b70..3da00b3 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -42,11 +42,13 @@ and sets it to the testing state:

int main(void)
{
- ebg_env_create_new();
- ebg_env_set("kernelfile", "vmlinux-new");
- ebg_env_set("kernelparams", "root=/dev/bootdevice");
- ebg_env_set("watchdog_timeout_sec", "30");
- ebg_env_close();
+ ebgenv_t e;
+
+ ebg_env_create_new(&e);
+ ebg_env_set(&e, "kernelfile", "vmlinux-new");
+ ebg_env_set(&e, "kernelparams", "root=/dev/bootdevice");
+ ebg_env_set(&e, "watchdog_timeout_sec", "30");
+ ebg_env_close(&e);
return 0;
}
```
@@ -66,9 +68,11 @@ modifies the kernel file name:

int main(void)
{
- ebg_env_open_current();
- ebg_env_set("kernelfile", "vmlinux-new");
- ebg_env_close();
+ ebgenv_t e;
+
+ ebg_env_open_current(&e);
+ ebg_env_set(&e, "kernelfile", "vmlinux-new");
+ ebg_env_close(&e);
return 0;
}
```
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index bed74d9..24235c0 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -18,6 +18,27 @@ const char *tmp_mnt_dir = "/tmp/mnt-XXXXXX";

static bool verbosity = false;

+static EBGENVKEY bgenv_str2enum(char *key)
+{
+ if (strncmp(key, "kernelfile", strlen("kernelfile") + 1) == 0) {
+ return EBGENV_KERNELFILE;
+ }
+ if (strncmp(key, "kernelparams", strlen("kernelparams") + 1) == 0) {
+ return EBGENV_KERNELPARAMS;
+ }
+ if (strncmp(key, "watchdog_timeout_sec",
+ strlen("watchdog_timeout_sec") + 1) == 0) {
+ return EBGENV_WATCHDOG_TIMEOUT_SEC;
+ }
+ if (strncmp(key, "revision", strlen("revision") + 1) == 0) {
+ return EBGENV_REVISION;
+ }
+ if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
+ return EBGENV_USTATE;
+ }
+ return EBGENV_UNKNOWN;
+}
+
void be_verbose(bool v)
{
verbosity = v;
@@ -348,89 +369,72 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
return result;
}

-CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
-BG_ENVDATA oldenvs[ENV_NUM_CONFIG_PARTS];
+static CONFIG_PART config_parts[ENV_NUM_CONFIG_PARTS];
+static BG_ENVDATA oldenvs[ENV_NUM_CONFIG_PARTS];

-bool bgenv_init(BGENVTYPE type)
+bool bgenv_init()
{
- switch (type) {
- case BGENVTYPE_FAT:
- memset((void *)&config_parts, 0,
- sizeof(CONFIG_PART) * ENV_NUM_CONFIG_PARTS);
- /* enumerate all config partitions */
- if (!probe_config_partitions(config_parts)) {
- VERBOSE(stderr, "Error finding config partitions.\n");
- return false;
- }
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- read_env(&config_parts[i], &oldenvs[i]);
- uint32_t sum = crc32(0, (Bytef *)&oldenvs[i],
- sizeof(BG_ENVDATA) - sizeof(oldenvs[i].crc32));
- if (oldenvs[i].crc32 != sum) {
- VERBOSE(stderr, "Invalid CRC32!\n");
- continue;
- }
+ memset((void *)&config_parts, 0,
+ sizeof(CONFIG_PART) * ENV_NUM_CONFIG_PARTS);
+ /* enumerate all config partitions */
+ if (!probe_config_partitions(config_parts)) {
+ VERBOSE(stderr, "Error finding config partitions.\n");
+ return false;
+ }
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ read_env(&config_parts[i], &oldenvs[i]);
+ uint32_t sum = crc32(0, (Bytef *)&oldenvs[i],
+ sizeof(BG_ENVDATA) - sizeof(oldenvs[i].crc32));
+ if (oldenvs[i].crc32 != sum) {
+ VERBOSE(stderr, "Invalid CRC32!\n");
+ continue;
}
- return true;
}
- return false;
+ return true;
}

-BGENV *bgenv_get_by_index(BGENVTYPE type, uint32_t index)
+BGENV *bgenv_open_by_index(uint32_t index)
{
BGENV *handle;

- switch (type) {
- case BGENVTYPE_FAT:
- /* get config partition by index and allocate handle */
- if (index >= ENV_NUM_CONFIG_PARTS) {
- return NULL;
- }
- if (!(handle = calloc(1, sizeof(BGENV)))) {
- return NULL;
- }
- handle->desc = (void *)&config_parts[index];
- handle->data = &oldenvs[index];
- handle->type = type;
- return handle;
+ /* get config partition by index and allocate handle */
+ if (index >= ENV_NUM_CONFIG_PARTS) {
+ return NULL;
}
- return NULL;
+ if (!(handle = calloc(1, sizeof(BGENV)))) {
+ return NULL;
+ }
+ handle->desc = (void *)&config_parts[index];
+ handle->data = &oldenvs[index];
+ return handle;
}

-BGENV *bgenv_get_oldest(BGENVTYPE type)
+BGENV *bgenv_open_oldest()
{
uint32_t minrev = 0xFFFFFFFF;
uint32_t min_idx = 0;

- switch (type) {
- case BGENVTYPE_FAT:
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (oldenvs[i].revision < minrev) {
- minrev = oldenvs[i].revision;
- min_idx = i;
- }
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ if (oldenvs[i].revision < minrev) {
+ minrev = oldenvs[i].revision;
+ min_idx = i;
}
- return bgenv_get_by_index(type, min_idx);
}
- return NULL;
+ return bgenv_open_by_index(min_idx);
}

-BGENV *bgenv_get_latest(BGENVTYPE type)
+BGENV *bgenv_open_latest()
{
uint32_t maxrev = 0;
uint32_t max_idx = 0;

- switch (type) {
- case BGENVTYPE_FAT:
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (oldenvs[i].revision > maxrev) {
- maxrev = oldenvs[i].revision;
- max_idx = i;
- }
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ if (oldenvs[i].revision > maxrev) {
+ maxrev = oldenvs[i].revision;
+ max_idx = i;
}
- return bgenv_get_by_index(type, max_idx);
}
- return NULL;
+ return bgenv_open_by_index(max_idx);
}

bool bgenv_write(BGENV *env)
@@ -440,23 +444,19 @@ bool bgenv_write(BGENV *env)
if (!env) {
return false;
}
- switch (env->type) {
- case BGENVTYPE_FAT:
- part = (CONFIG_PART *)env->desc;
- if (!part) {
- VERBOSE(
- stderr,
- "Invalid config partition to store environment.\n");
- return false;
- }
- if (!write_env(part, env->data)) {
- VERBOSE(stderr, "Could not write to %s\n",
- part->devpath);
- return false;
- }
- return true;
+ part = (CONFIG_PART *)env->desc;
+ if (!part) {
+ VERBOSE(
+ stderr,
+ "Invalid config partition to store environment.\n");
+ return false;
}
- return false;
+ if (!write_env(part, env->data)) {
+ VERBOSE(stderr, "Could not write to %s\n",
+ part->devpath);
+ return false;
+ }
+ return true;
}

BG_ENVDATA *bgenv_read(BGENV *env)
@@ -475,3 +475,151 @@ bool bgenv_close(BGENV *env)
}
return false;
}
+
+int bgenv_get(BGENV *env, char *key, char **type, void *data, size_t maxlen)
+{
+ EBGENVKEY e;
+
+ if (!key || !data || maxlen == 0) {
+ return EINVAL;
+ }
+ e = bgenv_str2enum(key);
+ if (e == EBGENV_UNKNOWN) {
+ return EINVAL;
+ }
+ if (!env) {
+ return EPERM;
+ }
+ switch (e) {
+ case EBGENV_KERNELFILE:
+ str16to8(data, env->data->kernelfile);
+ if (type) {
+ sprintf(*type, "char*");
+ }
+ break;
+ case EBGENV_KERNELPARAMS:
+ str16to8(data, env->data->kernelparams);
+ if (type) {
+ sprintf(*type, "char*");
+ }
+ break;
+ case EBGENV_WATCHDOG_TIMEOUT_SEC:
+ sprintf(data, "%lu", env->data->watchdog_timeout_sec);
+ if (type) {
+ sprintf(*type, "uint16_t");
+ }
+ break;
+ case EBGENV_REVISION:
+ sprintf(data, "%lu", env->data->revision);
+ if (type) {
+ sprintf(*type, "uint32_t");
+ }
+ break;
+ case EBGENV_USTATE:
+ sprintf(data, "%u", env->data->ustate);
+ if (type) {
+ sprintf(*type, "uint16_t");
+ }
+ break;
+ default:
+ return EINVAL;
+ }
+ return 0;
+}
+
+int bgenv_set(BGENV *env, char *key, char *type, void *data, size_t datalen)
+{
+ EBGENVKEY e;
+ int val;
+ char *p;
+ char *value = (char *)data;
+
+ if (!key || !data || datalen == 0) {
+ return EINVAL;
+ }
+
+ e = bgenv_str2enum(key);
+ if (e == EBGENV_UNKNOWN) {
+ return EINVAL;
+ }
+ if (!env) {
+ return EPERM;
+ }
+ switch (e) {
+ case EBGENV_REVISION:
+ 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->revision = val;
+ break;
+ case EBGENV_KERNELFILE:
+ str8to16(env->data->kernelfile, value);
+ break;
+ case EBGENV_KERNELPARAMS:
+ str8to16(env->data->kernelparams, value);
+ break;
+ case EBGENV_WATCHDOG_TIMEOUT_SEC:
+ 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->watchdog_timeout_sec = val;
+ break;
+ case EBGENV_USTATE:
+ 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->ustate = val;
+ break;
+ default:
+ return EINVAL;
+ }
+ return 0;
+}
+
+BGENV *bgenv_create_new()
+{
+ BGENV *env_latest;
+ BGENV *env_new;
+
+ env_latest = bgenv_open_latest();
+ if (!env_latest)
+ goto create_new_io_error;
+
+ int new_rev = env_latest->data->revision + 1;
+
+ if (!bgenv_close(env_latest))
+ goto create_new_io_error;
+
+ env_new = bgenv_open_oldest();
+ if (!env_new)
+ goto create_new_io_error;
+
+ /* zero fields */
+ 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;
+ /* set default watchdog timeout */
+ env_new->data->watchdog_timeout_sec = 30;
+
+ return env_new;
+
+create_new_io_error:
+ errno = EIO;
+ return NULL;
+}
diff --git a/env/fatvars.c b/env/fatvars.c
index 8f871b9..5d391c7 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -11,7 +11,6 @@
*/

#include <bootguard.h>
-#include <ebgdefs.h>
#include <syspart.h>
#include <envdata.h>

diff --git a/include/ebgdefs.h b/include/ebgdefs.h
deleted file mode 100644
index 4327d45..0000000
--- a/include/ebgdefs.h
+++ /dev/null
@@ -1,20 +0,0 @@
-/** @file
- *
- * EFI Boot Guard
- *
- * Copyright (c) Siemens AG, 2017
- *
- * Authors:
- * Andreas Reichel <andreas.r...@siemens.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2. See
- * the COPYING file in the top-level directory.
- *
- */
-
-#ifndef __EFI_BOOT_GUARD_DEFS_H__
-#define __EFI_BOOT_GUARD_DEFS_H__
-
-#define REVISION_FAILED 0
-
-#endif
diff --git a/include/env_api.h b/include/env_api.h
index 83c7123..dc53ae4 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -10,8 +10,8 @@
* the COPYING file in the top-level directory.
*/

-#ifndef __BG_UTILS_H__
-#define __BG_UTILS_H__
+#ifndef __ENV_API_H__
+#define __ENV_API_H__

#include <stddef.h>
#include <stdint.h>
@@ -42,7 +42,14 @@
if (verbosity) \
fprintf(o, __VA_ARGS__)

-typedef enum { BGENVTYPE_FAT } BGENVTYPE;
+typedef enum {
+ EBGENV_KERNELFILE,
+ EBGENV_KERNELPARAMS,
+ EBGENV_WATCHDOG_TIMEOUT_SEC,
+ EBGENV_REVISION,
+ EBGENV_USTATE,
+ EBGENV_UNKNOWN
+} EBGENVKEY;

typedef struct {
char *devpath;
@@ -51,7 +58,6 @@ typedef struct {
} CONFIG_PART;

typedef struct {
- BGENVTYPE type;
void *desc;
BG_ENVDATA *data;
} BGENV;
@@ -61,12 +67,18 @@ extern void be_verbose(bool v);
extern char *str16to8(char *buffer, wchar_t *src);
extern wchar_t *str8to16(wchar_t *buffer, char *src);

-extern bool bgenv_init(BGENVTYPE type);
-extern BGENV *bgenv_get_by_index(BGENVTYPE type, uint32_t index);
-extern BGENV *bgenv_get_oldest(BGENVTYPE type);
-extern BGENV *bgenv_get_latest(BGENVTYPE type);
+extern bool bgenv_init(void);
+extern BGENV *bgenv_open_by_index(uint32_t index);
+extern BGENV *bgenv_open_oldest(void);
+extern BGENV *bgenv_open_latest(void);
extern bool bgenv_write(BGENV *env);
extern BG_ENVDATA *bgenv_read(BGENV *env);
extern bool bgenv_close(BGENV *env);

-#endif // __BG_UTILS_H__
+extern BGENV *bgenv_create_new(void);
+extern int bgenv_get(BGENV *env, char *key, char **type, void *data,
+ size_t maxlen);
+extern int bgenv_set(BGENV *env, char *key, char *type, void *data,
+ size_t datalen);
+
+#endif // __ENV_API_H__
diff --git a/include/envdata.h b/include/envdata.h
index 78dda9b..3f1fb38 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -27,6 +27,8 @@
#define USTATE_MIN 0
#define USTATE_MAX 4

+#define REVISION_FAILED 0
+
#pragma pack(push)
#pragma pack(1)
struct _BG_ENVDATA {
diff --git a/swupdate-adapter/ebgenv.c b/swupdate-adapter/ebgenv.c
index 115e0a9..9b4e0c4 100644
--- a/swupdate-adapter/ebgenv.c
+++ b/swupdate-adapter/ebgenv.c
@@ -11,403 +11,133 @@
*/

#include "env_api.h"
-#include "ebgdefs.h"
#include "ebgenv.h"

-typedef enum {
- EBGENV_KERNELFILE,
- EBGENV_KERNELPARAMS,
- EBGENV_WATCHDOG_TIMEOUT_SEC,
- EBGENV_REVISION,
- EBGENV_USTATE,
- EBGENV_UNKNOWN
-} EBGENVKEY;
-
-static BGENV *env_current = NULL;
-
-typedef struct _PCOLLECTOR {
- void *p;
- struct _PCOLLECTOR *next;
-} PCOLLECTOR;
-
-static PCOLLECTOR ebg_gc;
-
-static bool ebg_new_env_created = false;
-
-static bool ebg_gc_addpointer(void *pnew)
-{
- PCOLLECTOR *pc = &ebg_gc;
-
- while (pc->next) {
- pc = pc->next;
- }
- pc->next = calloc(sizeof(PCOLLECTOR), 1);
- if (!pc->next) {
- return false;
- }
- pc = pc->next;
- pc->p = pnew;
- return true;
-}
-
-static void ebg_gc_cleanup()
-{
- PCOLLECTOR *pc = &ebg_gc;
-
- while (pc) {
- if (pc->p) {
- free(pc->p);
- }
- PCOLLECTOR *tmp = pc;
- pc = pc->next;
- if (tmp != &ebg_gc) {
- free(tmp);
- }
- }
- ebg_gc.p = NULL;
- ebg_gc.next = NULL;
-}
-
-static EBGENVKEY ebg_env_str2enum(char *key)
-{
- if (strncmp(key, "kernelfile", strlen("kernelfile") + 1) == 0) {
- return EBGENV_KERNELFILE;
- }
- if (strncmp(key, "kernelparams", strlen("kernelparams") + 1) == 0) {
- return EBGENV_KERNELPARAMS;
- }
- if (strncmp(key, "watchdog_timeout_sec",
- strlen("watchdog_timeout_sec") + 1) == 0) {
- return EBGENV_WATCHDOG_TIMEOUT_SEC;
- }
- if (strncmp(key, "revision", strlen("revision") + 1) == 0) {
- return EBGENV_REVISION;
- }
- if (strncmp(key, "ustate", strlen("ustate") + 1) == 0) {
- return EBGENV_USTATE;
- }
- return EBGENV_UNKNOWN;
-}
-
-void ebg_beverbose(bool v)
+void ebg_beverbose(ebgenv_t *e, bool v)
{
be_verbose(v);
}

-int ebg_env_create_new(void)
+int ebg_env_create_new(ebgenv_t *e)
{
- /* initialize garbage collector */
- ebg_gc.p = NULL;
- ebg_gc.next = NULL;
-
- if (!bgenv_init(BGENVTYPE_FAT)) {
+ if (!bgenv_init()) {
return EIO;
}

- env_current = bgenv_get_latest(BGENVTYPE_FAT);
-
- if (ebg_new_env_created)
- return env_current == NULL ? EIO : 0;
-
- if (!env_current) {
- return EIO;
+ if (!e->ebg_new_env_created) {
+ e->bgenv = (void *)bgenv_create_new();
}
- /* first time env is opened, a new one is created for update
- * purpose */
- int new_rev = env_current->data->revision + 1;

- if (!bgenv_close(env_current)) {
- return EIO;
- }
- env_current = bgenv_get_oldest(BGENVTYPE_FAT);
- if (!env_current) {
- return EIO;
- }
- /* zero fields */
- memset(env_current->data, 0, sizeof(BG_ENVDATA));
- /* update revision field and testing mode */
- env_current->data->revision = new_rev;
- env_current->data->ustate = USTATE_INSTALLED;
- /* set default watchdog timeout */
- env_current->data->watchdog_timeout_sec = 30;
- ebg_new_env_created = true;
+ e->ebg_new_env_created = e->bgenv != NULL;

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

-int ebg_env_open_current(void)
+int ebg_env_open_current(ebgenv_t *e)
{
- /* initialize garbage collector */
- ebg_gc.p = NULL;
- ebg_gc.next = NULL;
-
- if (!bgenv_init(BGENVTYPE_FAT)) {
+ if (!bgenv_init()) {
return EIO;
}

- env_current = bgenv_get_latest(BGENVTYPE_FAT);
+ e->bgenv = (void *)bgenv_open_latest();

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

-char *ebg_env_get(char *key)
+int ebg_env_get(ebgenv_t *e, char *key, char *buffer)
{
- EBGENVKEY e;
- char *buffer;
-
- if (!key) {
- errno = EINVAL;
- return NULL;
- }
- e = ebg_env_str2enum(key);
- if (e == EBGENV_UNKNOWN) {
- errno = EINVAL;
- return NULL;
- }
- if (!env_current) {
- errno = EPERM;
- return NULL;
- }
- switch (e) {
- case EBGENV_KERNELFILE:
- buffer = (char *)malloc(ENV_STRING_LENGTH);
- if (!buffer) {
- errno = ENOMEM;
- return NULL;
- }
- if (!ebg_gc_addpointer(buffer)) {
- errno = ENOMEM;
- return NULL;
- };
- str16to8(buffer, env_current->data->kernelfile);
- return buffer;
- case EBGENV_KERNELPARAMS:
- buffer = (char *)malloc(ENV_STRING_LENGTH);
- if (!buffer) {
- errno = ENOMEM;
- return NULL;
- }
- if (!ebg_gc_addpointer(buffer)) {
- errno = ENOMEM;
- return NULL;
- }
- str16to8(buffer, env_current->data->kernelparams);
- return buffer;
- case EBGENV_WATCHDOG_TIMEOUT_SEC:
- if (asprintf(&buffer, "%lu",
- env_current->data->watchdog_timeout_sec) < 0) {
- errno = ENOMEM;
- return NULL;
- }
- if (!ebg_gc_addpointer(buffer)) {
- errno = ENOMEM;
- return NULL;
- }
- return buffer;
- case EBGENV_REVISION:
- if (asprintf(&buffer, "%lu", env_current->data->revision) < 0) {
- errno = ENOMEM;
- return NULL;
- }
- if (!ebg_gc_addpointer(buffer)) {
- errno = ENOMEM;
- return NULL;
- }
- return buffer;
- case EBGENV_USTATE:
- if (asprintf(&buffer, "%u", env_current->data->ustate) < 0) {
- errno = ENOMEM;
- return NULL;
- }
- if (!ebg_gc_addpointer(buffer)) {
- errno = ENOMEM;
- return NULL;
- }
- return buffer;
- default:
- errno = EINVAL;
- return NULL;
- }
- errno = EINVAL;
- return NULL;
+ return bgenv_get((BGENV *)e->bgenv, key, NULL, buffer,
+ ENV_STRING_LENGTH);
}

-int ebg_env_set(char *key, char *value)
+int ebg_env_set(ebgenv_t *e, char *key, char *value)
{
- EBGENVKEY e;
- int val;
- char *p;
-
- if (!key || !value) {
- return EINVAL;
- }
- e = ebg_env_str2enum(key);
- if (e == EBGENV_UNKNOWN) {
- return EINVAL;
- }
- if (!env_current) {
- return EPERM;
- }
- 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;
- }
- env_current->data->revision = val;
- break;
- case EBGENV_KERNELFILE:
- str8to16(env_current->data->kernelfile, value);
- break;
- case EBGENV_KERNELPARAMS:
- str8to16(env_current->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;
- }
- env_current->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;
- }
- env_current->data->ustate = val;
- break;
- default:
- return EINVAL;
- }
- return 0;
-}
+ return bgenv_set((BGENV *)e->bgenv, key, "String", value,
+ strlen(value)); }

-bool ebg_env_isupdatesuccessful(void)
+uint16_t ebg_env_getglobalstate(ebgenv_t *e)
{
+ BGENV *env;
+ int res = 4;
+
/* find all environments with revision 0 */
for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- BGENV *env = bgenv_get_by_index(BGENVTYPE_FAT, i);
+ BGENV *env = bgenv_open_by_index(i);

if (!env) {
continue;
}
- /* update was unsuccessful if there is a revision 0 config,
- * with
- * testing and boot_once set */
+ /* update was unsuccessful if there is a config,
+ * with revision == REVISION_FAILED and
+ * with ustate == USTATE_FAILED */
if (env->data->revision == REVISION_FAILED &&
env->data->ustate == USTATE_FAILED) {
(void)bgenv_close(env);
- return false;
+ res = 3;
}
(void)bgenv_close(env);
- }
- return true;
-}
-
-int ebg_env_clearerrorstate(void)
-{
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- BGENV *env = bgenv_get_by_index(BGENVTYPE_FAT, i);
-
- if (!env) {
- continue;
- }
- if (env->data->revision == REVISION_FAILED &&
- env->data->ustate == USTATE_FAILED) {
- env->data->ustate = USTATE_OK;
- if (!bgenv_write(env)) {
- (void)bgenv_close(env);
- return EIO;
- }
- }
- if (!bgenv_close(env)) {
- return EIO;
+ if (res == 3) {
+ return res;
}
}
- return 0;
-}

-int ebg_env_confirmupdate(void)
-{
- return ebg_env_set("ustate", "0");
-}
-
-bool ebg_env_isokay(void)
-{
- BGENV *env;
- bool res = false;
-
- env = bgenv_get_latest(BGENVTYPE_FAT);
+ env = bgenv_open_latest();
if (!env) {
errno = EIO;
return res;
}
- if (env->data->ustate == USTATE_OK) {
- res = true;
- }
+
+ res = env->data->ustate;
bgenv_close(env);
+
return res;
}

-bool ebg_env_isinstalled(void)
+int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate)
{
- BGENV *env;
- bool res = false;
+ char buffer[2];
+ int res;

- env = bgenv_get_latest(BGENVTYPE_FAT);
- if (!env) {
- errno = EIO;
- return res;
- }
- if (env->data->ustate == USTATE_INSTALLED) {
- res = true;
+ if (ustate > USTATE_FAILED) {
+ return EINVAL;
}
- bgenv_close(env);
- return res;
-}
-
-bool ebg_env_istesting(void)
-{
- BGENV *env;
- bool res = false;
+ snprintf(buffer, sizeof(buffer), "%d", ustate);
+ res =
+ bgenv_set((BGENV *)e->bgenv, "ustate", "String", buffer,
+ strlen(buffer));

- env = bgenv_get_latest(BGENVTYPE_FAT);
- if (!env) {
- errno = EIO;
+ if (ustate != USTATE_OK) {
return res;
}
- if (env->data->ustate == USTATE_TESTING) {
- res = true;
+
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ BGENV *env = bgenv_open_by_index(i);
+
+ if (!env) {
+ continue;
+ }
+ env->data->ustate = ustate;
+ if (!bgenv_write(env)) {
+ (void)bgenv_close(env);
+ return EIO;
+ }
+ if (!bgenv_close(env)) {
+ return EIO;
+ }
}
- bgenv_close(env);
- return res;
+ return 0;
}

-int ebg_env_close(void)
+int ebg_env_close(ebgenv_t *e)
{
- /* free all allocated memory */
- ebg_gc_cleanup();
-
/* if no environment is open, just return EIO */
- if (!env_current) {
+ if (!e->bgenv) {
return EIO;
}

+ BGENV *env_current;
+ env_current = (BGENV *)e->bgenv;
+
/* recalculate checksum */
env_current->data->crc32 =
crc32(0, (Bytef *)env_current->data,
@@ -420,6 +150,6 @@ int ebg_env_close(void)
if (!bgenv_close(env_current)) {
return EIO;
}
- env_current = NULL;
+ e->bgenv = NULL;
return 0;
}
diff --git a/swupdate-adapter/ebgenv.h b/swupdate-adapter/ebgenv.h
index 97c23db..fb3e809 100644
--- a/swupdate-adapter/ebgenv.h
+++ b/swupdate-adapter/ebgenv.h
@@ -17,74 +17,68 @@

#include <errno.h>

+typedef struct {
+ void *bgenv;
+ bool ebg_new_env_created;
+} ebgenv_t;
+
/** @brief Tell the library to output information for the user.
+ * @param e A pointer to an ebgenv_t context.
+ * @param v A boolean to set verbosity.
*/
-void ebg_beverbose(bool v);
+void ebg_beverbose(ebgenv_t *e, bool v);

/** @brief Initialize environment library and open environment. The first
- * time this function is called, it will create a new environment with the
- * highest revision number for update purposes. Every next time it will
- * just open the environment with the highest revision number.
+ * time this function is called, it will create a new environment with
+ * the highest revision number for update purposes. Every next time it
+ * will just open the environment with the highest revision number.
+ * @param e A pointer to an ebgenv_t context.
* @return 0 on success, errno on failure
*/
-int ebg_env_create_new(void);
+int ebg_env_create_new(ebgenv_t *e);

/** @brief Initialize environment library and open current environment.
+ * @param e A pointer to an ebgenv_t context.
* @return 0 on success, errno on failure
*/
-int ebg_env_open_current(void);
+int ebg_env_open_current(ebgenv_t *e);

/** @brief Retrieve variable content
+ * @param e A pointer to an ebgenv_t context.
* @param key an enum constant to specify the variable
- * @return a pointer to the buffer with the variable content on success,
- * NULL on failure. The returned pointer must not be freed and is freed
- * automatically on closing the environment. If NULL is returned, errno is
- * set.
+ * @param buffer pointer to buffer containing requested value
+ * @return 0 on success, errno on failure
*/
-char *ebg_env_get(char *key);
+int ebg_env_get(ebgenv_t *e, char *key, char* buffer);

/** @brief Store new content into variable
+ * @param e A pointer to an ebgenv_t context.
* @param key name of the environment variable to set
* @param value a string to be stored into the variable
* @return 0 on success, errno on failure
*/
-int ebg_env_set(char *key, char *value);
-
-/** @brief Check if last update was successful
- * @return true if successful, false if not
- */
-bool ebg_env_isupdatesuccessful(void);
-
-/** @brief Reset all stored failure states
- * @return 0 if successful, errno on failure
- */
-int ebg_env_clearerrorstate(void);
-
-/** @brief Check if active env is clean
- * @return true if yes, errno set on failure
- */
-bool ebg_env_isokay(void);
-
-/** @brief Check if active env has state 'installed'
- * @return true if yes, errno set on failure
- */
-bool ebg_env_isinstalled(void);
+int ebg_env_set(ebgenv_t *e, char *key, char *value);

-/** @brief Check if active env is in testing state
- * @return true if yes, errno set on failure
+/** @brief Get global ustate value, accounting for all environments
+ * @param e A pointer to an ebgenv_t context.
+ * @return ustate value
*/
-bool ebg_env_istesting(void);
+uint16_t ebg_env_getglobalstate(ebgenv_t *e);

-/** @brief Confirm environment after update - sets testing and boot_once
- * both to 0
- * @return 0 if successful, errno on failure
+/** @brief Set global ustate value, accounting for all environments
+ * if state is set to zero and updating only current environment if
+ * state is set to a non-zero value.
+ * @param e A pointer to an ebgenv_t context.
+ * @param ustate The global ustate value to set.
+ * @return errno on error, 0 if okay.
*/
-int ebg_env_confirmupdate(void);
+int ebg_env_setglobalstate(ebgenv_t *e, uint16_t ustate);

-/** @brief Closes environment and finalize library. Changes are written
- * before closing.
+/** @brief Closes environment and finalize library. Changes are written before
+ * closing.
+ * @param e A pointer to an ebgenv_t context.
* @return 0 on success, errno on failure
*/
-int ebg_env_close(void);
+int ebg_env_close(ebgenv_t *e);

#endif //__EBGENV_H__
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index e678188..fc3628f 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -281,7 +281,7 @@ int main(int argc, char **argv)
}
int result = 0;
if (!arguments.output_to_file) {
- if (!bgenv_init(BGENVTYPE_FAT)) {
+ if (!bgenv_init()) {
fprintf(stderr,
"Error initializing FAT environment.\n");
return 1;
@@ -291,7 +291,7 @@ int main(int argc, char **argv)
printf("\n----------------------------\n");
printf(" Config Partition #%d ", i);
}
- BGENV *env = bgenv_get_by_index(BGENVTYPE_FAT, i);
+ BGENV *env = bgenv_open_by_index(i);
if (env) {
if (verbosity) {
dump_env(env->data);
@@ -309,7 +309,7 @@ int main(int argc, char **argv)
BGENV *env_current;
if (auto_update) {
/* search latest and oldest revision */
- env_current = bgenv_get_latest(BGENVTYPE_FAT);
+ env_current = bgenv_open_latest();
if (!env_current) {
fprintf(stderr, "Failed to retrieve "
"latest "
@@ -319,7 +319,7 @@ int main(int argc, char **argv)
arguments.tmpdata.revision =
env_current->data->revision + 1;

- env_new = bgenv_get_oldest(BGENVTYPE_FAT);
+ env_new = bgenv_open_oldest();
if (!env_new) {
fprintf(stderr, "Failed to retrieve "
"oldest "
@@ -353,12 +353,10 @@ int main(int argc, char **argv)
}
} else {
if (part_specified) {
- env_new = bgenv_get_by_index(
- BGENVTYPE_FAT,
+ env_new = bgenv_open_by_index(
arguments.which_part);
} else {
- env_new =
- bgenv_get_latest(BGENVTYPE_FAT);
+ env_new = bgenv_open_latest();
}
if (!env_new) {
fprintf(stderr, "Failed to retrieve "
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 994dd59..417e449 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -67,7 +67,7 @@ MOCKOBJS_test_api = $(ENV_API)

MOCKOBJS_SYMBOLS_$(ENV_API)-test_partitions = probe_config_file
MOCKOBJS_SYMBOLS_$(ENV_API)-test_environment = oldenvs configparts fopen fclose fread fwrite feof mount_partition
-MOCKOBJS_SYMBOLS_$(ENV_API)-test_api = bgenv_init bgenv_write bgenv_close bgenv_get_latest bgenv_get_by_index bgenv_get_oldest
+MOCKOBJS_SYMBOLS_$(ENV_API)-test_api = bgenv_init bgenv_write bgenv_close bgenv_open_latest bgenv_open_by_index bgenv_open_oldest
MOCKOBJS_SYMBOLS_ebgpart-test_partitions = ped_device_probe_all ped_device_get_next ped_disk_next_partition

TEST_TARGETS = test_partitions.target test_environment.target test_api.target
diff --git a/tools/tests/test_api.c b/tools/tests/test_api.c
index 53e573d..605a23b 100644
--- a/tools/tests/test_api.c
+++ b/tools/tests/test_api.c
@@ -29,12 +29,10 @@ static BG_ENVDATA dataupdate = {0};
#define DEFAULT_WATCHDOG_TIMEOUT_SEC 30
static int test_env_revision = 42;
static char *test_env_revision_str = "42";
+static ebgenv_t e;

-bool bgenv_init(BGENVTYPE type)
+bool bgenv_init()
{
- if (type != BGENVTYPE_FAT) {
- return false;
- }
return true;
}

@@ -48,17 +46,20 @@ bool bgenv_close(BGENV *env_current)
return true;
}

-BGENV *bgenv_get_by_index(BGENVTYPE type, uint32_t index)
+BGENV *bgenv_open_by_index(uint32_t index)
{
- return &envupdate;
+ if (index == 1) {
+ return &envupdate;
+ }
+ return &env;
}

-BGENV *bgenv_get_latest(BGENVTYPE type)
+BGENV *bgenv_open_latest()
{
return mock_ptr_type(BGENV *);
}

-BGENV *bgenv_get_oldest(BGENVTYPE type)
+BGENV *bgenv_open_oldest()
{
return mock_ptr_type(BGENV *);
}
@@ -67,24 +68,24 @@ static void test_api_openclose(void **state)
{
int ret;

- will_return(bgenv_get_latest, &env);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, 0);
will_return(bgenv_write, true);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, 0);

- will_return(bgenv_get_latest, &env);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, 0);
will_return(bgenv_write, false);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, EIO);

- will_return(bgenv_get_latest, NULL);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, NULL);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, EIO);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, EIO);

(void)state;
@@ -93,51 +94,55 @@ static void test_api_openclose(void **state)
static void test_api_accesscurrent(void **state)
{
int ret;
+ char buffer[4096];

- will_return(bgenv_get_latest, &env);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, 0);
will_return(bgenv_write, true);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, 0);

- ret = ebg_env_set("NonsenseKey", "AnyValue");
+ ret = ebg_env_set(&e, "NonsenseKey", "AnyValue");
assert_int_equal(ret, EINVAL);

- ret = ebg_env_set("kernelfile", "vmlinuz");
+ ret = ebg_env_set(&e, "kernelfile", "vmlinuz");
assert_int_equal(ret, EPERM);

- will_return(bgenv_get_latest, &env);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, 0);

- assert_int_equal(ebg_env_set("kernelfile", "vmlinuz"), 0);
- 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("ustate", "1"), 0);
+ assert_int_equal(ebg_env_set(&e, "kernelfile", "vmlinuz"), 0);
+ assert_int_equal(ebg_env_set(&e, "kernelparams", "root=/dev/sda"), 0);
+ assert_int_equal(ebg_env_set(&e, "watchdog_timeout_sec", "abc"), EINVAL);
+ assert_int_equal(ebg_env_set(&e, "watchdog_timeout_sec", "0013"), 0);
+ assert_int_equal(ebg_env_set(&e, "ustate", "1"), 0);

will_return(bgenv_write, true);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, 0);

- char *value;
- value = ebg_env_get("kernelfile");
- assert_null(value);
- assert_int_equal(errno, EPERM);
+ ret = ebg_env_get(&e, "kernelfile", buffer);
+ assert_int_equal(ret, EPERM);

- will_return(bgenv_get_latest, &env);
- ret = ebg_env_open_current();
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
assert_int_equal(ret, 0);

- 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("ustate"), "1");
- assert_string_equal(ebg_env_get("revision"), test_env_revision_str);
+ assert_int_equal(ebg_env_get(&e, "kernelfile", buffer), 0);
+ assert_string_equal(buffer, "vmlinuz");
+ assert_int_equal(ebg_env_get(&e, "kernelparams", buffer), 0);
+ assert_string_equal(buffer, "root=/dev/sda");
+ assert_int_equal(ebg_env_get(&e, "watchdog_timeout_sec", buffer), 0);
+ assert_string_equal(buffer, "13");
+ assert_int_equal(ebg_env_get(&e, "ustate", buffer), 0);
+ assert_string_equal(buffer, "1");
+ assert_int_equal(ebg_env_get(&e, "revision", buffer), 0);
+ assert_string_equal(buffer, test_env_revision_str);

will_return(bgenv_write, true);
- ret = ebg_env_close();
+ ret = ebg_env_close(&e);
assert_int_equal(ret, 0);

(void)state;
@@ -145,34 +150,37 @@ static void test_api_accesscurrent(void **state)

static void test_api_update(void **state)
{
- will_return(bgenv_get_latest, &env);
- will_return(bgenv_get_oldest, &envupdate);
- assert_int_equal(ebg_env_create_new(), 0);
+ will_return(bgenv_open_latest, &env);
+ will_return(bgenv_open_oldest, &envupdate);
+ assert_int_equal(ebg_env_create_new(&e), 0);

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("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("ustate", "3"), 0);
- assert_false(ebg_env_isupdatesuccessful());
+ assert_int_equal(ebg_env_set(&e, "ustate", "2"), 0);
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ will_return(bgenv_write, true);
+ }
+ assert_int_equal(ebg_env_setglobalstate(&e, 0), 0);

- assert_int_equal(ebg_env_set("revision", "0"), 0);
- assert_int_equal(ebg_env_set("ustate", "0"), 0);
- assert_true(ebg_env_isupdatesuccessful());
+ if (ENV_NUM_CONFIG_PARTS == 1) {
+ will_return(bgenv_open_latest, &envupdate);
+ }
+ assert_int_equal(ebg_env_set(&e, "revision", "0"), 0);
+ assert_int_equal(ebg_env_set(&e, "ustate", "3"), 0);
+ assert_int_equal(ebg_env_getglobalstate(&e), 3);

- assert_int_equal(ebg_env_set("revision", "0"), 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());
+ will_return(bgenv_open_latest, &env);
+ for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
+ will_return(bgenv_write, true);
+ }
+ assert_int_equal(ebg_env_setglobalstate(&e, 0), 0);
+ assert_int_equal(ebg_env_getglobalstate(&e), 0);

will_return(bgenv_write, true);
- assert_int_equal(ebg_env_close(), 0);
+ assert_int_equal(ebg_env_close(&e), 0);

(void)state;
}
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:43 AM9/11/17
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

Use github issues for unsolved tasks that should be
implemented.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/TODO.md | 13 -------------
1 file changed, 13 deletions(-)
delete mode 100644 docs/TODO.md

diff --git a/docs/TODO.md b/docs/TODO.md
deleted file mode 100644
index fe5edd2..0000000
--- a/docs/TODO.md
+++ /dev/null
@@ -1,13 +0,0 @@
-# The following items will be implemented #
-
-
-* Tools modification
- * Make `bg_setenv -c` and the underlying confirm mechanism to backup
- the current working environment to the (latest-1) environment, so
- that if the current environment breaks, there is a backup with the
- latest values.
-
-* API refactoring
- * Function / Datatype / Variable names remind of Parted and should be
- renamed if code developes independent of libparted.
-
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:43 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

Add possibility for user to store arbitrary variables into environment.
Using the API, the user can insert all kind of binary data into the
environment by defining user specific types, stored as `char *`. Please
note that the user is solely responsible for these defined types and the
API itself does not care and only handles raw binary data. The type
field is merely for convenience purpose.
Using `bg_setenv`, the user can specify multiple `-x KEY=VAL` arguments,
which will set corresponding user variables. By omitting the value `VAL`,
the variable can be deleted.
`bg_printenv` outputs a list of user variables and prints their content,
if the type is set to `"String"`. Thus, the type field is not only
practicable for the user, but also for the tools.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
Makefile.am | 1 +
configure.ac | 17 ++++
docs/TODO.md | 16 +--
docs/TOOLS.md | 10 ++
env/env_api.c | 29 +++++-
env/env_api_fat.c | 74 ++++++++++----
env/uservars.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++
include/ebgenv.h | 36 ++++++-
include/env_api.h | 9 +-
include/envdata.h | 1 +
include/uservars.h | 37 +++++++
tools/bg_setenv.c | 82 ++++++++++++++-
tools/tests/test_api.c | 124 ++++++++++++++++++++++-
13 files changed, 665 insertions(+), 37 deletions(-)
create mode 100644 env/uservars.c
create mode 100644 include/uservars.h

diff --git a/Makefile.am b/Makefile.am
index bbef557..af64646 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -44,6 +44,7 @@ lib_LIBRARIES = libebgenv.a
libebgenv_a_SOURCES = \
env/@env_api_file@.c \
env/env_api.c \
+ env/uservars.c \
tools/ebgpart.c

libebgenv_a_CPPFLAGS = \
diff --git a/configure.ac b/configure.ac
index 219c419..514bec7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -155,6 +155,22 @@ AC_ARG_WITH([num-config-parts],
[ ENV_NUM_CONFIG_PARTS=2 ])

AC_DEFINE_UNQUOTED([ENV_NUM_CONFIG_PARTS], [${ENV_NUM_CONFIG_PARTS}], [Number of config partitions])
+
+AC_ARG_WITH([mem-uservars],
+ AS_HELP_STRING([--with-mem-uservars=INT],
+ [specify amount of memory reserved for user variables in bytes]),
+ [
+ ENV_MEM_USERVARS=${withval:-0}
+ AS_IF([test "${ENV_MEM_USERVARS}" -lt "96"],
+ [
+ AC_MSG_ERROR([Minimum space allowed for uservars is 96 bytes])
+ ])
+ ],
+ [
+ ENV_MEM_USERVARS=131072
+ ])
+
+AC_DEFINE_UNQUOTED([ENV_MEM_USERVARS], [${ENV_MEM_USERVARS}], [Reserved memory for user variables])
# ------------------------------------------------------------------------------
AC_CONFIG_FILES([
Makefile
@@ -177,5 +193,6 @@ AC_MSG_RESULT([

environment backend: ${ENV_API_FILE}.c
number of config parts: $ENV_NUM_CONFIG_PARTS
+ reserved for uservars: $ENV_MEM_USERVARS bytes
])

diff --git a/docs/TODO.md b/docs/TODO.md
index 29b7fe0..6f104ab 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -6,12 +6,16 @@
the current working environment to the (latest-1) environment, so
that if the current environment breaks, there is a backup with the
latest values.
-
-* Application specific variables
- * applications may need to store their own variables into the
- bootloader environment. Currently this is not possible. A generic
- method must be defined and implemented to account for generic
- key-value pairs.
+ * Currently, `bg_setenv` generates a virtual environment copy while
+ parsing arguments and later uses an algorithm to find out, how to
+ merge this with the actual environment being modified. This led to
+ the introduction of a special marker byte which tells the algorithm
+ to not touch the original content. Deletion of user variables led to
+ another special case, where *negative* variables had to be defined by
+ a special `DELETED` type to tell the algorithm that the specific user
+ variable has to be deleted. This is rather complicated and a better
+ aproach has already been discussed using a journal with actions
+ instead of a prebuilt state.

* API refactoring
* Function / Datatype / Variable names remind of Parted and should be
diff --git a/docs/TOOLS.md b/docs/TOOLS.md
index 9883589..7dd1f83 100644
--- a/docs/TOOLS.md
+++ b/docs/TOOLS.md
@@ -83,3 +83,13 @@ issue:
```
bg_setenv --partition=1 --ustate=TESTING
```
+
+### Setting user variables ###
+
+`bg_setenv` has support for default user variables, meaning of type "String". To set a user variable, specify the `-x` flag:
+
+```
+bg_setenv -x key=value
+```
+
+This will set the variable named `key` to `value` in the current environment.
diff --git a/env/env_api.c b/env/env_api.c
index 26d823d..ed07c7f 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -12,6 +12,7 @@

#include "env_api.h"
#include "ebgenv.h"
+#include "uservars.h"

/* UEFI uses 16-bit wide unicode strings.
* However, wchar_t support functions are fixed to 32-bit wide
@@ -90,10 +91,34 @@ int ebg_env_get(ebgenv_t *e, char *key, char *buffer)
ENV_STRING_LENGTH);
}

+int ebg_env_get_ex(ebgenv_t *e, char *key, char *usertype, uint8_t *buffer,
+ size_t maxlen)
+{
+ return bgenv_get((BGENV *)e->bgenv, key, usertype, buffer, maxlen);
+}
+
int ebg_env_set(ebgenv_t *e, char *key, char *value)
{
- return bgenv_set((BGENV *)e->bgenv, key, "String", value,
- strlen(value)); }
+ return bgenv_set((BGENV *)e->bgenv, key, USERVAR_TYPE_DEFAULT, value,
+ strlen(value) + 1);
+}
+
+int ebg_env_set_ex(ebgenv_t *e, char *key, char *usertype, uint8_t *value,
+ size_t datalen)
+{
+ return bgenv_set((BGENV *)e->bgenv, key, usertype, value, datalen);
+}
+
+size_t ebg_env_user_free(ebgenv_t *e)
+{
+ if (!e->bgenv) {
+ return 0;
+ }
+ if (!((BGENV *)e->bgenv)->data) {
+ return 0;
+ }
+ return bgenv_user_free(((BGENV *)e->bgenv)->data->userdata);
+}

uint16_t ebg_env_getglobalstate(ebgenv_t *e)
{
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 10fe37b..ca9beb8 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -12,11 +12,12 @@

#include "env_api.h"
#include "ebgpart.h"
+#include "uservars.h"
#include "test-interface.h"

const char *tmp_mnt_dir = "/tmp/mnt-XXXXXX";

-static bool verbosity = false;
+bool bgenv_verbosity = false;

static EBGENVKEY bgenv_str2enum(char *key)
{
@@ -41,7 +42,7 @@ static EBGENVKEY bgenv_str2enum(char *key)

void bgenv_be_verbose(bool v)
{
- verbosity = v;
+ bgenv_verbosity = v;
ebgpart_beverbose(v);
}

@@ -439,52 +440,84 @@ bool bgenv_close(BGENV *env)
return false;
}

-int bgenv_get(BGENV *env, char *key, char **type, void *data, size_t maxlen)
+int bgenv_get(BGENV *env, char *key, char *type, void *data, size_t maxlen)
{
EBGENVKEY e;
+ char buffer[ENV_STRING_LENGTH];

- if (!key || !data || maxlen == 0) {
+ if (!key || maxlen == 0) {
return EINVAL;
}
e = bgenv_str2enum(key);
- if (e == EBGENV_UNKNOWN) {
- return EINVAL;
- }
if (!env) {
return EPERM;
}
+ if (e == EBGENV_UNKNOWN) {
+ if (!data) {
+ uint8_t *u;
+ size_t size;
+ u = bgenv_find_uservar(env->data->userdata, key);
+ bgenv_map_uservar(u, NULL, NULL, NULL, NULL, &size);
+ return size;
+ }
+ return bgenv_get_uservar(env->data->userdata, key, type, data,
+ maxlen);
+ }
switch (e) {
case EBGENV_KERNELFILE:
- str16to8(data, env->data->kernelfile);
+ str16to8(buffer, env->data->kernelfile);
+ if (!data) {
+ return strlen(buffer);
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
if (type) {
- sprintf(*type, "char*");
+ sprintf(type, "char*");
}
break;
case EBGENV_KERNELPARAMS:
- str16to8(data, env->data->kernelparams);
+ str16to8(buffer, env->data->kernelparams);
+ if (!data) {
+ return strlen(buffer);
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
if (type) {
- sprintf(*type, "char*");
+ sprintf(type, "char*");
}
break;
case EBGENV_WATCHDOG_TIMEOUT_SEC:
- sprintf(data, "%lu", env->data->watchdog_timeout_sec);
+ sprintf(buffer, "%lu", env->data->watchdog_timeout_sec);
+ if (!data) {
+ return strlen(buffer);
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
if (type) {
- sprintf(*type, "uint16_t");
+ sprintf(type, "uint16_t");
}
break;
case EBGENV_REVISION:
- sprintf(data, "%lu", env->data->revision);
+ sprintf(buffer, "%lu", env->data->revision);
+ if (!data) {
+ return strlen(buffer);
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
if (type) {
- sprintf(*type, "uint32_t");
+ sprintf(type, "uint32_t");
}
break;
case EBGENV_USTATE:
- sprintf(data, "%u", env->data->ustate);
+ sprintf(buffer, "%u", env->data->ustate);
+ if (!data) {
+ return strlen(buffer);
+ }
+ strncpy(data, buffer, strlen(buffer)+1);
if (type) {
- sprintf(*type, "uint16_t");
+ sprintf(type, "uint16_t");
}
break;
default:
+ if (!data) {
+ return 0;
+ }
return EINVAL;
}
return 0;
@@ -502,12 +535,13 @@ int bgenv_set(BGENV *env, char *key, char *type, void *data, size_t datalen)
}

e = bgenv_str2enum(key);
- if (e == EBGENV_UNKNOWN) {
- return EINVAL;
- }
if (!env) {
return EPERM;
}
+ if (e == EBGENV_UNKNOWN) {
+ return bgenv_set_uservar(env->data->userdata, key, type, data,
+ datalen);
+ }
switch (e) {
case EBGENV_REVISION:
val = strtol(value, &p, 10);
diff --git a/env/uservars.c b/env/uservars.c
new file mode 100644
index 0000000..8584d12
--- /dev/null
+++ b/env/uservars.c
@@ -0,0 +1,266 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2017
+ *
+ * Authors:
+ * Andreas Reichel <andreas.r...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include <string.h>
+#include "env_api.h"
+#include "uservars.h"
+
+void bgenv_map_uservar(uint8_t *udata, char **key, char **type, uint8_t **val,
+ size_t *record_size, size_t *data_size)
+{
+ /* Each user variable is encoded as follows:
+ * |------------|------------|-------------|----------------|
+ * | char KEY[] | size_t len | char type[] | uint8_t data[] |
+ * |------------|------------|-------------|----------------|
+ * | KEY | - - - - - - - PAYLOAD - - - - - - - - |
+ *
+ * stored in 'len' is the payload size
+ */
+ char *var_key;
+ size_t *payload_size;
+ char *var_type;
+ uint8_t *data;
+
+ /* Get the key */
+ var_key = (char *)udata;
+ if (key) {
+ *key = var_key;
+ }
+
+ /* Get position of payload size */
+ payload_size = (size_t *)(var_key + strlen(var_key) + 1);
+
+ /* Calculate the record size (size of the whole thing) */
+ if (record_size) {
+ *record_size = *payload_size + strlen(var_key) + 1;
+ }
+
+ /* Get position of the type field */
+ var_type = (char *)payload_size + sizeof(size_t);
+ if (type) {
+ *type = var_type;
+ }
+
+ /* Calculate the data size */
+ if (data_size) {
+ *data_size = *payload_size - sizeof(size_t) -
+ strlen(var_type) - 1;
+ }
+ /* Get the pointer to the data field */
+ data = (uint8_t *)(var_type + strlen(var_type) + 1);
+ if (val) {
+ *val = data;
+ }
+}
+
+void bgenv_serialize_uservar(uint8_t *p, char *key, char *type, void *data,
+ size_t record_size)
+{
+ size_t payload_size, data_size;
+
+ /* store key */
+ strncpy(p, key, strlen(key) + 1);
+ p += strlen(key) + 1;
+
+ /* store payload_size after key */
+ payload_size = record_size - strlen(key) - 1;
+ memcpy(p, &payload_size, sizeof(size_t));
+ p += sizeof(size_t);
+
+ /* store datatype */
+ memcpy(p, type, strlen(type) + 1);
+ p += strlen(type) + 1;
+
+ /* store data */
+ data_size = payload_size - strlen(type) - 1 - sizeof(size_t);
+ memcpy(p, data, data_size);
+}
+
+int bgenv_get_uservar(uint8_t *udata, char *key, char *type, void *data,
+ size_t maxlen)
+{
+ uint8_t *uservar, *value;
+ char *lkey, *ltype;
+ size_t dsize;
+
+ uservar = bgenv_find_uservar(udata, key);
+
+ if (!uservar) {
+ return EINVAL;
+ }
+
+ bgenv_map_uservar(uservar, &lkey, &ltype, &value, NULL, &dsize);
+
+ if (dsize > maxlen) {
+ dsize = maxlen;
+ }
+
+ memcpy(data, value, dsize);
+
+ if (type) {
+ memcpy(type, ltype, strlen(ltype) + 1);
+ }
+
+ return 0;
+}
+
+int bgenv_set_uservar(uint8_t *udata, char *key, char *type, void *data,
+ size_t datalen)
+{
+ size_t total_size;
+ uint8_t *p;
+
+ total_size = datalen + strlen(type) + 1 + sizeof(size_t) +
+ strlen(key) + 1;
+
+ p = bgenv_find_uservar(udata, key);
+ if (p) {
+ if (strncmp(type, USERVAR_TYPE_DELETED,
+ strlen(USERVAR_TYPE_DELETED) + 1) == 0) {
+ bgenv_del_uservar(udata, p);
+ return 0;
+ }
+
+ p = bgenv_uservar_realloc(udata, total_size, p);
+ } else {
+ p = bgenv_uservar_alloc(udata, total_size);
+ }
+ if (!p) {
+ return errno;
+ }
+
+ bgenv_serialize_uservar(p, key, type, data, total_size);
+
+ return 0;
+}
+
+uint8_t *bgenv_find_uservar(uint8_t *udata, char *key)
+{
+ char *varkey;
+
+ if (!udata) {
+ return NULL;
+ }
+ while (*udata) {
+ bgenv_map_uservar(udata, &varkey, NULL, NULL, NULL, NULL);
+
+ if (strncmp(varkey, key, strlen(key) + 1) == 0) {
+ return udata;
+ }
+ udata = bgenv_next_uservar(udata);
+ }
+ return NULL;
+}
+
+uint8_t *bgenv_next_uservar(uint8_t *udata)
+{
+ size_t record_size;
+
+ bgenv_map_uservar(udata, NULL, NULL, NULL, &record_size, NULL);
+
+ return udata + record_size;
+}
+
+uint8_t *bgenv_uservar_alloc(uint8_t *udata, size_t datalen)
+{
+ size_t spaceleft;
+
+ if (!udata) {
+ errno = EINVAL;
+ return NULL;
+ }
+ spaceleft = bgenv_user_free(udata);
+ VERBOSE(stdout, "uservar_alloc: free: %u requested: %u \n", spaceleft,
+ datalen);
+
+ /* To find the end of user variables, a 2nd 0 must be there after the
+ * last variable content, thus, we need one extra byte if appending a
+ * new variable. */
+ if (spaceleft < datalen + 1) {
+ errno = ENOMEM;
+ return NULL;
+ }
+
+ return udata + (ENV_MEM_USERVARS - spaceleft);
+}
+
+uint8_t *bgenv_uservar_realloc(uint8_t *udata, size_t new_rsize,
+ uint8_t *p)
+{
+ size_t spaceleft;
+ size_t rsize;
+
+ bgenv_map_uservar(p, NULL, NULL, NULL, &rsize, NULL);
+
+ /* Is the new record size equal to the old, so that we can
+ * keep the variable in place? */
+ if (new_rsize == rsize) {
+ return p;
+ }
+
+ /* Delete variable and return pointer to end of whole user vars */
+ bgenv_del_uservar(udata, p);
+
+ spaceleft = bgenv_user_free(udata);
+
+ if (spaceleft < new_rsize - 1) {
+ errno = ENOMEM;
+ return NULL;
+ }
+
+ return udata + ENV_MEM_USERVARS - spaceleft;
+}
+
+void bgenv_del_uservar(uint8_t *udata, uint8_t *var)
+{
+ size_t spaceleft;
+ size_t rsize;
+
+ /* Get the record size of the variable */
+ bgenv_map_uservar(var, NULL, NULL, NULL, &rsize, NULL);
+
+ /* Move variable out of place and close gap. */
+ spaceleft = bgenv_user_free(udata);
+
+ memmove(var,
+ var + rsize,
+ ENV_MEM_USERVARS - spaceleft - (var - udata) - rsize);
+
+ spaceleft = spaceleft + rsize;
+
+ memset(udata + ENV_MEM_USERVARS - spaceleft, 0, spaceleft);
+}
+
+size_t bgenv_user_free(uint8_t *udata)
+{
+ size_t rsize;
+ size_t spaceleft;
+
+ spaceleft = ENV_MEM_USERVARS;
+
+ if (!udata) {
+ return 0;
+ }
+ if (!*udata) {
+ return spaceleft;
+ }
+
+ while (*udata) {
+ bgenv_map_uservar(udata, NULL, NULL, NULL, &rsize, NULL);
+ spaceleft -= rsize;
+ if (spaceleft <= 0)
+ break;
+ udata = bgenv_next_uservar(udata);
+ }
+
+ return spaceleft;
+}
diff --git a/include/ebgenv.h b/include/ebgenv.h
index fb3e809..f89b23d 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -46,8 +46,11 @@ int ebg_env_open_current(ebgenv_t *e);
/** @brief Retrieve variable content
* @param e A pointer to an ebgenv_t context.
* @param key an enum constant to specify the variable
- * @param buffer pointer to buffer containing requested value
- * @return 0 on success, errno on failure
+ * @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
+ * If buffer == NULL: needed buffer size, 0 if variable
+ * is not found.
*/
int ebg_env_get(ebgenv_t *e, char *key, char* buffer);

@@ -59,6 +62,35 @@ int ebg_env_get(ebgenv_t *e, char *key, char* buffer);
*/
int ebg_env_set(ebgenv_t *e, char *key, char *value);

+/** @brief Store new content into variable
+ * @param e A pointer to an ebgenv_t context.
+ * @param key name of the environment variable to set
+ * @param datatype user specific string to identify the datatype of the value
+ * @param value arbitrary data to be stored into the variable
+ * @param datalen length of the data to be stored into the variable
+ * @return 0 on success, errno on failure
+ */
+int ebg_env_set_ex(ebgenv_t *e, char *key, char *datatype, uint8_t *value,
+ size_t datalen);
+
+/** @brief Get content of user variable
+ * @param e A pointer to an ebgenv_t context.
+ * @param key name of the environment variable to retrieve
+ * @param datatype buffer for user specific string to identify 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
+ */
+int ebg_env_get_ex(ebgenv_t *e, char *key, char *datatype, uint8_t *buffer,
+ size_t maxlen);
+
+/** @brief Get available space for user variables
+ * @param e A pointer to an ebgenv_t context.
+ * @return Free space in bytes
+ */
+size_t ebg_env_user_free(ebgenv_t *e);
+
/** @brief Get global ustate value, accounting for all environments
* @param e A pointer to an ebgenv_t context.
* @return ustate value
diff --git a/include/env_api.h b/include/env_api.h
index 8fe5454..e8ca99f 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -38,10 +38,15 @@
}
#endif

+extern bool bgenv_verbosity;
+
#define VERBOSE(o, ...) \
- if (verbosity) \
+ if (bgenv_verbosity) \
fprintf(o, __VA_ARGS__)

+#define USERVAR_TYPE_DEFAULT "String"
+#define USERVAR_TYPE_DELETED "\0xDE\0xAD"
+
typedef enum {
EBGENV_KERNELFILE,
EBGENV_KERNELPARAMS,
@@ -76,7 +81,7 @@ extern BG_ENVDATA *bgenv_read(BGENV *env);
extern bool bgenv_close(BGENV *env);

extern BGENV *bgenv_create_new(void);
-extern int bgenv_get(BGENV *env, char *key, char **type, void *data,
+extern int bgenv_get(BGENV *env, char *key, char *type, void *data,
size_t maxlen);
extern int bgenv_set(BGENV *env, char *key, char *type, void *data,
size_t datalen);
diff --git a/include/envdata.h b/include/envdata.h
index 3f1fb38..011053a 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -38,6 +38,7 @@ struct _BG_ENVDATA {
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
+ uint8_t userdata[ENV_MEM_USERVARS];
uint32_t crc32;
};
#pragma pack(pop)
diff --git a/include/uservars.h b/include/uservars.h
new file mode 100644
index 0000000..a0abb74
--- /dev/null
+++ b/include/uservars.h
@@ -0,0 +1,37 @@
+/*
+ * EFI Boot Guard
+ *
+ * Copyright (c) Siemens AG, 2017
+ *
+ * Authors:
+ * Andreas Reichel <andreas.r...@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef __USER_VARS_H__
+#define __USER_VARS_H__
+
+#include <stdint.h>
+
+void bgenv_map_uservar(uint8_t *udata, char **key, char **type, uint8_t **val,
+ size_t *record_size, size_t *data_size);
+void bgenv_serialize_uservar(uint8_t *p, char *key, char *type, void *data,
+ size_t record_size);
+
+int bgenv_get_uservar(uint8_t *udata, char *key, char *type, void *data,
+ size_t maxlen);
+int bgenv_set_uservar(uint8_t *udata, char *key, char *type, void *data,
+ size_t datalen);
+
+uint8_t *bgenv_find_uservar(uint8_t *udata, char *key);
+uint8_t *bgenv_next_uservar(uint8_t *udata);
+
+uint8_t *bgenv_uservar_alloc(uint8_t *udata, size_t datalen);
+uint8_t *bgenv_uservar_realloc(uint8_t *udata, size_t new_rsize,
+ uint8_t *p);
+void bgenv_del_uservar(uint8_t *udata, uint8_t *var);
+size_t bgenv_user_free(uint8_t *udata);
+
+#endif // __USER_VARS_H__
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 3911ca3..0c37452 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -11,6 +11,7 @@
*/

#include "env_api.h"
+#include "uservars.h"

static char doc[] =
"bg_setenv/bg_printenv - Environment tool for the EFI Boot Guard";
@@ -31,6 +32,9 @@ 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"},
+ {"uservar", 'x', "KEY=VAL", 0, "Set user-defined string variable. For "
+ "setting multiple variables, use this "
+ "option multiple times."},
{0}};

static struct argp_option options_printenv[] = {
@@ -85,6 +89,27 @@ static char *ustate2str(uint8_t ustate)
}
}

+static int set_uservars(uint8_t *uservars, char *arg)
+{
+ char *key, *value, *end_key;
+
+ key = strtok_r(arg, "=", &end_key);
+ if (key == NULL) {
+ return 0;
+ }
+
+ value = strtok_r(NULL, "=", &end_key);
+ if (value == NULL) {
+ bgenv_set_uservar(uservars, key, USERVAR_TYPE_DELETED, NULL, 0);
+ return 0;
+ }
+
+ bgenv_set_uservar(uservars, key, USERVAR_TYPE_DEFAULT, value,
+ strlen(value) + 1);
+
+ return 0;
+}
+
static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
@@ -200,6 +225,10 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
/* Set verbosity in the library */
bgenv_be_verbose(true);
break;
+ case 'x':
+ /* Set user-defined variable(s) */
+ set_uservars(arguments->tmpdata.userdata, arg);
+ break;
case ARGP_KEY_ARG:
/* too many arguments - program terminates with call to
* argp_usage with non-zero return code */
@@ -211,6 +240,33 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
return 0;
}

+static void merge_uservars(uint8_t *dest, uint8_t *src)
+{
+ char *key, *val, *type;
+ size_t rsize, dsize;
+ uint8_t *var;
+
+ if (!src || !dest) {
+ return;
+ }
+
+ while (*src) {
+ bgenv_map_uservar(src, &key, &type, (uint8_t **)&val, &rsize,
+ &dsize);
+ if (strncmp(type, USERVAR_TYPE_DELETED,
+ strlen(USERVAR_TYPE_DELETED) - 1) == 0) {
+ var = bgenv_find_uservar(dest, key);
+ if (var) {
+ bgenv_del_uservar(dest, var);
+ }
+ } else {
+ bgenv_set_uservar(dest, key, type, val,
+ strlen(val) + 1);
+ }
+ src = bgenv_next_uservar(src);
+ }
+}
+
static void update_environment(BG_ENVDATA *dest, BG_ENVDATA *src)
{
if (!dest || !src) {
@@ -237,10 +293,30 @@ static void update_environment(BG_ENVDATA *dest, BG_ENVDATA *src)
(void *)&src->watchdog_timeout_sec,
sizeof(src->watchdog_timeout_sec));
}
+ merge_uservars(dest->userdata, src->userdata);
+
dest->crc32 =
crc32(0, (Bytef *)dest, sizeof(BG_ENVDATA) - sizeof(dest->crc32));
}

+static void dump_uservars(uint8_t *udata)
+{
+ char *key, *value, *type;
+ size_t rsize, dsize;
+
+ while (*udata) {
+ bgenv_map_uservar(udata, &key, &type, (uint8_t **)&value,
+ &rsize, &dsize);
+ printf("%s ", key);
+ if (strcmp(type, USERVAR_TYPE_DEFAULT) == 0) {
+ printf("= %s\n", value);
+ } else {
+ printf("( User defined type )");
+ }
+ udata = bgenv_next_uservar(udata);
+ }
+}
+
static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];
@@ -251,6 +327,9 @@ static void dump_env(BG_ENVDATA *env)
printf("watchdog timeout: %u seconds\n", env->watchdog_timeout_sec);
printf("ustate: %u (%s)\n", (uint8_t)env->ustate,
ustate2str(env->ustate));
+ printf("\n");
+ printf("user variables:\n");
+ dump_uservars(env->userdata);
printf("\n\n");
}

@@ -274,7 +353,8 @@ int main(int argc, char **argv)
arguments.which_part = 0;
memset((void *)&arguments.tmpdata, IGNORE_MARKER_BYTE,
sizeof(BG_ENVDATA));
-
+ memset((void *)&arguments.tmpdata.userdata, 0,
+ sizeof(arguments.tmpdata.userdata));
error_t e;
if (e = argp_parse(argp, argc, argv, 0, 0, &arguments)) {
return e;
diff --git a/tools/tests/test_api.c b/tools/tests/test_api.c
index 605a23b..09c8d3e 100644
--- a/tools/tests/test_api.c
+++ b/tools/tests/test_api.c
@@ -103,9 +103,6 @@ static void test_api_accesscurrent(void **state)
ret = ebg_env_close(&e);
assert_int_equal(ret, 0);

- ret = ebg_env_set(&e, "NonsenseKey", "AnyValue");
- assert_int_equal(ret, EINVAL);
-
ret = ebg_env_set(&e, "kernelfile", "vmlinuz");
assert_int_equal(ret, EPERM);

@@ -185,12 +182,131 @@ static void test_api_update(void **state)
(void)state;
}

+static void test_api_uservars(void **state)
+{
+ int ret;
+ char *test_key = "NonsenseKey";
+ char *test_key2 = "TestKey2";
+
+ char *test_val = "AnyValue";
+ char *test_val2 = "BnyVbluf";
+ char *test_val3 = "TESTTESTTESTTEST";
+ char *test_val4 = "abc";
+ char buffer[ENV_MEM_USERVARS];
+ size_t space_left;
+
+ will_return(bgenv_open_latest, &env);
+ ret = ebg_env_open_current(&e);
+ assert_int_equal(ret, 0);
+
+ assert_int_equal(ebg_env_user_free(&e), ENV_MEM_USERVARS);
+
+ ret = ebg_env_set(&e, test_key, test_val);
+ assert_int_equal(ret, 0);
+
+ space_left = ENV_MEM_USERVARS - strlen(test_key) - 1
+ - strlen(test_val) - 1 - sizeof(size_t)
+ - strlen(USERVAR_TYPE_DEFAULT) - 1;
+
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ ret = ebg_env_get(&e, test_key, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val);
+
+ // replace value with same length value
+ ret = ebg_env_set(&e, test_key, test_val2);
+ assert_int_equal(ret, 0);
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ ret = ebg_env_get(&e, test_key, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val2);
+
+ // replace value with larger value
+ ret = ebg_env_set(&e, test_key, test_val3);
+ assert_int_equal(ret, 0);
+
+ space_left = ENV_MEM_USERVARS - strlen(test_key) - 1
+ - strlen(test_val3) - 1 - sizeof(size_t)
+ - strlen(USERVAR_TYPE_DEFAULT) - 1;
+
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ // replace value with smaller value
+ ret = ebg_env_set(&e, test_key, test_val4);
+ assert_int_equal(ret, 0);
+
+ space_left = ENV_MEM_USERVARS - strlen(test_key) - 1
+ - strlen(test_val4) - 1 - sizeof(size_t)
+ - strlen(USERVAR_TYPE_DEFAULT) - 1;
+
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ // add 2nd variable
+ ret = ebg_env_set(&e, test_key2, test_val2);
+ assert_int_equal(ret, 0);
+
+ space_left = space_left - strlen(test_key2) - 1
+ - strlen(test_val2) - 1 - sizeof(size_t)
+ - strlen(USERVAR_TYPE_DEFAULT) - 1;
+
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ // retrieve both variables
+ ret = ebg_env_get(&e, test_key2, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val2);
+ ret = ebg_env_get(&e, test_key, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val4);
+
+ // overwrite first variable
+ ret = ebg_env_set(&e, test_key, test_val3);
+ assert_int_equal(ret, 0);
+
+ space_left = space_left + strlen(test_val4)
+ - strlen(test_val3);
+ assert_int_equal(ebg_env_user_free(&e), space_left);
+
+ // retrieve both variables
+ ret = ebg_env_get(&e, test_key2, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val2);
+ ret = ebg_env_get(&e, test_key, buffer);
+ assert_int_equal(ret, 0);
+ assert_string_equal(buffer, test_val3);
+
+ void *dummymem = malloc(space_left);
+
+ // test out of memory
+ ret = ebg_env_set_ex(&e, "A", "B", dummymem, space_left);
+ free(dummymem);
+
+ assert_int_equal(ret, ENOMEM);
+
+ // test user data type
+ ret = ebg_env_set_ex(&e, "A", "B", "C", 2);
+ assert_int_equal(ret, 0);
+
+ char type[2];
+ char data[2];
+
+ ret = ebg_env_get_ex(&e, "A", type, data, sizeof(data));
+ assert_int_equal(ret, 0);
+ assert_string_equal("B", type);
+ assert_string_equal("C", data);
+
+ (void)state;
+}
+
int main(void)
{
const struct CMUnitTest tests[] = {
cmocka_unit_test(test_api_openclose),
cmocka_unit_test(test_api_accesscurrent),
- cmocka_unit_test(test_api_update)};
+ cmocka_unit_test(test_api_update),
+ cmocka_unit_test(test_api_uservars)};

env.data = &data;
data.revision = test_env_revision;
--
2.14.1

Andreas J. Reichel

unread,
Sep 11, 2017, 7:35:43 AM9/11/17
to efibootg...@googlegroups.com, Reichel Andreas
From: Reichel Andreas <andreas.r...@siemens.com>

* bg_setenv uses a virtual environment as a working copy. Changes are
stored as a temporary state.
However this makes it rather complicated to implement deletion of user
variables. A similarly ugly implementation is the use of a special
marker byte to indicate a part of the working copy to not be updated
to the real environment. By using a list as journal for outstanding
tasks, tasks can be accumulated while parsing the arguments and
applied in the end.

* Furthermore, the structure of the main function is simplified.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/TODO.md | 10 --
tools/bg_setenv.c | 432 ++++++++++++++++++++++++++++++------------------------
2 files changed, 242 insertions(+), 200 deletions(-)

diff --git a/docs/TODO.md b/docs/TODO.md
index 6f104ab..fe5edd2 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -6,16 +6,6 @@
the current working environment to the (latest-1) environment, so
that if the current environment breaks, there is a backup with the
latest values.
- * Currently, `bg_setenv` generates a virtual environment copy while
- parsing arguments and later uses an algorithm to find out, how to
- merge this with the actual environment being modified. This led to
- the introduction of a special marker byte which tells the algorithm
- to not touch the original content. Deletion of user variables led to
- another special case, where *negative* variables had to be defined by
- a special `DELETED` type to tell the algorithm that the specific user
- variable has to be deleted. This is rather complicated and a better
- aproach has already been discussed using a journal with actions
- instead of a prebuilt state.

* API refactoring
* Function / Datatype / Variable names remind of Parted and should be
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 0c37452..5a3aa3e 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -10,7 +10,10 @@
* the COPYING file in the top-level directory.
*/

+#include <sys/queue.h>
+
#include "env_api.h"
+#include "ebgenv.h"
#include "uservars.h"

static char doc[] =
@@ -44,15 +47,90 @@ static struct argp_option options_printenv[] = {
struct arguments {
bool output_to_file;
int which_part;
- BG_ENVDATA tmpdata;
};

-/* The following byte is used to mark parts of a temporary
- * environment structure as to be ignored and not overwrite
- * values of the real environment. Can be anything
- * except used ASCII codes (should be > 0x7F).
- */
-#define IGNORE_MARKER_BYTE 0xEA
+typedef enum { ENV_TASK_SET, ENV_TASK_DEL } BGENV_TASK;
+
+struct stailhead *headp;
+struct env_action {
+ char *key;
+ char *type;
+ uint8_t *data;
+ BGENV_TASK task;
+ STAILQ_ENTRY(env_action) journal;
+};
+
+STAILQ_HEAD(stailhead, env_action) head = STAILQ_HEAD_INITIALIZER(head);
+
+static void journal_add_action(BGENV_TASK task, char *key, char *type,
+ uint8_t *data, size_t datalen)
+{
+ struct env_action *new_action;
+
+ new_action = calloc(1, sizeof(struct env_action));
+ new_action->task = task;
+ if (key) {
+ asprintf(&(new_action->key), "%s", key);
+ }
+ if (type) {
+ asprintf(&(new_action->type), "%s", type);
+ }
+ if (data && datalen) {
+ new_action->data = (uint8_t *)malloc(datalen);
+ memcpy(new_action->data, data, datalen);
+ }
+ STAILQ_INSERT_TAIL(&head, new_action, journal);
+}
+
+static void journal_free_action(struct env_action *action)
+{
+ if (action->data) free(action->data);
+ if (action->type) free(action->type);
+ if (action->key) free(action->key);
+}
+
+static void journal_process_action(BGENV *env, struct env_action *action)
+{
+ uint8_t *var;
+ ebgenv_t e;
+ uint16_t ustate;
+ char *arg, *tmp;
+ int ret;
+
+ switch (action->task) {
+ case ENV_TASK_SET:
+ VERBOSE(stdout, "Task = SET, key = %s, type = %s, val = %s\n",
+ action->key, action->type, (char *)action->data);
+ if (strncmp(action->key, "ustate", strlen("ustate")+1) == 0) {
+ e.bgenv = env;
+ arg = (char *)action->data;
+ errno = 0;
+ ustate = strtol(arg, &tmp, 10);
+ if ((errno == ERANGE && (ustate == LONG_MAX ||
+ ustate == LONG_MIN)) ||
+ (errno != 0 && ustate == 0) || (tmp == arg)) {
+ fprintf(stderr, "Invalid value for ustate: %s",
+ (char *)action->data);
+ return;
+ }
+ if (ret = ebg_env_setglobalstate(&e, ustate) != 0) {
+ fprintf(stderr, "Error setting global state.",
+ strerror(ret));
+ }
+ return;
+ }
+ bgenv_set(env, action->key, action->type, action->data,
+ strlen(action->data) + 1);
+ break;
+ case ENV_TASK_DEL:
+ VERBOSE(stdout, "Task = DEL, key = %s\n", action->key);
+ var = bgenv_find_uservar(env->data->userdata, action->key);
+ if (var) {
+ bgenv_del_uservar(env->data->userdata, var);
+ }
+ break;
+ }
+}

/* auto update feature automatically updates partition with
* oldest environment revision (lowest value)
@@ -89,23 +167,23 @@ static char *ustate2str(uint8_t ustate)
}
}

-static int set_uservars(uint8_t *uservars, char *arg)
+static int set_uservars(char *arg)
{
- char *key, *value, *end_key;
+ char *key, *value;

- key = strtok_r(arg, "=", &end_key);
+ key = strtok(arg, "=");
if (key == NULL) {
return 0;
}

- value = strtok_r(NULL, "=", &end_key);
+ value = strtok(NULL, "=");
if (value == NULL) {
- bgenv_set_uservar(uservars, key, USERVAR_TYPE_DELETED, NULL, 0);
+ journal_add_action(ENV_TASK_DEL, key, NULL, NULL, 0);
return 0;
}

- bgenv_set_uservar(uservars, key, USERVAR_TYPE_DEFAULT, value,
- strlen(value) + 1);
+ journal_add_action(ENV_TASK_SET, key, USERVAR_TYPE_DEFAULT, value,
+ strlen(value) + 1);

return 0;
}
@@ -126,8 +204,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ENV_STRING_LENGTH);
return 1;
}
- memcpy(arguments->tmpdata.kernelfile, str8to16(buffer, arg),
- strlen(arg) * 2 + 2);
+ journal_add_action(ENV_TASK_SET, "kernelfile", "String", arg,
+ strlen(arg) + 1);
break;
case 'a':
if (strlen(arg) > ENV_STRING_LENGTH) {
@@ -137,8 +215,8 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ENV_STRING_LENGTH);
return 1;
}
- memcpy(arguments->tmpdata.kernelparams, str8to16(buffer, arg),
- strlen(arg) * 2 + 2);
+ journal_add_action(ENV_TASK_SET, "kernelparams", "String", arg,
+ strlen(arg) + 1);
break;
case 'p':
i = strtol(arg, &tmp, 10);
@@ -178,7 +256,9 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
ustatemap[3]);
return 1;
} else {
- arguments->tmpdata.ustate = i;
+ asprintf(&tmp, "%u", i);
+ journal_add_action(ENV_TASK_SET, "ustate", "String",
+ tmp, strlen(tmp) + 1);
VERBOSE(stdout, "Ustate set to %u (%s).\n", i,
ustate2str(i));
}
@@ -186,14 +266,16 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
case 'r':
i = atoi(arg);
VERBOSE(stdout, "Revision is set to %d.\n", i);
- arguments->tmpdata.revision = i;
+ journal_add_action(ENV_TASK_SET, "revision", "String", arg,
+ strlen(arg) + 1);
break;
case 'w':
i = atoi(arg);
if (i != 0) {
VERBOSE(stdout,
"Setting watchdog timeout to %d seconds.\n", i);
- arguments->tmpdata.watchdog_timeout_sec = i;
+ journal_add_action(ENV_TASK_SET, "watchdog_timeout_sec",
+ "String", arg, strlen(arg) + 1);
} else {
fprintf(stderr, "Watchdog timeout must be non-zero.\n");
return 1;
@@ -207,7 +289,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.ustate = 0;
+ journal_add_action(ENV_TASK_SET, "ustate", "String", "0", 2);
break;
case 'u':
if (part_specified) {
@@ -227,7 +309,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'x':
/* Set user-defined variable(s) */
- set_uservars(arguments->tmpdata.userdata, arg);
+ set_uservars(arg);
break;
case ARGP_KEY_ARG:
/* too many arguments - program terminates with call to
@@ -240,65 +322,6 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
return 0;
}

-static void merge_uservars(uint8_t *dest, uint8_t *src)
-{
- char *key, *val, *type;
- size_t rsize, dsize;
- uint8_t *var;
-
- if (!src || !dest) {
- return;
- }
-
- while (*src) {
- bgenv_map_uservar(src, &key, &type, (uint8_t **)&val, &rsize,
- &dsize);
- if (strncmp(type, USERVAR_TYPE_DELETED,
- strlen(USERVAR_TYPE_DELETED) - 1) == 0) {
- var = bgenv_find_uservar(dest, key);
- if (var) {
- bgenv_del_uservar(dest, var);
- }
- } else {
- bgenv_set_uservar(dest, key, type, val,
- strlen(val) + 1);
- }
- src = bgenv_next_uservar(src);
- }
-}
-
-static void update_environment(BG_ENVDATA *dest, BG_ENVDATA *src)
-{
- if (!dest || !src) {
- return;
- }
- if ((uint8_t)*src->kernelfile != IGNORE_MARKER_BYTE) {
- memcpy((void *)dest->kernelfile, (void *)src->kernelfile,
- sizeof(src->kernelfile));
- }
- if ((uint8_t)*src->kernelparams != IGNORE_MARKER_BYTE) {
- memcpy((void *)dest->kernelparams, (void *)src->kernelparams,
- sizeof(src->kernelparams));
- }
- 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,
- sizeof(src->revision));
- }
- if ((uint8_t)src->watchdog_timeout_sec != IGNORE_MARKER_BYTE) {
- memcpy((void *)&dest->watchdog_timeout_sec,
- (void *)&src->watchdog_timeout_sec,
- sizeof(src->watchdog_timeout_sec));
- }
- merge_uservars(dest->userdata, src->userdata);
-
- dest->crc32 =
- crc32(0, (Bytef *)dest, sizeof(BG_ENVDATA) - sizeof(dest->crc32));
-}
-
static void dump_uservars(uint8_t *udata)
{
char *key, *value, *type;
@@ -333,6 +356,46 @@ static void dump_env(BG_ENVDATA *env)
printf("\n\n");
}

+static void update_environment(BGENV *env)
+{
+ struct env_action *action;
+
+ printf("Processing journal...\n");
+
+ STAILQ_FOREACH(action, &head, journal)
+ {
+ journal_process_action(env, action);
+ journal_free_action(action);
+ STAILQ_REMOVE(&head, action, env_action, journal);
+ free(action);
+ }
+
+ env->data->crc32 = crc32(0, (const Bytef *)env->data,
+ sizeof(BG_ENVDATA) - sizeof(env->data->crc32));
+}
+
+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);
+ }
+ BGENV *env = bgenv_open_by_index(i);
+ if (env) {
+ if (verbosity) {
+ dump_env(env->data);
+ }
+ } else {
+ fprintf(stderr, "Error, could not read environment "
+ "for index %d\n",
+ i);
+ return;
+ }
+ bgenv_close(env);
+ }
+}
+
int main(int argc, char **argv)
{
static struct argp argp_setenv = {options_setenv, parse_opt, NULL, doc};
@@ -351,122 +414,31 @@ int main(int argc, char **argv)
struct arguments arguments;
arguments.output_to_file = false;
arguments.which_part = 0;
- memset((void *)&arguments.tmpdata, IGNORE_MARKER_BYTE,
- sizeof(BG_ENVDATA));
- memset((void *)&arguments.tmpdata.userdata, 0,
- sizeof(arguments.tmpdata.userdata));
+
+ STAILQ_INIT(&head);
+
error_t e;
if (e = argp_parse(argp, argc, argv, 0, 0, &arguments)) {
return e;
}
+
int result = 0;
- if (!arguments.output_to_file) {
- if (!bgenv_init()) {
- fprintf(stderr,
- "Error initializing FAT environment.\n");
- return 1;
- }
- for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) {
- if (verbosity) {
- printf("\n----------------------------\n");
- printf(" Config Partition #%d ", i);
- }
- BGENV *env = bgenv_open_by_index(i);
- if (env) {
- if (verbosity) {
- dump_env(env->data);
- }
- } else {
- fprintf(stderr, "Error, could not read "
- "environment for index %d\n",
- i);
- return 1;
- }
- bgenv_close(env);
- }
- if (write_mode) {
- BGENV *env_new;
- BGENV *env_current;
- if (auto_update) {
- /* search latest and oldest revision */
- env_current = bgenv_open_latest();
- if (!env_current) {
- fprintf(stderr, "Failed to retrieve "
- "latest "
- "environment.\n");
- return 1;
- }
- arguments.tmpdata.revision =
- env_current->data->revision + 1;
-
- env_new = bgenv_open_oldest();
- if (!env_new) {
- fprintf(stderr, "Failed to retrieve "
- "oldest "
- "environment.\n");
- return 1;
- }
- if (verbosity) {
- printf("Updating environment with "
- "revision %u\n",
- env_new->data->revision);
- }
- /* if auto-updating, copy data from current
- * revision to new
- * revision, so that all data which is not
- * overwritten is
- * kept
- */
- if (!env_current->data || !env_new->data) {
- fprintf(stderr, "Invalid environment "
- "data pointer.\n");
- bgenv_close(env_new);
- bgenv_close(env_current);
- return 1;
- }
- memcpy((char *)env_new->data,
- (char *)env_current->data,
- sizeof(BG_ENVDATA));
- if (!bgenv_close(env_current)) {
- fprintf(stderr,
- "Error closing environment.\n");
- }
- } else {
- if (part_specified) {
- env_new = bgenv_open_by_index(
- arguments.which_part);
- } else {
- env_new = bgenv_open_latest();
- }
- if (!env_new) {
- fprintf(stderr, "Failed to retrieve "
- "environment by "
- "index.\n");
- return 1;
- }
- }
- update_environment(env_new->data, &arguments.tmpdata);
- if (verbosity) {
- printf("New environment data:\n");
- printf("---------------------\n");
- dump_env(env_new->data);
- }
- if (!bgenv_write(env_new)) {
- fprintf(stderr, "Error storing environment.\n");
- return 1;
- }
- if (!bgenv_close(env_new)) {
- fprintf(stderr, "Error closing environment.\n");
- return 1;
- }
- }
- } else {
- /* arguments.output_to_file can only be true in write mode */
+
+ /* arguments are parsed, journal is filled */
+
+ /* is output to file ? */
+ if (arguments.output_to_file) {
+ /* execute journal and write to file */
+ BGENV env;
BG_ENVDATA data;
+
+ memset(&env, 0, sizeof(BGENV));
memset(&data, 0, sizeof(BG_ENVDATA));
- update_environment(&data, &arguments.tmpdata);
+ env.data = &data;
+
+ update_environment(&env);
if (verbosity) {
- dump_env(&data);
+ dump_env(env.data);
}
FILE *of = fopen(envfilepath, "wb");
if (of) {
@@ -487,7 +459,87 @@ int main(int argc, char **argv)
result = 1;
}
free(envfilepath);
+
+ return 0;
+ }
+
+ /* not in file mode */
+ if (!bgenv_init()) {
+ fprintf(stderr, "Error initializing FAT environment.\n");
+ return 1;
}

+ dump_envs();
+
+ if (!write_mode) {
+ return 0;
+ }
+
+ BGENV *env_new;
+ BGENV *env_current;
+ char *tmp;
+
+ if (auto_update) {
+ /* clone latest environment */
+
+ env_current = bgenv_open_latest();
+ if (!env_current) {
+ fprintf(stderr, "Failed to retrieve latest environment."
+ "\n");
+ return 1;
+ }
+ env_new = bgenv_open_oldest();
+ if (!env_new) {
+ fprintf(stderr, "Failed to retrieve oldest environment."
+ "\n");
+ return 1;
+ }
+ if (verbosity) {
+ printf("Updating environment with revision %u\n",
+ env_new->data->revision);
+ }
+
+ if (!env_current->data || !env_new->data) {
+ fprintf(stderr, "Invalid environment data pointer.\n");
+ bgenv_close(env_new);
+ bgenv_close(env_current);
+ return 1;
+ }
+
+ memcpy((char *)env_new->data, (char *)env_current->data,
+ sizeof(BG_ENVDATA));
+ env_new->data->revision = env_current->data->revision + 1;
+
+ if (!bgenv_close(env_current)) {
+ fprintf(stderr, "Error closing environment.\n");
+ }
+ } else {
+ if (part_specified) {
+ env_new = bgenv_open_by_index(arguments.which_part);
+ } else {
+ env_new = bgenv_open_latest();
+ }
+ if (!env_new) {
+ fprintf(stderr, "Failed to retrieve environment by "
+ "index.\n");
+ return 1;
+ }
+ }
+
+ update_environment(env_new);
+
+ if (verbosity) {
+ printf("New environment data:\n");
+ printf("---------------------\n");
+ dump_env(env_new->data);
+ }
+ if (!bgenv_write(env_new)) {
+ fprintf(stderr, "Error storing environment.\n");
+ return 1;
+ }
+ if (!bgenv_close(env_new)) {
+ fprintf(stderr, "Error closing environment.\n");
+ return 1;
+ }
return result;
}
--
2.14.1

Jan Kiszka

unread,
Sep 11, 2017, 10:21:06 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
The changes around all-local are needed for this fix or an unrelated
cleanup/fix? Not obvious to me at least.

Jan

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

Jan Kiszka

unread,
Sep 11, 2017, 10:24:05 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2017-09-11 13:35, [ext] Andreas J. Reichel wrote:
"Please", hmm, that sounds still optional to me. :)

I can remove that word if I have no other findings.

> + "only provide an output path. The "
> + "file name is automatically appended."},
> {"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
> {"confirm", 'c', 0, 0, "Confirm working environment"},
> {"update", 'u', 0, 0, "Automatically update oldest revision"},
> @@ -58,6 +60,8 @@ static bool part_specified = false;
>
> static bool verbosity = false;
>
> +static char *envfilepath = NULL;

Side note: Global vars are always 0-initialized. OTH one may argue that
NULL is not necessarily 0 - though it will be on our platforms.

Jan Kiszka

unread,
Sep 11, 2017, 10:26:54 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2017-09-11 13:35, [ext] Andreas J. Reichel wrote:
Tab indention here vs. spaces elsewhere. Maybe you want to enable tab
visualization in your editor ;).

Claudius Heine

unread,
Sep 11, 2017, 10:29:21 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
Hi,
Again with the documentation... ;) '\' should not be there, otherwise
'make would be a parameter.

Cheers,
Claudius
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: c...@denx.de

Jan Kiszka

unread,
Sep 11, 2017, 10:51:08 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
Why is 0 called "no action" here and "OK" below?
Is that a to-do like comment? The "//" suggests that. Maybe clarify this.

Jan

Jan Kiszka

unread,
Sep 11, 2017, 10:54:44 AM9/11/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2017-09-11 13:35, [ext] Andreas J. Reichel wrote:
I assume you considered doing this in smaller chunks and can confirm
that this would have been impractical. The patch is huge and, as such,
very hard to review.

Jan

Andreas Reichel

unread,
Sep 11, 2017, 11:13:54 AM9/11/17
to Jan Kiszka, efibootg...@googlegroups.com
Yes I did. No intention of torturing reviewers.

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

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

unread,
Sep 11, 2017, 11:15:42 AM9/11/17
to Jan Kiszka, efibootg...@googlegroups.com
Because 'no action' describes the update mechanism and 'OK' describes
the state of the whole environment.

Andreas Reichel

unread,
Sep 11, 2017, 11:26:11 AM9/11/17
to Jan Kiszka, efibootg...@googlegroups.com
This fix also affects the symlink creation - yes. The link was not
created under debian packaging test. Also, extending the all-target
is simpler than defining a strange _DATA variable imho.

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

Jan Kiszka

unread,
Sep 11, 2017, 11:28:05 AM9/11/17
to Andreas Reichel, efibootg...@googlegroups.com
For the future, just leave a note in the changelog for non-obvious but
related changes.

Andreas Reichel

unread,
Sep 11, 2017, 11:35:13 AM9/11/17
to Jan Kiszka, efibootg...@googlegroups.com
Really?
>
> I can remove that word if I have no other findings.
>
> > + "only provide an output path. The "
> > + "file name is automatically appended."},
> > {"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
> > {"confirm", 'c', 0, 0, "Confirm working environment"},
> > {"update", 'u', 0, 0, "Automatically update oldest revision"},
> > @@ -58,6 +60,8 @@ static bool part_specified = false;
> >
> > static bool verbosity = false;
> >
> > +static char *envfilepath = NULL;
>
> Side note: Global vars are always 0-initialized. OTH one may argue that
> NULL is not necessarily 0 - though it will be on our platforms.
>
Yes I know. For me it was just easier to read. In Visual Studio I
already had an issue where variables were NOT initialized correctly.
These few extra bytes don't hurt in my opinion.

Andreas Reichel

unread,
Sep 12, 2017, 4:33:35 AM9/12/17
to Jan Kiszka, efibootg...@googlegroups.com
It was already mixed before my patch... so I will convert
everything to tabs. Thank you.

Jan Kiszka

unread,
Sep 12, 2017, 4:51:28 AM9/12/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2017-09-11 13:35, [ext] Andreas J. Reichel wrote:
Is this the storage format? In that case, it leaves some open questions
regarding the widths of the type:

- size_t depends on the platform. Better use some uintX_t.
- What is char[]? A Null-terminated string of variable length?

> + * |------------|------------|-------------|----------------|
> + * | KEY | - - - - - - - PAYLOAD - - - - - - - - |

What does that line mean?

Jan

Jan Kiszka

unread,
Sep 12, 2017, 4:54:48 AM9/12/17
to [ext] Andreas J. Reichel, efibootg...@googlegroups.com
On 2017-09-11 13:35, [ext] Andreas J. Reichel wrote:
Didn't you just add this section in the last patch? That's overkill.
Reasoning should go into the changelog instead (but it looks like you
did this already).

Andreas Reichel

unread,
Sep 12, 2017, 4:55:20 AM9/12/17
to Jan Kiszka, efibootg...@googlegroups.com
On Mon, Sep 11, 2017 at 04:51:06PM +0200, Jan Kiszka wrote:
But I agree... if this question arises it's not good. I'll change it.

Andreas Reichel

unread,
Sep 12, 2017, 4:57:24 AM9/12/17
to Jan Kiszka, efibootg...@googlegroups.com
Yes of course, that's overkill, but that's how the TODO has to be kept
as long as we have it imho. Since we already decided to get rid of it,
this should not be a problem here...

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

Andreas Reichel

unread,
Sep 12, 2017, 6:26:27 AM9/12/17
to Jan Kiszka, efibootg...@googlegroups.com
> > +
> > +void bgenv_map_uservar(uint8_t *udata, char **key, char **type, uint8_t **val,
> > + size_t *record_size, size_t *data_size)
> > +{
> > + /* Each user variable is encoded as follows:
> > + * |------------|------------|-------------|----------------|
> > + * | char KEY[] | size_t len | char type[] | uint8_t data[] |
>
> Is this the storage format? In that case, it leaves some open questions
> regarding the widths of the type:
>
> - size_t depends on the platform. Better use some uintX_t.

Uff, that would indeed cause trouble if cross-compiling and pre-filling
a target's environment with a different, natively built tool and later
reading it with the cross-compiled tool with both tools having
a different size_t size. Big thanks for seeing this!

Andreas J. Reichel

unread,
Sep 12, 2017, 6:55:38 AM9/12/17
to efibootg...@googlegroups.com, Andreas Reichel
From: Andreas Reichel <andreas.r...@siemens.com>

For better usability, `bg_setenv`'s parameter -f is changed from
`--file` to `--filepath` and now accepts a path, where `BGENV.DAT`
is created so that it can directly be applied to the real environ-
ment location.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
docs/TODO.md | 1 -
docs/USAGE.md | 12 ++++++------
tools/bg_setenv.c | 20 ++++++++++++++------
3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/docs/TODO.md b/docs/TODO.md
index 40d5ba8..832f76b 100644
--- a/docs/TODO.md
+++ b/docs/TODO.md
@@ -6,7 +6,6 @@
the current working environment to the (latest-1) environment, so
that if the current environment breaks, there is a backup with the
latest values.
- * Make `bg_setenv -f` take a path to where the `BGENV.DAT` is stored.

* Application specific variables
* applications may need to store their own variables into the
diff --git a/docs/USAGE.md b/docs/USAGE.md
index 7dd89ce..97a25cb 100644
--- a/docs/USAGE.md
+++ b/docs/USAGE.md
@@ -98,13 +98,13 @@ This step first creates a custom label contained in `EFILABEL`, which is later
used to specify the kernel location.

```
-# mount /dev/sdX2 /mnt && cd /mnt
-# echo -n "KERNEL1" | iconv -f ascii -t UTF-16LE > EFILABEL
-# bg_setenv -f -r 1 --kernel="C:KERNEL1:vmlinuz-linux" --args="root=/dev/sdX4 noinitrd"
+# mount /dev/sdX2 /mnt
+# echo -n "KERNEL1" | iconv -f ascii -t UTF-16LE > /mnt/EFILABEL
+# bg_setenv -f /mnt -r 1 --kernel="C:KERNEL1:vmlinuz-linux" --args="root=/dev/sdX4 noinitrd"
# umount /mnt
-# mount /dev/sdX3 /mnt && cd /mnt
-# echo -n "KERNEL2" | iconv -f ascii -t UTF-16LE > EFILABEL
-# bg_setenv -f -r 2 --kernel="C:KERNEL2:vmlinuz-linux" --args="root=/dev/sdX5 noinitrd"
+# mount /dev/sdX3 /mnt
+# echo -n "KERNEL2" | iconv -f ascii -t UTF-16LE > /mnt/EFILABEL
+# bg_setenv -f /mnt -r 2 --kernel="C:KERNEL2:vmlinuz-linux" --args="root=/dev/sdX5 noinitrd"
# umount /mnt
```

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 5c313cf..5cc6403 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -24,7 +24,9 @@ static struct argp_option options_setenv[] = {
"above zero is updated."},
{"revision", 'r', "REVISION", 0, "Set revision value"},
{"testing", 't', "TESTING", 0, "Set test mode for environment"},
- {"file", 'f', 0, 0, "Output environment to file"},
+ {"filepath", 'f', "ENVFILE_DIR", 0, "Output environment to file. Expects "
+ "an output path where the file name "
+ "is automatically appended."},
{"watchdog", 'w', "WATCHDOG_TIMEOUT", 0, "Watchdog timeout in seconds"},
{"confirm", 'c', 0, 0, "Confirm working environment"},
{"update", 'u', 0, 0, "Automatically update oldest revision"},
@@ -58,6 +60,8 @@ static bool part_specified = false;

static bool verbosity = false;

+static char *envfilepath = NULL;
+
static error_t parse_opt(int key, char *arg, struct argp_state *state)
{
struct arguments *arguments = state->input;
--
2.14.1

Reply all
Reply to author
Forward
0 new messages