[PATCH 06/16] Add new fail-safe mode flag

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Add a flag that marks an environment as a fail-safe environment.

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
---
include/envdata.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/envdata.h b/include/envdata.h
index d1e5980..d2fa6bf 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -21,6 +21,7 @@
#define CONFIG_PARTITION_MAXCOUNT 64

#define ENV_STATUS_IN_PROGRESS 1
+#define ENV_STATUS_FAILSAFE 128

#define USTATE_OK 0
#define USTATE_INSTALLED 1
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

To compare device paths it is necessary to be able to split of
sub-paths from the right hand side. Implement a simple function
for that purpose.

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

include/utils.h | 1 +
utils.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index 285e45e..06721eb 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -29,6 +29,7 @@ typedef struct _VOLUME_DESC {

typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

+CHAR16 *StrRStrip(CHAR16 *input, CHAR16 delim);
uint32_t calc_crc32(void *data, int32_t size);
void __noreturn error_exit(CHAR16 *message, EFI_STATUS status);
VOID sleep(int32_t sec);
diff --git a/utils.c b/utils.c
index 33e3a90..4f44a37 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,29 @@
#include <bootguard.h>
#include <utils.h>

+CHAR16 *StrRStrip(CHAR16 *input, CHAR16 delim)
+{
+ CHAR16 *dst;
+ UINTN len;
+
+ len = StrLen(input);
+ dst = mmalloc((len + 1) * 2);
+ if (!dst) {
+ return NULL;
+ }
+
+ StrCpy(dst, input);
+
+ for (UINTN i = len; i > 0; i--)
+ {
+ if (dst[i * 2] == delim) {
+ dst[i * 2] = L'\0';
+ break;
+ }
+ }
+ return dst;
+}
+
uint32_t calc_crc32(void *data, int32_t size)
{
uint32_t crc;
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

During environment enumeration, notify if it is on the boot device

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

utils.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/utils.c b/utils.c
index 0477cbc..c36f428 100644
--- a/utils.c
+++ b/utils.c
@@ -198,6 +198,9 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
Print(L"Volume %d: %s, LABEL=%s, CLABEL=%s\n", rootCount,
DevicePathToStr(devpath), (*volumes)[rootCount].fslabel,
(*volumes)[rootCount].fscustomlabel);
+ if (IsOnBootDevice(devpath)) {
+ Print(L"(On the boot device)\n");
+ }
rootCount++;
}
*count = rootCount;
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Sometimes it is of advantage to have time to read the output
before the OS is booted. Implement a simple sleep function.

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

include/utils.h | 1 +
utils.c | 5 +++++
2 files changed, 6 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index a73c554..285e45e 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -31,6 +31,7 @@ typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

uint32_t calc_crc32(void *data, int32_t size);
void __noreturn error_exit(CHAR16 *message, EFI_STATUS status);

+VOID sleep(int32_t sec);
VOID *mmalloc(UINTN bytes);
EFI_STATUS mfree(VOID *p);
CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
diff --git a/utils.c b/utils.c
index 9d3eb4d..33e3a90 100644
--- a/utils.c
+++ b/utils.c
@@ -32,6 +32,11 @@ void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
unreachable();
}

+VOID sleep(int32_t sec)
+{
+ uefi_call_wrapper(BS->Stall, 1, sec * 1000 * 1000);
+}
+
VOID *mmalloc(UINTN bytes)
{
EFI_STATUS status;
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Code was wrong in trying to boot if additional config partitions
are added to the system. This can result in unwanted behavior.
If less are detected, try to boot, but if more are detected, stop.

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

env/fatvars.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 4b87f55..bbff777 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -105,13 +105,18 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
return BG_CONFIG_ERROR;
}

- if (numHandles != ENV_NUM_CONFIG_PARTS) {
- Print(L"Warning, unexpected number of config partitions: found "
+ if (numHandles > ENV_NUM_CONFIG_PARTS) {
+ Print(L"Error, too many config partitions found. Aborting.\n");
+ mfree(roots);
+ return BG_CONFIG_ERROR;
+ }
+
+ if (numHandles < ENV_NUM_CONFIG_PARTS) {
+ Print(L"Warning, too few config partitions: found: "
L"%d, but expected %d.\n",
numHandles, ENV_NUM_CONFIG_PARTS);
/* Don't treat this as error because we may still be able to
- * find a
- * valid config */
+ * find a valid config */
result = BG_CONFIG_PARTIALLY_CORRUPTED;
}

--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

This patch series implements the new fail-safe mode.

As discussed on the mailing list, if fail-safe mode is enabled, only
environments on the boot-device are used. If not, all environments
found are used.
Fail-safe mode is enabled by setting the corresponding flag in ANY
environment on the boot medium.

This series does the following:
* Add some support functions
* Rename in_progress to env_status and reimplement in_progress
from uint8_t to a single bit within this new env_status
register.
* Add new failsafe flag into the env_status register.
* Fix some odd return values in bgenv_set / get, which
depended on the wrong value given
* Improve some error handling code
* Make travis-ci work again
* Update documentation

Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
Andreas Reichel (16):
utils: Add sleep function for debugging purpose
Add StrRStrip function for device path manipulation
Add check if environment is on boot device
Print information if environment is on boot dev
Change implementation of in_progress to flag
Add new fail-safe mode flag
Correct config logic to NOT boot if too many partitions found
New functions for config file open/close/read
Implement fail-safe mode as config filter
Simplify config partition management
Fix return values of bg_setenv with negative values
Add getter/setter for ENV_STATUS_FAILSAFE flag
bg_setenv: Add option S to set fail-safe mode
bg_printenv: Display env_status_flags
travis-ci: Update .travis.yml to run apt-get update
Update documentation

.travis.yml | 1 +
bootguard.c | 1 +
docs/FAILSAFE.md | 10 +++
docs/UPDATE.md | 6 +-
env/env_api.c | 8 +--
env/env_api_fat.c | 46 ++++++++++++--
env/fatvars.c | 116 +++++++++++++++++++---------------
env/syspart.c | 90 +++++++++++++++++++++++---
include/env_api.h | 1 +
include/envdata.h | 5 +-
include/syspart.h | 7 +-
include/utils.h | 3 +
main.c | 6 ++
tools/bg_setenv.c | 20 +++++-
tools/tests/test_ebgenv_api.c | 6 +-
utils.c | 44 +++++++++++++
16 files changed, 289 insertions(+), 81 deletions(-)
create mode 100644 docs/FAILSAFE.md

--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Get the boot device out of the loaded image's handle and add a
function to compare every environment's device path with this boot
device.

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

bootguard.c | 1 +
include/utils.h | 1 +
main.c | 6 ++++++
utils.c | 13 +++++++++++++
4 files changed, 21 insertions(+)

diff --git a/bootguard.c b/bootguard.c
index 0aa95b4..9eea350 100644
--- a/bootguard.c
+++ b/bootguard.c
@@ -18,3 +18,4 @@ EFI_HANDLE this_image;

VOLUME_DESC *volumes = NULL;
UINTN volume_count = 128;
+CHAR16 *boot_device_path;
diff --git a/include/utils.h b/include/utils.h
index 06721eb..3c97bfa 100644
--- a/include/utils.h
+++ b/include/utils.h

@@ -29,6 +29,7 @@ typedef struct _VOLUME_DESC {

typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp);
CHAR16 *StrRStrip(CHAR16 *input, CHAR16 delim);

uint32_t calc_crc32(void *data, int32_t size);
void __noreturn error_exit(CHAR16 *message, EFI_STATUS status);

diff --git a/main.c b/main.c
index 3789319..f1cbf3c 100644
--- a/main.c
+++ b/main.c
@@ -24,6 +24,7 @@

extern const unsigned long init_array_start[];
extern const unsigned long init_array_end[];
+extern CHAR16 *boot_device_path;

static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
@@ -123,6 +124,11 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status);
}

