diff mbox series

Drop unneeded wrapper save_state_string

Message ID 20201223160119.825848-1-sbabic@denx.de
State Accepted
Headers show
Series Drop unneeded wrapper save_state_string | expand

Commit Message

Stefano Babic Dec. 23, 2020, 4:01 p.m. UTC
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(-)

Comments

Christian Storm Jan. 24, 2021, 7:29 p.m. UTC | #1
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
Stefano Babic Jan. 24, 2021, 7:36 p.m. UTC | #2
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
Christian Storm Jan. 26, 2021, 10:18 a.m. UTC | #3
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
Stefano Babic Jan. 26, 2021, 10:19 a.m. UTC | #4
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 mbox series

Patch

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: