> 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 > >
