306840 - Improve PR_GetEnv() usages

Status: RESOLVED INACTIVE

(Core :: General, task)

Description

[Serge Gautherie (:sgautherie)]

Spun off from <https://bugzilla.mozilla.org/show_bug.cgi?id=287540#c34>
{{
 ------- Additional Comment #34 From [e-mail] (working) 
2005-08-16 22:56 PDT  [reply] -------

the proper way to
use PR_GetEnv involves:

const char* foo = PR_GetEnv("BAR");
(foo && *foo)

not simply (foo).
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <XpCom/*>

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 194656 [details] [diff] [review]
(Bv1) <XpCom/*>

There's no need for all the whitespace noise in this patch, but regardless:
please don't request sr from me for this sort of trivial cleanup.  I just don't
have the bandwidth for it.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <XpCom/*> (-Bpuw for review)

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 194658 [details] [diff] [review]
(Bv1) <XpCom/*> (-Bpuw for review)

timeless should review this.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <XpFe/*> (-Bw for review)

Comment 6

[[e-mail]]

Comment on attachment 194661 [details] [diff] [review]
(Av1) <XpFe/*> (-Bw for review)

>   // Make sure to set a username if possible
>   if (!username) {
>     username = PR_GetEnv("LOGNAME");
>+    if (username && !*username) {
>+      username = nsnull;
>+    }
>   }
This bit looks wrong but please ask timeless for review next time.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 194661 [details] [diff] [review]
(Av1) <XpFe/*> (-Bw for review)

Timeless:
I'll do the removal suggested by Neil after your review...

Comment 8

[Doug Turner (:dougt)]

If the environment variable is defined, returns a pointer to its value;
otherwise returns NULL.  Why do we need to check both?

http://www.mozilla.org/projects/nspr/reference/html/prsystem.html#25208

Comment 9

[timeless]

dougt:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/prenv.h&rev=3.11&mark=68-76,88-93,83-86,95-100#52

Comment 10

[timeless]

Comment on attachment 194658 [details] [diff] [review]
(Bv1) <XpCom/*> (-Bpuw for review)

>Index: mozilla/xpcom/io/nsDirectoryService.cpp
>diff -u -p -B -p -u -w -r1.83 nsDirectoryService.cpp
>@@ -249,15 +249,15 @@ nsDirectoryService::GetCurrentProcessDir
>     // We do this py putenv()ing the default value into the environment.  Note that

someone should change "py" to "by" ...

>     // we only do this if it is not already set.
> #ifdef MOZ_DEFAULT_MOZILLA_FIVE_HOME
>-    if (PR_GetEnv("MOZILLA_FIVE_HOME") == nsnull)
>+    const char *home = PR_GetEnv("MOZILLA_FIVE_HOME");
>+    if (!(home && *home))

this could be written:
!home || !*home

>Index: mozilla/xpcom/obsolete/nsSpecialSystemDirectory.cpp
>diff -u -p -B -p -u -w -r1.9 nsSpecialSystemDirectory.cpp
>@@ -748,6 +748,8 @@ void nsSpecialSystemDirectory::operator 
>             *this = PR_GetEnv("HOME");

this is a trick piece of code, it's evil and magical and doesn't do what you think. don't change this file at all.

>+            if (*this && !**this)
>+              *this = nsnull;

please post a new patch and have dougt check it in.

Comment 11

[timeless]

Comment on attachment 194661 [details] [diff] [review]
(Av1) <XpFe/*> (-Bw for review)

this is fine, post a new patch and ask for sr?neil

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <XpFe/*> (-Bw for review)

Av1, with comment 6 and 11 suggestion(s).

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <XpCom/*> [Checkin: See comment 17+19]

Bv1, with comment 10 suggestion(s).

Doug, can you check it in on my behalf ? Thanks.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <XpFe/*> [Wrong file]

Av2-Bw, but with full diff for checkin.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <XpFe/*> [Checked in: Comment 16]

Av2-Bw, but with full diff for checkin.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 202427 [details] [diff] [review]
(Av2) <XpFe/*>
[Checked in: Comment 16]


Check in: { 2005-12-22 09:57	bugzilla%standard8.demon.co.uk }

Comment 17

[Doug Turner (:dougt)]

checked in patch bv2:

Checking in xpcom/ds/nsTimelineService.cpp;
/cvsroot/mozilla/xpcom/ds/nsTimelineService.cpp,v  <--  nsTimelineService.cpp
new revision: 3.17; previous revision: 3.16
done
Checking in xpcom/io/nsAppFileLocationProvider.cpp;
/cvsroot/mozilla/xpcom/io/nsAppFileLocationProvider.cpp,v  <--  nsAppFileLocationProvider.cpp
new revision: 1.58; previous revision: 1.57
done
Checking in xpcom/io/nsDirectoryService.cpp;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.cpp,v  <--  nsDirectoryService.cpp
new revision: 1.87; previous revision: 1.86
done
Checking in xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp;
/cvsroot/mozilla/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp,v  <--  xptiInterfaceInfoManager.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in xpcom/threads/nsEnvironment.cpp;
/cvsroot/mozilla/xpcom/threads/nsEnvironment.cpp,v  <--  nsEnvironment.cpp
new revision: 1.3; previous revision: 1.2
done

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #0)
> the proper way to
> use PR_GetEnv involves:
> 
> const char* foo = PR_GetEnv("BAR");
> (foo && *foo)

Why can't we make PR_GetEnv be consistent across platforms and avoid this mess?

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 202324 [details] [diff] [review]
(Bv2) <XpCom/*>
[Checkin: See comment 17+19]

Comment 17 checkin missed the <nsAtomTable.cpp> file, which had become in conflict.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <XpCom/nsAtomTable.cpp> (sync'ed Bv2)

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 202324 [details] [diff] [review]
(Bv2) <XpCom/*>
[Checkin: See comment 17+19]

Keeping
{{
(Av1) <XpFe/*> (-Bw for review)   	 patch   	2005-09-02 08:05 PST  	4.08 KB  	timeless: review+
(Av2) <XpFe/*> (-Bw for review)   	 patch   	2005-11-08 16:39 PST  	3.80 KB  	neil: superreview+
}}

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 202427 [details] [diff] [review]
(Av2) <XpFe/*>
[Checked in: Comment 16]

Keeping
{{
(Av1) <XpFe/*> (-Bw for review)   	 patch   	2005-09-02 08:05 PST  	4.08 KB  	timeless: review+
(Av2) <XpFe/*> (-Bw for review)   	 patch   	2005-11-08 16:39 PST  	3.80 KB  	neil: superreview+
}}

Comment 23

[timeless]

PR_GetEnv is designed not to limit your ability to distinguish things that the underlying os allows.
one thiing it does do is guarantee that there's only one clibrary mucking w/ the env block so all your answers will be coherent.

restricting the ability of the client to distinguish things is not usually something nspr goes for. and i certainly don't want to ask nspr to change its behaviors and limit other clients.

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> PR_GetEnv is designed not to limit your ability to distinguish things that the
> underlying os allows.

Just a thought/question from an "outsider":
Would adding another function like "PR_GetEnv2()" which would call PR_GetEnv() and filter out the null-string case, that clients/callers could decide to use or not, be acceptable, or is it not the NSPR way either ?
If not good for NSPR, would it make sense to add something like a "Gecko_GetEnv()" function ??
(Or should we simply keep it the way it is currently ?)

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) <XpCom/nsAtomTable.cpp> (= sync'ed Bv2) [Checkin: Comment 26]

Cv1, without the whitespace cleanups, and "hg"...

Keeping
{{
(Av1) <XpFe/*> (-Bw for review)        patch       2005-09-02 08:05 PST     
4.08 KB      timeless: review+
(Av2) <XpFe/*> (-Bw for review)        patch       2005-11-08 16:39 PST     
3.80 KB      neil: superreview+
}}

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 379354 [details] [diff] [review]
(Cv1a) <XpCom/nsAtomTable.cpp> (= sync'ed Bv2)
[Checkin: Comment 26]


http://hg.mozilla.org/mozilla-central/rev/6012e6d2b1fa

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> Would adding another function like "PR_GetEnv2()" which would call PR_GetEnv()
> and filter out the null-string case, that clients/callers could decide to use
> or not, be acceptable, or is it not the NSPR way either ?

I filed bug 497667.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #18)

> Why can't we make PR_GetEnv be consistent across platforms and avoid this 
> mess?

Q: On what platform is the environment defined such that variables cannot 
have empty string values?   Not on Windows, MacOSX or (my) Linux.  So where?

Seems to me the problem is that there's some code, somewhere, that was written by coders who believe that setting an environment variable to an empty string 
unsets it.  The change that Timeless advocated in this bug is a concession to 
that belief, attempting to make Firefox behave consistently as if that is true.

Maybe what's really needed is to find and fix all the places that call
putenv("VAR=") thinking that will unset $VAR, because *that* is the practice
that is inconsistent across platforms.

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> Q: On what platform is the environment defined such that variables cannot 
> have empty string values?   Not on Windows, MacOSX or (my) Linux.  So where?

(I would have replied "Windows, as in |set var=| command line removes the var from the env", but maybe you're referring to something else.)

> Maybe what's really needed is to find and fix all the places that call
> putenv("VAR=") thinking that will unset $VAR, because *that* is the practice
> that is inconsistent across platforms.

If the cause is within Mozilla code only, then that would probably be the best way to solve it.
But I would have thought we could be dealing with vars set externally, which the only solution for would then be bug 497667.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #29)
> (I would have replied "Windows, as in |set var=| command line removes the var
> from the env", but maybe you're referring to something else.)

Ah.  That's apparently a limitation of the set command in Windows' command line
interpreter.  That's distinct from the behavior of the libc functions in the 
process.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Closing as no activity for 10 years...
Feel free to reopen if you disagree