PATCH: environment handling rewrite

PATCH: environment handling rewrite

[Andrej Borsenkow]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

This rewrites environment handling to use system supplied putenv()/getenv() if
available. The main reason was Cygwin, where internal environment management
differs from Unix significantly.

There is no system dependencies (apart from existence of environ). The only
two assumptions are:

- if environ table is extended,  malloc() is used.
- if putenv() copies it's argument, malloc() is used (it is the case under
current Cygwin)

Else it dynamically checks, if putenv() has reused old memory or copied it's
argument.

I'm running it on both Unix and Cygwin and it works so far. With exception
of --enable-zsh-mem problem on Cygiwn, but ot cannot be related (error comes
even before we set up environment).

It depends on configure.in patch from my previous mail (patch for brk() on
Cygwin).

Could anybody commit them as I'd like to have clean state for Cygwin modules
stuff?

thanks

-andrej

Have a nice DOS!
B >>

[-- Attachment #2: zsh-env.diff --]
[-- Type: application/octet-stream, Size: 9420 bytes --]

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.29
diff -u -r1.29 builtin.c
--- Src/builtin.c	2000/07/19 20:43:51	1.29
+++ Src/builtin.c	2000/07/27 16:49:14
@@ -1634,7 +1634,6 @@
 		    pm->env = addenv(pname, getsparam(pname), pm->flags);
 	    } else if (pm->env && !(pm->flags & PM_HASHELEM)) {
 		delenv(pm->env);
-		zsfree(pm->env);
 		pm->env = NULL;
 	    }
 	    if (value)
