| Message ID | 20210126180823.44900-1-christian.storm@siemens.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | state: Make save_state() write to STATE_KEY unconditionally | expand |
Hi Stefano, > Analogously to get_state() unconditionally reading STATE_KEY, make > save_state() unconditionally write to STATE_KEY, for symmetry and > consistency. Writing (and reading) other bootloader environment > variables (as this is the only persistent state backing store > currently) is still possible directly via bootloader.h's methods. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> Any inputs/thoughts on this patch? Kind regards, Christian
On 09.03.21 09:50, Christian Storm wrote: > Hi Stefano, > >> Analogously to get_state() unconditionally reading STATE_KEY, make >> save_state() unconditionally write to STATE_KEY, for symmetry and >> consistency. Writing (and reading) other bootloader environment >> variables (as this is the only persistent state backing store >> currently) is still possible directly via bootloader.h's methods. >> >> Signed-off-by: Christian Storm <christian.storm@siemens.com> > > Any inputs/thoughts on this patch? Ouch..I have completely forgotten this. I review your patch, sorry for delay. Regards, Stefano
Hi Stefano, > > > Analogously to get_state() unconditionally reading STATE_KEY, make > > > save_state() unconditionally write to STATE_KEY, for symmetry and > > > consistency. Writing (and reading) other bootloader environment > > > variables (as this is the only persistent state backing store > > > currently) is still possible directly via bootloader.h's methods. > > > > > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > > > Any inputs/thoughts on this patch? > > Ouch..I have completely forgotten this. I review your patch, sorry for delay. No worries, just wanted to clean my pending patch queue prior to sending new ones :) Kind regards, Christian
Hi Christian, On 26.01.21 19:08, Christian Storm wrote: > Analogously to get_state() unconditionally reading STATE_KEY, make > save_state() unconditionally write to STATE_KEY, for symmetry and > consistency. Writing (and reading) other bootloader environment > variables (as this is the only persistent state backing store > currently) is still possible directly via bootloader.h's methods. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > core/network_thread.c | 2 +- > core/state.c | 4 ++-- > core/stream_interface.c | 4 ++-- > include/state.h | 2 +- > suricatta/server_hawkbit.c | 6 +++--- > test/test_server_hawkbit.c | 7 +++---- > 6 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/core/network_thread.c b/core/network_thread.c > index 99fbaf9..21ab75c 100644 > --- a/core/network_thread.c > +++ b/core/network_thread.c > @@ -512,7 +512,7 @@ void *network_thread (void *data) > case SET_UPDATE_STATE: > value = *(update_state_t *)msg.data.msg; > msg.type = (is_valid_state(value) && > - save_state((char *)STATE_KEY, value) == SERVER_OK) > + save_state(value) == SERVER_OK) Yes, this makes sense - the variablle is part of the configuration, and save_state() should just get the value. Fully agree. > ? ACK > : NACK; > break; > diff --git a/core/state.c b/core/state.c > index fe415ea..14905fe 100644 > --- a/core/state.c > +++ b/core/state.c > @@ -42,7 +42,7 @@ static int do_save_state(char *key, char* value) > return bootloader_env_set(key, value); > } > > -int save_state(char *key, update_state_t value) > +int save_state(update_state_t value) > { > char value_str[2] = {value, '\0'}; > ipc_message msg; > @@ -54,7 +54,7 @@ int save_state(char *key, update_state_t value) > return (ipc_send_cmd(&msg)); > } else { > /* Main process */ > - return do_save_state(key, value_str); > + return do_save_state((char *)STATE_KEY, value_str); > } > } > > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 3907ade..558e857 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -605,7 +605,7 @@ void *network_initializer(void *data) > inst.last_install = FAILURE; > if (!software->globals.dry_run > && software->bootloader_state_marker > - && save_state((char *)STATE_KEY, STATE_FAILED) != SERVER_OK) { > + && save_state(STATE_FAILED) != SERVER_OK) { > WARN("Cannot persistently store FAILED update state."); > } > } else { > @@ -618,7 +618,7 @@ void *network_initializer(void *data) > } > if (!software->globals.dry_run > && software->bootloader_state_marker > - && save_state((char *)STATE_KEY, STATE_INSTALLED) != SERVER_OK) { > + && save_state(STATE_INSTALLED) != SERVER_OK) { > ERROR("Cannot persistently store INSTALLED update state."); > notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !"); > inst.last_install = FAILURE; > diff --git a/include/state.h b/include/state.h > index 4ddb607..5814653 100644 > --- a/include/state.h > +++ b/include/state.h > @@ -58,5 +58,5 @@ static inline char* get_state_string(update_state_t state) { > return (char*)"<nil>"; > } > > -int save_state(char *key, update_state_t value); > +int save_state(update_state_t value); > update_state_t get_state(void); > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > index df48180..86b5c8a 100644 > --- a/suricatta/server_hawkbit.c > +++ b/suricatta/server_hawkbit.c > @@ -740,7 +740,7 @@ server_op_res_t server_has_pending_action(int *action_id) > /* > * Save the state > */ > - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { > + if ((result = save_state(STATE_OK)) != SERVER_OK) { > ERROR("Error while resetting update state on persistent " > "storage.\n"); > } > @@ -891,7 +891,7 @@ server_op_res_t server_handle_initial_state(update_state_t stateovrrd) > /* NOTE (Re-)setting STATE_KEY=STATE_OK == '0' instead of deleting it > * as it may be required for the switchback/recovery U-Boot logics. > */ > - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { > + if ((result = save_state(STATE_OK)) != SERVER_OK) { > ERROR("Error while resetting update state on persistent " > "storage.\n"); > return result; > @@ -1945,7 +1945,7 @@ static server_op_res_t server_activation_ipc(ipc_message *msg) > /* > * Save the state > */ > - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { > + if ((result = save_state(STATE_OK)) != SERVER_OK) { > ERROR("Error while resetting update state on persistent " > "storage.\n"); > } > diff --git a/test/test_server_hawkbit.c b/test/test_server_hawkbit.c > index b26d6a9..61e20fd 100644 > --- a/test/test_server_hawkbit.c > +++ b/test/test_server_hawkbit.c > @@ -115,11 +115,10 @@ channel_op_res_t __wrap_channel_get(channel_t *this, void *data) > return mock_type(channel_op_res_t); > } > > -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) > +extern int __real_save_state(update_state_t value); > +int __wrap_save_state(update_state_t *value); > +int __wrap_save_state(update_state_t *value) > { > - (void)key; > (void)value; > return mock_type(int); > } > Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
diff --git a/core/network_thread.c b/core/network_thread.c index 99fbaf9..21ab75c 100644 --- a/core/network_thread.c +++ b/core/network_thread.c @@ -512,7 +512,7 @@ void *network_thread (void *data) case SET_UPDATE_STATE: value = *(update_state_t *)msg.data.msg; msg.type = (is_valid_state(value) && - save_state((char *)STATE_KEY, value) == SERVER_OK) + save_state(value) == SERVER_OK) ? ACK : NACK; break; diff --git a/core/state.c b/core/state.c index fe415ea..14905fe 100644 --- a/core/state.c +++ b/core/state.c @@ -42,7 +42,7 @@ static int do_save_state(char *key, char* value) return bootloader_env_set(key, value); } -int save_state(char *key, update_state_t value) +int save_state(update_state_t value) { char value_str[2] = {value, '\0'}; ipc_message msg; @@ -54,7 +54,7 @@ int save_state(char *key, update_state_t value) return (ipc_send_cmd(&msg)); } else { /* Main process */ - return do_save_state(key, value_str); + return do_save_state((char *)STATE_KEY, value_str); } } diff --git a/core/stream_interface.c b/core/stream_interface.c index 3907ade..558e857 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -605,7 +605,7 @@ void *network_initializer(void *data) inst.last_install = FAILURE; if (!software->globals.dry_run && software->bootloader_state_marker - && save_state((char *)STATE_KEY, STATE_FAILED) != SERVER_OK) { + && save_state(STATE_FAILED) != SERVER_OK) { WARN("Cannot persistently store FAILED update state."); } } else { @@ -618,7 +618,7 @@ void *network_initializer(void *data) } if (!software->globals.dry_run && software->bootloader_state_marker - && save_state((char *)STATE_KEY, STATE_INSTALLED) != SERVER_OK) { + && save_state(STATE_INSTALLED) != SERVER_OK) { ERROR("Cannot persistently store INSTALLED update state."); notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !"); inst.last_install = FAILURE; diff --git a/include/state.h b/include/state.h index 4ddb607..5814653 100644 --- a/include/state.h +++ b/include/state.h @@ -58,5 +58,5 @@ static inline char* get_state_string(update_state_t state) { return (char*)"<nil>"; } -int save_state(char *key, update_state_t value); +int save_state(update_state_t value); update_state_t get_state(void); diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index df48180..86b5c8a 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -740,7 +740,7 @@ server_op_res_t server_has_pending_action(int *action_id) /* * Save the state */ - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { + if ((result = save_state(STATE_OK)) != SERVER_OK) { ERROR("Error while resetting update state on persistent " "storage.\n"); } @@ -891,7 +891,7 @@ server_op_res_t server_handle_initial_state(update_state_t stateovrrd) /* NOTE (Re-)setting STATE_KEY=STATE_OK == '0' instead of deleting it * as it may be required for the switchback/recovery U-Boot logics. */ - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { + if ((result = save_state(STATE_OK)) != SERVER_OK) { ERROR("Error while resetting update state on persistent " "storage.\n"); return result; @@ -1945,7 +1945,7 @@ static server_op_res_t server_activation_ipc(ipc_message *msg) /* * Save the state */ - if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { + if ((result = save_state(STATE_OK)) != SERVER_OK) { ERROR("Error while resetting update state on persistent " "storage.\n"); } diff --git a/test/test_server_hawkbit.c b/test/test_server_hawkbit.c index b26d6a9..61e20fd 100644 --- a/test/test_server_hawkbit.c +++ b/test/test_server_hawkbit.c @@ -115,11 +115,10 @@ channel_op_res_t __wrap_channel_get(channel_t *this, void *data) return mock_type(channel_op_res_t); } -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) +extern int __real_save_state(update_state_t value); +int __wrap_save_state(update_state_t *value); +int __wrap_save_state(update_state_t *value) { - (void)key; (void)value; return mock_type(int); }
Analogously to get_state() unconditionally reading STATE_KEY, make save_state() unconditionally write to STATE_KEY, for symmetry and consistency. Writing (and reading) other bootloader environment variables (as this is the only persistent state backing store currently) is still possible directly via bootloader.h's methods. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- core/network_thread.c | 2 +- core/state.c | 4 ++-- core/stream_interface.c | 4 ++-- include/state.h | 2 +- suricatta/server_hawkbit.c | 6 +++--- test/test_server_hawkbit.c | 7 +++---- 6 files changed, 12 insertions(+), 13 deletions(-)