| Message ID | 20201215134511.215576-1-sava.jakovljev@teufel.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] Make access to bootloader state exclusive also for GET_UPDATE_STATE | expand |
Hi Sava, On 15.12.20 14:45, Sava Jakovljev wrote: > * Introduce IPC call GET_UPDATE_STATE. > * Use plain Linux-style return values from function and keep things simple. > * Adjust tests for Suricatta. > > 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 call is in line with that design. > Tests are updated accordingly. > > Also, return values of the functions are simplified. > > v2 of this patch removes UNSET IPC call, since there's there's no need > for subprocesses to use it, and because the v1 of this patch implemented > UNSET IPC in a bad way. > Ok > save_state() could also be made better by removing key argument, since the > current implementation offers it, but the main process always uses STATE_KEY. > This is a not a subject of this patch, and it should be handeled separately. > > Best regards, > Sava Jakovljev > > > core/network_thread.c | 4 +++ > core/state.c | 71 +++++++++++++++++++++++--------------- > include/network_ipc.h | 1 + > include/state.h | 7 ++-- > test/test_server_hawkbit.c | 30 ++++++---------- > 5 files changed, 63 insertions(+), 50 deletions(-) > > diff --git a/core/network_thread.c b/core/network_thread.c > index bc9a7f8..534e5d0 100644 > --- a/core/network_thread.c > +++ b/core/network_thread.c > @@ -515,6 +515,10 @@ void *network_thread (void *data) > ? ACK > : NACK; > break; > + case GET_UPDATE_STATE: > + msg.data.msg[0] = get_state(); > + msg.type = ACK; > + break; > default: > msg.type = NACK; > } > diff --git a/core/state.c b/core/state.c > index 8ddd760..694290a 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,81 @@ 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; > But if unset() is removed, what is this, and why the prototype is added to the header ? > +int 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; > +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..d3d2694 100644 > --- a/include/network_ipc.h > +++ b/include/network_ipc.h > @@ -35,6 +35,7 @@ typedef enum { > SWUPDATE_SUBPROCESS, > SET_AES_KEY, > SET_UPDATE_STATE, /* set bootloader ustate */ > + GET_UPDATE_STATE, > REQ_INSTALL_EXT > } msgtype; > > 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..b26d6a9 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,10 @@ 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 +227,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 +239,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)); > } > > Bets regards, Stefano Babic
Hi Stefano, Thank you for your comments. Maybe I was not clear enough - what I meant was, unset_state IPC was removed from v1 of this patch. Comparing to the original implementation, unset_state was only adjusted for consistency reasons, but it was not removed. The prototype changed: -server_op_res_t unset_state(char *key); +int unset_state(char *key); And the implementation was a bit cleaned up. We can discuss about how to handle this logic - as far as I have seen, unset_state is not called by subprocesses and it probably should not be used from subprocesses. Maybe we need to simply return an error when called from subprocess? Please accept my apologizes for not being clear enough. Thank you. Best regards, Sava Jakovljev Stefano Babic schrieb am Sonntag, 20. Dezember 2020 um 12:43:57 UTC+1: > Hi Sava, > > On 15.12.20 14:45, Sava Jakovljev wrote: > > * Introduce IPC call GET_UPDATE_STATE. > > * Use plain Linux-style return values from function and keep things > simple. > > * Adjust tests for Suricatta. > > > > Signed-off-by: Sava Jakovljev <sava.ja...@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 call is in line with that design. > > Tests are updated accordingly. > > > > Also, return values of the functions are simplified. > > > > v2 of this patch removes UNSET IPC call, since there's there's no need > > for subprocesses to use it, and because the v1 of this patch implemented > > UNSET IPC in a bad way. > > > > Ok > > > save_state() could also be made better by removing key argument, since > the > > current implementation offers it, but the main process always uses > STATE_KEY. > > This is a not a subject of this patch, and it should be handeled > separately. > > > > Best regards, > > Sava Jakovljev > > > > > > core/network_thread.c | 4 +++ > > core/state.c | 71 +++++++++++++++++++++++--------------- > > include/network_ipc.h | 1 + > > include/state.h | 7 ++-- > > test/test_server_hawkbit.c | 30 ++++++---------- > > 5 files changed, 63 insertions(+), 50 deletions(-) > > > > diff --git a/core/network_thread.c b/core/network_thread.c > > index bc9a7f8..534e5d0 100644 > > --- a/core/network_thread.c > > +++ b/core/network_thread.c > > @@ -515,6 +515,10 @@ void *network_thread (void *data) > > ? ACK > > : NACK; > > break; > > + case GET_UPDATE_STATE: > > + msg.data.msg[0] = get_state(); > > + msg.type = ACK; > > + break; > > default: > > msg.type = NACK; > > } > > diff --git a/core/state.c b/core/state.c > > index 8ddd760..694290a 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,81 @@ 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; > > > > But if unset() is removed, what is this, and why the prototype is added > to the header ? > > > +int 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; > > +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..d3d2694 100644 > > --- a/include/network_ipc.h > > +++ b/include/network_ipc.h > > @@ -35,6 +35,7 @@ typedef enum { > > SWUPDATE_SUBPROCESS, > > SET_AES_KEY, > > SET_UPDATE_STATE, /* set bootloader ustate */ > > + GET_UPDATE_STATE, > > REQ_INSTALL_EXT > > } msgtype; > > > > 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..b26d6a9 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,10 @@ 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 +227,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 +239,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)); > > } > > > > > > Bets regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
On 15.12.20 14:45, Sava Jakovljev wrote: > * Introduce IPC call GET_UPDATE_STATE. > * Use plain Linux-style return values from function and keep things simple. > * Adjust tests for Suricatta. > > 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 call is in line with that design. > Tests are updated accordingly. > > Also, return values of the functions are simplified. > > v2 of this patch removes UNSET IPC call, since there's there's no need > for subprocesses to use it, and because the v1 of this patch implemented > UNSET IPC in a bad way. > > save_state() could also be made better by removing key argument, since the > current implementation offers it, but the main process always uses STATE_KEY. > This is a not a subject of this patch, and it should be handeled separately. > > Best regards, > Sava Jakovljev > > > core/network_thread.c | 4 +++ > core/state.c | 71 +++++++++++++++++++++++--------------- > include/network_ipc.h | 1 + > include/state.h | 7 ++-- > test/test_server_hawkbit.c | 30 ++++++---------- > 5 files changed, 63 insertions(+), 50 deletions(-) > > diff --git a/core/network_thread.c b/core/network_thread.c > index bc9a7f8..534e5d0 100644 > --- a/core/network_thread.c > +++ b/core/network_thread.c > @@ -515,6 +515,10 @@ void *network_thread (void *data) > ? ACK > : NACK; > break; > + case GET_UPDATE_STATE: > + msg.data.msg[0] = get_state(); > + msg.type = ACK; > + break; > default: > msg.type = NACK; > } > diff --git a/core/state.c b/core/state.c > index 8ddd760..694290a 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,81 @@ 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; > > +int 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; > +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..d3d2694 100644 > --- a/include/network_ipc.h > +++ b/include/network_ipc.h > @@ -35,6 +35,7 @@ typedef enum { > SWUPDATE_SUBPROCESS, > SET_AES_KEY, > SET_UPDATE_STATE, /* set bootloader ustate */ > + GET_UPDATE_STATE, > REQ_INSTALL_EXT > } msgtype; > > 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..b26d6a9 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,10 @@ 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 +227,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 +239,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)); > } > > Applied to -master, thanks ! Best regards, Stefano Babic
diff --git a/core/network_thread.c b/core/network_thread.c index bc9a7f8..534e5d0 100644 --- a/core/network_thread.c +++ b/core/network_thread.c @@ -515,6 +515,10 @@ void *network_thread (void *data) ? ACK : NACK; break; + case GET_UPDATE_STATE: + msg.data.msg[0] = get_state(); + msg.type = ACK; + break; default: msg.type = NACK; } diff --git a/core/state.c b/core/state.c index 8ddd760..694290a 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,81 @@ 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; +int 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; +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..d3d2694 100644 --- a/include/network_ipc.h +++ b/include/network_ipc.h @@ -35,6 +35,7 @@ typedef enum { SWUPDATE_SUBPROCESS, SET_AES_KEY, SET_UPDATE_STATE, /* set bootloader ustate */ + GET_UPDATE_STATE, REQ_INSTALL_EXT } msgtype; 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..b26d6a9 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,10 @@ 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 +227,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 +239,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)); }
* Introduce IPC call GET_UPDATE_STATE. * Use plain Linux-style return values from function and keep things simple. * Adjust tests for Suricatta. 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 call is in line with that design. Tests are updated accordingly. Also, return values of the functions are simplified. v2 of this patch removes UNSET IPC call, since there's there's no need for subprocesses to use it, and because the v1 of this patch implemented UNSET IPC in a bad way. save_state() could also be made better by removing key argument, since the current implementation offers it, but the main process always uses STATE_KEY. This is a not a subject of this patch, and it should be handeled separately. Best regards, Sava Jakovljev core/network_thread.c | 4 +++ core/state.c | 71 +++++++++++++++++++++++--------------- include/network_ipc.h | 1 + include/state.h | 7 ++-- test/test_server_hawkbit.c | 30 ++++++---------- 5 files changed, 63 insertions(+), 50 deletions(-)