@@ -1711,7 +1710,6 @@
 	tpm->ct = pm->ct;
 	if (pm->env) {
 	    delenv(pm->env);
-	    zsfree(pm->env);
 	}
 	tpm->env = pm->env = NULL;
 
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.23
diff -u -r1.23 params.c
--- Src/params.c	2000/07/13 17:06:19	1.23
+++ Src/params.c	2000/07/27 16:49:18
@@ -455,10 +455,13 @@
 createparamtable(void)
 {
     Param ip, pm;
-    char **new_environ, **envp, **envp2, **sigptr, **t;
-    char **old_environ = environ;
+#ifndef HAVE_PUTENV
+    char **new_environ;
+    int  envsize;
+#endif
+    char **envp, **envp2, **sigptr, **t;
     char buf[50], *str, *iname, *hostnam;
-    int num_env, oae = opts[ALLEXPORT];
+    int  oae = opts[ALLEXPORT];
 #ifdef HAVE_UNAME
     struct utsname unamebuf;
     char *machinebuf;
@@ -501,15 +504,19 @@
 
     setsparam("LOGNAME", ztrdup((str = getlogin()) && *str ? str : cached_username));
 
+#ifndef HAVE_PUTENV
     /* Copy the environment variables we are inheriting to dynamic *
      * memory, so we can do mallocs and frees on it.               */
-    num_env = arrlen(environ);
-    new_environ = (char **) zalloc(sizeof(char *) * (num_env + 1));
-    *new_environ = NULL;
+    envsize = sizeof(char *)*(1 + arrlen(environ));
+    new_environ = (char **) zalloc(envsize);
+    memcpy (new_environ, environ, envsize);
+    environ = new_environ;
+#endif
 
     /* Now incorporate environment variables we are inheriting *
-     * into the parameter hash table.                          */
-    for (envp = new_environ, envp2 = environ; *envp2; envp2++) {
+     * into the parameter hash table. Copy them into dynamic   *
+     * memory so that we can free them if needed               */
+    for (envp = envp2 = environ; *envp2; envp2++) {
 	for (str = *envp2; *str && *str != '='; str++);
 	if (*str == '=') {
 	    iname = NULL;
@@ -517,25 +524,22 @@
 	    if (!idigit(**envp2) && isident(*envp2) && !strchr(*envp2, '[')) {
 		iname = *envp2;
 		if ((!(pm = (Param) paramtab->getnode(paramtab, iname)) ||
-		     !(pm->flags & PM_DONTIMPORT)) &&
-		    (pm = setsparam(iname, metafy(str + 1, -1, META_DUP))) &&
-		    !(pm->flags & PM_EXPORTED)) {
+		     !(pm->flags & PM_DONTIMPORT || pm->flags & PM_EXPORTED)) &&
+		    (pm = setsparam(iname, metafy(str + 1, -1, META_DUP)))) {
 		    *str = '=';
 		    pm->flags |= PM_EXPORTED;
-		    pm->env = *envp++ = ztrdup(*envp2);
-		    *envp = NULL;
-		    if (pm->flags & PM_SPECIAL) {
-			environ = new_environ;
-			pm->env = replenv(pm->env, getsparam(pm->nam),
-					  pm->flags);
-			environ = old_environ;
-		    }
+		    if (pm->flags & PM_SPECIAL)
+			pm->env = mkenvstr (pm->nam,
+					    getsparam(pm->nam), pm->flags);
+		    else
+			pm->env = ztrdup(*envp2);
+		    *envp++ = pm->env;
 		}
 	    }
 	    *str = '=';
 	}
     }
-    environ = new_environ;
+    *envp = '\0';
     opts[ALLEXPORT] = oae;
 
     pm = (Param) paramtab->getnode(paramtab, "HOME");
@@ -660,7 +664,6 @@
 		 */
 		if (oldpm->env) {
 		    delenv(oldpm->env);
-		    zsfree(oldpm->env);
 		    oldpm->env = NULL;
 		}
 		paramtab->removenode(paramtab, name);
@@ -1489,7 +1492,7 @@
     else
 	val = pm->gets.cfn(pm);
     if (pm->env)
-	pm->env = replenv(pm->env, val, pm->flags);
+	pm->env = replenv(pm->nam, val, pm->flags);
     else {
 	pm->flags |= PM_EXPORTED;
 	pm->env = addenv(pm->nam, val, pm->flags);
@@ -2006,7 +2009,6 @@
     pm->unsetfn(pm, exp);
     if ((pm->flags & PM_EXPORTED) && pm->env) {
 	delenv(pm->env);
-	zsfree(pm->env);
 	pm->env = NULL;
     }
 
@@ -2824,8 +2826,7 @@
 void
 arrfixenv(char *s, char **t)
 {
-    char **ep, *u;
-    int len_s;
+    char *u;
     Param pm;
 
     pm = (Param) paramtab->getnode(paramtab, s);
@@ -2838,24 +2839,47 @@
     if (pm->flags & PM_HASHELEM)
 	return;
     u = t ? zjoin(t, ':', 1) : "";
-    len_s = strlen(s);
-    for (ep = environ; *ep; ep++)
-	if (!strncmp(*ep, s, len_s) && (*ep)[len_s] == '=') {
-	    pm->env = replenv(*ep, u, pm->flags);
-	    return;
-	}
+    if (findenv(s, 0)) {
+	pm->env = replenv(s, u, pm->flags);
+	return;
+    }
     if (isset(ALLEXPORT))
 	pm->flags |= PM_EXPORTED;
     if (pm->flags & PM_EXPORTED)
 	pm->env = addenv(s, u, pm->flags);
 }
 
-/* Given *name = "foo", it searchs the environment for string *
- * "foo=bar", and returns a pointer to the beginning of "bar" */
+#ifndef HAVE_PUTENV
 
-/**/
-mod_export char *
-zgetenv(char *name)
+static int
+putenv(char *str)
+{
+    char **ep;
+    int num_env;
+
+
+    /* First check if there is already an environment *
+     * variable matching string `name'.               */
+    if (findenv (str, &num_env)) {
+	environ[num_env] = str;
+    } else {
+    /* Else we have to make room and add it */
+	num_env = arrlen(environ);
+	environ = (char **) zrealloc(environ, (sizeof(char *)) * (num_env + 2));
+
+	/* Now add it at the end */
+	ep = environ + num_env;
+	*ep = str;
+	*(ep + 1) = NULL;
+    }
+    return 0;
+}
+#endif
+
+#ifndef HAVE_GETENV
+
+static char *
+getenv(char *name)
 {
     char **ep, *s, *t;
  
@@ -2866,6 +2890,37 @@
     }
     return NULL;
 }
+#endif
+
+/**/
+static int
+findenv (char *name, int *pos)
+{
+    char **ep, *eq;
+    int  nlen;
+
+
+    eq = strchr (name, '=');
+    nlen = eq ? eq - name : strlen (name);
+    for (ep = environ; *ep; ep++) 
+	if (!strncmp (*ep, name, nlen) && *((*ep)+nlen) == '=') {
+	    if (pos)
+		*pos = ep - environ;
+	    return 1;
+	}
+    
+    return 0;
+}
+
+/* Given *name = "foo", it searchs the environment for string *
+ * "foo=bar", and returns a pointer to the beginning of "bar" */
+
+/**/
+mod_export char *
+zgetenv(char *name)
+{
+    return getenv(name);
+}
 
 /**/
 static void
@@ -2881,27 +2936,51 @@
     }
 }
 
