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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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.

Cheers,
Ahmad

> 
> 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 |