----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71976/#review219222 -----------------------------------------------------------
Fix it, then Ship it! src/linux/systemd.cpp Lines 346 (patched) <https://reviews.apache.org/r/71976/#comment307370> Can you declare and initilize in one go? ``` int flags = ... ``` src/linux/systemd.cpp Lines 351-355 (patched) <https://reviews.apache.org/r/71976/#comment307371> Let's also declare and init in one go. Using a ternary operator shouldn't be too bad. src/linux/systemd.cpp Lines 384 (patched) <https://reviews.apache.org/r/71976/#comment307376> It might be helpful to rephrase the `Error` so we can show `*listenPidEnv` in quotes. src/linux/systemd.cpp Lines 385 (patched) <https://reviews.apache.org/r/71976/#comment307373> No period at the end of this `Error`. src/linux/systemd.cpp Lines 402 (patched) <https://reviews.apache.org/r/71976/#comment307377> It might be helpful to rephrase the `Error` so we can show `*listenPidEnv` in quotes. src/linux/systemd.cpp Lines 403 (patched) <https://reviews.apache.org/r/71976/#comment307374> No period at the end of this `Error`. src/linux/systemd.cpp Lines 408 (patched) <https://reviews.apache.org/r/71976/#comment307375> Remove period. src/linux/systemd.cpp Lines 414 (patched) <https://reviews.apache.org/r/71976/#comment307378> Can we add context here which fd we couldn't set cloexec for? ``` return Error("Could not set CLOEXEC for filedescriptor ...: + " cloexec.error()); ``` src/linux/systemd.cpp Lines 419 (patched) <https://reviews.apache.org/r/71976/#comment307372> Let's add spaces around `+` and a newline before `return`. src/linux/systemd.cpp Lines 458 (patched) <https://reviews.apache.org/r/71976/#comment307380> Can you document somewhere very prominently that this function is not safe to call from multiple threads since it mutates env? src/linux/systemd.cpp Lines 460-462 (patched) <https://reviews.apache.org/r/71976/#comment307379> Let's use `os::unsetenv` for "consistency". - Benjamin Bannier On Jan. 10, 2020, 2:48 a.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71976/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2020, 2:48 a.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Added support for the systemd socket activation api, > that allows systemd to pass listening file descriptors > to a given service. > > > Diffs > ----- > > src/linux/systemd.hpp f760e2c02683f0169ba5d3b0a3da53f66a7f3d91 > src/linux/systemd.cpp 8126b31a542b62ce51cc6e0f6372986bffcc948b > > > Diff: https://reviews.apache.org/r/71976/diff/1/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
