[PATCH] core: post-update command execution

[Christian Storm]

After having installed an update, an external agent (listening
on the progress interface) gets notified and should decide on
further actions to take if any, e.g., shutdown all running
applications and reboot.

Enabling the CONFIG_POSTUPDATE option allows to specify a fallback
command (CONFIG_POSTUPDATE_COMMAND) to be executed via system(3) if
(1) no external agent is registered and
(2) the --postupdate command line option is given.

Suricatta has been adapted to use this new method instead of
unconditionally executing a reboot.

Signed-off-by: Christian Storm <christi...@siemens.com>
Signed-off-by: Silvano Cirujano Cuesta <silvano.cir...@siemens.com>
---
Kconfig | 22 +++++++++++++
core/Makefile | 1 +
core/swupdate.c | 15 +++++++++
core/swupdate_postupdate.c | 60 +++++++++++++++++++++++++++++++++++
corelib/progress_thread.c | 9 ++++++
doc/source/suricatta.rst | 19 ++++++++++-
doc/source/swupdate.rst | 5 +++
include/progress.h | 2 ++
include/suricatta/state.h | 3 ++
include/swupdate.h | 1 +
suricatta/server_hawkbit.c | 61 ++++++++++++++++++------------------
suricatta/state.c | 9 ++++++
suricatta/test/Makefile | 15 +++++++--
suricatta/test/test.h | 9 ++++++
suricatta/test/test_server_hawkbit.c | 32 ++++++++++++++++---
15 files changed, 225 insertions(+), 38 deletions(-)
create mode 100644 core/swupdate_postupdate.c

diff --git a/Kconfig b/Kconfig
index c863adf..98bd65d 100644
--- a/Kconfig
+++ b/Kconfig
@@ -70,6 +70,28 @@ config SCRIPTS
in the image. For security reason, this option
can be switched off.

+config POSTUPDATE
+ bool "enable post-update command execution"
+ default y
+ help
+ After having installed an update, an external agent (listening
+ on the progress interface) gets notified and should decide on
+ further actions to take if any, e.g., shutdown all running
+ applications and reboot.
+
+ Enabling this option allows to specify a fallback command to be
+ executed if (1) no external agent is registered and (2) the
+ --postupdate command line option is given.
+
+config POSTUPDATE_COMMAND
+ string "post-update command"
+ default "reboot"
+ depends on POSTUPDATE
+ help
+ The command to be executed if no external agent is registered
+ and the --postupdate command line option is given.
+ The command is executed via SYSTEM(3)
+
config HW_COMPATIBILITY
bool "check for hardware / software compatibility"
default n
diff --git a/core/Makefile b/core/Makefile
index 659f55b..afe017a 100644
--- a/core/Makefile
+++ b/core/Makefile
@@ -16,3 +16,4 @@ obj-y += swupdate.o \
parser.o \
pctl.o \
syslog.o \
+ swupdate_postupdate.o \
diff --git a/core/swupdate.c b/core/swupdate.c
index 9567b09..644b435 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -112,10 +112,16 @@ static struct option long_options[] = {
{"webserver", required_argument, NULL, 'w'},
#endif
{"check", no_argument, NULL, 'c'},
+#ifdef CONFIG_POSTUPDATE
+ {"postupdate", no_argument, NULL, 'p'},
+#endif
{NULL, 0, NULL, 0}
};

int loglevel = ERRORLEVEL;
+#ifdef CONFIG_POSTUPDATE
+int run_postupdate_cmd = 0;
+#endif

static void usage(char *programname)
{
@@ -134,6 +140,9 @@ static void usage(char *programname)
" connection is broken (0 means undefinetly retries)\n"
" -t, --timeout : timeout to check if a connection is lost\n"
#endif
+#ifdef CONFIG_POSTUPDATE
+ " -p, --postupdate : execute post-update command if no agent is registered\n"
+#endif
" -e, --select <software>,<mode> : Select software images set and source\n"
" Ex.: stable,main\n"
" -i, --image <filename> : Software to be installed\n"
@@ -633,6 +642,11 @@ int main(int argc, char **argv)
case 'c':
opt_c = 1;
break;
+#ifdef CONFIG_POSTUPDATE
+ case 'p':
+ run_postupdate_cmd = 1;
+ break;
+#endif
default:
usage(argv[0]);
exit(1);
@@ -771,6 +785,7 @@ int main(int argc, char **argv)
cleanup_files(&swcfg);

notify(SUCCESS, 0, 0);
+ (void)swupdate_postupdate();
}

