| Message ID | 20201223160119.825848-1-sbabic@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Drop unneeded wrapper save_state_string | expand |
Hi Stefano, > This function is used just in one place to set the transaction flag. It > was thought to be generic, but the state is already saved using > set_/get_state, and this function is just a wrapper for bootloader > accessors, doing also some dangerous type conversion. This patch cleans > this up and calls directly the functions to access the bootloader's > environment. > > [...] > > -int unset_state(char *key) > -{ > - CHECK_STATE_VAR(key); > - return bootloader_env_unset(key); > -} > - Hm, can you explain why you also removed unset_state()? We have only save_state() and get_state() left in include/state.h now. Calling bootloader_env_unset() like in core/stream_interface.c breaks the (persistent) state abstraction by directly calling the bootloader implementation. So, I think either unset_state() needs to be reintroduced or, for consistency, the whole state abstraction be removed? Kind regards, Christian
Hi Christian, On 24.01.21 20:29, Christian Storm wrote: > Hi Stefano, > >> This function is used just in one place to set the transaction flag. It >> was thought to be generic, but the state is already saved using >> set_/get_state, and this function is just a wrapper for bootloader >> accessors, doing also some dangerous type conversion. This patch cleans >> this up and calls directly the functions to access the bootloader's >> environment. >> >> [...] >> >> -int unset_state(char *key) >> -{ >> - CHECK_STATE_VAR(key); >> - return bootloader_env_unset(key); >> -} >> - > > Hm, can you explain why you also removed unset_state()? > It was dead code. Nevertheless, it was a wrapper to set the transaction marker (the "recovery" variable), that is unrelated to the state. Even to remove double conversion to strings, setting the marker is done by calling the bootloader function. > We have only save_state() and get_state() left in include/state.h now. Right, but we have currently just these use cases. unset() was unused. > > Calling bootloader_env_unset() like in core/stream_interface.c > breaks the (persistent) state abstraction by directly calling the > bootloader implementation. Well, the use case is to set a marker in the bootloader environment. > So, I think either unset_state() needs to be reintroduced or, for > consistency, the whole state abstraction be removed? The state abstraction is used to set the state (often the variable "ustate") independently from the bootloader. The unset() was just used for the marker and not for the state. If it will be in future a persistent state outside the bootloader environment, I do not know, but I do not see reasons to remove the abstractions. Regards, Stefano
Hi Stefano, > > > This function is used just in one place to set the transaction flag. It > > > was thought to be generic, but the state is already saved using > > > set_/get_state, and this function is just a wrapper for bootloader > > > accessors, doing also some dangerous type conversion. This patch cleans > > > this up and calls directly the functions to access the bootloader's > > > environment. > > > > > > [...] > > > > > > -int unset_state(char *key) > > > -{ > > > - CHECK_STATE_VAR(key); > > > - return bootloader_env_unset(key); > > > -} > > > - > > > > Hm, can you explain why you also removed unset_state()? > > > > It was dead code. > > Nevertheless, it was a wrapper to set the transaction marker (the "recovery" > variable), that is unrelated to the state. Even to remove double conversion to > strings, setting the marker is done by calling the bootloader function. Thanks! I was a bit confused by the commit message not mentioning the dead code removal ― although it does make sense here with your explanations... Sorry for the noise :) Kind regards, Christian
Hi Christian, On 26.01.21 11:18, Christian Storm wrote: > Hi Stefano, > >>>> This function is used just in one place to set the transaction flag. It >>>> was thought to be generic, but the state is already saved using >>>> set_/get_state, and this function is just a wrapper for bootloader >>>> accessors, doing also some dangerous type conversion. This patch cleans >>>> this up and calls directly the functions to access the bootloader's >>>> environment. >>>> >>>> [...] >>>> >>>> -int unset_state(char *key) >>>> -{ >>>> - CHECK_STATE_VAR(key); >>>> - return bootloader_env_unset(key); >>>> -} >>>> - >>> >>> Hm, can you explain why you also removed unset_state()? >>> >> >> It was dead code. >> >> Nevertheless, it was a wrapper to set the transaction marker (the "recovery" >> variable), that is unrelated to the state. Even to remove double conversion to >> strings, setting the marker is done by calling the bootloader function. > > > Thanks! I was a bit confused by the commit message not mentioning the > dead code removal ― although it does make sense here with your > explanations... Sorry for the noise :) No problem - we can reintegrate an unset() function at any moment if we will have a use case. Regards, Stefano > > > Kind regards, > Christian >
diff --git a/core/state.c b/core/state.c index 694290a..fe415ea 100644 --- a/core/state.c +++ b/core/state.c @@ -58,17 +58,6 @@ int save_state(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)); -} - static update_state_t read_state(char *key) { CHECK_STATE_VAR(key); @@ -88,12 +77,6 @@ static update_state_t read_state(char *key) return val; } -int unset_state(char *key) -{ - CHECK_STATE_VAR(key); - return bootloader_env_unset(key); -} - static update_state_t do_get_state(void) { update_state_t state = read_state((char *)STATE_KEY); diff --git a/core/stream_interface.c b/core/stream_interface.c index c5073fd..7d481bc 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -44,6 +44,7 @@ #include "progress.h" #include "pctl.h" #include "state.h" +#include "bootloader.h" #define BUFF_SIZE 4096 #define PERCENT_LB_INDEX 4 @@ -250,7 +251,7 @@ static int extract_files(int fd, struct swupdate_cfg *software) */ if (!installed_directly) { if (!software->globals.dry_run && software->bootloader_transaction_marker) { - save_state_string((char*)BOOTVAR_TRANSACTION, STATE_IN_PROGRESS); + bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_IN_PROGRESS)); } installed_directly = true; } @@ -583,14 +584,14 @@ void *network_initializer(void *data) * initiated an update */ if (!software->globals.dry_run && software->bootloader_transaction_marker) { - save_state_string((char*)BOOTVAR_TRANSACTION, STATE_IN_PROGRESS); + bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_IN_PROGRESS)); } notify(RUN, RECOVERY_NO_ERROR, INFOLEVEL, "Installation in progress"); ret = install_images(software); if (ret != 0) { if (!software->globals.dry_run && software->bootloader_transaction_marker) { - save_state_string((char*)BOOTVAR_TRANSACTION, STATE_FAILED); + bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_FAILED)); } notify(FAILURE, RECOVERY_ERROR, ERRORLEVEL, "Installation failed !"); inst.last_install = FAILURE; @@ -603,7 +604,7 @@ void *network_initializer(void *data) * that it is not required to start recovery again */ if (!software->globals.dry_run && software->bootloader_transaction_marker) { - unset_state((char*)BOOTVAR_TRANSACTION); + bootloader_env_unset(BOOTVAR_TRANSACTION); } if (!software->globals.dry_run && save_state((char *)STATE_KEY, STATE_INSTALLED) != SERVER_OK) { ERROR("Cannot persistently store INSTALLED update state."); diff --git a/core/swupdate.c b/core/swupdate.c index 318a245..1987063 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -47,6 +47,7 @@ #include "swupdate_settings.h" #include "pctl.h" #include "state.h" +#include "bootloader.h" #ifdef CONFIG_SYSTEMD #include <systemd/sd-daemon.h> @@ -868,7 +869,7 @@ int main(int argc, char **argv) switch (result) { case EXIT_FAILURE: if (!swcfg.globals.dry_run && swcfg.bootloader_transaction_marker) { - save_state_string((char*)BOOTVAR_TRANSACTION, STATE_FAILED); + bootloader_env_set(BOOTVAR_TRANSACTION, get_state_string(STATE_FAILED)); } break; case EXIT_SUCCESS:
This function is used just in one place to set the transaction flag. It was thought to be generic, but the state is already saved using set_/get_state, and this function is just a wrapper for bootloader accessors, doing also some dangerous type conversion. This patch cleans this up and calls directly the functions to access the bootloader's environment. Signed-off-by: Stefano Babic <sbabic@denx.de> --- core/state.c | 17 ----------------- core/stream_interface.c | 9 +++++---- core/swupdate.c | 3 ++- 3 files changed, 7 insertions(+), 22 deletions(-)