Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

(Updated June 13, 2018, 6:37 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
Yu, Li Li, and Radhika Jandhyala.


Changes
---

Rebase


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 (updated)
-

  3rdparty/stout/include/stout/internal/windows/inherit.hpp 
7dbde820e775cbaeb8db4bc4559ab432903e75ea 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
8afcaa6dee8a4216e0ae54c6d94209c001046b10 


Diff: https://reviews.apache.org/r/67286/diff/5/

Changes: https://reviews.apache.org/r/67286/diff/4-5/


Testing
---

All Mesos-tests


Thanks,

Radhika Jandhyala

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

(Updated June 12, 2018, 10:56 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
Yu, 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 (updated)
-

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

Changes: https://reviews.apache.org/r/67286/diff/3-4/


Testing
---

All Mesos-tests


Thanks,

Radhika Jandhyala

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

(Updated June 12, 2018, 9:08 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
Yu, 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 (updated)
-

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

Changes: https://reviews.apache.org/r/67286/diff/2-3/


Testing
---

All Mesos-tests


Thanks,

Radhika Jandhyala

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Andrew Schwartzmeyer]

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


Ship it!





3rdparty/stout/include/stout/internal/windows/inherit.hpp
Lines 55 (patched)


`DeleteProcThreadAttributeList` doesn't seem to care if it fails, as it 
returns nothing, so to make the logic easier, we just bundled it into the 
destructor.


- Andrew Schwartzmeyer


On May 24, 2018, 3:47 p.m., Radhika Jandhyala wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67286/
> ---
> 
> (Updated May 24, 2018, 3:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
> Yu, 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/2/
> 
> 
> Testing
> ---
> 
> All Mesos-tests
> 
> 
> Thanks,
> 
> Radhika Jandhyala
> 
>

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Akash Gupta]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/internal/windows/inherit.hpp
Lines 55 (patched)


Note that this destructor is called regardless of the state of 
`attribute_list`. So, it will call `::DeleteProcThreadAttributeList()` for 
these two cases, where `::InitializeProcThreadAttributeList` hasn't succeeded 
yet.

```
  if (attribute_list == nullptr) {
return WindowsError(ERROR_OUTOFMEMORY);
// Destructor calls `::DeleteProc...(nullptr)`
  }

  result =
::InitializeProcThreadAttributeList(attribute_list.get(), 1, 0, );
  if (result == FALSE) {
return WindowsError();
// Destructor calls `::DeleteProc...()` on uninitialized attribute_list.
  }
```



3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 245 (patched)


I think the mesos empty vector style is `const std::vector& 
whitelist_fds = {}` .


- Akash Gupta


On May 24, 2018, 10:47 p.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:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
> Yu, 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/2/
> 
> 
> Testing
> ---
> 
> All Mesos-tests
> 
> 
> Thanks,
> 
> Radhika Jandhyala
> 
>

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

> On May 24, 2018, 6:42 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/internal/windows/inherit.hpp
> > Lines 23 (patched)
> > 
> >
> > 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.

It does not include windows.h


- Radhika


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


On May 24, 2018, 10:47 p.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:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
> Yu, 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/2/
> 
> 
> Testing
> ---
> 
> All Mesos-tests
> 
> 
> Thanks,
> 
> Radhika Jandhyala
> 
>

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

(Updated May 24, 2018, 10:47 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
Yu, Li Li, and Radhika Jandhyala.


Bugs: MESOS-8926
https://issues.apache.org/jira/browse/MESOS-8926


Repository: mesos


Description (updated)
---

White list fds that child processes can inherit in stout.


Diffs (updated)
-

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

Changes: https://reviews.apache.org/r/67286/diff/1-2/


Testing
---

All Mesos-tests


Thanks,

Radhika Jandhyala

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

(Updated May 24, 2018, 6:44 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Eric Mumau, Jie 
Yu, Li Li, and Radhika Jandhyala.


Bugs: MESOS-8926
https://issues.apache.org/jira/browse/MESOS-8926


Repository: mesos


Description (updated)
---

Stout: White list fds that child processes can inherit.


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

Re: Review Request 67286: White list fds that child processes can inherit in stout.

[Andrew Schwartzmeyer]

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


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)


const ref?



3rdparty/stout/include/stout/internal/windows/inherit.hpp
Lines 40 (patched)


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)


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)


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)


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)


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


Delete this change.



3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 305 (patched)


Same note as above.



3rdparty/stout/include/stout/os/windows/shell.hpp
Line 287 (original), 312 (patched)


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)


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

Review Request 67286: White list fds that child processes can inherit in stout.

[Radhika Jandhyala via Review Board]

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

Review request for mesos, Akash Gupta and Andrew Schwartzmeyer.


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