+static char *
+addenv_internal(char *name, char *value, int flags, int add)
+{
+    char *oldenv = 0, *newenv = 0, *env = 0;
+    int pos;
+
+    /* First check if there is already an environment *
+     * variable matching string `name'. If not, and   *
+     * we are not requested to add new, return        */
+    if (findenv (name, &pos))
+	oldenv = environ[pos];
+    else if (!add)
+	return NULL;
+
+    newenv = mkenvstr (name, value, flags);
+    if (putenv (newenv)) {
+	zsfree (newenv);
+	return NULL;
+    }
+    /*
+     * Under Cygwin we must use putenv() to maintain consistency.
+     * Unfortunately, current version (1.1.2) copies argument and may
+     * silently reuse exisiting environment string. This tries to
+     * check for both cases
+     */
+    if (findenv (name, &pos)) {
+	env = environ[pos];
+	if (env != oldenv)
+	    zsfree (oldenv);
+	if (env != newenv)
+	    zsfree (newenv);
+	return env;
+    }
+
+    return NULL; /* Cannot happen */
+}
+
 /* Change the value of an existing environment variable */
 
 /**/
 char *
-replenv(char *e, char *value, int flags)
+replenv(char *name, char *value, int flags)
 {
-    char **ep, *s;
-    int len_value;
 
-    for (ep = environ; *ep; ep++)
-	if (*ep == e) {
-	    for (len_value = 0, s = value;
-		 *s && (*s++ != Meta || *s++ != 32); len_value++);
-	    s = e;
-	    while (*s++ != '=');
-	    *ep = (char *) zrealloc(e, s - e + len_value + 1);
-	    s = s - e + *ep - 1;
-	    copyenvstr(s, value, flags);
-	    return *ep;
-	}
-    return NULL;
+    return addenv_internal (name, value, flags, 0);
 }
 
 /* Given strings *name = "foo", *value = "bar", *
@@ -2934,28 +3013,7 @@
 char *
 addenv(char *name, char *value, int flags)
 {
-    char **ep, *s, *t;
-    int num_env;
-
-    /* First check if there is already an environment *
-     * variable matching string `name'.               */
-    for (ep = environ; *ep; ep++) {
-	for (s = *ep, t = name; *s && *s == *t; s++, t++);
-	if (*s == '=' && !*t) {
-	    zsfree(*ep);
-	    return *ep = mkenvstr(name, value, flags);
-	}
-    }
-
-    /* Else we have to make room and add it */
-    num_env = arrlen(environ);
-    environ = (char **) zrealloc(environ, (sizeof(char *)) * (num_env + 2));
-
-    /* Now add it at the end */
-    ep = environ + num_env;
-    *ep = mkenvstr(name, value, flags);
-    *(ep + 1) = NULL;
-    return *ep;
+    return addenv_internal (name, value, flags, 1);
 }
 
 /* Delete a pointer from the list of pointers to environment *
@@ -2971,8 +3029,10 @@
 	if (*ep == x)
 	    break;
     }
-    if (*ep)
+    if (*ep) {
 	for (; (ep[0] = ep[1]); ep++);
+    }
+    zsfree(x);
 }
 
 /**/
@@ -3105,7 +3165,6 @@
 	    pm->ct = tpm->ct;
 	    if (pm->env) {
 		delenv(pm->env);
-		zsfree(pm->env);
 	    }
 	    pm->env = NULL;
 

Re: PATCH: environment handling rewrite

[Peter Stephenson]

Andrej wrote:
> This rewrites environment handling to use system supplied putenv()/getenv() i
> f
> available. The main reason was Cygwin, where internal environment management
> differs from Unix significantly.

I've committed the changes.  It's working on Solaris, too.  It might be a
good idea if someone could run a memory checker on this and do a bit of
environment manipulation.

-- 
Peter Stephenson <pws@csr.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: PATCH: environment handling rewrite

[Bart Schaefer]

On Jul 28, 12:50pm, Andrej Borsenkow wrote:
}
} This rewrites environment handling to use system supplied
} putenv()/getenv() if available.

I would have preferred that putenv/getenv not have been defined as
functions using those names, but rather that there was a new function
named zputenv and that the body of the new getenv had been left in
zgetenv; that is, put #ifdefs inside the bodies of zputenv/zgetenv.
It's probably not a real issue because the #ifdef'd putenv/getenv are
declared static, but in general it's best to avoid redeclaring symbols
that some other library might also decide to redeclare.

Also, minor nit-pick, but I dislike the coding style of putting a space
between a function's name and the opening paren of the parameter list.
I won't repeat "hate" three times, but it's not consistent with the rest
of the zsh style.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

RE: PATCH: environment handling rewrite

[Andrej Borsenkow]

