[PATCH 0/4] Fix clang warnings

[Michael Adler]

This patch series contains fixes for compiler warnings triggered by clang.
Despite each warning being valid, they were essentially harmless and did not
lead to any actual problems. Nevertheless it makes sense to silence these
warnings by fixing the code.

Michael Adler (4):
fix: correctly parse ustate in journal_process_action
fix: complete initialization of struct argp_option
chore: fix compiler warning about unused parameters
chore: use function declaration from header

env/env_api.c | 4 ++--
env/env_api_fat.c | 4 ++--
tools/bg_envtools.h | 9 ++++++---
tools/bg_printenv.c | 2 +-
tools/bg_setenv.c | 17 +++++------------
tools/ebgpart.c | 2 +-
6 files changed, 17 insertions(+), 21 deletions(-)

--
2.41.0

[Michael Adler]

This commit addresses an issue where the strtol function, which returns
a 'long', was assigned to an 'unsigned long' variable. As a result, the
error handling routine check 't == LONG_MIN' was ineffectual, leading to
an inability to correctly check for underflow.
Now the code uses the existing parsing logic 'parse_int' and verifies
that the parsed 'ustate' value falls within the correct range.

Note: the mismanagement of parse errors was not causing any issues
because 'ebg_env_setglobalstate' validates its input arguments
separately.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
tools/bg_setenv.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index d685412..ab2e2ad 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -115,7 +115,6 @@ newaction_nomem:
static void journal_process_action(BGENV *env, struct env_action *action)
{
ebgenv_t e;
- char *tmp;

switch (action->task) {
case ENV_TASK_SET:
@@ -123,22 +122,16 @@ static void journal_process_action(BGENV *env, struct env_action *action)
action->key, (long long unsigned int)action->type,
(char *)action->data);
if (strcmp(action->key, "ustate") == 0) {
- uint16_t ustate;
- unsigned long t;
+ int ustate;
char *arg;
int ret;
e.bgenv = env;
arg = (char *)action->data;
- errno = 0;
- t = strtol(arg, &tmp, 10);
- if ((errno == ERANGE && (t == LONG_MAX ||
- t == LONG_MIN)) ||
- (errno != 0 && t == 0) || tmp == arg) {
- fprintf(stderr, "Invalid value for ustate: %s",
- (char *)action->data);
+ ustate = parse_int(arg);
+ if (ustate < 0 || ustate > UINT16_MAX) {
+ fprintf(stderr, "Invalid ustate value: %s", arg);
return;
}
- ustate = (uint16_t)t;;
if ((ret = ebg_env_setglobalstate(&e, ustate)) != 0) {
fprintf(stderr,
"Error setting global state: %s.",
--
2.41.0

[Michael Adler]

Modifies the syntax from `*__attribute__((unused))` to the correct
format `__attribute__((unused))*` to resolve a warning issued by clang
regarding unused arguments.
Additionally, adds the 'unused' attribute to other public functions to
suppress further compiler warnings.

Signed-off-by: Michael Adler <michae...@siemens.com>
---

env/env_api.c | 4 ++--
tools/ebgpart.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/env/env_api.c b/env/env_api.c
index 173b764..2b5d061 100644
--- a/env/env_api.c
+++ b/env/env_api.c
@@ -56,7 +56,7 @@ char16_t *str8to16(char16_t *buffer, const char *src)
return tmp;
}

-void ebg_beverbose(ebgenv_t *e, bool v)
+void ebg_beverbose(ebgenv_t __attribute__((unused)) *e, bool v)
{
bgenv_be_verbose(v);
}
@@ -141,7 +141,7 @@ uint32_t ebg_env_user_free(ebgenv_t *e)
return bgenv_user_free(((BGENV *)e->bgenv)->data->userdata);
}

-uint16_t ebg_env_getglobalstate(ebgenv_t *e)
+uint16_t ebg_env_getglobalstate(ebgenv_t __attribute__((unused)) *e)
{
BGENV *env;
int res = USTATE_UNKNOWN;
diff --git a/tools/ebgpart.c b/tools/ebgpart.c
index 54cb66a..e058793 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 *__attribute__((unused)) pd,
+PedPartition *ped_disk_next_partition(const PedDisk __attribute__((unused)) *pd,
const PedPartition *part)
{
return part->next;
--
2.41.0

[Michael Adler]

Corrects the partial initialization of the struct argp_option, which
previously had only 5 out of its 6 members initialized. This commit
ensures full initialization of all members by leveraging C99's
designated initializers.

Signed-off-by: Michael Adler <michae...@siemens.com>
---

tools/bg_envtools.h | 9 ++++++---
tools/bg_printenv.c | 2 +-

tools/bg_setenv.c | 2 +-
3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/bg_envtools.h b/tools/bg_envtools.h
index 895b53f..8081d86 100644
--- a/tools/bg_envtools.h
+++ b/tools/bg_envtools.h
@@ -18,9 +18,12 @@

#include "env_api.h"

-#define OPT(name, key, arg, flags, doc) \
- { \
- name, key, arg, flags, doc \
+#define OPT(_name, _key, _arg, _flags, _doc) \
+ { .name = (_name) \
+ , .key = (_key) \
+ , .arg = (_arg) \
+ , .flags = (_flags) \
+ , .doc = (_doc) \
}

/* if you change these, do not forget to update completion/common.py */
diff --git a/tools/bg_printenv.c b/tools/bg_printenv.c
index 69d428b..9c52505 100644
--- a/tools/bg_printenv.c
+++ b/tools/bg_printenv.c
@@ -32,7 +32,7 @@ static struct argp_option options_printenv[] = {
"watchdog_timeout, ustate, user. "
"If omitted, all available fields are printed."),
OPT("raw", 'r', 0, 0, "Raw output mode, e.g. for shell scripting"),
- {},
+ {0},
};

/* Arguments used by bg_printenv. */
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index ab2e2ad..f318f12 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -42,7 +42,7 @@ static struct argp_option options_setenv[] = {
"use this option multiple times."),
OPT("in_progress", 'i', "IN_PROGRESS", 0,
"Set in_progress variable to simulate a running update process."),
- {},
+ {0},
};

/* Arguments used by bg_setenv. */
--
2.41.0

[Michael Adler]

This commit rectifies the issue of using 'f()' instead of 'f(void)',
which are not equivalent.

Affected functions: bgenv_open_oldest, bgenv_open_latest

Resolves the compiler warning: a function declaration without a
prototype is deprecated in all versions of C
[-Werror,-Wstrict-prototypes]

Signed-off-by: Michael Adler <michae...@siemens.com>
---

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

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index 503dedf..0f4f474 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -191,7 +191,7 @@ BGENV *bgenv_open_by_index(uint32_t index)
return handle;
}

-BGENV *bgenv_open_oldest()
+BGENV *bgenv_open_oldest(void)
{
uint32_t minrev = 0xFFFFFFFF;
uint32_t min_idx = 0;
@@ -205,7 +205,7 @@ BGENV *bgenv_open_oldest()
return bgenv_open_by_index(min_idx);
}

-BGENV *bgenv_open_latest()
+BGENV *bgenv_open_latest(void)
{
uint32_t maxrev = 0;
uint32_t max_idx = 0;
--
2.41.0

[Jan Kiszka]

I would not classify this as fix. Static vars are zero-initialized for
anything not explicitly specified, aren't they?

Jan

--
Siemens AG, Technology
Linux Expert Center

[Michael Adler]

> I would not classify this as fix. Static vars are zero-initialized for
> anything not explicitly specified, aren't they?

Correct. I would go ahead and revise the commit message in patch series
v2.


--
Michael Adler

Siemens AG
Technology
Connectivity & Edge
Smart Embedded Systems
T CED SES-DE
Otto-Hahn-Ring 6
81739 Munich, Germany

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim
Hagemann Snabe; Managing Board: Roland Busch, Chairman, President and
Chief Executive Officer; Cedrik Neike, Matthias Rebellius, Ralf P.
Thomas, Judith Wiese; Registered offices: Berlin and Munich, Germany;
Commercial registries: Berlin-Charlottenburg, HRB 12300, Munich, HRB
6684; WEEE-Reg.-No. DE 23691322

[Jan Kiszka]

On 26.07.23 14:45, Michael Adler wrote:
> > I would not classify this as fix. Static vars are zero-initialized for
>> anything not explicitly specified, aren't they?
>
> Correct. I would go ahead and revise the commit message in patch series
> v2.

Or just send v2 fpr this patch alone. I'm fine with the rest.

Thanks,

[Michael Adler]

The usage of the OPT macro may result in uninitialized struct members
when applied to non-static variables. This commit mitigates this risk by
taking advantage of C99's designated initializers to ensure proper
initialization.
In the case of efibootguard, we're dealing with a static array of
structs, so each struct member is correctly initialized regardless of
this adjustment.

This fixes a clang compiler warning.

--
2.41.0

[Jan Kiszka]

On 26.07.23 10:40, 'Michael Adler' via EFI Boot Guard wrote:

Thanks, applied (with patch 2 in v2).