mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior
@ 2020-11-12 17:23 Ahmad Fatoum
  2020-11-12 17:23 ` [PATCH 2/2] setenv: align with POSIX in handling of setenv(var, "") Ahmad Fatoum
  2020-11-23 15:48 ` [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Sascha Hauer
  0 siblings, 2 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-11-12 17:23 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

Currently, we treat setenv(var, "") and setenv(var, NULL) the same
and delete var, which is surprising and leads to subtle quirks:

 - setenv(var, "") is specified by POSIX to set var to an empty string,
   but barebox uses it to delete variables

 - nv.user= calls nv_set with NULL parameter, but nv user="" doesn't

Make the API more POSIX-like by providing unsetenv with the expected
semantics. Most user code can then use unsetenv without worrying about
whether "" or NULL is the magic deletion value.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/readlink.c   | 2 +-
 commands/setenv.c     | 7 ++++++-
 fs/fs.c               | 2 +-
 include/environment.h | 5 +++++
 net/ifup.c            | 2 +-
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/commands/readlink.c b/commands/readlink.c
index fdcf175f56bb..81ad25c733ab 100644
--- a/commands/readlink.c
+++ b/commands/readlink.c
@@ -50,7 +50,7 @@ static int do_readlink(int argc, char *argv[])
 
 	return 0;
 err:
-	setenv(argv[optind + 1], "");
+	unsetenv(argv[optind + 1]);
 	return 1;
 }
 
diff --git a/commands/setenv.c b/commands/setenv.c
index ad2677065552..6992f604f513 100644
--- a/commands/setenv.c
+++ b/commands/setenv.c
@@ -10,6 +10,7 @@
 static int do_setenv(int argc, char *argv[])
 {
 	char *equal;
+	int ret;
 
 	if (argc < 2)
 		return COMMAND_ERROR_USAGE;
@@ -21,8 +22,12 @@ static int do_setenv(int argc, char *argv[])
 			argv[2] = &equal[1];
 	}
 
+	if (argv[2])
+		ret = setenv(argv[1], argv[2]);
+	else
+		ret = unsetenv(argv[1]);
 
-	return setenv(argv[1], argv[2]) ? COMMAND_ERROR : COMMAND_SUCCESS;
+	return ret ? COMMAND_ERROR : COMMAND_SUCCESS;
 }
 
 BAREBOX_CMD_HELP_START(setenv)
diff --git a/fs/fs.c b/fs/fs.c
index f41e4b9b72cd..00b8645fb0e9 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -3066,7 +3066,7 @@ static int automount_mount(struct dentry *dentry)
 		setenv("automount_path", am->path);
 		export("automount_path");
 		ret = run_command(am->cmd);