+ boot_device_path = DevicePathToStr(DevicePathFromHandle(
+ loaded_image->DeviceHandle));
+ boot_device_path = StrRStrip(boot_device_path, L'/');
+ Print(L"Boot device: %s\n", boot_device_path);
+
status = get_volumes(&volumes, &volume_count);
if (EFI_ERROR(status)) {
error_exit(L"Could not get volumes installed on system.\n",
diff --git a/utils.c b/utils.c
index 4f44a37..0477cbc 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,19 @@
#include <bootguard.h>
#include <utils.h>

+BOOLEAN IsOnBootDevice(EFI_DEVICE_PATH *dp)
+{
+ extern CHAR16 *boot_device_path;
+ CHAR16 *device_path;
+
+ device_path = DevicePathToStr(dp);
+ device_path = StrRStrip(device_path, L'/');
+ if (StrCmp(device_path, boot_device_path) == 0) {
+ return TRUE;
+ }
+ return FALSE;
+}
+
CHAR16 *StrRStrip(CHAR16 *input, CHAR16 delim)
{
CHAR16 *dst;
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

For future implementation of a fail-safe mechanism an additional flag is
needed. Since 'in_progress' only needs one bit, the meaning of the
internal 'in_progress' variable is changed to a flags register and
'in_progress' is represented by the flag setting the LSB.
This way 7 additional flags can be implemented without the need for
additional variables.

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

env/env_api.c | 8 ++++----
env/env_api_fat.c | 16 +++++++++++++---
env/fatvars.c | 2 +-
include/envdata.h | 4 +++-
tools/bg_setenv.c | 5 ++++-
tools/tests/test_ebgenv_api.c | 6 ++++--
6 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index d6f1795..6cf6449 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -74,7 +74,7 @@ int ebg_env_create_new(ebgenv_t *e)

BG_ENVDATA *latest_data = ((BGENV *)latest_env)->data;

- if (latest_data->in_progress != 1) {
+ if (!(latest_data->status_flags & ENV_STATUS_IN_PROGRESS)) {
e->bgenv = (void *)bgenv_create_new();
if (!e->bgenv) {
bgenv_close(latest_env);
@@ -82,10 +82,10 @@ int ebg_env_create_new(ebgenv_t *e)
}
BG_ENVDATA *new_data = ((BGENV *)e->bgenv)->data;
uint32_t new_rev = new_data->revision;
- uint8_t new_in_progress = new_data->in_progress;
+ uint8_t new_status_flags = new_data->status_flags;
memcpy(new_data, latest_env->data, sizeof(BG_ENVDATA));
new_data->revision = new_rev;
- new_data->in_progress = new_in_progress;
+ new_data->status_flags = new_status_flags;
bgenv_close(latest_env);
} else {
e->bgenv = latest_env;
@@ -289,7 +289,7 @@ int ebg_env_finalize_update(ebgenv_t *e)
pgci = &tmp;
}

- ((BGENV *)e->bgenv)->data->in_progress = 0;
+ ((BGENV *)e->bgenv)->data->status_flags &= ~ENV_STATUS_IN_PROGRESS;
((BGENV *)e->bgenv)->data->ustate = USTATE_INSTALLED;
return 0;
}
diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 378b20e..c51eec5 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -319,7 +319,8 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
USERVAR_TYPE_UINT8);
case EBGENV_IN_PROGRESS:
return bgenv_get_uint(buffer, type, data,
- env->data->in_progress,
+ env->data->status_flags &
+ ENV_STATUS_IN_PROGRESS,
USERVAR_TYPE_UINT8);
default:
if (!data) {
@@ -399,7 +400,16 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
if (val < 0) {
return val;
}
- env->data->in_progress = val;
+ switch(val) {
+ case 1:
+ env->data->status_flags |= ENV_STATUS_IN_PROGRESS;
+ break;
+ case 0:
+ env->data->status_flags &= ~ENV_STATUS_IN_PROGRESS;
+ break;
+ default:
+ return -EINVAL;
+ }
break;
default:
return -EINVAL;
@@ -432,7 +442,7 @@ BGENV *bgenv_create_new(void)
memset(env_new->data, 0, sizeof(BG_ENVDATA));
/* update revision field and testing mode */
env_new->data->revision = new_rev;
- env_new->data->in_progress = 1;
+ env_new->data->status_flags = ENV_STATUS_IN_PROGRESS;
/* set default watchdog timeout */
env_new->data->watchdog_timeout_sec = 30;

diff --git a/env/fatvars.c b/env/fatvars.c
index 0645af8..4b87f55 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -195,7 +195,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

/* Test if this environment is currently 'in_progress'. If yes,
* do not boot from it, instead ignore it */
- if (env[latest_idx].in_progress == 1) {
+ if (env[latest_idx].status_flags & ENV_STATUS_IN_PROGRESS) {
current_partition = pre_latest_idx;
} else if (env[latest_idx].ustate == USTATE_TESTING) {
/* If it has already been booted, this indicates a failed
diff --git a/include/envdata.h b/include/envdata.h
index 25bd6c5..d1e5980 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -20,6 +20,8 @@

#define CONFIG_PARTITION_MAXCOUNT 64

+#define ENV_STATUS_IN_PROGRESS 1
+

#define USTATE_OK 0
#define USTATE_INSTALLED 1

#define USTATE_TESTING 2
@@ -36,7 +38,7 @@
struct _BG_ENVDATA {
uint16_t kernelfile[ENV_STRING_LENGTH];
uint16_t kernelparams[ENV_STRING_LENGTH];
- uint8_t in_progress;
+ uint8_t status_flags;
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 5d8e9b8..cae268d 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -475,9 +475,12 @@ static void dump_uservars(uint8_t *udata)
static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];
+ bool in_progress;
+
+ in_progress = env->status_flags & ENV_STATUS_IN_PROGRESS;
fprintf(stdout, "Values:\n");
fprintf(stdout,
- "in_progress: %s\n",env->in_progress ? "yes" : "no");
+ "in_progress: %s\n", in_progress ? "yes" : "no");
fprintf(stdout, "revision: %u\n", env->revision);
fprintf(stdout,
"kernel: %s\n", str16to8(buffer, env->kernelfile));
diff --git a/tools/tests/test_ebgenv_api.c b/tools/tests/test_ebgenv_api.c
index f7a3410..54d2441 100644
--- a/tools/tests/test_ebgenv_api.c
+++ b/tools/tests/test_ebgenv_api.c
@@ -130,7 +130,8 @@ START_TEST(ebgenv_api_ebg_env_create_new)

ck_assert(((BGENV *)e.bgenv)->data == &envdata[0]);

- ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 1);
+ ck_assert((((BGENV *)e.bgenv)->data->status_flags &
+ ENV_STATUS_IN_PROGRESS) != 0);
ck_assert_int_eq(
((BGENV *)e.bgenv)->data->revision, ENV_NUM_CONFIG_PARTS+1);

@@ -157,7 +158,8 @@ START_TEST(ebgenv_api_ebg_env_create_new)

ck_assert_int_eq(ret, 0);
ck_assert_int_eq(((BGENV *)e.bgenv)->data->ustate, USTATE_INSTALLED);
- ck_assert_int_eq(((BGENV *)e.bgenv)->data->in_progress, 0);
+ ck_assert((((BGENV *)e.bgenv)->data->status_flags &
+ ENV_STATUS_IN_PROGRESS) == 0);
}
END_TEST

--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

If a variable like watchdog is set with a negative argument,
bg_setenv returns this negative argument instead of -EINVAL.
Fix this to -EINVAL.

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

env/env_api_fat.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index c51eec5..2f71094 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -371,7 +371,7 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
case EBGENV_REVISION:
val = bgenv_convert_to_long(value);
if (val < 0) {
- return val;
+ return -EINVAL;
}
env->data->revision = val;
break;
@@ -384,21 +384,21 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
case EBGENV_WATCHDOG_TIMEOUT_SEC:
val = bgenv_convert_to_long(value);
if (val < 0) {
- return val;
+ return -EINVAL;
}
env->data->watchdog_timeout_sec = val;
break;
case EBGENV_USTATE:
val = bgenv_convert_to_long(value);
if (val < 0) {
- return val;
+ return -EINVAL;
}
env->data->ustate = val;
break;
case EBGENV_IN_PROGRESS:
val = bgenv_convert_to_long(value);
if (val < 0) {
- return val;
+ return -EINVAL;
}
switch(val) {
case 1:
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

If a config partition on the boot device has the fail-safe flag
set, only use environments on the boot device.

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

env/fatvars.c | 49 ++++++++++++++++++++++++++++++++++---
env/syspart.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++-
include/syspart.h | 7 +++++-
3 files changed, 112 insertions(+), 6 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index ab1b5c4..cefff0c 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -25,6 +25,7 @@ BG_STATUS save_current_config(void)
EFI_STATUS efistatus;
UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
EFI_FILE_HANDLE *roots;
+ EFI_DEVICE_PATH **root_devices;

roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
CONFIG_PARTITION_MAXCOUNT);
@@ -34,8 +35,25 @@ BG_STATUS save_current_config(void)
return BG_CONFIG_ERROR;
}

- if (EFI_ERROR(enumerate_cfg_parts(roots, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.");
+ root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *
+ CONFIG_PARTITION_MAXCOUNT);
+ if (!root_devices) {
+ Print(L"Error, could not allocate memory for config partition "
+ L"device paths.\n");

+ mfree(roots);
+ return BG_CONFIG_ERROR;
+ }
+

+ if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+ Print(L"Error, could not enumerate config partitions.\n");
+ mfree(root_devices);

+ mfree(roots);
+ return BG_CONFIG_ERROR;
+ }
+

+ if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {
+ Print(L"Error, could not filter config partitions.\n");
+ mfree(root_devices);
mfree(roots);
return BG_CONFIG_ERROR;
}
@@ -46,6 +64,7 @@ BG_STATUS save_current_config(void)
numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */
+ mfree(root_devices);
mfree(roots);
return BG_CONFIG_ERROR;
}
@@ -57,6 +76,7 @@ BG_STATUS save_current_config(void)
Print(L"Error, could not open environment file on system "
L"partition %d: %r\n",
current_partition, efistatus);
+ mfree(root_devices);
mfree(roots);
return BG_CONFIG_ERROR;
}
@@ -77,6 +97,7 @@ BG_STATUS save_current_config(void)
Print(L"Error, could not close environment config file.\n");
result = BG_CONFIG_ERROR;
}
+ mfree(root_devices);
mfree(roots);
return result;
}
@@ -86,6 +107,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
BG_STATUS result = BG_SUCCESS;
UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
EFI_FILE_HANDLE *roots;
+ EFI_DEVICE_PATH **root_devices;
UINTN i;
int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

@@ -97,14 +119,32 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
return BG_CONFIG_ERROR;
}

- if (EFI_ERROR(enumerate_cfg_parts(roots, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.");
+ root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *
+ CONFIG_PARTITION_MAXCOUNT);
+ if (!root_devices) {
+ Print(L"Error, could not allocate memory for config partition "
+ L"device paths.\n");

+ mfree(roots);
+ return BG_CONFIG_ERROR;
+ }
+

+ if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+ Print(L"Error, could not enumerate config partitions.\n");
+ mfree(root_devices);

+ mfree(roots);
+ return BG_CONFIG_ERROR;
+ }
+

+ if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {
+ Print(L"Error, could not filter config partitions.\n");
+ mfree(root_devices);
mfree(roots);
return BG_CONFIG_ERROR;
}

if (numHandles > ENV_NUM_CONFIG_PARTS) {

Print(L"Error, too many config partitions found. Aborting.\n");

+ mfree(root_devices);
mfree(roots);
return BG_CONFIG_ERROR;
}
@@ -223,6 +263,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
Print(L" kernel: %s\n", bglp->payload_path);
Print(L" args: %s\n", bglp->payload_options);
Print(L" timeout: %d seconds\n", bglp->timeout);
+ mfree(root_devices);
mfree(roots);
return result;
}
diff --git a/env/syspart.c b/env/syspart.c
index aa848d0..86628a0 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -37,7 +37,9 @@ EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer)
return uefi_call_wrapper(fh->Read, 3, fh, &readlen, buffer);
}

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
+EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
+ EFI_DEVICE_PATH **root_devices,
+ UINTN *numHandles)
{
EFI_STATUS status;
UINTN rootCount = 0;
@@ -57,6 +59,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
if (status == EFI_SUCCESS) {
Print(L"Config file found on volume %d.\n", index);
roots[rootCount] = volumes[index].root;
+ root_devices[rootCount] = volumes[index].devpath;
rootCount++;
status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {
@@ -70,3 +73,60 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
Print(L"%d config partitions detected.\n", rootCount);
return EFI_SUCCESS;
}
+
+EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
+ EFI_DEVICE_PATH **root_devices, UINTN *numHandles)
+{
+ BOOLEAN only_envs_on_bootdevice = FALSE;
+
+ Print(L"Config filter: \n");
+ for (UINTN index = 0; index < *numHandles; index++) {
+ EFI_FILE_HANDLE fh = NULL;
+ EFI_STATUS status;
+ BG_ENVDATA env;
+
+ status = open_cfg_file(roots[index], &fh, EFI_FILE_MODE_READ);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+
+ status = read_cfg_file(fh, &env);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+
+ if (IsOnBootDevice(root_devices[index]) &&
+ (env.status_flags & ENV_STATUS_FAILSAFE)) {
+ only_envs_on_bootdevice = TRUE;
+ };
+
+ status = close_cfg_file(roots[index], fh);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ }
+
+ if (!only_envs_on_bootdevice) {
+ // nothing to do
+ return EFI_SUCCESS;
+ }
+
+ Print(L"Fail-Safe Mode enabled.\n");
+ UINTN index = 0;
+ do {
+ if (!IsOnBootDevice(root_devices[index])) {
+ for (UINTN j = index; j < *numHandles-1; j++) {
+ roots[j] = roots[j+1];
+ root_devices[j] = root_devices[j+1];
+ }
+ mfree(roots[*numHandles-1]);
+ mfree(root_devices[*numHandles-1]);
+ (*numHandles)--;
+ Print(L"Filtered Config #%d\n", index);
+ }
+ index++;
+ } while(index < *numHandles);
+
+ Print(L"Remaining Config Partitions: %d\n", *numHandles);
+ return EFI_SUCCESS;
+}
diff --git a/include/syspart.h b/include/syspart.h
index 27caa93..269fa25 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -25,6 +25,11 @@ EFI_STATUS open_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE *fh,
UINT64 mode);
EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh);
EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer);
-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *maxHandles);
+EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
+ EFI_DEVICE_PATH **root_devices,
+ UINTN *maxHandles);
+EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
+ EFI_DEVICE_PATH **root_devices,
+ UINTN *maxHandles);

#endif // __H_SYSPART__
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

For additional config file checks like fail-safe mode, define one
common open/read and close function to simplify code.

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

env/fatvars.c | 21 +++++++--------------
env/syspart.c | 29 +++++++++++++++++++++++------
include/syspart.h | 4 ++++
3 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index bbff777..ab1b5c4 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -51,10 +51,8 @@ BG_STATUS save_current_config(void)
}

EFI_FILE_HANDLE fh = NULL;
- efistatus = uefi_call_wrapper(
- roots[current_partition]->Open, 5, roots[current_partition], &fh,
- ENV_FILE_NAME, EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ,
- EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM);
+ efistatus = open_cfg_file(roots[current_partition], &fh,
+ EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ);
if (EFI_ERROR(efistatus)) {

Print(L"Error, could not open environment file on system "
L"partition %d: %r\n",

@@ -75,7 +73,7 @@ BG_STATUS save_current_config(void)
result = BG_CONFIG_ERROR;
}

- if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {
+ if (EFI_ERROR(close_cfg_file(roots[current_partition], fh))) {

Print(L"Error, could not close environment config file.\n");
result = BG_CONFIG_ERROR;
}

@@ -123,11 +121,8 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
/* Load all config data */
for (i = 0; i < numHandles; i++) {
EFI_FILE_HANDLE fh = NULL;
- if (EFI_ERROR(uefi_call_wrapper(
- roots[i]->Open, 5, roots[i], &fh, ENV_FILE_NAME,
- EFI_FILE_MODE_READ,
- EFI_FILE_READ_ONLY | EFI_FILE_ARCHIVE |
- EFI_FILE_HIDDEN | EFI_FILE_SYSTEM))) {
+ if (EFI_ERROR(open_cfg_file(roots[i], &fh,
+ EFI_FILE_MODE_READ))) {
Print(L"Warning, could not open environment file on "
L"config partition %d\n",
i);
@@ -135,14 +130,12 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
continue;
}

- UINTN readlen = sizeof(BG_ENVDATA);
- if (EFI_ERROR(uefi_call_wrapper(fh->Read, 3, fh, &readlen,
- (VOID *)&env[i]))) {
+ if (EFI_ERROR(read_cfg_file(fh, (VOID *)&env[i]))) {
Print(L"Error reading environment from config "
L"partition %d.\n",
i);
env_invalid[i] = 1;
- if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {
+ if (EFI_ERROR(close_cfg_file(roots[i], fh))) {

Print(L"Error, could not close environment "

L"config file.\n");
}
diff --git a/env/syspart.c b/env/syspart.c
index 7bdf595..aa848d0 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -14,9 +14,29 @@

#include <syspart.h>
#include <utils.h>
+#include <envdata.h>

#define MAX_INFO_SIZE 1024

+EFI_STATUS open_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE *fh,
+ UINT64 mode)
+{
+ return uefi_call_wrapper(root->Open, 5, root, fh, ENV_FILE_NAME, mode,
+ EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN |
+ EFI_FILE_SYSTEM);
+}
+
+EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh)
+{
+ return uefi_call_wrapper(root->Close, 1, fh);
+}
+
+EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer)
+{
+ UINTN readlen = sizeof(BG_ENVDATA);
+ return uefi_call_wrapper(fh->Read, 3, fh, &readlen, buffer);
+}
+

EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)

{
EFI_STATUS status;
@@ -32,16 +52,13 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *numHandles)
if (!volumes[index].root) {
continue;
}
- status = uefi_call_wrapper(
- volumes[index].root->Open, 5, volumes[index].root, &fh,
- ENV_FILE_NAME, EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ,
- EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM);
+ status = open_cfg_file(volumes[index].root, &fh,
+ EFI_FILE_MODE_READ);

if (status == EFI_SUCCESS) {
Print(L"Config file found on volume %d.\n", index);
roots[rootCount] = volumes[index].root;

rootCount++;
- status = uefi_call_wrapper(volumes[index].root->Close,
- 1, fh);
+ status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {
Print(L"Could not close config file on "
L"partition %d.\n",
diff --git a/include/syspart.h b/include/syspart.h
index 2b70ed5..27caa93 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -21,6 +21,10 @@
#include <efipciio.h>
#include "bootguard.h"

+EFI_STATUS open_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE *fh,
+ UINT64 mode);
+EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh);
+EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer);
EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots, UINTN *maxHandles);

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Add new enum member for ENV_STATUS_FAILSAFE flag to use bg_setenv
and bg_getenv for it.

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

env/env_api_fat.c | 24 ++++++++++++++++++++++++
include/env_api.h | 1 +
2 files changed, 25 insertions(+)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 2f71094..6f759f0 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -43,6 +43,10 @@ EBGENVKEY bgenv_str2enum(char *key)
if (strncmp(key, "in_progress", strlen("in_progress") + 1) == 0) {
return EBGENV_IN_PROGRESS;
}
+ if (strncmp(key, "env_status_failsafe",
+ strlen("env_status_failsafe") + 1) == 0) {
+ return EBGENV_FAILSAFE;
+ }
return EBGENV_UNKNOWN;
}

@@ -322,6 +326,11 @@ int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data,
env->data->status_flags &
ENV_STATUS_IN_PROGRESS,
USERVAR_TYPE_UINT8);
+ case EBGENV_FAILSAFE:
+ return bgenv_get_uint(buffer, type, data,

+ env->data->status_flags &

+ ENV_STATUS_FAILSAFE,
+ USERVAR_TYPE_UINT8);
default:
if (!data) {
return 0;
@@ -411,6 +420,21 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
return -EINVAL;
}
break;
+ case EBGENV_FAILSAFE:
+ val = bgenv_convert_to_long(value);
+ if (val < 0) {
+ return val;
+ }

