HEADS DOWN

[Sean C. Farley]

On Sat, 5 May 2007, Andrey Chernov wrote:

> On Fri, May 04, 2007 at 01:21:50PM -0500, Sean C. Farley wrote:
>>  I believe I check all that you did in your changes.  Mine looks a
>>  little different since some checks were combined for speed (i.e.,
>>  __strleneq()).
>
> Looking at
> http://www.farley.org/freebsd/tmp/setenv-8/POSIX/sysenv.c
>
> __strleneq() - "Inline strlen() for performance." - no performance
> gain such way but lost, because strlen() already auto-inlined with gcc
> -O2 in much better assembler form:
> repnz
> scasb
> So there is no point to call __strleneq(..., false); too instead of
> just strlen() (which will be auto-inlined by gcc) directly.

Interestingly, gcc 3.4.6 on -STABLE does not do such a great job even
with -minline-all-stringops.  I do have CPUTYPE?=pentium4 in
/etc/make.conf.  Maybe this is only an issue with i386 similar to what
you mention below?

> Currently __clean_env is unused. Why don't #if 0 it?

It currently is used when the library is unloaded.  My hope was for
dmalloc to stop reporting leaks by cleaning up afterwards.  It is still
good for catching my mistakes.  It can be #if 0'd out when everything
else is good.

Actually, I will keep it, so it can be used if __build_env() fails on
the strdup().

> I think "Check for malformed name" should be before "Initialize
> environment" - with malformed arg complex environment
> mallocing/copying initialization ever not needed.

I reversed the checks to be before the initialization.

> In __build_env() when strdup() returns NULL, you better free all previous
> strdups in that loop and envVars too before returning (-1)

Calling __clean_env() now to clean up everything upon failure.

> Why you use strstr(envVars[envNdx].name, "="); instead of
> strchr(envVars[envNdx].name, '='); ? In theory strchr() can be
> gcc-inlined or assembler-implemented with more probability than
> strstr() (currently both are not inlined at i386).

I did not think about gcc optimizations.  I fixed it to use strchr().

> __rebuild_environ() needs to use realloc() instead of reallocf()
> because realloc() not touch original pointer/content when fails. I.e.
> _whole_ "environ" will not be lost in case can't be enlarged. Something
> like that:
>
> 		TempEnvironSize = newEnvSize * 2;
> 		TempEnviron = realloc(environ, sizeof (*environ) *
> 				(TempEnvironSize +  1));
> 		if (TempEnviron == NULL)
> 			return (-1);
> 		environ = TempEnviron;
> 		environSize = TempEnvironSize;

True enough.  I had thought about that awhile ago but had forgotten
about going back to fix the reallocf() calls.

>>  The only other question I have is about leading whitespace in the
>>  name passed to *env():
>>  1. Should it be removed up to the first non-whitespace character?
>>  2. Treated as part of the variable name.  Is this allowed by the
>>     specification?
>>  3. Return errno = EINVAL.  getenv() would just return NULL.
>
> POSIX says about names:
> 1) "names shall not contain the character '='" and "points to an empty
> string".
> 2) "Other characters may be permitted by an implementation;
> applications shall tolerate the presence of such names."
> So I see no point to treat ' ' some special way.

Agreed.

Concerning the optimization of putenv() you mentioned in a later e-mail,
I fixed that too.  It is much faster.

I also placed my regression tests (not all work with the old code) in
the same directory[1] as the latest code.  One oddity I noticed:
/bin/sh as the shell runs the timings much slower than either /bin/tcsh
or /usr/local/bin/zsh.  This is regardless if its my code or the old
code.

Sean
   1. http://www.farley.org/freebsd/tmp/setenv-8/POSIX/
-- 
sean-freebsd at farley.org