if (opt_w || opt_s || opt_u || opt_d)
diff --git a/core/swupdate_postupdate.c b/core/swupdate_postupdate.c
new file mode 100644
index 0000000..2c646a6
--- /dev/null
+++ b/core/swupdate_postupdate.c
@@ -0,0 +1,60 @@
+/*
+ * Author: Christian Storm
+ * Copyright (C) 2016, Siemens AG
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc.
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stdlib.h>
+
+#include "util.h"
+#include "globals.h"
+#include "progress.h"
+
+#ifdef CONFIG_POSTUPDATE
+extern int run_postupdate_cmd;
+#endif
+
+int swupdate_postupdate(void)
+{
+ if (swupdate_progress_has_subscribers()) {
+ swupdate_progress_done();
+ DEBUG("Notified progress subscribers may take actions.\n");
+ return 0;
+ }
+
+#if defined(CONFIG_POSTUPDATE)
+ if (!run_postupdate_cmd) {
+ DEBUG("Post-update command execution not enabled.\n");
+ return 0;
+ }
+
+ int ret = system(CONFIG_POSTUPDATE_COMMAND);
+ if (WIFEXITED(ret)) {
+ DEBUG("Post-update command returned with %d",
+ WEXITSTATUS(ret));
+ } else {
+ ERROR("Post-update command returned with %d: '%s'", ret,
+ strerror(errno));
+ return -1;
+ }
+#endif
+ return 0;
+}
diff --git a/corelib/progress_thread.c b/corelib/progress_thread.c
index 6b51ec9..b7f18a8 100644
--- a/corelib/progress_thread.c
+++ b/corelib/progress_thread.c
@@ -94,6 +94,15 @@ static void send_progress_msg(void)
}
}

+bool swupdate_progress_has_subscribers(void)
+{
+ struct swupdate_progress *prbar = &progress;
+ pthread_mutex_lock(&prbar->lock);
+ bool has_subscribers = !SIMPLEQ_EMPTY(&prbar->conns);
+ pthread_mutex_unlock(&prbar->lock);
+ return has_subscribers;
+}
+
void swupdate_progress_init(unsigned int nsteps) {
struct swupdate_progress *prbar = &progress;
pthread_mutex_lock(&prbar->lock);
diff --git a/doc/source/suricatta.rst b/doc/source/suricatta.rst
index 21f7c25..b6e8650 100644
--- a/doc/source/suricatta.rst
+++ b/doc/source/suricatta.rst
@@ -48,7 +48,7 @@ a hawkBit instance at ``http://10.0.0.2:8080`` with tenant ``default``
and device ID ``25``.


-Note that initially and after having installed an update, suricatta
+Note that on startup when having installed an update, suricatta
tries to report the update status to its upstream server, e.g.,
hawkBit, prior to entering the main loop awaiting further updates.
If this initial report fails, e.g., because of a not (yet) configured
@@ -60,6 +60,23 @@ to its upstream server irrespective of the error conditions, this has
to be realized externally in terms of restarting swupdate on exit.


+After an update has been performed, an agent listening on the progress
+interface may execute post-update actions, e.g., a reboot, on receiving
+``DONE``. If no such agent is registered and running, a post-update
+action defined at compile-time via ``CONFIG_POSTUPDATE_COMMAND`` can be
+executed if (1) ``CONFIG_POSTUPDATE=y`` is set and (2) the ``-p`` command
+line option is given to SWUpdate.
+Note that currently, at least a restart of SWUpdate has to be performed
+as post-update action since only then suricatta tries to report the
+update status to its upstream server.
+Otherwise, on the next polling interval elapsed, the very same update is
+announced by hawkBit as pending and is installed again if no update
+status storage, i.e., ``CONFIG_SURICATTA_STATE_CHOICE_NONE``, is selected.
+If an update status storage, e.g., ``CONFIG_SURICATTA_STATE_CHOICE_UBOOT``,
+is selected, succinct update actions announced by hawkBit are skipped until
+a restart of SWUpdate.
+
+
Supporting different Servers
----------------------------

diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index e0d986d..2de5f8e 100644
--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -546,6 +546,11 @@ Command line parameters
| | | referenced in sw-description are present. |
| | | Usage: swupdate -c -i <file> |
+-------------+----------+--------------------------------------------+
+| -p | - | Active only if CONFIG_POSTUPDATE is set. |
+| | | Execute fallback post-update command if no |
+| | | external agent listens on the progress |
+| | | interface. |
++-------------+----------+--------------------------------------------+

Changes in boot-loader code
===========================
diff --git a/include/progress.h b/include/progress.h
index 5035b58..f146772 100644
--- a/include/progress.h
+++ b/include/progress.h
@@ -22,6 +22,7 @@
#ifndef _INSTALL_PROGRESS_H
#define _INSTALL_PROGRESS_H

+#include <stdbool.h>
#include <swupdate_status.h>

#define SOCKET_PROGRESS_PATH "/tmp/swupdateprog"
@@ -47,6 +48,7 @@ void swupdate_progress_inc_step(char *image);
void swupdate_progress_step_completed(void);
void swupdate_progress_end(RECOVERY_STATUS status);
void swupdate_progress_done(void);
+bool swupdate_progress_has_subscribers(void);

void *progress_bar_thread (void *data);

diff --git a/include/suricatta/state.h b/include/suricatta/state.h
index 80243b6..2879dee 100644
--- a/include/suricatta/state.h
+++ b/include/suricatta/state.h
@@ -18,6 +18,7 @@
*/

#pragma once
+#include <stdbool.h>
#include "suricatta.h"