> 
> I would have preferred that putenv/getenv not have been defined as
> functions using those names, but rather that there was a new function
> named zputenv and that the body of the new getenv had been left in
> zgetenv; that is, put #ifdefs inside the bodies of zputenv/zgetenv.
> It's probably not a real issue because the #ifdef'd putenv/getenv are
> declared static, but in general it's best to avoid redeclaring symbols
> that some other library might also decide to redeclare.
> 
> Also, minor nit-pick, but I dislike the coding style of putting a space
> between a function's name and the opening paren of the parameter list.
> I won't repeat "hate" three times, but it's not consistent with the rest
> of the zsh style.
> 

O.K.

-andrej

Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.24
diff -u -r1.24 params.c
--- Src/params.c        2000/07/28 09:10:37     1.24
+++ Src/params.c        2000/08/03 12:49:36
@@ -509,7 +509,7 @@
      * memory, so we can do mallocs and frees on it.               */
     envsize = sizeof(char *)*(1 + arrlen(environ));
     new_environ = (char **) zalloc(envsize);
-    memcpy (new_environ, environ, envsize);
+    memcpy(new_environ, environ, envsize);
     environ = new_environ;
 #endif
 
@@ -2849,18 +2849,20 @@
        pm->env = addenv(s, u, pm->flags);
 }
 
-#ifndef HAVE_PUTENV
 
 static int
-putenv(char *str)
+zputenv(char *str)
 {
+#ifdef HAVE_PUTENV
+    return putenv(str);
+#else
     char **ep;
     int num_env;
 
 
     /* First check if there is already an environment *
      * variable matching string `name'.               */
-    if (findenv (str, &num_env)) {
+    if (findenv(str, &num_env)) {
        environ[num_env] = str;
     } else {
     /* Else we have to make room and add it */
@@ -2873,35 +2875,19 @@
        *(ep + 1) = NULL;
     }
     return 0;
-}
 #endif
-
-#ifndef HAVE_GETENV
-
-static char *
-getenv(char *name)
-{
-    char **ep, *s, *t;
- 
-    for (ep = environ; *ep; ep++) {
-       for (s = *ep, t = name; *s && *s == *t; s++, t++);
-       if (*s == '=' && !*t)
-           return s + 1;
-    }
-    return NULL;
 }
-#endif
 
 /**/
 static int
-findenv (char *name, int *pos)
+findenv(char *name, int *pos)
 {
     char **ep, *eq;
     int  nlen;
 
 
-    eq = strchr (name, '=');
-    nlen = eq ? eq - name : strlen (name);
+    eq = strchr(name, '=');
+    nlen = eq ? eq - name : strlen(name);
     for (ep = environ; *ep; ep++) 
        if (!strncmp (*ep, name, nlen) && *((*ep)+nlen) == '=') {
            if (pos)
@@ -2919,7 +2905,18 @@
 mod_export char *
 zgetenv(char *name)
 {
+#ifdef HAVE_GETENV
     return getenv(name);
+#else
+    char **ep, *s, *t;
+ 
+    for (ep = environ; *ep; ep++) {
+       for (s = *ep, t = name; *s && *s == *t; s++, t++);
+       if (*s == '=' && !*t)
+           return s + 1;
+    }
+    return NULL;
+#endif
 }
 
 /**/
@@ -2945,14 +2942,14 @@
     /* First check if there is already an environment *
      * variable matching string `name'. If not, and   *
      * we are not requested to add new, return        */
-    if (findenv (name, &pos))
+    if (findenv(name, &pos))
        oldenv = environ[pos];
     else if (!add)
        return NULL;
 
-    newenv = mkenvstr (name, value, flags);
-    if (putenv (newenv)) {
-       zsfree (newenv);
+    newenv = mkenvstr(name, value, flags);
+    if (zputenv(newenv)) {
+       zsfree(newenv);
        return NULL;
     }
     /*
@@ -2961,12 +2958,12 @@
      * silently reuse exisiting environment string. This tries to
      * check for both cases
      */
-    if (findenv (name, &pos)) {
+    if (findenv(name, &pos)) {
        env = environ[pos];
        if (env != oldenv)
-           zsfree (oldenv);
+           zsfree(oldenv);
        if (env != newenv)
-           zsfree (newenv);
+           zsfree(newenv);
        return env;
     }
 
@@ -2980,7 +2977,7 @@
 replenv(char *name, char *value, int flags)
 {
 
-    return addenv_internal (name, value, flags, 0);
+    return addenv_internal(name, value, flags, 0);
 }
 
 /* Given strings *name = "foo", *value = "bar", *
@@ -3013,7 +3010,7 @@
 char *
 addenv(char *name, char *value, int flags)
 {
-    return addenv_internal (name, value, flags, 1);
+    return addenv_internal(name, value, flags, 1);
 }
 
 /* Delete a pointer from the list of pointers to environment *