----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67286/#review203795 -----------------------------------------------------------
3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 23 (patched) <https://reviews.apache.org/r/67286/#comment286092> Do you know if this header might include `windows.h`? If it does, we should move this `#include` to `stout/windows.hpp` and then include that here to preserve Windows header ordering. 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 33 (patched) <https://reviews.apache.org/r/67286/#comment286109> const ref? 3rdparty/stout/include/stout/internal/windows/inherit.hpp Lines 40 (patched) <https://reviews.apache.org/r/67286/#comment286093> We should document in a comment the arguments to this system call. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 252-255 (original), 253-257 (patched) <https://reviews.apache.org/r/67286/#comment286095> Just a style suggestion, but we could collapse this to: ``` DWORD creation_flags = EXTENDED_STARTUPINFO_PRESENT | CREATE_UNICODE_ENVIRONMENT; if (create_suspended) { creation_flags |= CREATE_SUSPENDED; } ``` and update the comment (or rather, delete the comment because it's quite superfluous... my bad, pretty sure I wrote it). 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 274 (patched) <https://reviews.apache.org/r/67286/#comment286098> s/inherted/inherited But really, this comment should describe what we're doing here in detail, as it's rather odd. (1) We're setting the pipe handles and whitelisted handles to be temporarily inheritable. (2) We're explicitly whitelisting the handles using a Windows API. (3) We're then setting the handles to back to non-inheritable. Maybe a little more detailed than that too. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 276-277 (patched) <https://reviews.apache.org/r/67286/#comment286096> Style: collapse. I think the line break is from an old iteartion, but the type name got wayyy shorter. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 284 (patched) <https://reviews.apache.org/r/67286/#comment286101> I know we're not consistent about this, but let's be explicit here so it's clear why we're not just using the `pipes` array: `handles.emplace_back(static_cast<HANDLE>(fd))`. This will also lessen the work to make the cast operators of `int_fd` explicit (which I really should have already done...). This should also be a line up so that we don't split the `Try` from its associated `if (isError())` 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 276-279 (original), 293-296 (patched) <https://reviews.apache.org/r/67286/#comment286097> Delete this change. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 305 (patched) <https://reviews.apache.org/r/67286/#comment286102> Same note as above. 3rdparty/stout/include/stout/os/windows/shell.hpp Line 287 (original), 312 (patched) <https://reviews.apache.org/r/67286/#comment286103> Can't we declare and assign here? I don't think we need to declare it above. 3rdparty/stout/include/stout/os/windows/shell.hpp Lines 320 (patched) <https://reviews.apache.org/r/67286/#comment286104> We should set this as soon as we've declared `startup_info_ex`. - Andrew Schwartzmeyer On May 24, 2018, 10:50 a.m., Radhika Jandhyala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67286/ > ----------------------------------------------------------- > > (Updated May 24, 2018, 10:50 a.m.) > > > Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Li > Li, and Radhika Jandhyala. > > > Bugs: MESOS-8926 > https://issues.apache.org/jira/browse/MESOS-8926 > > > Repository: mesos > > > Description > ------- > > White list fds that child processes can inherit in stout. > > > Diffs > ----- > > 3rdparty/stout/include/stout/internal/windows/inherit.hpp > 7dbde820e775cbaeb8db4bc4559ab432903e75ea > 3rdparty/stout/include/stout/os/windows/shell.hpp > 8da612af2888ff4d4d458ea5b16cdb08779b6f4c > > > Diff: https://reviews.apache.org/r/67286/diff/1/ > > > Testing > ------- > > All Mesos-tests > > > Thanks, > > Radhika Jandhyala > >
