Re: [PATCH v2 1/3] globalvar: suppress nvvar_save when no external environment was loaded

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Dec 12, 2025 at 02:25:54PM +0100, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 12/12/25 10:37 AM, Sascha Hauer wrote:
> > On Thu, Dec 11, 2025 at 09:48:10PM +0100, Ahmad Fatoum wrote:
> >> nvvar_save will load the extenral environment before writing it back
> >> with nv changed, which we means we still end up parsing the environment
> >> in this case, even if we don't execute init scripts or import nv out of
> >> it.
> >>
> >> Fix this to only parse the environment when we actually loaded it
> >> before.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  common/environment.c | 7 +++++++
> >>  common/globalvar.c   | 8 +++++++-
> >>  include/globalvar.h  | 1 +
> >>  3 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/common/environment.c b/common/environment.c
> >> index 0e551c90352e..ec14d0629a14 100644
> >> --- a/common/environment.c
> >> +++ b/common/environment.c
> >> @@ -453,6 +453,7 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
> >>  	int envfd;
> >>  	int ret = 0;
> >>  	size_t size, rsize;
> >> +	__maybe_unused const char *defenv_path;
> >>  
> >>  #ifdef __BAREBOX__
> >>  	if (!IS_ALLOWED(SCONFIG_ENVIRONMENT_LOAD))
> >> @@ -531,6 +532,12 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
> >>  
> >>  	ret = 0;
> >>  
> >> +#ifdef CONFIG_NVVAR
> >> +	defenv_path = default_environment_path_get();
> >> +	if (defenv_path && !strcmp(filename, defenv_path))
> >> +	    nv_var_set_persistable();
> >> +#endif
> >> +
> >>  out:
> >>  	close(envfd);
> >>  	free(buf);
> >> diff --git a/common/globalvar.c b/common/globalvar.c
> >> index 77af6733a6a0..1e06fb43775f 100644
> >> --- a/common/globalvar.c
> >> +++ b/common/globalvar.c
> >> @@ -15,6 +15,7 @@
> >>  #include <fnmatch.h>
> >>  
> >>  static int nv_dirty;
> >> +static bool nv_persistable;
> >>  
> >>  struct device global_device = {
> >>  	.name = "global",
> >> @@ -31,6 +32,11 @@ void nv_var_set_clean(void)
> >>  	nv_dirty = 0;
> >>  }
> >>  
> >> +void nv_var_set_persistable(void)
> >> +{
> >> +	nv_persistable = true;
> >> +}
> >> +
> >>  void globalvar_remove(const char *name)
> >>  {
> >>  	struct param_d *p, *tmp;
> >> @@ -713,7 +719,7 @@ int nvvar_save(void)
> >>  	const char *env = default_environment_path_get();
> >>  	int ret = 0;
> >>  #define TMPDIR "/.env.tmp"
> >> -	if (!nv_dirty || !env)
> >> +	if (!nv_dirty || !env || !nv_persistable)
> >>  		return 0;
> > 
> > With this "nv -s" or whatever calls this just silently does nothing.
> > This doesn't sound like a desired behaviour. At least a message would be
> > useful.
> > 
> > What's the purpose of this patch anyway?
> 
> In a later commit, we skip envfs_load if autoload_external_env() is
> disabled. I thought that it's strange for nv -s to still load the
> external environment to write variables into it.

Ok. I think a message for this would be good.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |