Hi Pawel. Thanks for fixing this!
Comments below. Please excuse my nit-pickiness. :-)
On Friday 19 September 2008 00:31:11, Pawel Veselov wrote:
> --
> With best of best regards
> Pawel S. Veselov
> 7.diff
> Index: newlib/libc/stdlib/setenv_r.c
> ===================================================================
> --- newlib/libc/stdlib/setenv_r.c (revision 1182)
> +++ newlib/libc/stdlib/setenv_r.c (working copy)
> @@ -58,6 +58,9 @@
Can you please use diff -up in your patches? Without p, it
is harder to see what you're changing.
>
> ENV_LOCK;
>
> + // $TODO: this seems to be bogus, environment variable value
> + // can certainly start with '=' in general case.
> +
> if (*value == '=') /* no `=' in value */
> ++value;
> l_value = strlen (value);
Hmmm, interesting. Why is that there, in common code, and nobody
has complained? It is still here in newlib HEAD. Maybe you
should ask upstream.
> Index: newlib/libc/sys/wince/sys/shared.h
> ===================================================================
> --- newlib/libc/sys/wince/sys/shared.h (revision 1182)
> +++ newlib/libc/sys/wince/sys/shared.h (working copy)
> @@ -30,10 +30,14 @@
>
> _SHMBLK _shared_init(int pgid);
> void _shared_dump(_SHMBLK shmblk);
> -int _shared_getshowwindow();
> +int _shared_getshowwindow();
Can you make this (void), while you're at it?
> void _shared_setshowwindow(_SHMBLK shmblk, int show);
> void _shared_setenvironblk(_SHMBLK shmblk, char **env);
> -void _shared_getenvironblk(_SHMBLK shmblk, char **env);
> +
> +// returns the amount of environment variables extracted
> +// from the shared block. The environment variables are
> +// written into *env, which is allocated using malloc()
> +int _shared_getenvironblk(_SHMBLK shmblk, char **env);
Please, no // in C. Here an elsewhere, especially in headers. It
breaks gcc -std=c89. If there are instances of it in headers, we should
fix them (but separatelly). Can we make sure sentences are capitalized,
and that there are two spaces after full stops? E.g.,
/* Returns the amount of environment variables extracted
from the shared block. The environment variables are
written into *ENV, which is allocated using malloc. */
> void _shared_reset(_SHMBLK shmblk);
> void _shared_getcwd(_SHMBLK shmblk, char *cwd);
> void _shared_setcwd(_SHMBLK shmblk, char *cwd);
> Index: newlib/libc/sys/wince/startup.c
> ===================================================================
> --- newlib/libc/sys/wince/startup.c (revision 1182)
> +++ newlib/libc/sys/wince/startup.c (working copy)
> @@ -548,7 +548,6 @@
>
> atexit(_ioatexit);
> _shared_dump(shmblk);
> - _shared_getenvblk(shmblk, environ);
> __pgid = _shared_getpgid(shmblk);
> _shared_getcwd(shmblk, buf);
>
> Index: newlib/libc/sys/wince/env.c
> ===================================================================
> --- newlib/libc/sys/wince/env.c (revision 1182)
> +++ newlib/libc/sys/wince/env.c (working copy)
> @@ -80,20 +80,24 @@
> {
> if (shmblk)
> {
> - char buf[MAX_ENVIRONBLK];
> + char * buf;
>
> - /* We are being initialized from a cegcc app.
> - Kill environ set from the registry. */
> - environ[0] = 0;
> - _shared_getenvblk(shmblk, buf);
> - if(buf[0] != 0)
> - _initenv_from_envblock(buf);
> - else
> - _initenv_from_reg();
> - _shared_setenvblk(shmblk, "");
> + // We are being initialized from a cegcc app.
> + // Only use registry environment if there are
> + // no environment variables in the shared block
> +
> + int no = _shared_getenvironblk(shmblk, &buf);
^ let's be consistent and not assume c99/gnu. Please
declare variables at the beginning of the scope not in the middle.
> +
> + if (no) {
> + _initenv_from_envblock(buf);
> + free(buf);
> + } else {
> + _initenv_from_reg();
> + }
> }
> else
> {
> _initenv_from_reg();
> }
> }
> +
> Index: newlib/libc/sys/wince/spawn.c
> ===================================================================
> --- newlib/libc/sys/wince/spawn.c (revision 1182)
> +++ newlib/libc/sys/wince/spawn.c (working copy)
> @@ -93,7 +93,7 @@
> }
>
> WCETRACE(WCE_IO, "spawnv: _shared_init returns %x", shmblk);
> - _shared_setenvblk(shmblk, environ);
> + _shared_setenvironblk(shmblk, environ);
> getcwd(buf, BUFSIZE);
> WCETRACE(WCE_IO, "spawnv: cwd \"%s\"", buf);
> _shared_setcwd(shmblk, buf);
> @@ -155,7 +155,7 @@
> }
>
> WCETRACE(WCE_IO, "spawnv: _shared_init returns %x", shmblk);
> - _shared_setenvblk(shmblk, environ);
> + _shared_setenvironblk(shmblk, environ);
> getcwd(buf, BUFSIZE);
> WCETRACE(WCE_IO, "spawnvp: cwd \"%s\"", buf);
> _shared_setcwd(shmblk, buf);
> @@ -190,7 +190,7 @@
> return(-1);
> }
>
> - _shared_setenvblk(shmblk, environ);
> + _shared_setenvironblk(shmblk, environ);
What the rename? You've renamed a couple of instances, but
left other functions still using envblk. I don't think this
brings any clarity to the code. Can you undo that?
> getcwd(buf, BUFSIZE);
> _shared_setcwd(shmblk, buf);
> _shared_setstdinfd(shmblk, (infd >= 0) ? _fdtab[infd].fd : infd);
> Index: newlib/libc/sys/wince/shared.c
> ===================================================================
> --- newlib/libc/sys/wince/shared.c (revision 1182)
> +++ newlib/libc/sys/wince/shared.c (working copy)
> @@ -194,7 +194,7 @@
> }
>
> void
> -_shared_setenvblk(_SHMBLK shmblk, char **env)
> +_shared_setenvironblk(_SHMBLK shmblk, char **env)
> {
> char *d;
> int i, len;
> @@ -209,7 +209,7 @@
> for (i = 0, d = shmblk->pginfo.environ; env[i] != NULL; i++) {
> len = strlen(env[i]);
> if (d + len >= endp) {
> - WCETRACE(WCE_IO, "_shared_setenvblk: FATAL space
> exhausted (max %d)", + WCETRACE(WCE_IO,
> "_shared_setenvironblk: FATAL space exhausted (max %d)", MAX_ENVIRONBLK);
> exit(1);
> }
> @@ -219,28 +219,32 @@
> *d = 0;
> }
>
> -void
> -_shared_getenvblk(_SHMBLK shmblk, char **env)
> +int
> +_shared_getenvironblk(_SHMBLK shmblk, char **env)
> {
> char *s;
> int i, len;
>
> - if (shmblk == NULL)
> - return;
> + if (!shmblk) { return; }
Please pleave the return statement on its own line (good for
putting breakpoints at).
>
> for (i = 0, s = shmblk->pginfo.environ; *s; i++) {
> - len = strlen(s);
> - if (env[i] != NULL) {
> - free(env[i]);
> - }
> - env[i] = malloc(len + 1);
> - if (env[i] == NULL) {
> - WCETRACE(WCE_IO, "_shared_getenvblk: FATAL ERROR
> malloc failed"); - exit(1);
> - }
> - memcpy(env[i], s, len + 1);
> - s += len + 1;
> - }
> + s += strlen(s) + 1;
> + }
> +
> + if (i) {
> +
> + len = s - shmblk->pginfo.environ + 1;
> + *env = (char*)malloc(len);
^ cast not needed.
> + if (!*env) {
> + WCETRACE(WCE_IO, "_shared_getenvironblk: FATAL ERROR
> malloc failed for %d more bytes", len);
Hmmm, should we make these fatal errors verbose even if tracing
is not enabled.
> + exit(1);
I know this is already the case, but, shouldn't that be _exit or
better yet abort instead of exit?
> + }
> +
> + memcpy(*env, shmblk->pginfo.environ, len);
> + }
> +
> +
> + return i;
I don't see anything clearing *env, either here, of at the caller
if this returns 0. You're calling free on this pointer
unconditionally, which in that case, would be a dangling pointer.
> }
>
> BOOL
> Index: ChangeLog.cegcc
> ===================================================================
> --- ChangeLog.cegcc (revision 1182)
> +++ ChangeLog.cegcc (working copy)
> @@ -1,3 +1,18 @@
> +2008-09-18 Pawel Veselov <[EMAIL PROTECTED]>
> + * newlib/libc/stdlib/setenv_r.c: flag a possible problem with
> + environment variable values that start with '='
> + * newlib/libc/sys/wince/sys/shared.h: _shared_getenvironblk() now
> + returns (int). Also added description for
> _shared_getenvironblk() + * newlib/libc/sys/wince/startup.c: do not
> reset environment variables + from the shared block, as it's
> already been done
> + * newlib/libc/sys/wince/env.c: fix shared block environment
> variables + initialization
> + * newlib/libc/sys/wince/spawn.c: use proper function names
> + * newlib/libc/sys/wince/shared.c: change _shared_getenvironblk()
> + behavior to dump full environment block as is, not chop it up
> + into separate variables. Rename _shared_setenvblk() and
> + _shared_setenvblk() to match the header file declarations.
> +
There are some issues with this changelog entry. We stick to the GNU format
in our sources (which newlib also uses). Here's an example of what could
like like:
2008-09-18 Pawel Veselov <[EMAIL PROTECTED]>
^ two spaces here
* newlib/libc/stdlib/setenv_r.c (_setenv_r): Flag a possible
problem with environment variable values that start with '='.
* newlib/libc/sys/wince/sys/shared.h (_shared_getenvironblk):
Change return type to int. Add description.
* newlib/libc/sys/wince/startup.c (foo): Do not reset environment
variables from the shared block.
* newlib/libc/sys/wince/env.c (bar): Fix shared block environment
variables initialization.
...
(_shared_setenvblk): Rename to ...
(_shared_setenvironblk): ... this.
--
Pedro Alves
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Cegcc-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/cegcc-devel
