[PATCH 0/5] misc fixes

[Dominique Martinet]

patch 1 is a big problem with hawkbit, swupdate not sending
notifications to hawkbit properly, I'm ashamed I let that through...

other patches all are minor and probably don't really matter;
I had no uboot env setup when testing from PC so also found another
couple of issues with that, but most people will probably never notice.

Dominique Martinet (5):
hawkbit: fix process notification not sending logs
swupdate_vars: check namespace init worked
hawkbit: do not reset action_id when getting from env
main: free cfgname before exiting
network_thread DEBUG_IPC: move off the msglock section

core/network_thread.c | 7 ++++---
core/swupdate.c | 1 +
core/swupdate_vars.c | 4 ++++
suricatta/server_hawkbit.c | 14 ++++++--------
4 files changed, 15 insertions(+), 11 deletions(-)

--
2.39.2

[Dominique Martinet]

ipc_get_status_timeout() returns the size of the message on success, but
ipc_get_status() returns 0 in this case (both return a negative value on
error)

Changing the function to use ipc_get_status() didn't update the return
value check: fix this, and also properly check for errors.
Also remove obsolete comment describing the return value.

Reported-by: James Hilliard <james.h...@gmail.com>
Fixes: da48265ad29f ("suricatta process notification: improve ipc_get_status scheduling")
Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---
suricatta/server_hawkbit.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index 9c3e97418361..26260b6bd5e0 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -1003,7 +1003,12 @@ static void *process_notification_thread(void *data)
bool data_avail = false;
int ret = ipc_get_status(&msg);

- data_avail = ret > 0 && (strlen(msg.data.status.desc) != 0);
+ if (ret < 0) {
+ ERROR("Error getting status, stopping notification thread");
+ stop = true;;
+ } else {
+ data_avail = (strlen(msg.data.status.desc) != 0);
+ }

/*
* Mutex used to synchronize end of the thread
@@ -1017,12 +1022,6 @@ static void *process_notification_thread(void *data)

if (data_avail && msg.data.status.current == PROGRESS)
continue;
- /*
- * If there is a message
- * ret > 0: data available
- * ret == 0: TIMEOUT, no more messages
- * ret < 0 : ERROR, exit
- */
if (data_avail && numdetails < MAX_DETAILS) {
for (int c = 0; c < strlen(msg.data.status.desc); c++) {
switch (msg.data.status.desc[c]) {
--
2.39.2

[Dominique Martinet]

This is not a problem but building with ASAN (CONFIG_EXTRA_CFLAGS and
LDFLAGS with -fsanitize=address) prints a warning that this value is not
freed every time; there is no harm in freeing it before exiting.

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

core/swupdate.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/core/swupdate.c b/core/swupdate.c
index 8d6467b7b688..4f0ad963346e 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -997,5 +997,6 @@ int main(int argc, char **argv)
if (!opt_c && !opt_i)
pthread_join(network_daemon, NULL);

+ free(cfgfname);
return exit_code;
}
--
2.39.2

[Dominique Martinet]

if the fw_env config didn't contain the requested namespace (or config
was invalid) then libuboot_get_namespace() will fail and that will
segfault swupdate

This is an admin error so not a critical bug, but might as well
check and print an error.

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

core/swupdate_vars.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/core/swupdate_vars.c b/core/swupdate_vars.c
index f1e536a845be..081e6097e1e2 100644
--- a/core/swupdate_vars.c
+++ b/core/swupdate_vars.c
@@ -47,6 +47,10 @@ int swupdate_vars_initialize(struct uboot_ctx **ctx, const char *namespace)
}

*ctx = libuboot_get_namespace(*ctx, namespace);
+ if (!*ctx) {
+ ERROR("Cannot get namespace %s from %s", namespace, get_fwenv_config());
+ return -EINVAL;
+ }