+ switch(val) {
+ case 1:

+ env->data->status_flags |= ENV_STATUS_FAILSAFE;

+ break;
+ case 0:

+ env->data->status_flags &= ~ENV_STATUS_FAILSAFE;

+ default:
+ return -EINVAL;
+ }
+ break;
default:
return -EINVAL;
}

diff --git a/include/env_api.h b/include/env_api.h
index 45bf4b7..04516bb 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -54,6 +54,7 @@ typedef enum {
EBGENV_REVISION,
EBGENV_USTATE,
EBGENV_IN_PROGRESS,
+ EBGENV_FAILSAFE,
EBGENV_UNKNOWN
} EBGENVKEY;

--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Display flags set in env_status_flags.

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

tools/bg_setenv.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index 0847de1..8abd015 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -483,8 +483,10 @@ static void dump_env(BG_ENVDATA *env)
{
char buffer[ENV_STRING_LENGTH];
bool in_progress;
+ bool failsafe;

in_progress = env->status_flags & ENV_STATUS_IN_PROGRESS;

+ failsafe = env->status_flags & ENV_STATUS_FAILSAFE;

fprintf(stdout, "Values:\n");
fprintf(stdout,

"in_progress: %s\n", in_progress ? "yes" : "no");

@@ -497,7 +499,11 @@ static void dump_env(BG_ENVDATA *env)
"watchdog timeout: %u seconds\n", env->watchdog_timeout_sec);
fprintf(stdout, "ustate: %u (%s)\n", (uint8_t)env->ustate,
ustate2str(env->ustate));
- fprintf(stdout, "\n");
+ fprintf(stdout, "flags: ");
+ if (failsafe) {
+ fprintf(stdout, "FAILSAFE ");
+ }
+ fprintf(stdout, "\n\n");
fprintf(stdout, "user variables:\n");
dump_uservars(env->userdata);
fprintf(stdout, "\n\n");
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Prevent 404 during apt-get install with apt-get update in before_install

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

.travis.yml | 1 +

1 file changed, 1 insertion(+)

diff --git a/.travis.yml b/.travis.yml
index 8098fe2..9cf215f 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -25,6 +25,7 @@ compiler:
sudo: required

