mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 67286: White list fds that child processes can inherit in stout.
Date Thu, 24 May 2018 18:42:35 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message