/* (Persistent) Update State Management Functions.
@@ -34,8 +35,10 @@ typedef enum {
STATE_TESTING = '2',
STATE_FAILED = '3',
STATE_NOT_AVAILABLE = '4',
+ STATE_ERROR = '5'
} update_state_t;

server_op_res_t save_state(char *key, update_state_t value);
server_op_res_t read_state(char *key, update_state_t *value);
server_op_res_t reset_state(char *key);
+bool is_state_valid(update_state_t state);
diff --git a/include/swupdate.h b/include/swupdate.h
index b244d3d..8911b82 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -144,5 +144,6 @@ struct swupdate_cfg {

off_t extract_sw_description(int fd, const char *descfile, off_t start);
int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start);
+int swupdate_postupdate(void);

#endif
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index fd641e9..6793d75 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -93,7 +93,6 @@ json_object *json_get_key(json_object *json_root, const char *key);
json_object *json_get_path_key(json_object *json_root, const char **json_path);
char *json_get_data_url(json_object *json_root, const char *key);
server_op_res_t map_channel_retcode(channel_op_res_t response);
-void server_reboot(void);
server_op_res_t server_handle_initial_state(update_state_t stateovrrd);
int server_update_status_callback(ipc_message *msg);
int server_update_done_callback(RECOVERY_STATUS status);
@@ -101,6 +100,7 @@ server_op_res_t server_process_update_artifact(json_object *json_data_artifact);
void suricatta_print_help(void);
server_op_res_t server_set_polling_interval(json_object *json_root);
server_op_res_t server_set_config_data(json_object *json_root);
+static update_state_t get_state(void);
server_op_res_t
server_send_deployment_reply(const int action_id, const int job_cnt_max,
const int job_cnt_cur, const char *finished,
@@ -182,17 +182,6 @@ server_op_res_t map_channel_retcode(channel_op_res_t response)
return SERVER_EERR;
}

-#ifdef UNIT_TESTING
-void server_reboot(void) { return; }
-#else
-__attribute__((noreturn)) void server_reboot(void)
-{
- (void)execl("/bin/sh", "sh", "-c", "reboot", (char *)0);
- ERROR("Cannot reboot system: %s\n", strerror(errno));
- exit(EXIT_FAILURE);
-}
-#endif
-
server_op_res_t server_send_cancel_reply(const int action_id)
{
assert(server_hawkbit.url != NULL);
@@ -505,27 +494,43 @@ server_op_res_t server_has_pending_action(int *action_id)
(void)server_send_cancel_reply(*action_id);
result = SERVER_OK;
}
+ if ((result == SERVER_UPDATE_AVAILABLE) &&
+ (get_state() == STATE_INSTALLED)) {
+ WARN("An already installed update is pending testing, "
+ "ignoring available update action.");
+ INFO("Please restart SWUpdate to report the test results "
+ "upstream.");
+ result = SERVER_NO_UPDATE_AVAILABLE;
+ }
return result;
}

+
+static update_state_t get_state(void) {
+ update_state_t state;
+
+ if (read_state((char *)STATE_KEY, &state) != SERVER_OK) {
+ ERROR("Cannot read stored update state.\n");
+ return STATE_ERROR;
+ }
+ TRACE("Read state=%c from persistent storage.\n", state);
+
+ return is_state_valid(state) ? state : STATE_ERROR;
+}
+
server_op_res_t server_handle_initial_state(update_state_t stateovrrd)
{
update_state_t state = STATE_OK;
if (stateovrrd != STATE_NOT_AVAILABLE) {
state = stateovrrd;
TRACE("Got state=%c from command line.\n", state);
+ if (!is_state_valid(state)) {
+ return SERVER_EINIT;
+ }
} else {
- if (read_state((char *)STATE_KEY, &state) != SERVER_OK) {
- ERROR("Cannot read stored update state.\n");
- return SERVER_OK;
+ if ((state = get_state()) == STATE_ERROR) {
+ return SERVER_EINIT;
}
- TRACE("Read state=%c from persistent storage.\n", state);
- }
-
- if ((state < STATE_OK) || (state > STATE_NOT_AVAILABLE)) {
- ERROR("Unknown persistent state=%c, please fix it manually!\n",
- state);
- return SERVER_EINIT;
}

const char *reply_result;
@@ -549,6 +554,7 @@ server_op_res_t server_handle_initial_state(update_state_t stateovrrd)
break;
case STATE_OK:
case STATE_NOT_AVAILABLE:
+ default:
DEBUG("State is STATE_OK/STATE_NOT_AVAILABLE, nothing to "
"report to server.\n");
return SERVER_OK;
@@ -922,14 +928,8 @@ cleanup:
ERROR("JSON object should be freed but was not.\n");
}
if (result == SERVER_OK) {
- /* TODO Notify running applications of successful installation
- * and a pending reboot via IPC and time-out wait. The
- * reboot cannot be done in a post-install script as this
- * is called immediately after installation, after which
- * actions such as, e.g., setting the U-Boot environment
- * are performed. */
- INFO("Update successful, rebooting the system now.\n");
- server_reboot();
+ INFO("Update successful, executing post-update commands.\n");
+ result = swupdate_postupdate() == 0 ? SERVER_OK : SERVER_EERR;
}
return result;
}
@@ -1252,6 +1252,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])
if (channel.open(&channel_data_defaults) != CHANNEL_OK) {
return SERVER_EINIT;
}
+
/* If an update was performed, report its status to the hawkBit server
* prior to entering the main loop. May run indefinitely if server is
* unavailable. In case of an error, the error is returned to the main
diff --git a/suricatta/state.c b/suricatta/state.c
index 12ff9f2..da2962a 100644
--- a/suricatta/state.c
+++ b/suricatta/state.c
@@ -18,11 +18,20 @@
*/

#include <stdio.h>
+#include <stdbool.h>
#include <errno.h>
#include <util.h>
#include <fw_env.h>
#include "suricatta/state.h"

+bool is_state_valid(update_state_t state) {
+ if ((state < STATE_OK) || (state > STATE_ERROR)) {
+ ERROR("Unknown update state=%c\n", state);
+ return false;
+ }
+ return true;
+}
+
#ifndef CONFIG_SURICATTA_STATE_CHOICE_UBOOT
server_op_res_t save_state(char *key, update_state_t value)
{
diff --git a/suricatta/test/Makefile b/suricatta/test/Makefile
index 14e2f8e..6415c20 100644
--- a/suricatta/test/Makefile
+++ b/suricatta/test/Makefile
@@ -37,10 +37,17 @@ TEST_SRCS = $(wildcard test_*.c)
TEST_OBJS = $(addsuffix .o, $(basename $(TEST_SRCS)))
TARGETS = $(basename $(TEST_SRCS))

-SURICATTA_OBJS = $(SURICATTA_DIR)/server_hawkbit.o $(SURICATTA_DIR)/channel_hawkbit.o $(SURICATTA_DIR)/state.o $(SURICATTA_DIR)/suricatta.o
+SURICATTA_OBJS = $(SURICATTA_DIR)/server_hawkbit.o \
+ $(SURICATTA_DIR)/channel_hawkbit.o \
+ $(SURICATTA_DIR)/state.o \
+ $(SURICATTA_DIR)/suricatta.o
+
SURICATTA_SRCS = $(addsuffix .c, $(basename $(SURICATTA_OBJS)))

-SWUPDATE_OBJS = $(SWUPDATE_DIR)/ipc/network_ipc.o $(SWUPDATE_DIR)/corelib/swupdate_dict.o
+SWUPDATE_OBJS = $(SWUPDATE_DIR)/ipc/network_ipc.o \
+ $(SWUPDATE_DIR)/corelib/swupdate_dict.o \
+ $(SWUPDATE_DIR)/corelib/progress_thread.o \
+ $(SWUPDATE_DIR)/core/swupdate_postupdate.o

COMPILE.c = $(CC) $(CFLAGS) $(SURICATTA_CFLAGS) $(SURICATTA_DEFINES) $(SURICATTA_INCLUDES) -c
LINK.c = $(CC) $(CFLAGS) $(SURICATTA_LDFLAGS) $(SURICATTA_LIBS) $(SURICATTA_LDMOCKS)
@@ -56,7 +63,9 @@ test_server_hawkbit test_json: % : clean %.c $(SURICATTA_OBJS) $(SURICATTA_SRCS)
@$(COMPILE.c) $@.c -o $@.o
## NOTE: assuming libubootenv.a is available in $(SWUPDATE_DIR)/ for linking
## NOTE: assuming network_ipc.o is available in $(SWUPDATE_DIR)/ipc/ for linking
- ## NOTE: assuming swupdate_dict.o is available in $(SWUPDATE_DIR)/corelib/swupdate_dict.o for linking
+ ## NOTE: assuming swupdate_dict.o is available in $(SWUPDATE_DIR)/corelib/ for linking
+ ## NOTE: assuming progress_thread.o is available in $(SWUPDATE_DIR)/corelib/ for linking
+ ## NOTE: assuming swupdate_postupdate.o is available in $(SWUPDATE_DIR)/core/ for linking
@$(LINK.c) $(SWUPDATE_OBJS) $(SURICATTA_OBJS) $@.o -L$(SWUPDATE_DIR) -lubootenv -o $@
@CMOCKA_MESSAGE_OUTPUT=TAP $(SURICATTA_TEST_DIR)/$@

diff --git a/suricatta/test/test.h b/suricatta/test/test.h
index 9a1430a..bff9fc5 100644
--- a/suricatta/test/test.h
+++ b/suricatta/test/test.h
@@ -24,6 +24,9 @@
/* Disable swupdate core's logging output to not clutter test output */
int loglevel = OFF;

+/* Do not run post-update command in unit tests */
+int run_postupdate_cmd = 0;
+
/* mock notify() to spare linking against swupdate core's notifier.c */
void notify(RECOVERY_STATUS status, int level, const char *msg)
{
@@ -94,3 +97,9 @@ pthread_t start_thread(void *(* start_routine) (void *), void *arg)
(void)arg;
return 0;
}
+
+int listener_create(const char *path, int type) {
+ (void)path;
+ (void)type;
+ return 99;
+}
diff --git a/suricatta/test/test_server_hawkbit.c b/suricatta/test/test_server_hawkbit.c
index 8d5c512..11ec4dc 100644
--- a/suricatta/test/test_server_hawkbit.c
+++ b/suricatta/test/test_server_hawkbit.c
@@ -73,10 +73,20 @@ channel_op_res_t __wrap_channel_get(void *data)
}

extern server_op_res_t __real_save_state(char *key, update_state_t value);
-server_op_res_t __wrap_save_state(void *data);
-server_op_res_t __wrap_save_state(void *data)
+server_op_res_t __wrap_save_state(char *key, update_state_t *value);
+server_op_res_t __wrap_save_state(char *key, update_state_t *value)
{
- (void)data;
+ (void)key;
+ (void)value;
+ return mock_type(server_op_res_t);
+}
+
+extern server_op_res_t __real_read_state(char *key, update_state_t *value);
+server_op_res_t __wrap_read_state(char *key, update_state_t *value);
+server_op_res_t __wrap_read_state(char *key, update_state_t *value)
+{
+ (void)key;
+ *value = mock_type(update_state_t);
return mock_type(server_op_res_t);
}

@@ -158,16 +168,30 @@ static void test_server_has_pending_action(void **state)
assert_int_equal(SERVER_NO_UPDATE_AVAILABLE,
server_has_pending_action(&action_id));

- /* Test Case: Update Action available. */
+ /* Test Case: Update Action available && !STATE_INSTALLED. */
will_return(__wrap_channel_get,
json_tokener_parse(json_reply_update_available));
will_return(__wrap_channel_get, CHANNEL_OK);
will_return(__wrap_channel_get,
json_tokener_parse(json_reply_update_data));
will_return(__wrap_channel_get, CHANNEL_OK);
+ will_return(__wrap_read_state, STATE_NOT_AVAILABLE);
+ will_return(__wrap_read_state, SERVER_OK);
assert_int_equal(SERVER_UPDATE_AVAILABLE,
server_has_pending_action(&action_id));

+ /* Test Case: Update Action available && STATE_INSTALLED. */
+ will_return(__wrap_channel_get,
+ json_tokener_parse(json_reply_update_available));
+ will_return(__wrap_channel_get, CHANNEL_OK);
+ will_return(__wrap_channel_get,
+ json_tokener_parse(json_reply_update_data));
+ will_return(__wrap_channel_get, CHANNEL_OK);
+ will_return(__wrap_read_state, STATE_INSTALLED);
+ will_return(__wrap_read_state, SERVER_OK);
+ assert_int_equal(SERVER_NO_UPDATE_AVAILABLE,
+ server_has_pending_action(&action_id));
+
/* Test Case: Cancel Action available. */
will_return(__wrap_channel_get,
json_tokener_parse(json_reply_cancel_available));
--
2.10.2

[Stefano Babic]

Hi Christian,

On 12/12/2016 13:03, Christian Storm wrote:
> After having installed an update, an external agent (listening
> on the progress interface) gets notified and should decide on
> further actions to take if any, e.g., shutdown all running
> applications and reboot.
>

Thanks - a change here is highly required. In fact, running the network
processes with a degraded user, does not allow to simply run "reboot" as
before.

post-install command should run from the installer itself, because is
the one accessing the hardware.

> Enabling the CONFIG_POSTUPDATE option allows to specify a fallback
> command (CONFIG_POSTUPDATE_COMMAND) to be executed via system(3) if
> (1) no external agent is registered and
> (2) the --postupdate command line option is given.

I would prefer to not bind the logic inside SWUpdate. If a user would
use "-p" for the postupdate command, this should be called in any case -
independently if there are registered user to the progress interface.
There is also the not written rule that command line parameters have
priority on configuration file.

In fact, the following use case does not work:

- swupdate running in suricatta mode
- target has a display. Some clients connect to SWUpdate to show the
update status, but nothing more

There are listeners, but just as readers. IMHO if a user decides to use
-p, it is. Else we have some strange behaviors, with the target
rebooting if some clients are connected or not.

>
> Suricatta has been adapted to use this new method instead of
> unconditionally executing a reboot.

suricatta *cannot* - after splitting into processes, suricatta and all
other processes has no right anymore to run reboot.

So this cannot work.

Waht about to drop the CONFIG_ options and pass "-p <command to be
executed> ?

I would like to see this also in the configuration file (in the general
sections).

As I said, checking for subscribers is just a use case: there is a
progress client that is responsible for rebooting the system. But if I
have progress clients simply showing what is going on, this does not work.

> +
> +#if defined(CONFIG_POSTUPDATE)
> + if (!run_postupdate_cmd) {
> + DEBUG("Post-update command execution not enabled.\n");
> + return 0;
> + }

We have two setups, one at compile time (CONFIG_POSTUPDATE*) and one at
run time. We can rely on the runtime check, and if "-p" is passed, the
feature is required.

see above.

This looks a different issue - should this addressed by another patch ?