-		setenv("automount_path", NULL);
+		unsetenv("automount_path");
 
 		if (ret) {
 			printf("running automount command '%s' failed\n",
diff --git a/include/environment.h b/include/environment.h
index 9488e4e1ac1f..19e522cfb6b4 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -87,6 +87,11 @@ int env_push_context(void);
 
 int export(const char *);
 
+static inline int unsetenv(const char *var)
+{
+	return setenv(var, NULL);
+}
+
 #endif	/* _ENVIRONMENT_H_ */
 
 /**
diff --git a/net/ifup.c b/net/ifup.c
index a74037939b8a..1870f7401714 100644
--- a/net/ifup.c
+++ b/net/ifup.c
@@ -86,7 +86,7 @@ static int source_env_network(struct eth_device *edev)
 	env_push_context();
 
 	for (i = 0; i < ARRAY_SIZE(vars); i++)
-		setenv(vars[i], "");
+		unsetenv(vars[i]);
 
 	cmd = basprintf("source /env/network/%s", edev->devname);
 	ret = run_command(cmd);
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 2/2] setenv: align with POSIX in handling of setenv(var, "")
  2020-11-12 17:23 [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Ahmad Fatoum
@ 2020-11-12 17:23 ` Ahmad Fatoum
  2020-11-13  8:26   ` [PATCH] fixup! " Ahmad Fatoum
  2020-11-23 15:48 ` [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Sascha Hauer
  1 sibling, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2020-11-12 17:23 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

setenv(var, "") to delete var is a barebox idiosyncrasy. Previous commit
added unsetenv and changed all users of setenv(var, ""), so lets set the
behavior of setenv to what's expected: set var to the empty string.

Previously, "" was turned into NULL, which meant it wasn't possible to
set variables to the empty string from the shell. This is now possible.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/env.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/common/env.c b/common/env.c
index fbaaac4f2f5c..2830551bc0a1 100644
--- a/common/env.c
+++ b/common/env.c
@@ -251,15 +251,21 @@ static int dev_setenv(const char *name, const char *val)
 	return -ENODEV;
 }
 
+/**
+ * setenv - set environment variables
+ * @_name - Variable name
+ * @value - the value to set, empty string not handled specially
+ *
+ * Returns 0 for success and a negative error code otherwise
+ * Use unsetenv() to unset.
+ */
+
 int setenv(const char *_name, const char *value)
 {
 	char *name = strdup(_name);
 	int ret = 0;
 	struct list_head *list;
 
-	if (value && !*value)
-		value = NULL;
-
 	if (strchr(name, '.')) {
 		ret = dev_setenv(name, value);
 		if (ret)
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] fixup! setenv: align with POSIX in handling of setenv(var, "")
  2020-11-12 17:23 ` [PATCH 2/2] setenv: align with POSIX in handling of setenv(var, "") Ahmad Fatoum
@ 2020-11-13  8:26   ` Ahmad Fatoum
  2020-11-16  8:21     ` Sascha Hauer
  0 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2020-11-13  8:26 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

Previously setenv xxx= deleted x like xxx= does. With xxx= now instead
assigning the empty string, setenv xxx= should too.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

---
 commands/setenv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/commands/setenv.c b/commands/setenv.c
index 6992f604f513..9aeb8f010bc5 100644
--- a/commands/setenv.c
+++ b/commands/setenv.c
@@ -18,8 +18,7 @@ static int do_setenv(int argc, char *argv[])
 	equal = strrchr(argv[1], '=');
 	if (equal) {
 		equal[0] = '\0';
-		if (equal[1])
-			argv[2] = &equal[1];
+		argv[2] = &equal[1];
 	}
 
 	if (argv[2])
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] fixup! setenv: align with POSIX in handling of setenv(var, "")
  2020-11-13  8:26   ` [PATCH] fixup! " Ahmad Fatoum
@ 2020-11-16  8:21     ` Sascha Hauer
  2020-11-16  8:58       ` Ahmad Fatoum
  0 siblings, 1 reply; 6+ messages in thread
From: Sascha Hauer @ 2020-11-16  8:21 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Fri, Nov 13, 2020 at 09:26:00AM +0100, Ahmad Fatoum wrote:
> Previously setenv xxx= deleted x like xxx= does. With xxx= now instead
> assigning the empty string, setenv xxx= should too.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> ---
>  commands/setenv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/commands/setenv.c b/commands/setenv.c
> index 6992f604f513..9aeb8f010bc5 100644
> --- a/commands/setenv.c
> +++ b/commands/setenv.c
> @@ -18,8 +18,7 @@ static int do_setenv(int argc, char *argv[])
>  	equal = strrchr(argv[1], '=');
>  	if (equal) {
>  		equal[0] = '\0';
> -		if (equal[1])
> -			argv[2] = &equal[1];
> +		argv[2] = &equal[1];
>  	}

Fine with me, but I just had a second look at fbf145dc86 ("commands: setenv:
support setenv dev.var=VAL syntax"). This patch introduced using argv[2]
when argc was only to, thus only argv[0] and argv[1] are valid. I wonder
if this is allowed. KAsan doesn't complain, but still I think this
should be changed. Could you do this before further modifying this?

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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] fixup! setenv: align with POSIX in handling of setenv(var, "")
  2020-11-16  8:21     ` Sascha Hauer
@ 2020-11-16  8:58       ` Ahmad Fatoum
  0 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-11-16  8:58 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox

Hello,

On 16.11.20 09:21, Sascha Hauer wrote:
> On Fri, Nov 13, 2020 at 09:26:00AM +0100, Ahmad Fatoum wrote:
>> Previously setenv xxx= deleted x like xxx= does. With xxx= now instead
>> assigning the empty string, setenv xxx= should too.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>
>> ---
>>  commands/setenv.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/commands/setenv.c b/commands/setenv.c
>> index 6992f604f513..9aeb8f010bc5 100644
>> --- a/commands/setenv.c
>> +++ b/commands/setenv.c
>> @@ -18,8 +18,7 @@ static int do_setenv(int argc, char *argv[])
>>  	equal = strrchr(argv[1], '=');
>>  	if (equal) {
>>  		equal[0] = '\0';
>> -		if (equal[1])
>> -			argv[2] = &equal[1];
>> +		argv[2] = &equal[1];
>>  	}
> 
> Fine with me, but I just had a second look at fbf145dc86 ("commands: setenv:
> support setenv dev.var=VAL syntax"). This patch introduced using argv[2]
> when argc was only to, thus only argv[0] and argv[1] are valid. I wonder
> if this is allowed. KAsan doesn't complain, but still I think this
> should be changed. Could you do this before further modifying this?

barebox always has argv[argc] = NULL, so just like standard C, accessing
argv[2] here is fine.

See
  common/parser.c:29: argv[nargs] = NULL;
  common/hush.c:698:  pglob->gl_pathv[pathc] = NULL;
  lib/glob.c:398:     pglob->gl_pathv[pglob->gl_pathc] = NULL;
  lib/glob.c:232:     pglob->gl_pathv[pglob->gl_pathc] = NULL;

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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior
  2020-11-12 17:23 [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Ahmad Fatoum
  2020-11-12 17:23 ` [PATCH 2/2] setenv: align with POSIX in handling of setenv(var, "") Ahmad Fatoum
@ 2020-11-23 15:48 ` Sascha Hauer
  1 sibling, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2020-11-23 15:48 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Thu, Nov 12, 2020 at 06:23:46PM +0100, Ahmad Fatoum wrote:
> Currently, we treat setenv(var, "") and setenv(var, NULL) the same
> and delete var, which is surprising and leads to subtle quirks:
> 
>  - setenv(var, "") is specified by POSIX to set var to an empty string,
>    but barebox uses it to delete variables
> 
>  - nv.user= calls nv_set with NULL parameter, but nv user="" doesn't
> 
> Make the API more POSIX-like by providing unsetenv with the expected
> semantics. Most user code can then use unsetenv without worrying about
> whether "" or NULL is the magic deletion value.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  commands/readlink.c   | 2 +-
>  commands/setenv.c     | 7 ++++++-
>  fs/fs.c               | 2 +-
>  include/environment.h | 5 +++++
>  net/ifup.c            | 2 +-
>  5 files changed, 14 insertions(+), 4 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/commands/readlink.c b/commands/readlink.c
> index fdcf175f56bb..81ad25c733ab 100644
> --- a/commands/readlink.c
> +++ b/commands/readlink.c
> @@ -50,7 +50,7 @@ static int do_readlink(int argc, char *argv[])
>  
>  	return 0;
>  err:
> -	setenv(argv[optind + 1], "");
> +	unsetenv(argv[optind + 1]);
>  	return 1;
>  }
>  
> diff --git a/commands/setenv.c b/commands/setenv.c
> index ad2677065552..6992f604f513 100644
> --- a/commands/setenv.c
> +++ b/commands/setenv.c
> @@ -10,6 +10,7 @@
>  static int do_setenv(int argc, char *argv[])
>  {
>  	char *equal;
> +	int ret;
>  
>  	if (argc < 2)
>  		return COMMAND_ERROR_USAGE;
> @@ -21,8 +22,12 @@ static int do_setenv(int argc, char *argv[])
>  			argv[2] = &equal[1];
>  	}
>  
> +	if (argv[2])
> +		ret = setenv(argv[1], argv[2]);
> +	else
> +		ret = unsetenv(argv[1]);
>  
> -	return setenv(argv[1], argv[2]) ? COMMAND_ERROR : COMMAND_SUCCESS;
> +	return ret ? COMMAND_ERROR : COMMAND_SUCCESS;
>  }
>  
>  BAREBOX_CMD_HELP_START(setenv)
> diff --git a/fs/fs.c b/fs/fs.c
> index f41e4b9b72cd..00b8645fb0e9 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -3066,7 +3066,7 @@ static int automount_mount(struct dentry *dentry)
>  		setenv("automount_path", am->path);
>  		export("automount_path");
>  		ret = run_command(am->cmd);
> -		setenv("automount_path", NULL);
> +		unsetenv("automount_path");
>  
>  		if (ret) {
>  			printf("running automount command '%s' failed\n",
> diff --git a/include/environment.h b/include/environment.h
> index 9488e4e1ac1f..19e522cfb6b4 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -87,6 +87,11 @@ int env_push_context(void);
>  
>  int export(const char *);
>  
> +static inline int unsetenv(const char *var)
> +{
> +	return setenv(var, NULL);
> +}
> +
>  #endif	/* _ENVIRONMENT_H_ */
>  
>  /**
> diff --git a/net/ifup.c b/net/ifup.c
> index a74037939b8a..1870f7401714 100644
> --- a/net/ifup.c
> +++ b/net/ifup.c
> @@ -86,7 +86,7 @@ static int source_env_network(struct eth_device *edev)
>  	env_push_context();
>  
>  	for (i = 0; i < ARRAY_SIZE(vars); i++)
> -		setenv(vars[i], "");
> +		unsetenv(vars[i]);
>  
>  	cmd = basprintf("source /env/network/%s", edev->devname);
>  	ret = run_command(cmd);
> -- 
> 2.28.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-23 15:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 17:23 [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Ahmad Fatoum
2020-11-12 17:23 ` [PATCH 2/2] setenv: align with POSIX in handling of setenv(var, "") Ahmad Fatoum
2020-11-13  8:26   ` [PATCH] fixup! " Ahmad Fatoum
2020-11-16  8:21     ` Sascha Hauer
2020-11-16  8:58       ` Ahmad Fatoum
2020-11-23 15:48 ` [PATCH 1/2] fs: introduce unsetenv() to prepare for changing setenv(var, "") behavior Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox