diff mbox series

Make access to bootloader state exclusive also for GET and UNSET

Message ID 20201210215218.646959-1-sava.jakovljev@teufel.de
State Changes Requested
Headers show
Series Make access to bootloader state exclusive also for GET and UNSET | expand

Commit Message

Sava Jakovljev Dec. 10, 2020, 9:52 p.m. UTC 
* Introduce IPC calls GET_UPDATE_STATE and UNSET_UPDATE_STATE.
* Use plain Linux-style return values from function and keep things simple.

Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de>
---
When RAM bootloader environment is used, get_state() from subprocesses 
like Suricatta is broken, since they will have their own copy of the 
dictionary structure (defined in bootloader/none.c). 
Semantically the main process is owning the state, thus introducing these
new IPC calls fit to that design.
Tests are updated accordingly. 

Also, return values of the functions are simplified. 


 core/network_thread.c      | 13 +++++-
 core/state.c               | 86 ++++++++++++++++++++++++++------------
 include/network_ipc.h      |  6 ++-
 include/state.h            |  7 ++--
 test/test_server_hawkbit.c | 33 ++++++---------
 5 files changed, 90 insertions(+), 55 deletions(-)
diff mbox series

Patch

diff --git a/core/network_thread.c b/core/network_thread.c
index 0e6080d..ae4eb24 100644
--- a/core/network_thread.c
+++ b/core/network_thread.c
@@ -87,9 +87,9 @@  static bool is_selection_allowed(const char *software_set, char *running_mode,
 
 	if (allowed) {
 		INFO("Accepted selection %s,%s", software_set, running_mode);
-	}else 
+	}else
 		ERROR("Selection %s,%s is not allowed, rejected !",
-		      software_set, running_mode); 
+		      software_set, running_mode);
 	return allowed;
 }
 
@@ -445,6 +445,15 @@  void *network_thread (void *data)
 					       ? ACK
 					       : NACK;
 				break;
+			case GET_UPDATE_STATE:
+				msg.data.msg[0] = get_state();
+				msg.type = ACK;
+				break;
+			case UNSET_UPDATE_STATE:
+				msg.type = unset_state(msg.data.msg[0]) == 0 ?
+					ACK : NACK;
+
+				break;
 			default:
 				msg.type = NACK;
 			}
diff --git a/core/state.c b/core/state.c
index 8ddd760..74c1d55 100644
--- a/core/state.c
+++ b/core/state.c
@@ -30,19 +30,19 @@ 
 	} \
 } while(0)
 
-static server_op_res_t do_save_state(char *key, char* value)
+static int do_save_state(char *key, char* value)
 {
-	char c;
 	CHECK_STATE_VAR(key);
 	if (!value)
 		return -EINVAL;
-	c = *value;
+	char c = *value;
 	if (c < STATE_OK || c > STATE_LAST)
 		return -EINVAL;
-	return bootloader_env_set(key, value) == 0 ? SERVER_OK : SERVER_EERR;
+
+	return bootloader_env_set(key, value);
 }
 
-server_op_res_t save_state(char *key, update_state_t value)
+int save_state(char *key, update_state_t value)
 {
 	char value_str[2] = {value, '\0'};
 	ipc_message msg;
@@ -52,64 +52,96 @@  server_op_res_t save_state(char *key, update_state_t value)
 		msg.type = SET_UPDATE_STATE;
 		msg.data.msg[0] = (char)value;
 		return (ipc_send_cmd(&msg));
-	} else { /* Main process */
+	} else {
+		/* Main process */
 		return do_save_state(key, value_str);
 	}
 }
 
 
-server_op_res_t save_state_string(char *key, update_state_t value)
+int save_state_string(char *key, update_state_t value)
 {
 	CHECK_STATE_VAR(key);
 	if (!value)
 		return -EINVAL;
 	if (value < STATE_OK || value > STATE_LAST)
 		return -EINVAL;
-	return bootloader_env_set(key, get_state_string(value)) == 0 ?
-		SERVER_OK : SERVER_EERR;
+	return bootloader_env_set(key, get_state_string(value));
 }
 
-server_op_res_t read_state(char *key, update_state_t *value)
+static update_state_t read_state(char *key)
 {
-	char *envval;
 	CHECK_STATE_VAR(key);
 
-	envval = bootloader_env_get(key);
+	char *envval = bootloader_env_get(key);
 	if (envval == NULL) {
 		INFO("Key '%s' not found in Bootloader's environment.", key);
-		*value = STATE_NOT_AVAILABLE;
-		return SERVER_OK;
+		return STATE_NOT_AVAILABLE;
 	}
 	/* TODO It's a bit whacky just to cast this but as we're the only */
 	/*      ones touching the variable, it's maybe OK for a PoC now. */
-	*value = (update_state_t)*envval;
 
+	update_state_t val = (update_state_t)*envval;
 	/* bootloader get env allocates space for the value */
 	free(envval);
 
-	return SERVER_OK;
+	return val;
 }
-server_op_res_t unset_state(char *key)
-{
-	int ret;
 
+static int do_unset_state(char *key)
+{
 	CHECK_STATE_VAR(key);
-	ret = bootloader_env_unset(key);
-	return ret == 0 ? SERVER_OK : SERVER_EERR;
+	return bootloader_env_unset(key);
 }
 
-update_state_t get_state(void) {
-	update_state_t state;
+int unset_state(char *key)
+{
+	if (pid == getpid())
+	{
+		ipc_message msg;
+		memset(&msg, 0, sizeof(msg));
+
+		msg.type = UNSET_UPDATE_STATE;
+		return ipc_send_cmd(&msg) == 0 && msg.type == ACK;
+	} else {
+		// Main process
+		return do_unset_state(key);
+	}
+}
+
+static update_state_t do_get_state(void) {
+	update_state_t state = read_state((char *)STATE_KEY);
 
-	if (read_state((char *)STATE_KEY, &state) != SERVER_OK) {
+	if (state == STATE_NOT_AVAILABLE) {
 		ERROR("Cannot read stored update state.");
-		return STATE_ERROR;
+		return STATE_NOT_AVAILABLE;
 	}
-	TRACE("Read state=%c from persistent storage.", state);
 
 	if (is_valid_state(state)) {
+		TRACE("Read state=%c from persistent storage.", state);
 		return state;
 	}
+
 	ERROR("Unknown update state=%c", state);
-	return STATE_ERROR;
+	return STATE_NOT_AVAILABLE;
+}
+
+update_state_t get_state(void) {
+	if (pid == getpid())
+	{
+		ipc_message msg;
+		memset(&msg, 0, sizeof(msg));
+
+		msg.type = GET_UPDATE_STATE;
+
+		if (ipc_send_cmd(&msg) || msg.type == NACK) {
+			ERROR("Failed to get current bootloader update state.");
+			return STATE_NOT_AVAILABLE;
+		}
+
+		return (update_state_t)msg.data.msg[0];
+	} else {
+		// Main process
+		return do_get_state();
+	}
 }
diff --git a/include/network_ipc.h b/include/network_ipc.h
index 22ecb9a..34bc43d 100644
--- a/include/network_ipc.h
+++ b/include/network_ipc.h
@@ -35,6 +35,8 @@  typedef enum {
 	SWUPDATE_SUBPROCESS,
 	SET_AES_KEY,
 	SET_UPDATE_STATE,	/* set bootloader ustate */
+	GET_UPDATE_STATE,
+	UNSET_UPDATE_STATE,
 	REQ_INSTALL_EXT
 } msgtype;
 
@@ -71,7 +73,7 @@  struct swupdate_request {
 
 typedef union {
 	char msg[128];
-	struct { 
+	struct {
 		int current;
 		int last_result;
 		int error;
@@ -100,7 +102,7 @@  typedef union {
 		char ivt_ascii[33]; /* Key size in ASCII (16 bytes bin) + termination */
 	} aeskeymsg;
 } msgdata;
-	
+
 typedef struct {
 	int magic;	/* magic number */
 	int type;
diff --git a/include/state.h b/include/state.h
index f006b6c..cb9a947 100644
--- a/include/state.h
+++ b/include/state.h
@@ -63,8 +63,7 @@  static inline char* get_state_string(update_state_t state) {
 	return (char*)"<nil>";
 }
 
-server_op_res_t save_state(char *key, update_state_t value);
-server_op_res_t save_state_string(char *key, update_state_t value);
-server_op_res_t read_state(char *key, update_state_t *value);
-server_op_res_t unset_state(char *key);
+int save_state(char *key, update_state_t value);
+int save_state_string(char *key, update_state_t value);
+int unset_state(char *key);
 update_state_t get_state(void);
diff --git a/test/test_server_hawkbit.c b/test/test_server_hawkbit.c
index 9c7c128..03c5475 100644
--- a/test/test_server_hawkbit.c
+++ b/test/test_server_hawkbit.c
@@ -115,22 +115,20 @@  channel_op_res_t __wrap_channel_get(channel_t *this, void *data)
 	return mock_type(channel_op_res_t);
 }
 
-extern server_op_res_t __real_save_state(char *key, update_state_t value);
-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)
+extern int  __real_save_state(char *key, update_state_t value);
+int __wrap_save_state(char *key, update_state_t *value);
+int __wrap_save_state(char *key, update_state_t *value)
 {
 	(void)key;
 	(void)value;
-	return mock_type(server_op_res_t);
+	return mock_type(int);
 }
 
-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)
+extern update_state_t __real_get_state(void);
+update_state_t __wrap_get_state(void);
+update_state_t __wrap_get_state(void)
 {
-	(void)key;
-	*value = mock_type(update_state_t);
-	return mock_type(server_op_res_t);
+	return mock_type(update_state_t);
 }
 
 extern server_op_res_t server_has_pending_action(int *action_id);
@@ -218,14 +216,11 @@  static void test_server_has_pending_action(void **state)
 	will_return(__wrap_channel_get,
 		    json_tokener_parse(json_reply_update_data));
 	will_return(__wrap_channel_get, CHANNEL_OK);
-#if 0
-	will_return(__wrap_read_state, STATE_NOT_AVAILABLE);
-	will_return(__wrap_read_state, SERVER_OK);
-#endif
+
+	will_return(__wrap_get_state, STATE_NOT_AVAILABLE);
 	assert_int_equal(SERVER_UPDATE_AVAILABLE,
 			 server_has_pending_action(&action_id));
 
-#if 0
 	/* Test Case: Update Action available && STATE_INSTALLED. */
 	will_return(__wrap_channel_get,
 		    json_tokener_parse(json_reply_update_available));
@@ -233,11 +228,9 @@  static void test_server_has_pending_action(void **state)
 	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);
+	will_return(__wrap_get_state, STATE_INSTALLED);
 	assert_int_equal(SERVER_NO_UPDATE_AVAILABLE,
 			 server_has_pending_action(&action_id));
-#endif
 
 	/* Test Case: Cancel Action available. */
 	will_return(__wrap_channel_get,
@@ -247,7 +240,7 @@  static void test_server_has_pending_action(void **state)
 		    json_tokener_parse(json_reply_cancel_data));
 	will_return(__wrap_channel_get, CHANNEL_OK);
 	will_return(__wrap_channel_put, CHANNEL_OK);
-	will_return(__wrap_save_state, SERVER_OK);
+	will_return(__wrap_save_state, 0);
 	assert_int_equal(SERVER_OK, server_has_pending_action(&action_id));
 }
 
@@ -542,7 +535,7 @@  static void test_server_install_update(void **state)
 	will_return(__wrap_channel_get_file, CHANNEL_OK);
 	will_return(__wrap_ipc_wait_for_complete, SUCCESS);
 	will_return(__wrap_channel_put, CHANNEL_OK);
-	//will_return(__wrap_save_state, SERVER_OK);
+	//will_return(__wrap_save_state, 0);
 	will_return(__wrap_channel_put, CHANNEL_OK);
 	assert_int_equal(SERVER_OK, server_install_update());
 }