> return result;
> }
>
> +
> +static update_state_t get_state(void) {
> + update_state_t state;
> +
> + if (read_state((char *)STATE_KEY, &state) != SERVER_OK) {
> + ERROR("Cannot read stored update state.\n");
> + return STATE_ERROR;
> + }
> + TRACE("Read state=%c from persistent storage.\n", state);
> +
> + return is_state_valid(state) ? state : STATE_ERROR;
> +}
> +
> server_op_res_t server_handle_initial_state(update_state_t stateovrrd)
> {
> update_state_t state = STATE_OK;
> if (stateovrrd != STATE_NOT_AVAILABLE) {
> state = stateovrrd;
> TRACE("Got state=%c from command line.\n", state);
> + if (!is_state_valid(state)) {
> + return SERVER_EINIT;
> +

Ditto.

This does not work, as I have explained.

Best regards,
Stefano

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Christian Storm]

Hi Stefano,

> > After having installed an update, an external agent (listening
> > on the progress interface) gets notified and should decide on
> > further actions to take if any, e.g., shutdown all running
> > applications and reboot.
> >
>
> Thanks - a change here is highly required. In fact, running the network
> processes with a degraded user, does not allow to simply run "reboot" as
> before.
>
> post-install command should run from the installer itself, because is
> the one accessing the hardware.

Yes, but the installer itself may also not have the rights to execute
/sbin/reboot under all circumstances. Imagine that one comes up with a
sophisticated user/groups/capabilities setup to run swupate "root"-less...

Either way, what's needed here is a way to signal the execution of the
command to one process that has got sufficient and possibly less-than-
root rights to execute it.

> > Enabling the CONFIG_POSTUPDATE option allows to specify a fallback
> > command (CONFIG_POSTUPDATE_COMMAND) to be executed via system(3) if
> > (1) no external agent is registered and
> > (2) the --postupdate command line option is given.
>
> I would prefer to not bind the logic inside SWUpdate. If a user would
> use "-p" for the postupdate command, this should be called in any case -
> independently if there are registered user to the progress interface.
> There is also the not written rule that command line parameters have
> priority on configuration file.
>
> In fact, the following use case does not work:
>
> - swupdate running in suricatta mode
> - target has a display. Some clients connect to SWUpdate to show the
> update status, but nothing more
>
> There are listeners, but just as readers. IMHO if a user decides to use
> -p, it is. Else we have some strange behaviors, with the target
> rebooting if some clients are connected or not.

Well, yes. But I think this is a matter of what you define to be the
"progress interface's" intention:
By commit c8e461c, the progress interface is sent 'DONE' so that it can
handle/execute exactly this post-update command. However, it mustn't and
may just display some nice information. If it doesn't execute a command
then there's nothing executed, right.

Then there's the question of how to verify the update was successful,
meaning not only the installation itself by swupdate but the
functionality of the installed update. I think this suits the -- then
unsuitably named "progress interface" -- much better than swupdate's
core since this task is highly application-specific and hence should be
external to the core.

Then there's the question of who decides whether a reboot should be
performed and when and what to do beforehand, e.g., gracefully shutting
down applications. Again, I think this can be a job for the "progress
interface".

Then there's the possibility of not only delivering firmware but also
software package updates via hawkBit. A reboot is seldom necessary after
having upgraded a program. Whether to reboot in this case has to be
decided by an external entity, e.g. an agent listening on the "progress
interface".

I would even like to see some external agent acting as a signal for the
installation by swupdate: For example, prior to the actual installation,
swupdate asks the agent if it should install now and the answer depends
on some GPIO pin raised and that pin is read by the external agent. As
this is highly application-specific, it shouldn't be in swupdate's core.
Of course, the external agent has to be trusted by swupdate, whatever
that means implementation-wise...

... but all this is maybe too much for the progress interface, at the
very least it has to be renamed then :)


That said, the -p switch is just meant as a safety fallback in case an
external agent is absent, either because it has crashed or it is
deliberately not running but nonetheless, a command should be executed
at all means as indicated by giving -p. This doesn't work if more than
one agent is subscribed, granted. This may be a matter of documentation.
The progress client has to be adapted/(re-)written depending on the
specific use case at hand/splash implementation anyway...


> [...]

> Waht about to drop the CONFIG_ options and pass "-p <command to be
> executed> ?
>
> I would like to see this also in the configuration file (in the general
> sections).
>

> [...]

> We have two setups, one at compile time (CONFIG_POSTUPDATE*) and one at
> run time. We can rely on the runtime check, and if "-p" is passed, the
> feature is required.
>

> [...]

> As I said, checking for subscribers is just a use case: there is a
> progress client that is responsible for rebooting the system. But if I
> have progress clients simply showing what is going on, this does not work.

Would be OK for me. So your suggestion then is to just have the command
line flag -p and in case it's given or an according config file value is
set (with the command line argument having priority), then the command
is executed, regardless of any progress interface subscribers?

> > Suricatta has been adapted to use this new method instead of
> > unconditionally executing a reboot.
>
> suricatta *cannot* - after splitting into processes, suricatta and all
> other processes has no right anymore to run reboot.
>
> So this cannot work.

If you start swupdate as root and you don't explicitly configure them to
run as a different user than root then they're run as root and can
execute this command. Granted, they shouldn't run as root in the first
place for the reasons you mentioned...

> > [...]

> > server_op_res_t server_send_cancel_reply(const int action_id)
> > {
> > assert(server_hawkbit.url != NULL);
> > @@ -505,27 +494,43 @@ server_op_res_t server_has_pending_action(int *action_id)
> > (void)server_send_cancel_reply(*action_id);
> > result = SERVER_OK;
> > }
> > + if ((result == SERVER_UPDATE_AVAILABLE) &&
> > + (get_state() == STATE_INSTALLED)) {
> > + WARN("An already installed update is pending testing, "
> > + "ignoring available update action.");
> > + INFO("Please restart SWUpdate to report the test results "
> > + "upstream.");
> > + result = SERVER_NO_UPDATE_AVAILABLE;
> > + }
>
> This looks a different issue - should this addressed by another patch ?

Well, not entirely but to a large extent. I've opened a ticket for
hawkBit's support on providing the necessary information:
https://github.com/eclipse/hawkbit/issues/381
This should indeed be addressed by another patch once the information is
available from hawkBit.

For the time being, it is handled here so that -- if one has state
support enabled -- the re-installation of the very same update artifact
is prevented until swupdate is restarted or a reboot has taken place,
effectively resulting in a restart of swupdate.
In case no state support is available, the very same update is installed
again and again as hawkBit announces this on the next polling interval
as to be installed and there's (yet) no way to detect this and handle
accordingly.
See above in the doc/source/suricatta.rst part for this documented.

The decision whether to acknowledge the update and hence avoid
re-installing the same update again and again may be taken by an
external agent that bi-directionally communicates with swupdate.

> > server_op_res_t server_handle_initial_state(update_state_t stateovrrd)
> > {
> > update_state_t state = STATE_OK;
> > if (stateovrrd != STATE_NOT_AVAILABLE) {
> > state = stateovrrd;
> > TRACE("Got state=%c from command line.\n", state);
> > + if (!is_state_valid(state)) {
> > + return SERVER_EINIT;
> > +
>
> Ditto.

This is different: It's just a test if the STATE_ variables are valid,
i.e., in the valid range from STATE_OK to STATE_ERROR.

> > if (result == SERVER_OK) {
> > - /* TODO Notify running applications of successful installation
> > - * and a pending reboot via IPC and time-out wait. The
> > - * reboot cannot be done in a post-install script as this
> > - * is called immediately after installation, after which
> > - * actions such as, e.g., setting the U-Boot environment
> > - * are performed. */
> > - INFO("Update successful, rebooting the system now.\n");
> > - server_reboot();
> > + INFO("Update successful, executing post-update commands.\n");
> > + result = swupdate_postupdate() == 0 ? SERVER_OK : SERVER_EERR;
>
> This does not work, as I have explained.

Then swupdate_postupdate() should incorporate some IPC to some swupdate
process that does have the appropriate rights to execute the post-update
command. Currently, this is the installer. This IPC must also have a
means to report back failure or success.



What do you suggest on how to proceed from here?



Kind regards,
Christian

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

[Stefano Babic]

Hi Christian,

On 12/12/2016 18:04, Christian Storm wrote:
> Hi Stefano,
>
>>> After having installed an update, an external agent (listening
>>> on the progress interface) gets notified and should decide on
>>> further actions to take if any, e.g., shutdown all running
>>> applications and reboot.
>>>
>>
>> Thanks - a change here is highly required. In fact, running the network
>> processes with a degraded user, does not allow to simply run "reboot" as
>> before.
>>
>> post-install command should run from the installer itself, because is
>> the one accessing the hardware.
>
> Yes, but the installer itself may also not have the rights to execute
> /sbin/reboot under all circumstances. Imagine that one comes up with a
> sophisticated user/groups/capabilities setup to run swupate "root"-less...
>

This is the reason why the status is sent via the progress interface and
an external process, with the correct rights, can take the decision.

We are talking here about built-in command, that means when we want that
swupdate is able to take this decision itself. And in that case, it must
have enough right.

> Either way, what's needed here is a way to signal the execution of the
> command to one process that has got sufficient and possibly less-than-
> root rights to execute it.

Exactly.

>> Enabling the CONFIG_POSTUPDATE option allows to specify a fallback
>>> command (CONFIG_POSTUPDATE_COMMAND) to be executed via system(3) if
>>> (1) no external agent is registered and
>>> (2) the --postupdate command line option is given.
>>
>> I would prefer to not bind the logic inside SWUpdate. If a user would
>> use "-p" for the postupdate command, this should be called in any case -
>> independently if there are registered user to the progress interface.
>> There is also the not written rule that command line parameters have
>> priority on configuration file.
>>
>> In fact, the following use case does not work:
>>
>> - swupdate running in suricatta mode
>> - target has a display. Some clients connect to SWUpdate to show the
>> update status, but nothing more
>>
>> There are listeners, but just as readers. IMHO if a user decides to use
>> -p, it is. Else we have some strange behaviors, with the target
>> rebooting if some clients are connected or not.
>
> Well, yes. But I think this is a matter of what you define to be the
> "progress interface's" intention:
> By commit c8e461c, the progress interface is sent 'DONE' so that it can
> handle/execute exactly this post-update command.

Exactly. SWUpdate informs about status and sends all required
information, and an external process can decide.

> However, it mustn't and
> may just display some nice information. If it doesn't execute a command
> then there's nothing executed, right.

Right again.

>
> Then there's the question of how to verify the update was successful,
> meaning not only the installation itself by swupdate but the
> functionality of the installed update. I think this suits the -- then
> unsuitably named "progress interface" -- much better than swupdate's
> core since this task is highly application-specific and hence should be
> external to the core.

This is my intention. A verification can take place before rebooting,
and the process that verifies should then call reboot.

Or system reboots and the new updated software will started and it
verifies itself. And if something is going wrong, it asks for a fallback
(this is possible with U-Boot together with "bootcounter").

>
> Then there's the question of who decides whether a reboot should be
> performed and when and what to do beforehand, e.g., gracefully shutting
> down applications. Again, I think this can be a job for the "progress
> interface".

This is one of the goal of this interface.

>
> Then there's the possibility of not only delivering firmware but also
> software package updates via hawkBit.

I do not understand the subtle difference between firmware and software
package update. A .swu is a software package update. We could split one
.swu into more .swus, but we have to guarantee that the update is atomic.

> A reboot is seldom necessary after
> having upgraded a program. Whether to reboot in this case has to be
> decided by an external entity, e.g. an agent listening on the "progress
> interface".

Anyway, right. SWUpdate cannot decide in all cases.

>
> I would even like to see some external agent acting as a signal for the
> installation by swupdate:

This is generally done before running SWUpdate. If there is this
external agent, this must be responsible to start swupdate, instead of
starting it with init scripts / systemd. SWUpdate is then started when
an update is possible.

> For example, prior to the actual installation,
> swupdate asks the agent if it should install now and the answer depends
> on some GPIO pin raised and that pin is read by the external agent. As
> this is highly application-specific, it shouldn't be in swupdate's core.

Right.

> Of course, the external agent has to be trusted by swupdate, whatever
> that means implementation-wise...
>
> ... but all this is maybe too much for the progress interface, at the
> very least it has to be renamed then :)

ok, it seems we agree on all but the name of the interface. Can you
suggest a better name ? No problem to rename it.

>
>
> That said, the -p switch is just meant as a safety fallback in case an
> external agent is absent, either because it has crashed or it is
> deliberately not running but nonetheless, a command should be executed
> at all means as indicated by giving -p.

ok

> This doesn't work if more than
> one agent is subscribed, granted.

If at the same time suricatta and webserver are running ? Well, they can
run at the same time, and of course a second installation is refused if
there is one currently running. I will say this is a use case.

> This may be a matter of documentation.
> The progress client has to be adapted/(re-)written depending on the
> specific use case at hand/splash implementation anyway...

Make proposals :-)

>
>
>> [...]
>> Waht about to drop the CONFIG_ options and pass "-p <command to be
>> executed> ?
>>
>> I would like to see this also in the configuration file (in the general
>> sections).
>>
>> [...]
>> We have two setups, one at compile time (CONFIG_POSTUPDATE*) and one at
>> run time. We can rely on the runtime check, and if "-p" is passed, the
>> feature is required.
>>
>> [...]
>> As I said, checking for subscribers is just a use case: there is a
>> progress client that is responsible for rebooting the system. But if I
>> have progress clients simply showing what is going on, this does not work.
>
> Would be OK for me. So your suggestion then is to just have the command
> line flag -p and in case it's given or an according config file value is
> set (with the command line argument having priority), then the command
> is executed, regardless of any progress interface subscribers?

Exactly. It is project specific. For projects having a "master
controller", the -p parameter is not passed and SWUpdate does not start
any action. For projects as you have without any external controller,
the -p informs SWUpdate to do the job.

Both cases at the same time makes no sense: we have a controller or not,
and if we have, we do not want that SWUpdate thinks to be too smart and
takes decisions on its own. We can drop the CONFIG_ option

>
>
>>> Suricatta has been adapted to use this new method instead of
>>> unconditionally executing a reboot.
>>
>> suricatta *cannot* - after splitting into processes, suricatta and all
>> other processes has no right anymore to run reboot.
>>
>> So this cannot work.
>
> If you start swupdate as root and you don't explicitly configure them to
> run as a different user than root then they're run as root and can
> execute this command. Granted, they shouldn't run as root in the first
> place for the reasons you mentioned...

Anyway, it must works in all conditions and not just if everything is
running under root. I am now imagining SWUpdate as:

- the installer: this is the main process and it runs (presumably) as
root. Of course, a different user can be set, but all accesses to
hardware must be granted. In any case, this is the process with higher
privileges.

- the network processes: they must not run as root, and a project can
decide the rights. They should be able to run with less privileges,
enough for the communication they are thought.
That means: SWUpdate does not make further checks. If the Webserver does
not listen from port 8080 (default), but from a privileged port (<
1024), it is duty of the developer to ensure this with the correct
rights. SWUpdate will stop simply because bind() is not successful.

- if a network process requires a service with higher privileges, it
must inform the installer about what it wants. And the installer can
even refuse to do it.

What I mean is: this address a different issue as " core: post-update
command execution" in the commit message. It has nothing to do with it,
and it is better explained in the git history if a separate patch just
fix it.

I have nothing against fixing this in SWUpdate. But it would be nice, as
this is related to Hawkbit, if the Hawkbit's version (0.2M2 ?) is
mentioned in the commit message, so that we can easy track the issue
from the git history.

>
> For the time being, it is handled here so that -- if one has state
> support enabled -- the re-installation of the very same update artifact
> is prevented until swupdate is restarted or a reboot has taken place,
> effectively resulting in a restart of swupdate.
> In case no state support is available, the very same update is installed
> again and again as hawkBit announces this on the next polling interval
> as to be installed and there's (yet) no way to detect this and handle
> accordingly.
> See above in the doc/source/suricatta.rst part for this documented.

ok - just split the patch for this.

>
> The decision whether to acknowledge the update and hence avoid
> re-installing the same update again and again may be taken by an
> external agent that bi-directionally communicates with swupdate.

ok

>
>
>>> server_op_res_t server_handle_initial_state(update_state_t stateovrrd)
>>> {
>>> update_state_t state = STATE_OK;
>>> if (stateovrrd != STATE_NOT_AVAILABLE) {
>>> state = stateovrrd;
>>> TRACE("Got state=%c from command line.\n", state);
>>> + if (!is_state_valid(state)) {
>>> + return SERVER_EINIT;
>>> +
>>
>> Ditto.
>
> This is different: It's just a test if the STATE_ variables are valid,
> i.e., in the valid range from STATE_OK to STATE_ERROR.

Yes, it is different, that means this is a *third* patch. I want just to
avoid to have a "super-patch" fixing more issues at the same time,
making difficult to track them in the git history. I would like to have
1 patch == 1 issue, at least as much as possible.

>
>
>>> if (result == SERVER_OK) {
>>> - /* TODO Notify running applications of successful installation
>>> - * and a pending reboot via IPC and time-out wait. The
>>> - * reboot cannot be done in a post-install script as this
>>> - * is called immediately after installation, after which
>>> - * actions such as, e.g., setting the U-Boot environment
>>> - * are performed. */
>>> - INFO("Update successful, rebooting the system now.\n");
>>> - server_reboot();
>>> + INFO("Update successful, executing post-update commands.\n");
>>> + result = swupdate_postupdate() == 0 ? SERVER_OK : SERVER_EERR;
>>
>> This does not work, as I have explained.
>
> Then swupdate_postupdate() should incorporate some IPC to some swupdate
> process that does have the appropriate rights to execute the post-update
> command.

Exactly !

> Currently, this is the installer.

Right. Just the installer in the SWUpdate multi-process environment has
the right to do this.

> This IPC must also have a
> means to report back failure or success.
>
>
>
> What do you suggest on how to proceed from here?
>

My idea before this patch is to extend the IPC to have some more
commands. The IPC message is already prepared: after the magic, there is
a field for the message type. Up now, just "INST" and "GET STATUS" were
necessary, and it looks like we need something more. At least a "POST
Update " command.