before_install:
+ - sudo apt-get update
- if [ $TRAVIS_BRANCH = coverity_scan ] && [ ${TRAVIS_JOB_NUMBER##*.} != 1 ]; then exit 0; fi
- echo -n | openssl s_client -connect scan.coverity.com:444 | sed -ne '/-BEGIN CERTIFICATE-/,/-END CERTIFICATE-/p' | sudo tee -a /etc/ssl/certs/ca-certificates.crt

--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

* Update UPDATE.md
* Add new FAILSAFE.md to explain fail-safe mode

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

docs/FAILSAFE.md | 10 ++++++++++
docs/UPDATE.md | 6 +++---
2 files changed, 13 insertions(+), 3 deletions(-)
create mode 100644 docs/FAILSAFE.md

diff --git a/docs/FAILSAFE.md b/docs/FAILSAFE.md
new file mode 100644
index 0000000..d3d0a66
--- /dev/null
+++ b/docs/FAILSAFE.md
@@ -0,0 +1,10 @@
+# Fail-Safe Mode #
+
+If more than the expected number of environments is detected during boot, the
+system stops booting. This can be a problem if the user wants to boot the
+system with a memory stick to update a broken installation.
+
+In order to allow external boot devices with other environment configurations,
+the FAILSAFE flag was introduced. If any environment on the boot device has the
+ENV_STATUS_FAILSAVE bit set in the `status_flags`, the boot loader will filter
+out all found environments which are NOT on the boot device.
diff --git a/docs/UPDATE.md b/docs/UPDATE.md
index 19c2f39..9f00fd7 100644
--- a/docs/UPDATE.md
+++ b/docs/UPDATE.md
@@ -11,7 +11,7 @@ The structure of the environment data is as follows:

struct _BG_ENVDATA {
uint16_t kernelfile[ENV_STRING_LENGTH];
uint16_t kernelparams[ENV_STRING_LENGTH];

- uint8_t padding;

+ uint8_t status_flags;
uint8_t ustate;
uint16_t watchdog_timeout_sec;
uint32_t revision;

@@ -23,8 +23,8 @@ struct _BG_ENVDATA {
The fields have the following meaning:
* `kernelfile`: Path to the kernel image, utf-16 encoded
* `kernelparams`: Arguments to the kernel, utf-16 encoded
-* `padding`: Padding byte to stay compatible with the offsets of the previous
- version.
+* `status_flags`: Register to store special environment flags. Currently only
+ ENV_STATUS_IN_PROGRESS and ENV_STATUS_FAILSAFE are defined.
* `ustate`: Update status (`0` OK, `1` INSTALLED, `2` TESTING, `3`: FAILED)
* `watchdog_timeout_sec`: Number of seconds, the watchdog times out after
* `revision`: The revision number explained above
--
2.17.0

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

* volumes[] already contains all needed data, so instead of
allocating two additional arrays (roots, root_devpaths),
just create a mapping config_volumes[] and use
volumes[config_volumes[]]
* remove CONFIG_PARTITION_MAXCOUNT, since the maximum number of
config partitions is always the number of volumes present
* simplify error-handling in config loading and writing

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

env/fatvars.c | 121 ++++++++++++++++++----------------------------
env/syspart.c | 27 +++++------
include/envdata.h | 2 -
include/syspart.h | 8 +--
4 files changed, 60 insertions(+), 98 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index cefff0c..556b836 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -21,41 +21,27 @@ static BG_ENVDATA env[ENV_NUM_CONFIG_PARTS];

BG_STATUS save_current_config(void)
{
- BG_STATUS result = BG_SUCCESS;
+ BG_STATUS result = BG_CONFIG_ERROR;
EFI_STATUS efistatus;
- UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
- EFI_FILE_HANDLE *roots;
- EFI_DEVICE_PATH **root_devices;
+ UINTN numHandles = volume_count;
+ UINTN *config_volumes;

- roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
- CONFIG_PARTITION_MAXCOUNT);
- if (!roots) {
+ config_volumes = (UINTN *)mmalloc(sizeof(EFI_FILE_HANDLE) *
+ volume_count);
+ if (!config_volumes) {

Print(L"Error, could not allocate memory for config partition "

- L"handles.\n");
- return BG_CONFIG_ERROR;
+ L"mapping.\n");
+ return result;
}

- root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *
- CONFIG_PARTITION_MAXCOUNT);
- if (!root_devices) {
- Print(L"Error, could not allocate memory for config partition "
- L"device paths.\n");
- mfree(roots);
- return BG_CONFIG_ERROR;
- }
-
- if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+ if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {

Print(L"Error, could not enumerate config partitions.\n");

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

- if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {
+ if (EFI_ERROR(filter_cfg_parts(config_volumes, &numHandles))) {

Print(L"Error, could not filter config partitions.\n");

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

if (numHandles != ENV_NUM_CONFIG_PARTS) {
@@ -64,21 +50,18 @@ BG_STATUS save_current_config(void)

numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
* overwrite another partition's config file. */

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

+ VOLUME_DESC *v = &volumes[config_volumes[current_partition]];
EFI_FILE_HANDLE fh = NULL;
- efistatus = open_cfg_file(roots[current_partition], &fh,
- EFI_FILE_MODE_WRITE | EFI_FILE_MODE_READ);
+ efistatus = open_cfg_file(v->root, &fh, EFI_FILE_MODE_WRITE |
+ EFI_FILE_MODE_READ);

if (EFI_ERROR(efistatus)) {
Print(L"Error, could not open environment file on system "
L"partition %d: %r\n",

current_partition, efistatus);
- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto scc_cleanup;
}

UINTN writelen = sizeof(BG_ENVDATA);
@@ -90,63 +73,50 @@ BG_STATUS save_current_config(void)
(VOID *)&env[current_partition]);
if (EFI_ERROR(efistatus)) {
Print(L"Error writing environment to file: %r\n", efistatus);
- result = BG_CONFIG_ERROR;
+ (VOID) close_cfg_file(v->root, fh);
+ goto scc_cleanup;
}

- if (EFI_ERROR(close_cfg_file(roots[current_partition], fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {

Print(L"Error, could not close environment config file.\n");

- result = BG_CONFIG_ERROR;
+ goto scc_cleanup;
}
- mfree(root_devices);
- mfree(roots);
+
+ result = BG_SUCCESS;
+scc_cleanup:
+ mfree(config_volumes);
return result;
}

BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
{
- BG_STATUS result = BG_SUCCESS;
- UINTN numHandles = CONFIG_PARTITION_MAXCOUNT;
- EFI_FILE_HANDLE *roots;
- EFI_DEVICE_PATH **root_devices;
+ BG_STATUS result = BG_CONFIG_ERROR;
+ UINTN numHandles = volume_count;
+ UINTN *config_volumes;

UINTN i;
int env_invalid[ENV_NUM_CONFIG_PARTS] = {0};

- roots = (EFI_FILE_HANDLE *)mmalloc(sizeof(EFI_FILE_HANDLE) *
- CONFIG_PARTITION_MAXCOUNT);
- if (!roots) {
+ config_volumes = (UINTN *)mmalloc(sizeof(EFI_FILE_HANDLE) *
+ volume_count);
+ if (!config_volumes) {

Print(L"Error, could not allocate memory for config partition "

- L"handles.\n");
- return BG_CONFIG_ERROR;
+ L"mapping.\n");
+ return result;
}

- root_devices = (EFI_DEVICE_PATH **)mmalloc(sizeof(EFI_DEVICE_PATH *) *
- CONFIG_PARTITION_MAXCOUNT);
- if (!root_devices) {
- Print(L"Error, could not allocate memory for config partition "
- L"device paths.\n");
- mfree(roots);
- return BG_CONFIG_ERROR;
- }
-
- if (EFI_ERROR(enumerate_cfg_parts(roots, root_devices, &numHandles))) {
+ if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {

Print(L"Error, could not enumerate config partitions.\n");

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto lc_cleanup;
}

- if (EFI_ERROR(filter_cfg_parts(roots, root_devices, &numHandles))) {
+ if (EFI_ERROR(filter_cfg_parts(config_volumes, &numHandles))) {

Print(L"Error, could not filter config partitions.\n");

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto lc_cleanup;

}

if (numHandles > ENV_NUM_CONFIG_PARTS) {
Print(L"Error, too many config partitions found. Aborting.\n");

- mfree(root_devices);
- mfree(roots);
- return BG_CONFIG_ERROR;
+ goto lc_cleanup;
}

if (numHandles < ENV_NUM_CONFIG_PARTS) {
@@ -161,8 +131,9 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

/* Load all config data */
for (i = 0; i < numHandles; i++) {
EFI_FILE_HANDLE fh = NULL;

- if (EFI_ERROR(open_cfg_file(roots[i], &fh,
- EFI_FILE_MODE_READ))) {
+ VOLUME_DESC *v = &volumes[config_volumes[i]];
+ if (EFI_ERROR(open_cfg_file(v->root, &fh,

+ EFI_FILE_MODE_READ))) {
Print(L"Warning, could not open environment file on "
L"config partition %d\n",
i);

@@ -175,7 +146,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

L"partition %d.\n",
i);
env_invalid[i] = 1;

- if (EFI_ERROR(close_cfg_file(roots[i], fh))) {
+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {

Print(L"Error, could not close environment "
L"config file.\n");
}

@@ -198,7 +169,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
result = BG_CONFIG_PARTIALLY_CORRUPTED;

}

- if (EFI_ERROR(uefi_call_wrapper(fh->Close, 1, fh))) {

+ if (EFI_ERROR(close_cfg_file(v->root, fh))) {

Print(L"Error, could not close environment config "

L"file.\n");
/* Don't abort, so we may still be able to boot a
@@ -263,8 +234,10 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

Print(L" kernel: %s\n", bglp->payload_path);
Print(L" args: %s\n", bglp->payload_options);
Print(L" timeout: %d seconds\n", bglp->timeout);

- mfree(root_devices);
- mfree(roots);
+
+ result = BG_SUCCESS;
+lc_cleanup:
+ mfree(config_volumes);
return result;
}

diff --git a/env/syspart.c b/env/syspart.c
index 86628a0..48bb9a2 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -37,14 +37,12 @@ EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer)

return uefi_call_wrapper(fh->Read, 3, fh, &readlen, buffer);
}

-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
- EFI_DEVICE_PATH **root_devices,
- UINTN *numHandles)
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)

{
EFI_STATUS status;
UINTN rootCount = 0;

- if (!roots || !numHandles) {
+ if (!config_volumes || !numHandles) {
Print(L"Invalid parameter in system partition enumeration.\n");
return EFI_INVALID_PARAMETER;
}
@@ -58,8 +56,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,

EFI_FILE_MODE_READ);
if (status == EFI_SUCCESS) {
Print(L"Config file found on volume %d.\n", index);

- roots[rootCount] = volumes[index].root;
- root_devices[rootCount] = volumes[index].devpath;
+ config_volumes[rootCount] = index;
rootCount++;

status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {

@@ -74,8 +71,7 @@ EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,
return EFI_SUCCESS;
}

-EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
- EFI_DEVICE_PATH **root_devices, UINTN *numHandles)
+EFI_STATUS filter_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
{
BOOLEAN only_envs_on_bootdevice = FALSE;

@@ -84,8 +80,9 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
EFI_FILE_HANDLE fh = NULL;
EFI_STATUS status;
BG_ENVDATA env;
+ VOLUME_DESC *v = &volumes[config_volumes[index]];

- status = open_cfg_file(roots[index], &fh, EFI_FILE_MODE_READ);
+ status = open_cfg_file(v->root, &fh, EFI_FILE_MODE_READ);
if (EFI_ERROR(status)) {
return status;
}
@@ -95,12 +92,12 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
return status;
}

- if (IsOnBootDevice(root_devices[index]) &&
+ if (IsOnBootDevice(v->devpath) &&
(env.status_flags & ENV_STATUS_FAILSAFE)) {
only_envs_on_bootdevice = TRUE;
};

- status = close_cfg_file(roots[index], fh);
+ status = close_cfg_file(v->root, fh);
if (EFI_ERROR(status)) {
return status;
}
@@ -114,13 +111,11 @@ EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,

Print(L"Fail-Safe Mode enabled.\n");

UINTN index = 0;
do {
- if (!IsOnBootDevice(root_devices[index])) {
+ VOLUME_DESC *v = &volumes[config_volumes[index]];
+ if (!IsOnBootDevice(v->devpath)) {

for (UINTN j = index; j < *numHandles-1; j++) {

- roots[j] = roots[j+1];
- root_devices[j] = root_devices[j+1];
+ config_volumes[j] = config_volumes[j+1];
}
- mfree(roots[*numHandles-1]);
- mfree(root_devices[*numHandles-1]);
(*numHandles)--;

Print(L"Filtered Config #%d\n", index);
}

diff --git a/include/envdata.h b/include/envdata.h
index d2fa6bf..46dd718 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -18,8 +18,6 @@
#define FAT_ENV_FILENAME "BGENV.DAT"
#define ENV_STRING_LENGTH 255

-#define CONFIG_PARTITION_MAXCOUNT 64
-
#define ENV_STATUS_IN_PROGRESS 1
#define ENV_STATUS_FAILSAFE 128

diff --git a/include/syspart.h b/include/syspart.h
index 269fa25..18b9a23 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -25,11 +25,7 @@ EFI_STATUS open_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE *fh,

UINT64 mode);
EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh);
EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer);
-EFI_STATUS enumerate_cfg_parts(EFI_FILE_HANDLE *roots,

- EFI_DEVICE_PATH **root_devices,
- UINTN *maxHandles);
-EFI_STATUS filter_cfg_parts(EFI_FILE_HANDLE *roots,
- EFI_DEVICE_PATH **root_devices,
- UINTN *maxHandles);
+EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
+EFI_STATUS filter_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

New option S sets or resets the fail-safe mode flag in the new
status flag register.

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

tools/bg_setenv.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index cae268d..0847de1 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -45,6 +45,7 @@ static struct argp_option options_setenv[] = {
"simulate a running update "
"process."},
{"version", 'V', 0, 0, "Print version"},
+ {"failsafe", 'S', "FAILSAFE", 0, "Set Fail-Safe Flag"},
{0}};

static struct argp_option options_printenv[] = {
@@ -393,6 +394,12 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
* argp_usage with non-zero return code */
argp_usage(state);
break;
+ case 'S':
+ VERBOSE(stdout, "Enabeling fail-safe mode.\n");
+ e = journal_add_action(ENV_TASK_SET,
+ "env_status_failsafe", 0,
+ (uint8_t *)arg, strlen(arg) + 1);
+ break;
default:
return ARGP_ERR_UNKNOWN;
}
--
2.17.0

[Claudius Heine]

Hi Andreas,

here we go again ;)

Why not as macro? (I guess because of the includes?)
Anyway, you could also put a 'inline' there.

Claudius

> VOID *mmalloc(UINTN bytes)
> {
> EFI_STATUS status;
>

--

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

[Claudius Heine]

Hi,

On 2018-05-25 14:00, [ext] Andreas J. Reichel wrote:

Magic numbers. Use 'sizeof'.

> + if (!dst) {
> + return NULL;
> + }
> +
> + StrCpy(dst, input);
> +
> + for (UINTN i = len; i > 0; i--)
> + {
> + if (dst[i * 2] == delim) {
> + dst[i * 2] = L'\0';
> + break;
> + }
> + }

Here I get this itching in my coding fingers. I would try do copying and
striping in one loop.

Claudius

> + return dst;
> +}
> +
> uint32_t calc_crc32(void *data, int32_t size)
> {
> uint32_t crc;
>

--

[Andreas Reichel]

On Mon, May 28, 2018 at 01:05:58PM +0200, Claudius Heine wrote:
> Hi Andreas,
>
> here we go again ;)
>
> On 2018-05-25 14:00, [ext] Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >

> > +++ b/utils.c
> > @@ -32,6 +32,11 @@ void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
> > unreachable();
> > }
> > +VOID sleep(int32_t sec)
> > +{
> > + uefi_call_wrapper(BS->Stall, 1, sec * 1000 * 1000);
> > +}
> > +
>
> Why not as macro? (I guess because of the includes?)

Because I wrote it as a function.
Technically you gave no reason.

> Anyway, you could also put a 'inline' there.

No, an 'inline' is static and static is the opposite of what I want.

Extern inline would be possible but that would produce the same code
inside the scope of each translation unit which is rather... hm ugly?

On the other hand... the compiler may decide to not inline the function.

> Claudius
>
>
> > VOID *mmalloc(UINTN bytes)
> > {
> > EFI_STATUS status;
> >
>
> --
> 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

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

[Claudius Heine]

Hi,

On 2018-05-25 14:00, [ext] Andreas J. Reichel wrote:

You are allocating memory in StrRStrip and are not freeing it.

[Claudius Heine]

Hi Andreas,

On 2018-05-28 13:28, Andreas Reichel wrote:
> On Mon, May 28, 2018 at 01:05:58PM +0200, Claudius Heine wrote:
>> Hi Andreas,
>>
>> here we go again ;)
>>
>> On 2018-05-25 14:00, [ext] Andreas J. Reichel wrote:
>>> From: Andreas Reichel <andreas.r...@siemens.com>
>>>
>>> +++ b/utils.c
>>> @@ -32,6 +32,11 @@ void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
>>> unreachable();
>>> }
>>> +VOID sleep(int32_t sec)
>>> +{
>>> + uefi_call_wrapper(BS->Stall, 1, sec * 1000 * 1000);
>>> +}
>>> +
>>
>> Why not as macro? (I guess because of the includes?)
> Because I wrote it as a function.
> Technically you gave no reason.

What you wanted to have is some kind of parameter binding to an existing
function and shortening the name.

I do such things normally with macros or inline functions. That is the
pattern I am used to. Argument is: better performance and memory usage.

That might not be that much if you have just some small cases. And with
a sleep that takes whole seconds as a parameter and only used for
debugging not important. But it might get more if used constantly or on
performance critical paths and can be avoided very simply. So its good
to get used to programming this way generally.

[Claudius Heine]

merge with patch 9 (where you use it)

On 2018-05-25 14:00, [ext] Andreas J. Reichel wrote:

[Claudius Heine]

Here you could use macros or inline functions as well. Just like with
all other functions look the same.

> +
> +EFI_STATUS close_cfg_file(EFI_FILE_HANDLE root, EFI_FILE_HANDLE fh)
> +{
> + return uefi_call_wrapper(root->Close, 1, fh);
> +}
> +
> +EFI_STATUS read_cfg_file(EFI_FILE_HANDLE fh, VOID *buffer)
> +{
> + UINTN readlen = sizeof(BG_ENVDATA);
> + return uefi_call_wrapper(fh->Read, 3, fh, &readlen, buffer);
> +}

This is also a macro candidate. readlen is a static const.

[Claudius Heine]

On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:

With that many error cases, checking them and jumping to a label that
does mfrees and return BG_CONFIG_ERROR becomes handy.

Same here.

Sometimes I like to use a Macro for these kind of repeating error checks
and returns.

[Claudius Heine]

On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:

Yep that is the kind of thing I don't like in patchsets :) Don't add
code in one patch and then change it again in one of the next ones.

If you want to fix this, then take a look at 'git add -p'.

You could also split the implementation of the fail-safe from the
integration and maybe merge that into this patch if that is easier.

Or just merge 9 and 10.

[Andreas Reichel]

On Mon, May 28, 2018 at 03:46:03PM +0200, Claudius Heine wrote:
>
>
> On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:
> > From: Andreas Reichel <andreas.r...@siemens.com>
> >
> > * volumes[] already contains all needed data, so instead of
> > allocating two additional arrays (roots, root_devpaths),
> > just create a mapping config_volumes[] and use
> > volumes[config_volumes[]]
> > * remove CONFIG_PARTITION_MAXCOUNT, since the maximum number of
> > config partitions is always the number of volumes present
> > * simplify error-handling in config loading and writing
> >
> > Signed-off-by: Andreas Reichel <andreas.r...@siemens.com>
> > ---
> > env/fatvars.c | 121 ++++++++++++++++++----------------------------
> > env/syspart.c | 27 +++++------
> > include/envdata.h | 2 -
> > include/syspart.h | 8 +--
> > 4 files changed, 60 insertions(+), 98 deletions(-)
> >
> > diff --git a/env/fatvars.c b/env/fatvars.c
> > index cefff0c..556b836 100644

> > - mfree(root_devices);
> > - mfree(roots);
> > - return BG_CONFIG_ERROR;
> > + goto scc_cleanup;
>
> Yep that is the kind of thing I don't like in patchsets :) Don't add code in
> one patch and then change it again in one of the next ones.
>
> If you want to fix this, then take a look at 'git add -p'.
>
> You could also split the implementation of the fail-safe from the
> integration and maybe merge that into this patch if that is easier.
>
> Or just merge 9 and 10.
>

As our long discussion just pointed out, merging 9 and 10 seems to be
the best solution.

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

[Andreas Reichel]

okay...

> > + if (!dst) {
> > + return NULL;
> > + }
> > +
> > + StrCpy(dst, input);
> > +
> > + for (UINTN i = len; i > 0; i--)
> > + {
> > + if (dst[i * 2] == delim) {
> > + dst[i * 2] = L'\0';
> > + break;
> > + }
> > + }
>
> Here I get this itching in my coding fingers. I would try do copying and
> striping in one loop.
>

As discussed, I used predefined functions and we had different ideas
about what a 'RStrip' function should do. Changing the name to make
clear that this is a function of special intent for device paths is
better.

> Claudius
>
> > + return dst;
> > +}
> > +
> > uint32_t calc_crc32(void *data, int32_t size)
> > {
> > uint32_t crc;
> >
>
> --
> 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

[Claudius Heine]

On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:
> From: Andreas Reichel <andreas.r...@siemens.com>
>
> If a variable like watchdog is set with a negative argument,
> bg_setenv returns this negative argument instead of -EINVAL.
> Fix this to -EINVAL.

This patch could have been posted outside of this patchset.

[Claudius Heine]

On 2018-05-25 14:01, [ext] Andreas J. Reichel wrote:

I should be possible to merge EBGENV_FAILSAFE with EBGENV_IN_PROGRESS.

> default:
> if (!data) {
> return 0;
> @@ -411,6 +420,21 @@ int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
> return -EINVAL;
> }
> break;
> + case EBGENV_FAILSAFE:
> + val = bgenv_convert_to_long(value);
> + if (val < 0) {
> + return val;
> + }
> + switch(val) {
> + case 1:
> + env->data->status_flags |= ENV_STATUS_FAILSAFE;
> + break;
> + case 0:
> + env->data->status_flags &= ~ENV_STATUS_FAILSAFE;
> + default:
> + return -EINVAL;
> + }
> + break;

This as well?

> default:
> return -EINVAL;
> }
> diff --git a/include/env_api.h b/include/env_api.h
> index 45bf4b7..04516bb 100644
> --- a/include/env_api.h
> +++ b/include/env_api.h
> @@ -54,6 +54,7 @@ typedef enum {
> EBGENV_REVISION,
> EBGENV_USTATE,
> EBGENV_IN_PROGRESS,
> + EBGENV_FAILSAFE,
> EBGENV_UNKNOWN
> } EBGENVKEY;
>
>

--

[Claudius Heine]

Ok, you can ignore this. I haven't read exactly enough.
EBGENV_FAILSAFE != ENV_STATUS_FAILSAFE

[Andreas Reichel]

These patch series will be split and the new two series start with
version 1 again.

[Andreas J. Reichel]

From: Andreas Reichel <andreas.r...@siemens.com>

Prevent 404 during apt-get install with apt-get update in before_install

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

.travis.yml | 1 +

1 file changed, 1 insertion(+)