[PATCH 1/6] Classify core console output and make it more colorful

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Introduce ERROR(), WARNING(), and INFO() console message
output macros to classify the messages and make their output
more colorful so to increase readability for EBG's core output.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
env/fatvars.c | 44 ++++++++++++++++++++++----------------------
env/syspart.c | 14 +++++++-------
include/utils.h | 19 +++++++++++++++++++
main.c | 14 ++++++--------
utils.c | 49 ++++++++++++++++++++++++++++---------------------
5 files changed, 82 insertions(+), 58 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 99ac37e..e86743e 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -28,20 +28,20 @@ BG_STATUS save_current_config(void)

config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
if (!config_volumes) {
- Print(L"Error, could not allocate memory for config partition "
+ ERROR(L"Could not allocate memory for config partition "
L"mapping.\n");
return result;
}

if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.\n");
+ ERROR(L"Could not enumerate config partitions.\n");
goto scc_cleanup;
}

numHandles = filter_cfg_parts(config_volumes, numHandles);

if (numHandles != ENV_NUM_CONFIG_PARTS) {
- Print(L"Error, unexpected number of config partitions: found "
+ ERROR(L"Unexpected number of config partitions: found "
L"%d, but expected %d.\n",
numHandles, ENV_NUM_CONFIG_PARTS);
/* In case of saving, this must be treated as error, to not
@@ -54,7 +54,7 @@ BG_STATUS save_current_config(void)
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 "
+ ERROR(L"Could not open environment file on system "
L"partition %d: %r\n",
current_partition, efistatus);
goto scc_cleanup;
@@ -68,13 +68,13 @@ BG_STATUS save_current_config(void)
efistatus = uefi_call_wrapper(fh->Write, 3, fh, &writelen,
(VOID *)&env[current_partition]);
if (EFI_ERROR(efistatus)) {
- Print(L"Error writing environment to file: %r\n", efistatus);
+ ERROR(L"Cannot write environment to file: %r\n", efistatus);
(VOID) close_cfg_file(v->root, fh);
goto scc_cleanup;
}

if (EFI_ERROR(close_cfg_file(v->root, fh))) {
- Print(L"Error, could not close environment config file.\n");
+ ERROR(L"Could not close environment config file.\n");
goto scc_cleanup;
}

@@ -94,25 +94,25 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)

config_volumes = (UINTN *)mmalloc(sizeof(UINTN) * volume_count);
if (!config_volumes) {
- Print(L"Error, could not allocate memory for config partition "
+ ERROR(L"Could not allocate memory for config partition "
L"mapping.\n");
return result;
}

if (EFI_ERROR(enumerate_cfg_parts(config_volumes, &numHandles))) {
- Print(L"Error, could not enumerate config partitions.\n");
+ ERROR(L"Could not enumerate config partitions.\n");
goto lc_cleanup;
}

numHandles = filter_cfg_parts(config_volumes, numHandles);

if (numHandles > ENV_NUM_CONFIG_PARTS) {
- Print(L"Error, too many config partitions found. Aborting.\n");
+ ERROR(L"Too many config partitions found. Aborting.\n");
goto lc_cleanup;
}

if (numHandles < ENV_NUM_CONFIG_PARTS) {
- Print(L"Warning, too few config partitions: found: "
+ WARNING(L"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
@@ -126,7 +126,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
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 "
+ WARNING(L"Could not open environment file on "
L"config partition %d\n",
i);
result = BG_CONFIG_PARTIALLY_CORRUPTED;
@@ -134,12 +134,12 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
}
UINTN readlen = sizeof(BG_ENVDATA);
if (EFI_ERROR(read_cfg_file(fh, &readlen, (VOID *)&env[i]))) {
- Print(L"Error reading environment from config "
+ ERROR(L"Cannot read environment from config "
L"partition %d.\n",
i);
env_invalid[i] = 1;
if (EFI_ERROR(close_cfg_file(v->root, fh))) {
- Print(L"Error, could not close environment "
+ ERROR(L"Could not close environment "
L"config file.\n");
}
result = BG_CONFIG_PARTIALLY_CORRUPTED;
@@ -149,11 +149,11 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
uint32_t crc32 = calc_crc32(&env[i], sizeof(BG_ENVDATA) -
sizeof(env[i].crc32));
if (crc32 != env[i].crc32) {
- Print(L"CRC32 error in environment data on config "
+ ERROR(L"CRC32 error in environment data on config "
L"partition %d.\n",
i);
- Print(L"calculated: %lx\n", crc32);
- Print(L"stored: %lx\n", env[i].crc32);
+ INFO(L"calculated: %lx\n", crc32);
+ INFO(L"stored: %lx\n", env[i].crc32);
/* Don't treat this as fatal error because we may still
* have
* valid environments */
@@ -162,7 +162,7 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
}

if (EFI_ERROR(close_cfg_file(v->root, fh))) {
- Print(L"Error, could not close environment config "
+ ERROR(L"Could not close environment config "
L"file.\n");
/* Don't abort, so we may still be able to boot a
* config */
@@ -220,12 +220,12 @@ BG_STATUS load_config(BG_LOADER_PARAMS *bglp)
StrDuplicate(env[current_partition].kernelparams);
bglp->timeout = env[current_partition].watchdog_timeout_sec;

- Print(L"Config Revision: %d:\n", latest_rev);
- Print(L" ustate: %d\n",
+ INFO(L"Config Revision: %d:\n", latest_rev);
+ INFO(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);
+ INFO(L" kernel: %s\n", bglp->payload_path);
+ INFO(L" args: %s\n", bglp->payload_options);
+ INFO(L" timeout: %d seconds\n", bglp->timeout);

result = BG_SUCCESS;
lc_cleanup:
diff --git a/env/syspart.c b/env/syspart.c
index 5e248e5..44a18d3 100644
--- a/env/syspart.c
+++ b/env/syspart.c
@@ -24,7 +24,7 @@ EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
UINTN rootCount = 0;

if (!config_volumes || !numHandles) {
- Print(L"Invalid parameter in system partition enumeration.\n");
+ ERROR(L"Invalid parameter in system partition enumeration.\n");
return EFI_INVALID_PARAMETER;
}
for (UINTN index = 0; index < volume_count && rootCount < *numHandles;
@@ -36,19 +36,19 @@ EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *numHandles)
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);
+ INFO(L"Config file found on volume %d.\n", index);
config_volumes[rootCount] = index;
rootCount++;
status = close_cfg_file(volumes[index].root, fh);
if (EFI_ERROR(status)) {
- Print(L"Could not close config file on "
+ ERROR(L"Could not close config file on "
L"partition %d.\n",
index);
}
}
}
*numHandles = rootCount;
- Print(L"%d config partitions detected.\n", rootCount);
+ INFO(L"%d config partitions detected.\n", rootCount);
return EFI_SUCCESS;
}

@@ -64,7 +64,7 @@ UINTN filter_cfg_parts(UINTN *config_volumes, UINTN numHandles)
{
BOOLEAN use_envs_on_bootmedium_only = FALSE;

- Print(L"Config filter: \n");
+ INFO(L"Config filter: \n");
for (UINTN index = 0; index < numHandles; index++) {
VOLUME_DESC *v = &volumes[config_volumes[index]];

@@ -78,7 +78,7 @@ UINTN filter_cfg_parts(UINTN *config_volumes, UINTN numHandles)
return numHandles;
}

- Print(L"Booting with environments from boot medium only.\n");
+ INFO(L"Booting with environments from boot medium only.\n");
UINTN num_sorted = 0;
for (UINTN j = 0; j < numHandles; j++) {
UINTN cvi = config_volumes[j];
@@ -88,7 +88,7 @@ UINTN filter_cfg_parts(UINTN *config_volumes, UINTN numHandles)
swap_uintn(&config_volumes[j],
&config_volumes[num_sorted++]);
} else {
- Print(L"Ignoring config on volume #%d\n", cvi);
+ WARNING(L"Ignoring config on volume #%d\n", cvi);
}
}

diff --git a/include/utils.h b/include/utils.h
index 8ce5ac6..8978ec9 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -42,4 +42,23 @@ CHAR16 *GetBootMediumPath(CHAR16 *input);
BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp);
VOID Color(EFI_SYSTEM_TABLE *system_table, char fgcolor, char bgcolor);

+VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...);
+#define ERROR(fmt, ...) \
+ do { \
+ PrintC(EFI_LIGHTRED, L"ERROR: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define WARNING(fmt, ...) \
+ do { \
+ PrintC(EFI_YELLOW, L"WARNING: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define INFO(fmt, ...) \
+ do { \
+ PrintC(EFI_WHITE, L"> "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
#endif // __H_UTILS__
diff --git a/main.c b/main.c
index 9fe8bbe..320e4dc 100644
--- a/main.c
+++ b/main.c
@@ -114,9 +114,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
this_image = image_handle;
InitializeLib(this_image, system_table);

- Color(system_table, 3, 0);
- Print(L"\nEFI Boot Guard %s\n", L""EFIBOOTGUARD_VERSION);
- Color(system_table, 7, 0);
+ PrintC(EFI_CYAN, L"\nEFI Boot Guard %s\n", L"" EFIBOOTGUARD_VERSION);

status =
uefi_call_wrapper(BS->OpenProtocol, 6, this_image,
@@ -131,7 +129,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
tmp = DevicePathToStr(DevicePathFromHandle(loaded_image->DeviceHandle));
boot_medium_path = GetBootMediumPath(tmp);
mfree(tmp);
- Print(L"Boot medium: %s\n", boot_medium_path);
+ INFO(L"Boot medium: %s\n", boot_medium_path);

status = get_volumes(&volumes, &volume_count);
if (EFI_ERROR(status)) {
@@ -139,7 +137,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status);
}

- Print(L"Loading configuration...\n");
+ INFO(L"Loading configuration...\n");

bg_status = load_config(&bg_loader_params);
if (BG_ERROR(bg_status)) {
@@ -151,7 +149,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
EFI_ABORTED);
break;
case BG_CONFIG_PARTIALLY_CORRUPTED:
- Print(L"Config is partially corrupted. Please check.\n"
+ WARNING(L"Config is partially corrupted. Please check.\n"
L"efibootguard will try to boot.\n");
break;
default:
@@ -170,7 +168,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
}

if (bg_loader_params.timeout == 0) {
- Print(L"Watchdog is disabled.\n");
+ WARNING(L"Watchdog is disabled.\n");
} else {
status = scan_devices(loaded_image, bg_loader_params.timeout);
if (EFI_ERROR(status)) {
@@ -202,7 +200,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
loaded_image->LoadOptionsSize =
(StrLen(bg_loader_params.payload_options) + 1) * sizeof(CHAR16);

- Print(L"Starting %s with watchdog set to %d seconds\n",
+ INFO(L"Starting %s with watchdog set to %d seconds ...",
bg_loader_params.payload_path, bg_loader_params.timeout);

return uefi_call_wrapper(BS->StartImage, 3, payload_handle, 0, 0);
diff --git a/utils.c b/utils.c
index cda0e92..cb9569d 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,19 @@
#include <bootguard.h>
#include <utils.h>

+VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...)
+{
+ INT32 attr = ST->ConOut->Mode->Attribute;
+ (VOID)uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, color);
+
+ va_list args;
+ va_start(args, fmt);
+ (VOID)VPrint(fmt, args);
+ va_end(args);
+
+ (VOID)uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, attr);
+}
+
BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp)
{
extern CHAR16 *boot_medium_path;
@@ -44,7 +57,7 @@ uint32_t calc_crc32(void *data, int32_t size)

void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
{
- Print(L"%s ( %r )\n", message, status);
+ ERROR(L"%s ( %r )\n", message, status);
uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);
uefi_call_wrapper(BS->Exit, 4, this_image, status, 0, NULL);
unreachable();
@@ -121,21 +134,21 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
EFI_FILE_HANDLE tmp;

if (!volumes || !count) {
- Print(L"Invalid volume enumeration.\n");
+ ERROR(L"Invalid volume enumeration.\n");
return EFI_INVALID_PARAMETER;
}

status = uefi_call_wrapper(BS->LocateHandleBuffer, 5, ByProtocol,
&sfspGuid, NULL, &handleCount, &handles);
if (EFI_ERROR(status)) {
- Print(L"Could not locate handle buffer.\n");
+ ERROR(L"Could not locate handle buffer.\n");
return EFI_OUT_OF_RESOURCES;
}
- Print(L"Found %d handles for file IO\n\n", handleCount);
+ INFO(L"Found %d handles for file IO\n\n", handleCount);

*volumes = (VOLUME_DESC *)mmalloc(sizeof(VOLUME_DESC) * handleCount);
if (!*volumes) {
- Print(L"Could not allocate memory for volume descriptors.\n");
+ ERROR(L"Could not allocate memory for volume descriptors.\n");
return EFI_OUT_OF_RESOURCES;
}

@@ -148,7 +161,7 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
&sfspGuid, (VOID **)&fs);
if (EFI_ERROR(status)) {
/* skip failed handle and continue enumerating */
- Print(L"File IO handle %d does not support "
+ ERROR(L"File IO handle %d does not support "
L"SIMPLE_FILE_SYSTEM_PROTOCOL, skipping.\n",
index);
continue;
@@ -156,14 +169,14 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
status = uefi_call_wrapper(fs->OpenVolume, 2, fs, &tmp);
if (EFI_ERROR(status)) {
/* skip failed handle and continue enumerating */
- Print(L"Could not open file system for IO handle %d, "
+ ERROR(L"Could not open file system for IO handle %d, "
L"skipping.\n",
index);
continue;
}
EFI_DEVICE_PATH *devpath = DevicePathFromHandle(handles[index]);
if (devpath == NULL) {
- Print(
+ ERROR(
L"Could not get device path for config partition, "
L"skipping.\n");
continue;
@@ -176,11 +189,11 @@ EFI_STATUS get_volumes(VOLUME_DESC **volumes, UINTN *count)
get_volume_label((*volumes)[rootCount].root);
(*volumes)[rootCount].fscustomlabel =
get_volume_custom_label((*volumes)[rootCount].root);
- Print(L"Volume %d: ", rootCount);
+ INFO(L"Volume %d: ", rootCount);
if (IsOnBootMedium(devpath)) {
- Print(L"(On boot medium) ");
+ PrintC(EFI_LIGHTGRAY, L"(On boot medium) ");
}
- Print(L"%s, LABEL=%s, CLABEL=%s\n",
+ PrintC(EFI_LIGHTGRAY, L"%s, LABEL=%s, CLABEL=%s\n",
devpathstr, (*volumes)[rootCount].fslabel,
(*volumes)[rootCount].fscustomlabel);

@@ -198,20 +211,20 @@ EFI_STATUS close_volumes(VOLUME_DESC *volumes, UINTN count)
UINTN i;

if (!volumes) {
- Print(L"Invalid parameter for closing volumes.\n");
+ ERROR(L"Invalid parameter for closing volumes.\n");
return EFI_INVALID_PARAMETER;
}
for (i = 0; i < count; i++) {
EFI_STATUS status;

if (!volumes[i].root) {
- Print(L"Error, invalid handle for volume %d.\n", i);
+ ERROR(L"Invalid handle for volume %d.\n", i);
result = EFI_INVALID_PARAMETER;
}
status = uefi_call_wrapper(volumes[i].root->Close, 1,
volumes[i].root);
if (EFI_ERROR(status)) {
- Print(L"Could not close volume %d.\n", i);
+ ERROR(L"Could not close volume %d.\n", i);
result = EFI_DEVICE_ERROR;
}
}
@@ -277,7 +290,7 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,

StrCpy(fullpath, pathprefix);
StrCat(fullpath, payloadpath + prefixlen + 3);
- Print(L"Full path for kernel is: %s\n", fullpath);
+ INFO(L"Full path for kernel is: %s\n", fullpath);

mfree(fullpath);
mfree(pathprefix);
@@ -317,9 +330,3 @@ CHAR16 *GetBootMediumPath(CHAR16 *input)

return dst;
}
-
-VOID Color(EFI_SYSTEM_TABLE *system_table, char fgcolor, char bgcolor)
-{
- SIMPLE_TEXT_OUTPUT_INTERFACE *con = system_table->ConOut;
- (VOID)uefi_call_wrapper(con->SetAttribute, 3, con, (bgcolor << 8) | fgcolor);
-}
--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Any modern compiler (e.g., gcc > 3.4, released 2004)
supports #pragma once -- and it doesn't pollute the
namespace with a preprocessor symbol.
So, get rid of overly verbose include guards.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

include/bootguard.h | 4 +---
include/configuration.h | 4 +---
include/ebgenv.h | 4 +---
include/ebgpart.h | 4 +---
include/env_api.h | 4 +---
include/env_config_file.h | 4 +---
include/env_config_partitions.h | 4 +---
include/env_disk_utils.h | 4 +---
include/envdata.h | 4 +---
include/syspart.h | 4 +---
include/test-interface.h | 5 +----
include/uservars.h | 5 +----
include/utils.h | 4 +---
tools/tests/fake_devices.h | 4 +---
14 files changed, 14 insertions(+), 44 deletions(-)

diff --git a/include/bootguard.h b/include/bootguard.h
index bf244a4..159a396 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_BOOTGUARD__
-#define __H_BOOTGUARD__
+#pragma once

#if defined(__GNUC__)
#define __noreturn __attribute__((noreturn))
@@ -58,4 +57,3 @@ typedef struct _BG_LOADER_PARAMS {
UINTN timeout;
} BG_LOADER_PARAMS;

-#endif // __H_BOOTGUARD__
diff --git a/include/configuration.h b/include/configuration.h
index 34a1054..9f1425e 100644
--- a/include/configuration.h
+++ b/include/configuration.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_CONFIG__
-#define __H_CONFIG__
+#pragma once

#include <efi.h>
#include <efilib.h>
@@ -24,4 +23,3 @@
BG_STATUS load_config(BG_LOADER_PARAMS *bg_loader_params);
BG_STATUS save_config(BG_LOADER_PARAMS *bg_loader_params);

-#endif // __H_CONFIG__
diff --git a/include/ebgenv.h b/include/ebgenv.h
index 67e2686..95b5bd4 100644
--- a/include/ebgenv.h
+++ b/include/ebgenv.h
@@ -14,8 +14,7 @@
*
*/

-#ifndef __EBGENV_H__
-#define __EBGENV_H__
+#pragma once

#include <errno.h>

@@ -144,4 +143,3 @@ int ebg_env_register_gc_var(ebgenv_t *e, char *key);
*/
int ebg_env_finalize_update(ebgenv_t *e);

-#endif //__EBGENV_H__
diff --git a/include/ebgpart.h b/include/ebgpart.h
index a2b7552..d02cfbe 100644
--- a/include/ebgpart.h
+++ b/include/ebgpart.h
@@ -16,8 +16,7 @@
* partition tables.
*/

-#ifndef __EBGPART_H__
-#define __EBGPART_H__
+#pragma once

#ifndef _LARGEFILE64_SOURCE
#define _LARGEFILE64_SOURCE
@@ -141,4 +140,3 @@ PedPartition *ped_disk_next_partition(const PedDisk *pd,

void ebgpart_beverbose(bool v);

-#endif // __EBGPART_H__
diff --git a/include/env_api.h b/include/env_api.h
index 9a5c2a8..eccfd4e 100644
--- a/include/env_api.h
+++ b/include/env_api.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_API_H__
-#define __ENV_API_H__
+#pragma once

#include <stddef.h>
#include <stdint.h>
@@ -94,4 +93,3 @@ extern int bgenv_set(BGENV *env, char *key, uint64_t type, void *data,
uint32_t datalen);
extern uint8_t *bgenv_find_uservar(uint8_t *userdata, char *key);

-#endif // __ENV_API_H__
diff --git a/include/env_config_file.h b/include/env_config_file.h
index 2679174..081c922 100644
--- a/include/env_config_file.h
+++ b/include/env_config_file.h
@@ -12,12 +12,10 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_CONFIG_FILE_H__
-#define __ENV_CONFIG_FILE_H__
+#pragma once

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);
FILE *open_config_file(char *configfilepath, char *mode);
int close_config_file(FILE *config_file_handle);
bool probe_config_file(CONFIG_PART *cfgpart);

-#endif // __ENV_CONFIG_FILE_H__
diff --git a/include/env_config_partitions.h b/include/env_config_partitions.h
index ebe6712..c6cd033 100644
--- a/include/env_config_partitions.h
+++ b/include/env_config_partitions.h
@@ -12,9 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_CONFIG_PARTITIONS_H__
-#define __ENV_CONFIG_PARTITIONS_H__
+#pragma once

bool probe_config_partitions(CONFIG_PART *cfgpart);

-#endif // __ENV_CONFIG_PARTITIONS_H__
diff --git a/include/env_disk_utils.h b/include/env_disk_utils.h
index 7450f20..eafdb59 100644
--- a/include/env_disk_utils.h
+++ b/include/env_disk_utils.h
@@ -12,11 +12,9 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_DISK_UTILS_H__
-#define __ENV_DISK_UTILS_H__
+#pragma once

char *get_mountpoint(char *devpath);
bool mount_partition(CONFIG_PART *cfgpart);
void unmount_partition(CONFIG_PART *cfgpart);

-#endif // __ENV_DISK_UTILS_H__
diff --git a/include/envdata.h b/include/envdata.h
index 13cdb8d..e1f6195 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_ENV_DATA__
-#define __H_ENV_DATA__
+#pragma once

#define FAT_ENV_FILENAME "BGENV.DAT"
#define ENV_STRING_LENGTH 255
@@ -45,4 +44,3 @@ struct _BG_ENVDATA {

typedef struct _BG_ENVDATA BG_ENVDATA;

-#endif // __H_ENV_DATA__
diff --git a/include/syspart.h b/include/syspart.h
index b22577b..a96b3b1 100644
--- a/include/syspart.h
+++ b/include/syspart.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_SYSPART__
-#define __H_SYSPART__
+#pragma once

#include <efi.h>
#include <efilib.h>
@@ -35,4 +34,3 @@
EFI_STATUS enumerate_cfg_parts(UINTN *config_volumes, UINTN *maxHandles);
UINTN filter_cfg_parts(UINTN *config_volumes, UINTN maxHandles);

-#endif // __H_SYSPART__
diff --git a/include/test-interface.h b/include/test-interface.h
index 9adbf56..9d43b1f 100644
--- a/include/test-interface.h
+++ b/include/test-interface.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __TEST_INTERFACE_H__
-#define __TEST_INTERFACE_H__
+#pragma once

#include "env_api.h"

@@ -25,5 +24,3 @@ bool probe_config_partitions(CONFIG_PART *cfgparts);
bool mount_partition(CONFIG_PART *cfgpart);

EBGENVKEY bgenv_str2enum(char *key);
-
-#endif // __TEST_INTERFACE_H__
diff --git a/include/uservars.h b/include/uservars.h
index 11783a7..f2f3587 100644
--- a/include/uservars.h
+++ b/include/uservars.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __USER_VARS_H__
-#define __USER_VARS_H__
+#pragma once

#include <stdint.h>

@@ -36,5 +35,3 @@ uint8_t *bgenv_uservar_realloc(uint8_t *udata, uint32_t new_rsize,
uint8_t *p);
void bgenv_del_uservar(uint8_t *udata, uint8_t *var);
uint32_t bgenv_user_free(uint8_t *udata);
-
-#endif // __USER_VARS_H__
diff --git a/include/utils.h b/include/utils.h
index 8978ec9..2f4fa87 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -13,8 +13,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_UTILS__
-#define __H_UTILS__
+#pragma once

#include "bootguard.h"

@@ -61,4 +60,3 @@ VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...);
PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
} while (0)

-#endif // __H_UTILS__
diff --git a/tools/tests/fake_devices.h b/tools/tests/fake_devices.h
index 40a8de1..4b03a36 100644
--- a/tools/tests/fake_devices.h
+++ b/tools/tests/fake_devices.h
@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __FAKE_DEVICES_H__
-#define __FAKE_DEVICES_H__
+#pragma once

#include <ebgpart.h>

@@ -27,4 +26,3 @@ void free_fake_devices(void);

PedDevice *ped_device_get_next_custom_fake(const PedDevice *dev);

-#endif // __FAKE_DEVICES_H__
--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

The headers bootguard.h and utils.h mutually #include each other
for no apparent reason. Hence, disentangle their #include relation
to allow selective inclusion.

One problem of this is, e.g., utils.h defines _VOLUME_DESC
after #include "bootguard.h". However, bootguard.h uses
this in extern VOLUME_DESC *volumes.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

bootguard.c | 1 +
env/fatvars.c | 1 +
include/bootguard.h | 16 ----------------
include/utils.h | 16 +++++++++++++++-
4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/bootguard.c b/bootguard.c
index 22a2a8a..55811dd 100644
--- a/bootguard.c
+++ b/bootguard.c
@@ -13,6 +13,7 @@
*/

#include "bootguard.h"
+#include "utils.h"

EFI_HANDLE this_image;

diff --git a/env/fatvars.c b/env/fatvars.c
index e86743e..5c63212 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c
@@ -13,6 +13,7 @@
*/

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

diff --git a/include/bootguard.h b/include/bootguard.h
index 159a396..99d8fe4 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -14,20 +14,7 @@

#pragma once

-#if defined(__GNUC__)
-#define __noreturn __attribute__((noreturn))
-#define unreachable() __builtin_unreachable()
-#else
-#define __noreturn /**/
-#define unreachable() \
- do { \
- } while (1)
-#endif
-
#include <efi.h>
-#include <efilib.h>
-#include <efiprot.h>
-#include <utils.h>

/* The following definitions regarding status and error constants are
* implemented the same way the corresponding gnu-efi constants are
@@ -48,9 +35,6 @@ typedef int BG_STATUS;

extern EFI_HANDLE this_image;

-extern VOLUME_DESC *volumes;
-extern UINTN volume_count;
-
typedef struct _BG_LOADER_PARAMS {
CHAR16 *payload_path;
CHAR16 *payload_options;
diff --git a/include/utils.h b/include/utils.h
index 2f4fa87..ff8f9ef 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -15,10 +15,21 @@

#pragma once

-#include "bootguard.h"
+#include <efi.h>
+#include <efilib.h>

#define MAX_INFO_SIZE 1024

+#if defined(__GNUC__)
+#define __noreturn __attribute__((noreturn))
+#define unreachable() __builtin_unreachable()
+#else
+#define __noreturn /**/
+#define unreachable() \
+ do { \
+ } while (1)
+#endif
+
typedef struct _VOLUME_DESC {
EFI_DEVICE_PATH *devpath;
CHAR16 *fslabel;
@@ -26,6 +37,9 @@ typedef struct _VOLUME_DESC {
EFI_FILE_HANDLE root;
} VOLUME_DESC;

+extern VOLUME_DESC *volumes;
+extern UINTN volume_count;
+
typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

uint32_t calc_crc32(void *data, int32_t size);
--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

__builtin_unreachable() is available since gcc 4.5, released 2010.
__attribute__((noreturn)) is avilable since gcc 2.5.0, released 1993.
__attribute__((unused)) is available since gcc 2.7, released 1995.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

include/ebgpart.h | 4 ----
include/utils.h | 12 +-----------
tools/ebgpart.c | 2 +-
utils.c | 4 ++--
4 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/include/ebgpart.h b/include/ebgpart.h
index d02cfbe..85b3ad7 100644
--- a/include/ebgpart.h
+++ b/include/ebgpart.h
@@ -33,10 +33,6 @@
if (verbosity) fprintf(o, __VA_ARGS__)
#endif

-#ifndef __unused
-#define __unused __attribute__((unused))
-#endif
-
#include <unistd.h>
#include <errno.h>
#include <stdio.h>
diff --git a/include/utils.h b/include/utils.h
index ff8f9ef..b496031 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -20,16 +20,6 @@

#define MAX_INFO_SIZE 1024

-#if defined(__GNUC__)
-#define __noreturn __attribute__((noreturn))
-#define unreachable() __builtin_unreachable()
-#else
-#define __noreturn /**/
-#define unreachable() \
- do { \
- } while (1)
-#endif
-

typedef struct _VOLUME_DESC {
EFI_DEVICE_PATH *devpath;
CHAR16 *fslabel;

@@ -43,7 +33,7 @@ extern UINTN volume_count;

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 __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);
VOID *mmalloc(UINTN bytes);
EFI_STATUS mfree(VOID *p);
CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index fc3bf3b..7880101 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -553,7 +553,7 @@ PedDisk *ped_disk_new(const PedDevice *dev)
return &g_ped_dummy_disk;
}

-PedPartition *ped_disk_next_partition(const PedDisk *__unused pd,
+PedPartition *ped_disk_next_partition(const PedDisk *__attribute__((unused)) pd,
const PedPartition *part)
{
return part->next;
diff --git a/utils.c b/utils.c
index cb9569d..422356f 100644
--- a/utils.c
+++ b/utils.c
@@ -55,12 +55,12 @@ uint32_t calc_crc32(void *data, int32_t size)
return crc;
}

-void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
+void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
{

ERROR(L"%s ( %r )\n", message, status);
uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);
uefi_call_wrapper(BS->Exit, 4, this_image, status, 0, NULL);

- unreachable();
+ __builtin_unreachable();
}

VOID *mmalloc(UINTN bytes)
--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Make the #include'd headers explicit rather than implicit.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

include/env_config_file.h | 2 ++
include/env_config_partitions.h | 3 +++
include/env_disk_utils.h | 3 +++
include/envdata.h | 2 ++
4 files changed, 10 insertions(+)

diff --git a/include/env_config_file.h b/include/env_config_file.h
index 081c922..7c286da 100644
--- a/include/env_config_file.h
+++ b/include/env_config_file.h
@@ -13,6 +13,8 @@
*/

#pragma once
+#include <stdio.h>
+#include "env_api.h"

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);
FILE *open_config_file(char *configfilepath, char *mode);

diff --git a/include/env_config_partitions.h b/include/env_config_partitions.h
index c6cd033..ca7eeeb 100644
--- a/include/env_config_partitions.h
+++ b/include/env_config_partitions.h
@@ -14,5 +14,8 @@

#pragma once

+#include <stdbool.h>
+#include "env_api.h"
+
bool probe_config_partitions(CONFIG_PART *cfgpart);

diff --git a/include/env_disk_utils.h b/include/env_disk_utils.h
index eafdb59..d26f905 100644
--- a/include/env_disk_utils.h
+++ b/include/env_disk_utils.h
@@ -14,6 +14,9 @@

#pragma once

+#include <stdbool.h>
+#include "env_api.h"
+

char *get_mountpoint(char *devpath);
bool mount_partition(CONFIG_PART *cfgpart);
void unmount_partition(CONFIG_PART *cfgpart);

diff --git a/include/envdata.h b/include/envdata.h
index e1f6195..458e948 100644
--- a/include/envdata.h
+++ b/include/envdata.h
@@ -14,6 +14,8 @@

#pragma once

+#include <stdint.h>
+

#define FAT_ENV_FILENAME "BGENV.DAT"
#define ENV_STRING_LENGTH 255

--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

While the InitializeLib() call in efi_main(), the global
ST (SystemTable),
BS (BootServices), and
RT (RuntimeServices)
pointers as well as the global
EFI_HANDLE LibImageHandle
are initialized.
Hence, there's no need to pass along the this_image
handle as GNU EFI's already initialized LibImageHandle
can be used instead.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

bootguard.c | 2 --
include/bootguard.h | 2 --
main.c | 28 +++++++++++++++++++---------
utils.c | 8 +++++++-
4 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/bootguard.c b/bootguard.c
index 55811dd..4e83399 100644
--- a/bootguard.c
+++ b/bootguard.c
@@ -15,8 +15,6 @@
#include "bootguard.h"
#include "utils.h"

-EFI_HANDLE this_image;
-
VOLUME_DESC *volumes = NULL;
UINTN volume_count = 128;
CHAR16 *boot_medium_path;
diff --git a/include/bootguard.h b/include/bootguard.h
index 99d8fe4..2d484aa 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -33,8 +33,6 @@ typedef int BG_STATUS;

#define ENV_FILE_NAME L"BGENV.DAT"

-extern EFI_HANDLE this_image;

-
typedef struct _BG_LOADER_PARAMS {
CHAR16 *payload_path;
CHAR16 *payload_options;

diff --git a/main.c b/main.c
index 320e4dc..517ad13 100644
--- a/main.c
+++ b/main.c
@@ -27,6 +27,12 @@ extern const unsigned long init_array_start[];
extern const unsigned long init_array_end[];
extern CHAR16 *boot_medium_path;

+/*
+ * NOTE: LibImageHandle is a global initialized while efi_main()'s
+ * InitializeLib() call.
+ */
+extern EFI_HANDLE LibImageHandle;
+
static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
UINT16 pci_device_id, UINTN timeout)
@@ -72,7 +78,7 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)

status = uefi_call_wrapper(BS->OpenProtocol, 6, device,
&PciIoProtocol, (VOID **)&pci_io,
- this_image, NULL,
+ LibImageHandle, NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL);
if (EFI_ERROR(status)) {
error_exit(L"Could not open PciIoProtocol while "
@@ -93,13 +99,13 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
value >> 16, timeout);

uefi_call_wrapper(BS->CloseProtocol, 4, device, &PciIoProtocol,
- this_image, NULL);
+ LibImageHandle, NULL);
} while (status != EFI_SUCCESS && count > 0);

return status;
}

-EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
+EFI_STATUS efi_main(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
{
EFI_DEVICE_PATH *payload_dev_path;
EFI_LOADED_IMAGE *loaded_image;
@@ -111,15 +117,19 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

ZeroMem(&bg_loader_params, sizeof(bg_loader_params));

- this_image = image_handle;
- InitializeLib(this_image, system_table);
+ /*
+ * NOTE: InitializeLib() initializes the global ST (SystemTable),
+ * BS (BootServices), and RT (RuntimeServices) pointers as
+ * well as LibImageHandle from efi_main()'s parameters.
+ */
+ InitializeLib(ImageHandle, SystemTable);

PrintC(EFI_CYAN, L"\nEFI Boot Guard %s\n", L"" EFIBOOTGUARD_VERSION);

status =

- uefi_call_wrapper(BS->OpenProtocol, 6, this_image,
+ uefi_call_wrapper(BS->OpenProtocol, 6, LibImageHandle,
&LoadedImageProtocol, (VOID **)&loaded_image,
- this_image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ LibImageHandle, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
if (EFI_ERROR(status)) {
error_exit(L"Could not open LoadedImageProtocol to get image "
L"information.",
@@ -177,7 +187,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
}

/* Load and start image */
- status = uefi_call_wrapper(BS->LoadImage, 6, TRUE, this_image,
+ status = uefi_call_wrapper(BS->LoadImage, 6, TRUE, LibImageHandle,
payload_dev_path, NULL, 0, &payload_handle);
if (EFI_ERROR(status)) {
error_exit(L"Could not load specified kernel image.", status);
@@ -189,7 +199,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
status =
uefi_call_wrapper(BS->OpenProtocol, 6, payload_handle,
&LoadedImageProtocol, (VOID **)&loaded_image,
- this_image, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ LibImageHandle, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
if (EFI_ERROR(status)) {
error_exit(L"Could not open LoadedImageProtocol to set kernel "
L"load options.",
diff --git a/utils.c b/utils.c
index 422356f..64918ae 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,12 @@
#include <bootguard.h>
#include <utils.h>

+/*
+ * NOTE: LibImageHandle is a global initialized while efi_main()'s
+ * InitializeLib() call.
+ */
+extern EFI_HANDLE LibImageHandle;
+

VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...)

{

INT32 attr = ST->ConOut->Mode->Attribute;

@@ -59,7 +65,7 @@ void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
{

ERROR(L"%s ( %r )\n", message, status);
uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);

- uefi_call_wrapper(BS->Exit, 4, this_image, status, 0, NULL);
+ uefi_call_wrapper(BS->Exit, 4, LibImageHandle, status, 0, NULL);
__builtin_unreachable();
}

--
2.30.0

[Jan Kiszka]

On 08.01.21 15:43, [ext] Christian Storm wrote:
> From: Christian Storm <christi...@siemens.com>
>
> Any modern compiler (e.g., gcc > 3.4, released 2004)
> supports #pragma once -- and it doesn't pollute the
> namespace with a preprocessor symbol.
> So, get rid of overly verbose include guards.

Interesting. I wonder why the kernel isn't using it widely yet. I only
find it in one of its tools. Maybe the reason is the limitations of
#pragma once when it comes to identifying the same file included from
different locations (via links etc.).

I do not really expect any such complex cases with our simple API
(ebgenv.h), and internal stuff is under our control, so it should be
fine in this specific case.

Jan

Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Jan Kiszka]

On 08.01.21 15:43, [ext] Christian Storm wrote:

If you pull patch 4 to the front, you can avoid this code shuffling.

Jan

[Jan Kiszka]

On 08.01.21 15:43, [ext] Christian Storm wrote:

> From: Christian Storm <christi...@siemens.com>
>
> __builtin_unreachable() is available since gcc 4.5, released 2010.
> __attribute__((noreturn)) is avilable since gcc 2.5.0, released 1993.
> __attribute__((unused)) is available since gcc 2.7, released 1995.

It's probably also worth noting that removing the "#if
defined(__GNUC__)" special case fine because we depend on gnuefi anyway
- right?

Jan

[Jan Kiszka]

On 08.01.21 15:43, [ext] Christian Storm wrote:

> From: Christian Storm <christi...@siemens.com>
>
> Make the #include'd headers explicit rather than implicit.
>

How did you check that an include is needed (and not others)? Manually?
Or via something analogous to
https://github.com/siemens/jailhouse/blob/master/scripts/header_check?

Jan

[Jan Kiszka]

On 08.01.21 15:43, [ext] Christian Storm wrote:

> From: Christian Storm <christi...@siemens.com>
>
> While the InitializeLib() call in efi_main(), the global
> ST (SystemTable),
> BS (BootServices), and
> RT (RuntimeServices)
> pointers as well as the global
> EFI_HANDLE LibImageHandle
> are initialized.
> Hence, there's no need to pass along the this_image
> handle as GNU EFI's already initialized LibImageHandle
> can be used instead.

Could you provide a pointer to something that states that LibImageHandle
is a public API? I'm specifically concerned because you have to declare
it explicitly.

With that pointer dropped here, you can also remove any explanations
from the code below.

Jan

[[ext] Christian Storm]

Hi Jan,

> > Any modern compiler (e.g., gcc > 3.4, released 2004)
> > supports #pragma once -- and it doesn't pollute the
> > namespace with a preprocessor symbol.
> > So, get rid of overly verbose include guards.
>
> Interesting. I wonder why the kernel isn't using it widely yet. I only
> find it in one of its tools. Maybe the reason is the limitations of
> #pragma once when it comes to identifying the same file included from
> different locations (via links etc.).

Yes, exactly, see, e.g., this thread (there are more though):
http://lkml.iu.edu/hypermail/linux/kernel/1401.0/02689.html
The kernel did not go this route so to have very explicit control
over the #include's, something we do not have here.
They could've used it in some places but then the idea was dropped for
consistency reasons (and the work involved) I guess.

> I do not really expect any such complex cases with our simple API
> (ebgenv.h), and internal stuff is under our control, so it should be
> fine in this specific case.

Yes, this was my impression as well.


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[[ext] Christian Storm]

Hi Jan,

> > __builtin_unreachable() is available since gcc 4.5, released 2010.
> > __attribute__((noreturn)) is avilable since gcc 2.5.0, released 1993.
> > __attribute__((unused)) is available since gcc 2.7, released 1995.
>
> It's probably also worth noting that removing the "#if
> defined(__GNUC__)" special case fine because we depend on gnuefi anyway
> - right?

Well, GNU EFI does actually support a lot of compilers. The interesting
part is in GNU EFI's inc/x86_64/efibind.h:26 (and in the Makefile.defaults
as well), e.g.,

#if defined(GNU_EFI_USE_MS_ABI)
#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7)))||(defined(__clang__) && (__clang_major__ > 3 || (__clang_major__ == 3 && __clang_minor__ >= 2)))
#define HAVE_USE_MS_ABI 1
#else
#error Compiler is too old for GNU_EFI_USE_MS_ABI
#endif
#endif

since you do want to have MS_ABI support. So, reasonable compilers do
support the attributes such that the #else case can simply be dropped
and by this the whole construct.


Christian

Besten Gruß,

[[ext] Christian Storm]

Hi Jan,

> > While the InitializeLib() call in efi_main(), the global
> > ST (SystemTable),
> > BS (BootServices), and
> > RT (RuntimeServices)
> > pointers as well as the global
> > EFI_HANDLE LibImageHandle
> > are initialized.
> > Hence, there's no need to pass along the this_image
> > handle as GNU EFI's already initialized LibImageHandle
> > can be used instead.
>
> Could you provide a pointer to something that states that LibImageHandle
> is a public API? I'm specifically concerned because you have to declare
> it explicitly.

Yes, I see. It's defined in GNU EFI's lib/lib.h:87 (EFI library header files)
under the section
//
// Globals
//
as
extern EFI_HANDLE LibImageHandle;
so I considered it a public API.
See also lib/data.c:30 (EFI library global data).

I did not want to (explicitly) pull in the rest of lib/lib.h, that's why
I declared it explicitly here...

> With that pointer dropped here, you can also remove any explanations
> from the code below.

True, was just for explicitness. I could drop it.


Christian

[[ext] Christian Storm]

Hi Jan,

> > Make the #include'd headers explicit rather than implicit.
> >
>
> How did you check that an include is needed (and not others)? Manually?
> Or via something analogous to
> https://github.com/siemens/jailhouse/blob/master/scripts/header_check?

Well, manually while staring at the code :)
It's kind of suspicious for, e.g., env_config_file.h having *no*
#include's and then using CONFIG_PART and char so it's quite apparent
that this is provided implicitly. Then I went finding the appropriate
include files... Same procedure applied to the others.

Christian

[[ext] Christian Storm]

Hi Jan,

> > The headers bootguard.h and utils.h mutually #include each other
> > for no apparent reason. Hence, disentangle their #include relation
> > to allow selective inclusion.
> >
> > One problem of this is, e.g., utils.h defines _VOLUME_DESC
> > after #include "bootguard.h". However, bootguard.h uses
> > this in extern VOLUME_DESC *volumes.
> >
> > Signed-off-by: Christian Storm <christi...@siemens.com>
> > ---
> >

> > [...]

> >
> > diff --git a/include/bootguard.h b/include/bootguard.h
> > index 159a396..99d8fe4 100644
> > --- a/include/bootguard.h
> > +++ b/include/bootguard.h
> > @@ -14,20 +14,7 @@
> >
> > #pragma once
> >
> > -#if defined(__GNUC__)
> > -#define __noreturn __attribute__((noreturn))
> > -#define unreachable() __builtin_unreachable()
> > -#else
> > -#define __noreturn /**/
> > -#define unreachable() \
> > - do { \
> > - } while (1)
> > -#endif
>
> If you pull patch 4 to the front, you can avoid this code shuffling.

Yes, true. Haven't paid attention to yield a minimal overall patch...


Kind regards,
Christian

[Jan Kiszka]

On 08.01.21 21:20, [ext] [ext] Christian Storm wrote:
> Hi Jan,
>
>>> __builtin_unreachable() is available since gcc 4.5, released 2010.
>>> __attribute__((noreturn)) is avilable since gcc 2.5.0, released 1993.
>>> __attribute__((unused)) is available since gcc 2.7, released 1995.
>>
>> It's probably also worth noting that removing the "#if
>> defined(__GNUC__)" special case fine because we depend on gnuefi anyway
>> - right?
>
> Well, GNU EFI does actually support a lot of compilers. The interesting
> part is in GNU EFI's inc/x86_64/efibind.h:26 (and in the Makefile.defaults
> as well), e.g.,
>
> #if defined(GNU_EFI_USE_MS_ABI)
> #if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 7)))||(defined(__clang__) && (__clang_major__ > 3 || (__clang_major__ == 3 && __clang_minor__ >= 2)))
> #define HAVE_USE_MS_ABI 1
> #else
> #error Compiler is too old for GNU_EFI_USE_MS_ABI
> #endif
> #endif
>
> since you do want to have MS_ABI support. So, reasonable compilers do
> support the attributes such that the #else case can simply be dropped
> and by this the whole construct.
>

In any case, please leave a reasoning for the cut-off case in the commit
log.

Thanks,
Jan

[Jan Kiszka]

lib/lib.h is no public API. It's an internal header, and the above is
for gnuefi-internal global use. So this is a no-go.

Jan

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Introduce ERROR(), WARNING(), and INFO() console message
output macros to classify the messages and make their output
more colorful so to increase readability for EBG's core output.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

env/fatvars.c | 44 ++++++++++++++++++++++----------------------
env/syspart.c | 14 +++++++-------
include/utils.h | 19 +++++++++++++++++++
main.c | 14 ++++++--------
utils.c | 49 ++++++++++++++++++++++++++++---------------------
5 files changed, 82 insertions(+), 58 deletions(-)

diff --git a/env/fatvars.c b/env/fatvars.c
index 99ac37e..e86743e 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c

diff --git a/include/utils.h b/include/utils.h
index 8ce5ac6..8978ec9 100644
--- a/include/utils.h
+++ b/include/utils.h

@@ -42,4 +42,23 @@ CHAR16 *GetBootMediumPath(CHAR16 *input);
BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp);
VOID Color(EFI_SYSTEM_TABLE *system_table, char fgcolor, char bgcolor);

+VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...);
+#define ERROR(fmt, ...) \
+ do { \
+ PrintC(EFI_LIGHTRED, L"ERROR: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define WARNING(fmt, ...) \
+ do { \
+ PrintC(EFI_YELLOW, L"WARNING: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define INFO(fmt, ...) \
+ do { \
+ PrintC(EFI_WHITE, L"> "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
#endif // __H_UTILS__

diff --git a/main.c b/main.c
index 9fe8bbe..320e4dc 100644
--- a/main.c
+++ b/main.c

@@ -114,9 +114,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
this_image = image_handle;
InitializeLib(this_image, system_table);

- Color(system_table, 3, 0);
- Print(L"\nEFI Boot Guard %s\n", L""EFIBOOTGUARD_VERSION);
- Color(system_table, 7, 0);

+ PrintC(EFI_CYAN, L"\nEFI Boot Guard %s\n", L"" EFIBOOTGUARD_VERSION);

status =

diff --git a/utils.c b/utils.c
index cda0e92..cb9569d 100644
--- a/utils.c
+++ b/utils.c
@@ -16,6 +16,19 @@
#include <bootguard.h>
#include <utils.h>

+VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...)
+{
+ INT32 attr = ST->ConOut->Mode->Attribute;
+ (VOID)uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, color);
+
+ va_list args;
+ va_start(args, fmt);
+ (VOID)VPrint(fmt, args);
+ va_end(args);
+
+ (VOID)uefi_call_wrapper(ST->ConOut->SetAttribute, 2, ST->ConOut, attr);
+}
+
BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp)
{
extern CHAR16 *boot_medium_path;

@@ -44,7 +57,7 @@ uint32_t calc_crc32(void *data, int32_t size)

void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)

{
- Print(L"%s ( %r )\n", message, status);
+ ERROR(L"%s ( %r )\n", message, status);

uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);

uefi_call_wrapper(BS->Exit, 4, this_image, status, 0, NULL);

[Christian Storm]

[Christian Storm]

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Make the #include'd headers explicit rather than implicit.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Any reasonable modern compiler (e.g., gcc > 3.4, released

2004) supports #pragma once -- and it doesn't pollute the
namespace with a preprocessor symbol.

So, get rid of overly verbose include guards as we don't
have the need for very explicit #include control like,
e.g., the Linux Kernel has.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

include/bootguard.h | 4 +---
include/configuration.h | 4 +---
include/ebgenv.h | 4 +---
include/ebgpart.h | 4 +---
include/env_api.h | 4 +---
include/env_config_file.h | 4 +---
include/env_config_partitions.h | 4 +---
include/env_disk_utils.h | 4 +---
include/envdata.h | 4 +---
include/syspart.h | 4 +---
include/test-interface.h | 5 +----
include/uservars.h | 5 +----
include/utils.h | 4 +---
tools/tests/fake_devices.h | 4 +---
14 files changed, 14 insertions(+), 44 deletions(-)

diff --git a/include/bootguard.h b/include/bootguard.h
index bf244a4..159a396 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h

diff --git a/include/ebgpart.h b/include/ebgpart.h
index a2b7552..d02cfbe 100644
--- a/include/ebgpart.h
+++ b/include/ebgpart.h

diff --git a/include/env_config_file.h b/include/env_config_file.h
index 2679174..081c922 100644
--- a/include/env_config_file.h
+++ b/include/env_config_file.h

@@ -12,12 +12,10 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_CONFIG_FILE_H__
-#define __ENV_CONFIG_FILE_H__
+#pragma once

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);
FILE *open_config_file(char *configfilepath, char *mode);

int close_config_file(FILE *config_file_handle);
bool probe_config_file(CONFIG_PART *cfgpart);

-#endif // __ENV_CONFIG_FILE_H__

diff --git a/include/env_config_partitions.h b/include/env_config_partitions.h
index ebe6712..c6cd033 100644
--- a/include/env_config_partitions.h
+++ b/include/env_config_partitions.h

@@ -12,9 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_CONFIG_PARTITIONS_H__
-#define __ENV_CONFIG_PARTITIONS_H__
+#pragma once

bool probe_config_partitions(CONFIG_PART *cfgpart);

-#endif // __ENV_CONFIG_PARTITIONS_H__

diff --git a/include/env_disk_utils.h b/include/env_disk_utils.h
index 7450f20..eafdb59 100644
--- a/include/env_disk_utils.h
+++ b/include/env_disk_utils.h

@@ -12,11 +12,9 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __ENV_DISK_UTILS_H__
-#define __ENV_DISK_UTILS_H__
+#pragma once

char *get_mountpoint(char *devpath);
bool mount_partition(CONFIG_PART *cfgpart);
void unmount_partition(CONFIG_PART *cfgpart);

-#endif // __ENV_DISK_UTILS_H__
diff --git a/include/envdata.h b/include/envdata.h
index 13cdb8d..e1f6195 100644
--- a/include/envdata.h
+++ b/include/envdata.h

@@ -12,8 +12,7 @@
* SPDX-License-Identifier: GPL-2.0
*/

-#ifndef __H_ENV_DATA__
-#define __H_ENV_DATA__
+#pragma once

#define FAT_ENV_FILENAME "BGENV.DAT"
#define ENV_STRING_LENGTH 255

diff --git a/include/utils.h b/include/utils.h
index 8978ec9..2f4fa87 100644
--- a/include/utils.h
+++ b/include/utils.h

2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

__builtin_unreachable() is available since gcc 4.5, released 2010.

__attribute__((noreturn)) is available since gcc 2.5.0, released 1993.

__attribute__((unused)) is available since gcc 2.7, released 1995.

Any reasonable modern compiler supports these directly, so the wrapping
constructs can be dropped entirely.

Using a sufficiently modern compiler is encouraged nonetheless to benefit
from MS_ABI support giving type checking for EFI calls.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

include/bootguard.h | 10 ----------
include/ebgpart.h | 4 ----
include/utils.h | 2 +-

tools/ebgpart.c | 2 +-
utils.c | 4 ++--

5 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/include/bootguard.h b/include/bootguard.h
index 159a396..5c99264 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -14,16 +14,6 @@

#pragma once

-#if defined(__GNUC__)
-#define __noreturn __attribute__((noreturn))
-#define unreachable() __builtin_unreachable()
-#else
-#define __noreturn /**/
-#define unreachable() \
- do { \
- } while (1)
-#endif

-
#include <efi.h>
#include <efilib.h>
#include <efiprot.h>
diff --git a/include/ebgpart.h b/include/ebgpart.h
index d02cfbe..85b3ad7 100644
--- a/include/ebgpart.h
+++ b/include/ebgpart.h

@@ -33,10 +33,6 @@
if (verbosity) fprintf(o, __VA_ARGS__)
#endif

-#ifndef __unused
-#define __unused __attribute__((unused))
-#endif
-
#include <unistd.h>
#include <errno.h>
#include <stdio.h>

diff --git a/include/utils.h b/include/utils.h
index 2f4fa87..a6fbab7 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -29,7 +29,7 @@ typedef struct _VOLUME_DESC {

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 __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status);
VOID *mmalloc(UINTN bytes);
EFI_STATUS mfree(VOID *p);
CHAR16 *get_volume_label(EFI_FILE_HANDLE fh);
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index fc3bf3b..7880101 100644
--- a/tools/ebgpart.c
+++ b/tools/ebgpart.c
@@ -553,7 +553,7 @@ PedDisk *ped_disk_new(const PedDevice *dev)
return &g_ped_dummy_disk;
}

-PedPartition *ped_disk_next_partition(const PedDisk *__unused pd,
+PedPartition *ped_disk_next_partition(const PedDisk *__attribute__((unused)) pd,
const PedPartition *part)
{
return part->next;

diff --git a/utils.c b/utils.c
index cb9569d..422356f 100644
--- a/utils.c
+++ b/utils.c
@@ -55,12 +55,12 @@ uint32_t calc_crc32(void *data, int32_t size)
return crc;
}

-void __noreturn error_exit(CHAR16 *message, EFI_STATUS status)
+void __attribute__((noreturn)) error_exit(CHAR16 *message, EFI_STATUS status)
{

ERROR(L"%s ( %r )\n", message, status);
uefi_call_wrapper(BS->Stall, 1, 3 * 1000 * 1000);
uefi_call_wrapper(BS->Exit, 4, this_image, status, 0, NULL);

- unreachable();
+ __builtin_unreachable();
}

VOID *mmalloc(UINTN bytes)
--

2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

The headers bootguard.h and utils.h mutually #include each other
for no apparent reason. Hence, disentangle their #include relation
to allow selective inclusion.

One problem of this is, e.g., utils.h defines _VOLUME_DESC
after #include "bootguard.h". However, bootguard.h uses
this in extern VOLUME_DESC *volumes.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

bootguard.c | 1 +
env/fatvars.c | 1 +

include/bootguard.h | 6 ------
include/utils.h | 6 +++++-
4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/bootguard.c b/bootguard.c
index 22a2a8a..55811dd 100644
--- a/bootguard.c
+++ b/bootguard.c

@@ -13,6 +13,7 @@
*/

#include "bootguard.h"
+#include "utils.h"

EFI_HANDLE this_image;

diff --git a/env/fatvars.c b/env/fatvars.c
index e86743e..5c63212 100644
--- a/env/fatvars.c
+++ b/env/fatvars.c

@@ -13,6 +13,7 @@
*/

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

diff --git a/include/bootguard.h b/include/bootguard.h
index 5c99264..99d8fe4 100644
--- a/include/bootguard.h
+++ b/include/bootguard.h
@@ -15,9 +15,6 @@
#pragma once

#include <efi.h>
-#include <efilib.h>
-#include <efiprot.h>
-#include <utils.h>

/* The following definitions regarding status and error constants are
* implemented the same way the corresponding gnu-efi constants are

@@ -38,9 +35,6 @@ typedef int BG_STATUS;

extern EFI_HANDLE this_image;

-extern VOLUME_DESC *volumes;
-extern UINTN volume_count;

-
typedef struct _BG_LOADER_PARAMS {
CHAR16 *payload_path;
CHAR16 *payload_options;

diff --git a/include/utils.h b/include/utils.h
index a6fbab7..b496031 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -15,7 +15,8 @@

#pragma once

-#include "bootguard.h"
+#include <efi.h>
+#include <efilib.h>

#define MAX_INFO_SIZE 1024

@@ -26,6 +27,9 @@ typedef struct _VOLUME_DESC {

EFI_FILE_HANDLE root;
} VOLUME_DESC;

+extern VOLUME_DESC *volumes;
+extern UINTN volume_count;
+

typedef enum { DOSFSLABEL, CUSTOMLABEL, NOLABEL } LABELMODE;

uint32_t calc_crc32(void *data, int32_t size);

--
2.30.0

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Introduce ERROR(), WARNING(), and INFO() console message
output macros to classify the messages and make their output
more colorful so to increase readability for EBG's core output.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
env/fatvars.c | 44 ++++++++++++++++++++++----------------------
env/syspart.c | 14 +++++++-------

include/utils.h | 20 +++++++++++++++++++-

main.c | 14 ++++++--------
utils.c | 49 ++++++++++++++++++++++++++++---------------------

5 files changed, 82 insertions(+), 59 deletions(-)

index 8ce5ac6..72a7cbb 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -40,6 +40,24 @@ EFI_DEVICE_PATH *FileDevicePathFromConfig(EFI_HANDLE device,
CHAR16 *payloadpath);

CHAR16 *GetBootMediumPath(CHAR16 *input);
BOOLEAN IsOnBootMedium(EFI_DEVICE_PATH *dp);

-VOID Color(EFI_SYSTEM_TABLE *system_table, char fgcolor, char bgcolor);
+

+VOID PrintC(const UINT8 color, const CHAR16 *fmt, ...);
+#define ERROR(fmt, ...) \
+ do { \
+ PrintC(EFI_LIGHTRED, L"ERROR: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define WARNING(fmt, ...) \
+ do { \
+ PrintC(EFI_YELLOW, L"WARNING: "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)
+
+#define INFO(fmt, ...) \
+ do { \
+ PrintC(EFI_WHITE, L"> "); \
+ PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
+ } while (0)

[Jan Kiszka]

On 11.01.21 09:02, [ext] Christian Storm wrote:
> From: Christian Storm <christi...@siemens.com>
>
> Introduce ERROR(), WARNING(), and INFO() console message
> output macros to classify the messages and make their output
> more colorful so to increase readability for EBG's core output.
>
> Signed-off-by: Christian Storm <christi...@siemens.com>
> ---

Patch revision changelog here would be appreciated. I assume you only
removed also the prototype of Color().

Jan

[Jan Kiszka]

On 11.01.21 09:07, [ext] Jan Kiszka wrote:
> On 11.01.21 09:02, [ext] Christian Storm wrote:
>> From: Christian Storm <christi...@siemens.com>
>>
>> Introduce ERROR(), WARNING(), and INFO() console message
>> output macros to classify the messages and make their output
>> more colorful so to increase readability for EBG's core output.
>>
>> Signed-off-by: Christian Storm <christi...@siemens.com>
>> ---
>
> Patch revision changelog here would be appreciated. I assume you only
> removed also the prototype of Color().
>

Rebased and massaged a bit (rewrapped messages).

Also applied the other 4 patches of this series.

Thanks,
Jan

[[ext] Christian Storm]

Hi Jan,

> On 11.01.21 09:02, [ext] Christian Storm wrote:
> > From: Christian Storm <christi...@siemens.com>
> >
> > Introduce ERROR(), WARNING(), and INFO() console message
> > output macros to classify the messages and make their output
> > more colorful so to increase readability for EBG's core output.
> >
> > Signed-off-by: Christian Storm <christi...@siemens.com>
> > ---
>
> Patch revision changelog here would be appreciated. I assume you only
> removed also the prototype of Color().

Yes, you're right. I'll include it next time!

Thanks!

[Jan Kiszka]

On 11.01.21 09:02, [ext] Christian Storm wrote:

> From: Christian Storm <christi...@siemens.com>
>
> Introduce ERROR(), WARNING(), and INFO() console message
> output macros to classify the messages and make their output
> more colorful so to increase readability for EBG's core output.
>
> Signed-off-by: Christian Storm <christi...@siemens.com>
> ---
> env/fatvars.c | 44 ++++++++++++++++++++++----------------------
> env/syspart.c | 14 +++++++-------
> include/utils.h | 20 +++++++++++++++++++-
> main.c | 14 ++++++--------
> utils.c | 49 ++++++++++++++++++++++++++++---------------------
> 5 files changed, 82 insertions(+), 59 deletions(-)
>

...

Only realized why trying out: What's the reason for the "> " prefix?

Jan

> + PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__); \
> + } while (0)
>
> #endif // __H_UTILS__

[Jan Kiszka]

On 11.01.21 09:02, [ext] Christian Storm wrote:

> From: Christian Storm <christi...@siemens.com>
>
> Introduce ERROR(), WARNING(), and INFO() console message
> output macros to classify the messages and make their output
> more colorful so to increase readability for EBG's core output.
>
> Signed-off-by: Christian Storm <christi...@siemens.com>
> ---
> env/fatvars.c | 44 ++++++++++++++++++++++----------------------
> env/syspart.c | 14 +++++++-------
> include/utils.h | 20 +++++++++++++++++++-
> main.c | 14 ++++++--------
> utils.c | 49 ++++++++++++++++++++++++++++---------------------
> 5 files changed, 82 insertions(+), 59 deletions(-)
>

...

Lost a newline here. Fixing up while dropping that INFO tag.

Jan

[Storm, Christian]

Hi Jan,

> > Introduce ERROR(), WARNING(), and INFO() console message output
> macros
> > to classify the messages and make their output more colorful so to
> > increase readability for EBG's core output.
> >
> > Signed-off-by: Christian Storm <christi...@siemens.com>

> > [...]

> > +#define INFO(fmt, ...) \
> > + do { \
> > + PrintC(EFI_WHITE, L"> "); \
>
> Only realized why trying out: What's the reason for the "> " prefix?

The reason is to differentiate EBG's core output from other output, e.g., by the
watchdog driver probings. I thought about an "INFO" or no prefix but found this
to be more outstanding while not overly intrusive nor too long.

One can think about such a console printing infrastructure for the watchdog drivers
as well (at the cost of introducing a dependency), then also including, e.g., DEBUG
output to improve on, e.g., AMDFCH_WDT_DEBUG.

That said, I don't have a strong opinion on the concrete measure though..

[Christian Storm]

Hi Jan,

> Lost a newline here. Fixing up while dropping that INFO tag.

Hm, why did you remove the
do { ... } while (0)
enclosing in INFO() while your modifications of the patch?

It now reads (see [1])
#define INFO(fmt, ...) \
PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__);
which means it effectively expands to double ;; and it breaks,
e.g., for some if-else constructs such as
if (condition)
INFO(L"foo");
else
ERROR(L"bar");
as the double ;; "eats" the else part so the compiler is unhappy.

Is there a reason for this I'm missing?


Kind regards,
Christian

[1] https://github.com/siemens/efibootguard/commit/c828b814fafbba03eb7fbb808030e72e58c2532c#diff-95e9251d9e82720a3917beffdcad74f93a0f67b12b4388c9de6205ce71874024R57

[Jan Kiszka]

On 13.01.21 13:40, [ext] Christian Storm wrote:
> Hi Jan,
>
>> Lost a newline here. Fixing up while dropping that INFO tag.
>
> Hm, why did you remove the
> do { ... } while (0)
> enclosing in INFO() while your modifications of the patch?
>
> It now reads (see [1])
> #define INFO(fmt, ...) \
> PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__);
> which means it effectively expands to double ;; and it breaks,
> e.g., for some if-else constructs such as
> if (condition)
> INFO(L"foo");
> else
> ERROR(L"bar");
> as the double ;; "eats" the else part so the compiler is unhappy.
>
> Is there a reason for this I'm missing?
>

Nope, I just missed the remove the additional ";" as well. Fixed.

[Christian Storm]

Hi Jan,

> >> Lost a newline here. Fixing up while dropping that INFO tag.
> >
> > Hm, why did you remove the
> > do { ... } while (0)
> > enclosing in INFO() while your modifications of the patch?
> >
> > It now reads (see [1])
> > #define INFO(fmt, ...) \
> > PrintC(EFI_LIGHTGRAY, fmt, ##__VA_ARGS__);
> > which means it effectively expands to double ;; and it breaks,
> > e.g., for some if-else constructs such as
> > if (condition)
> > INFO(L"foo");
> > else
> > ERROR(L"bar");
> > as the double ;; "eats" the else part so the compiler is unhappy.
> >
> > Is there a reason for this I'm missing?
> >
>
> Nope, I just missed the remove the additional ";" as well. Fixed.

OK, github has catched up now. I'm fine with your modifications.

Nit picking: There's a typo in your modification explanation:
"[Jan: rebase & rewrap, using long lines to make error stings
searchable, dropped prefix of INFO output]"
stings → strings.



Kind regards,
Christian