Linux: Add support for loading environment variables at plugin start-up from ...

Linux: Add support for loading environment variables at plugin start-up from a file on disk. This lets us set any needed environment variables in a cross-browser way (i.e., without trying to modify the browsers' program launcher scripts to set variables for us). This is needed on some 64-bit systems where an environment variable has to be set in order for MESA to know where to find its GPU drivers.

BUG=none
TEST=built, created envvars file, and put something in it; verified that it was set successfully


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30799

[piman, 2009-10-30 20:20:54]

http://codereview.chromium.org/340044/diff/4001/4002
File plugin/linux/main_linux.cc (right):

http://codereview.chromium.org/340044/diff/4001/4002#newcode62
Line 62: static const char *kEnvVarsFilePath = "/opt/google/o3d/envvars";
I think that will cause pains for the distribution packages (Ubuntu etc.)
Do you know if there's any way to find this file relative to the .so ?
Otherwise it'd be good do externalize this path into the a scons/gyp variable so
that it's more easily changeable.

http://codereview.chromium.org/340044/diff/4001/4002#newcode629
Line 629: 
Could you break all of this into a separate function ? If possible a separate
file with a unit test, string parsing always makes me nervous.

http://codereview.chromium.org/340044/diff/4001/4002#newcode637
Line 637: // dependencies.
Note that InitializePlugin can be called several times in the same process (e.g.
if Firefox reloads plugins). I don't think it causes problems here, but just in
case you do.

http://codereview.chromium.org/340044/diff/4001/4002#newcode642
Line 642: char buf[kMaxEnvVarLength];
buf -> buffer

http://codereview.chromium.org/340044/diff/4001/4002#newcode647
Line 647: LOG(ERROR) << "Malformed env var line";
s/env var/environment variable/ here and other places

http://codereview.chromium.org/340044/diff/4001/4002#newcode648
Line 648: continue;
I'd abort the parsing at this point. If it turns out you have a long line in the
file, you will end up parsing the second part as if it was a full line and may
have an unintended side effect.

http://codereview.chromium.org/340044/diff/4001/4002#newcode652
Line 652: int len = strlen(value);
s/len/length/

[Tristan Schmelcher 2, 2009-11-02 13:59:30]

http://codereview.chromium.org/340044/diff/4001/4002
File plugin/linux/main_linux.cc (right):

http://codereview.chromium.org/340044/diff/4001/4002#newcode62
Line 62: static const char *kEnvVarsFilePath = "/opt/google/o3d/envvars";
On 2009/10/30 20:20:54, piman wrote:
> I think that will cause pains for the distribution packages (Ubuntu etc.)
> Do you know if there's any way to find this file relative to the .so ?
> Otherwise it'd be good do externalize this path into the a scons/gyp variable
so
> that it's more easily changeable.

I think finding it relative to the .so might be an even bigger headache for some
distributions if they want the .so file and data in different paths, for
example. I've made it a build define, though I don't see any way in GYP to make
it overriddable via the command-line. :(

http://codereview.chromium.org/340044/diff/4001/4002#newcode629
Line 629: 
On 2009/10/30 20:20:54, piman wrote:
> Could you break all of this into a separate function ? If possible a separate
> file with a unit test, string parsing always makes me nervous.

Done. No unit tests yet though b/c the unit test binary on Linux with GYP
doesn't run yet (missing rpath settings). I'll work on fixing that later.

http://codereview.chromium.org/340044/diff/4001/4002#newcode637
Line 637: // dependencies.
On 2009/10/30 20:20:54, piman wrote:
> Note that InitializePlugin can be called several times in the same process
(e.g.
> if Firefox reloads plugins). I don't think it causes problems here, but just
in
> case you do.

I know. That won't cause problems, it'll just overwrite the previous definitions
with the same value.

http://codereview.chromium.org/340044/diff/4001/4002#newcode642
Line 642: char buf[kMaxEnvVarLength];
On 2009/10/30 20:20:54, piman wrote:
> buf -> buffer

Done.

http://codereview.chromium.org/340044/diff/4001/4002#newcode647
Line 647: LOG(ERROR) << "Malformed env var line";
On 2009/10/30 20:20:54, piman wrote:
> s/env var/environment variable/ here and other places

Done.

http://codereview.chromium.org/340044/diff/4001/4002#newcode648
Line 648: continue;
On 2009/10/30 20:20:54, piman wrote:
> I'd abort the parsing at this point. If it turns out you have a long line in
the
> file, you will end up parsing the second part as if it was a full line and may
> have an unintended side effect.

That wouldn't really protect against long lines, because if the end of the value
is truncated but the equals is still there then the same problem happens. I'd
rather keep it as-is so that it's robust against malformed lines (i.e., the
following lines are still parsed correctly in the event of a missing equals).

http://codereview.chromium.org/340044/diff/4001/4002#newcode652
Line 652: int len = strlen(value);
On 2009/10/30 20:20:54, piman wrote:
> s/len/length/

Done.

[piman, 2009-11-02 19:06:01]

LGTM, but could you add the standard copyright header on top of each new file ?

http://codereview.chromium.org/340044/diff/9001/5003
File plugin/linux/envvars.cc (right):

http://codereview.chromium.org/340044/diff/9001/5003#newcode1
Line 1: #include <stdio.h>
Could you add the copyright header to this file ?

http://codereview.chromium.org/340044/diff/9001/5004
File plugin/linux/envvars.h (right):

http://codereview.chromium.org/340044/diff/9001/5004#newcode1
Line 1: #ifndef O3D_PLUGIN_LINUX_ENVVARS_H_
Could you add the copyright header to this file ?

[piman, 2009-11-02 19:06:56]

On 2009/11/02 13:59:30, tschmelcher wrote:
> http://codereview.chromium.org/340044/diff/4001/4002
> File plugin/linux/main_linux.cc (right):
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode62
> Line 62: static const char *kEnvVarsFilePath = "/opt/google/o3d/envvars";
> On 2009/10/30 20:20:54, piman wrote:
> > I think that will cause pains for the distribution packages (Ubuntu etc.)
> > Do you know if there's any way to find this file relative to the .so ?
> > Otherwise it'd be good do externalize this path into the a scons/gyp
variable
> so
> > that it's more easily changeable.
> 
> I think finding it relative to the .so might be an even bigger headache for
some
> distributions if they want the .so file and data in different paths, for
> example. I've made it a build define, though I don't see any way in GYP to
make
> it overriddable via the command-line. :(

The way to do this is through a gyp variable. Let's worry about that later
maybe.

> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode629
> Line 629: 
> On 2009/10/30 20:20:54, piman wrote:
> > Could you break all of this into a separate function ? If possible a
separate
> > file with a unit test, string parsing always makes me nervous.
> 
> Done. No unit tests yet though b/c the unit test binary on Linux with GYP
> doesn't run yet (missing rpath settings). I'll work on fixing that later.
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode637
> Line 637: // dependencies.
> On 2009/10/30 20:20:54, piman wrote:
> > Note that InitializePlugin can be called several times in the same process
> (e.g.
> > if Firefox reloads plugins). I don't think it causes problems here, but just
> in
> > case you do.
> 
> I know. That won't cause problems, it'll just overwrite the previous
definitions
> with the same value.
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode642
> Line 642: char buf[kMaxEnvVarLength];
> On 2009/10/30 20:20:54, piman wrote:
> > buf -> buffer
> 
> Done.
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode647
> Line 647: LOG(ERROR) << "Malformed env var line";
> On 2009/10/30 20:20:54, piman wrote:
> > s/env var/environment variable/ here and other places
> 
> Done.
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode648
> Line 648: continue;
> On 2009/10/30 20:20:54, piman wrote:
> > I'd abort the parsing at this point. If it turns out you have a long line in
> the
> > file, you will end up parsing the second part as if it was a full line and
may
> > have an unintended side effect.
> 
> That wouldn't really protect against long lines, because if the end of the
value
> is truncated but the equals is still there then the same problem happens. I'd
> rather keep it as-is so that it's robust against malformed lines (i.e., the
> following lines are still parsed correctly in the event of a missing equals).
> 
> http://codereview.chromium.org/340044/diff/4001/4002#newcode652
> Line 652: int len = strlen(value);
> On 2009/10/30 20:20:54, piman wrote:
> > s/len/length/
> 
> Done.

[Tristan Schmelcher 2, 2009-11-03 06:03:23]

http://codereview.chromium.org/340044/diff/9001/5003
File plugin/linux/envvars.cc (right):

http://codereview.chromium.org/340044/diff/9001/5003#newcode1
Line 1: #include <stdio.h>
On 2009/11/02 19:06:01, piman wrote:
> Could you add the copyright header to this file ?

Oh right, duh. Done.

http://codereview.chromium.org/340044/diff/9001/5004
File plugin/linux/envvars.h (right):

http://codereview.chromium.org/340044/diff/9001/5004#newcode1
Line 1: #ifndef O3D_PLUGIN_LINUX_ENVVARS_H_
On 2009/11/02 19:06:01, piman wrote:
> Could you add the copyright header to this file ?

Oh right, duh. Done.

[Tristan Schmelcher 2, 2009-11-03 06:04:12]

On 2009/11/02 19:06:56, piman wrote:
> On 2009/11/02 13:59:30, tschmelcher wrote:
> > http://codereview.chromium.org/340044/diff/4001/4002
> > File plugin/linux/main_linux.cc (right):
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode62
> > Line 62: static const char *kEnvVarsFilePath = "/opt/google/o3d/envvars";
> > On 2009/10/30 20:20:54, piman wrote:
> > > I think that will cause pains for the distribution packages (Ubuntu etc.)
> > > Do you know if there's any way to find this file relative to the .so ?
> > > Otherwise it'd be good do externalize this path into the a scons/gyp
> variable
> > so
> > > that it's more easily changeable.
> > 
> > I think finding it relative to the .so might be an even bigger headache for
> some
> > distributions if they want the .so file and data in different paths, for
> > example. I've made it a build define, though I don't see any way in GYP to
> make
> > it overriddable via the command-line. :(
> 
> The way to do this is through a gyp variable. Let's worry about that later
> maybe.

I tried that already actually, but I couldn't find a way to override the value
on the command-line, and I didn't see any documentation about that anywhere.

Submitting.

> 
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode629
> > Line 629: 
> > On 2009/10/30 20:20:54, piman wrote:
> > > Could you break all of this into a separate function ? If possible a
> separate
> > > file with a unit test, string parsing always makes me nervous.
> > 
> > Done. No unit tests yet though b/c the unit test binary on Linux with GYP
> > doesn't run yet (missing rpath settings). I'll work on fixing that later.
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode637
> > Line 637: // dependencies.
> > On 2009/10/30 20:20:54, piman wrote:
> > > Note that InitializePlugin can be called several times in the same process
> > (e.g.
> > > if Firefox reloads plugins). I don't think it causes problems here, but
just
> > in
> > > case you do.
> > 
> > I know. That won't cause problems, it'll just overwrite the previous
> definitions
> > with the same value.
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode642
> > Line 642: char buf[kMaxEnvVarLength];
> > On 2009/10/30 20:20:54, piman wrote:
> > > buf -> buffer
> > 
> > Done.
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode647
> > Line 647: LOG(ERROR) << "Malformed env var line";
> > On 2009/10/30 20:20:54, piman wrote:
> > > s/env var/environment variable/ here and other places
> > 
> > Done.
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode648
> > Line 648: continue;
> > On 2009/10/30 20:20:54, piman wrote:
> > > I'd abort the parsing at this point. If it turns out you have a long line
in
> > the
> > > file, you will end up parsing the second part as if it was a full line and
> may
> > > have an unintended side effect.
> > 
> > That wouldn't really protect against long lines, because if the end of the
> value
> > is truncated but the equals is still there then the same problem happens.
I'd
> > rather keep it as-is so that it's robust against malformed lines (i.e., the
> > following lines are still parsed correctly in the event of a missing
equals).
> > 
> > http://codereview.chromium.org/340044/diff/4001/4002#newcode652
> > Line 652: int len = strlen(value);
> > On 2009/10/30 20:20:54, piman wrote:
> > > s/len/length/
> > 
> > Done.