> On Feb. 19, 2016, 2:32 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 329
> > <https://reviews.apache.org/r/43271/diff/2/?file=1254918#file1254918line329>
> >
> >     There are so many things I'd like to comment on here, please allow me 
> > to just go for it and implement all the changes that I recommend, then 
> > compare. 
> >     
> >         static map<string, string> subprocess_environment(
> >             const Option<map<string, string>>& environment)
> >         {
> >           // Check whether this environment comes with a conflicting
> >           // `LIBPROCESS_PORT` value. Warn if so.
> >           if (environment.isSome() && 
> > environment.get().count("LIBPROCESS_PORT") > 0) {     
> >             Option<string> port = os::getenv("LIBPROCESS_PORT");
> >             if (port.isSome() && port.get() == 
> > environment.get()["LIBPROCESS_PORT"]) {
> >               LOG(WARNING)
> >                 << "Subprocess's LIBPROCESS_PORT environment variable "
> >                 << "conflicts with its parent process's LIBPROCESS_PORT; "
> >                 << "dropping LIBPROCESS_PORT from subprocess's 
> > environment.";
> >             }
> >           }
> >     
> >           map<string, string> result = 
> > environment.getOrElse(os::environment());
> >     
> >           // Remove the libprocess port variables so that the subprocess
> >           // will not attempt to bind to an already occupied port.
> >           result.erase("LIBPROCESS_PORT");
> >           result.erase("LIBPROCESS_ADVERTISE_PORT");
> >     
> >           // NOTE: We retain the IP variables ("LIBPROCESS_IP" and
> >           // "LIBPROCESS_ADVERTISE_IP") because, if DNS is not available in 
> > the
> >           // subprocess, the absence of the IP variables will cause an error
> >           // when libprocess attempts a hostname lookup.
> >     
> >           return result;
> >         }

Note: We can't use the `operator[]` because it's not const.


> On Feb. 19, 2016, 2:32 a.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 450
> > <https://reviews.apache.org/r/43271/diff/2/?file=1254918#file1254918line450>
> >
> >     You can use 'subprocess_environment(environment)' here without the temp 
> > var, avoiding the ugly underscore naming. 
> >     
> >     If you ewant to keep it, though, the underscore goes to the end: 
> > "environment_". And then you should use const ref.

Note: I pulled out the temp var since we will never fully inherit the 
environment with this change.  So the surrounding `if` (in the original) is no 
longer necessary.


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43271/#review119851
-----------------------------------------------------------


On Feb. 19, 2016, 10:54 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43271/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2016, 10:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4609
>     https://issues.apache.org/jira/browse/MESOS-4609
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> * Adds a helper method for getting the current environment plus 
> considerations for libprocess.
> * Changes the default behavior of `process::subprocess` to use the above 
> helper when given `environment = None()`.
> * Adds a warning inside `process::subprocess` if `LIBPROCESS_PORT` conflicts 
> with the current process's `LIBPROCESS_PORT`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44ca6d0869f3dbcfda1ac01d0d6b79dc20c4267c 
> 
> Diff: https://reviews.apache.org/r/43271/diff/
> 
> 
> Testing
> -------
> 
> make
> 
> Tests are run in the next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Reply via email to