if (libuboot_open(*ctx) < 0) {
WARN("Cannot read environment, maybe still empty ?");
--
2.39.2

[Dominique Martinet]

server_handle_initial_state() firsts get an action id from the server
and then overrides it with the env value, but there is no check if there
had been no env value, sending a request to id -1 to server.

The commit that added this reset also added code to only call
get_action_id_from_env() if action_id <= 0, so it did not need to reset
it: just leave the status quo of using the last value if it wasn't set.

Fixes: f3b1e1e26dfb ("hawkbit: check for validity of action_id")

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

suricatta/server_hawkbit.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index 26260b6bd5e0..2f7562c543db 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -867,7 +867,6 @@ static void get_action_id_from_env(int *action_id)
* Get the acction_id that corresponds to the done update if it was
* stored.
*/
- *action_id = -1;
char *action_str = swupdate_vars_get("action_id", NULL);
if (action_str) {
int tmp = ustrtoull(action_str, NULL, 10);
--
2.39.2

[Dominique Martinet]

logging something can call into network_notifier() through notify(),
which will dead lock on msglock if logging while that lock is held

That probably isn't used by anyone so we could just remove it,
but if we're keeping it it should at least work...

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

That log wasn't even very useful to me -- I'd suggest removing the two
small ifdef DEBUG_IPC sections instead if you prefer.

core/network_thread.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/core/network_thread.c b/core/network_thread.c
index d7b713fb2154..f664b267d4c5 100644
--- a/core/network_thread.c
+++ b/core/network_thread.c
@@ -513,13 +513,14 @@ void *network_thread (void *data)
nrmsgs--;
strncpy(msg.data.status.desc, notification->msg,
sizeof(msg.data.status.desc) - 1);
-#ifdef DEBUG_IPC
- DEBUG("GET STATUS: %s\n", msg.data.status.desc);
-#endif
msg.data.status.current = notification->status;
msg.data.status.error = notification->error;
}
pthread_mutex_unlock(&msglock);
+#ifdef DEBUG_IPC
+ if (notification)
+ DEBUG("GET STATUS: %s\n", msg.data.status.desc);
+#endif

break;
case NOTIFY_STREAM:
--
2.39.2

[Dominique Martinet]

Dominique Martinet wrote on Fri, Jul 05, 2024 at 03:54:30PM +0900:
> ipc_get_status_timeout() returns the size of the message on success, but
> ipc_get_status() returns 0 in this case (both return a negative value on
> error)
>
> Changing the function to use ipc_get_status() didn't update the return
> value check: fix this, and also properly check for errors.
> Also remove obsolete comment describing the return value.
>
> Reported-by: James Hilliard <james.h...@gmail.com>
> Fixes: da48265ad29f ("suricatta process notification: improve ipc_get_status scheduling")
> Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
> ---
> suricatta/server_hawkbit.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
> index 9c3e97418361..26260b6bd5e0 100644
> --- a/suricatta/server_hawkbit.c
> +++ b/suricatta/server_hawkbit.c
> @@ -1003,7 +1003,12 @@ static void *process_notification_thread(void *data)
> bool data_avail = false;
> int ret = ipc_get_status(&msg);
>
> - data_avail = ret > 0 && (strlen(msg.data.status.desc) != 0);
> + if (ret < 0) {
> + ERROR("Error getting status, stopping notification thread");
> + stop = true;;

I always notice this kind of things after sending the mail... there's
two ';' here

It doesn't really matter, but might as well remove the second one when
you apply it (not bothering to resend, please ask if you want me to)

--
Dominique

[Stefano Babic]

I can fix this myself.

Stefano

[Stefano Babic]

Thanks, I forget to check it when I added this code !

Acked-by: Stefano Babic <stefan...@swupdate.org>

Best regards,
Stefano

[Stefano Babic]

On 05.07.24 08:54, Dominique Martinet wrote:

[Stefano Babic]

On 05.07.24 08:54, Dominique Martinet wrote:

[Stefano Babic]

Hi Dominique,

On 05.07.24 08:54, Dominique Martinet wrote:

> logging something can call into network_notifier() through notify(),
> which will dead lock on msglock if logging while that lock is held
>
> That probably isn't used by anyone so we could just remove it,
> but if we're keeping it it should at least work...
>
> Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
> ---
> That log wasn't even very useful to me -- I'd suggest removing the two
> small ifdef DEBUG_IPC sections instead if you prefer.

I used this #ifdef just at the beginning and inside this module, and it
is neither helpful nor used. DEBUG_IPC is never set. Yes, I strongly
prefer to remove it, it just a remain from my first implementation.

Best regards,
Stefano