mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 68644: Closed all file descriptors except `whitelist_fds` in posix/subprocess.
Date Tue, 16 Oct 2018 15:45:09 GMT


> On Oct. 12, 2018, 5:40 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/posix/subprocess.hpp
> > Lines 213 (patched)
> > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line213>
> >
> >     Since you are planning a different code path for macOS, maybe hoist this out
into a static support function in preparation?
> 
> Qian Zhang wrote:
>     Did you mean we should put those codes into a static function like `static int convertStringToInt(const
char *name)`?

Yes, exactly.


> On Oct. 12, 2018, 5:40 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/posix/subprocess.hpp
> > Lines 255 (patched)
> > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line255>
> >
> >     You can just use `std::find()` here.
> 
> Qian Zhang wrote:
>     That's what I was thinking. But it seems `std::find()` may allocate memory (search
`allocate memory` in https://en.cppreference.com/w/cpp/algorithm/find )?

Oh, I guess that it's not guaranteed to not allocate. Let's drop this then.


> On Oct. 12, 2018, 5:40 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/posix/subprocess.hpp
> > Lines 275 (patched)
> > <https://reviews.apache.org/r/68644/diff/2/?file=2096378#file2096378line275>
> >
> >     Unfortunately, the `Try` here is not async-signal-safe. However, that is already
used by `UNSET_CLOEXEC`, so I think we can just leave a TODO here.
> >     
> >     Can you file a JIRA to add something like `signal_save::uncloexec()`?
> 
> Qian Zhang wrote:
>     I see we also use `Try` in another place in `childMain`, e.g., `Try<Nothing>
callback = hook();`, so that one needs to be changed too?
>     
>     And I'd like to call `fcntl` here directly to unset the `close-on-exec` flag.

> I see we also use Try in another place in childMain, e.g., Try<Nothing> callback
= hook();, so that one needs to be changed too?

Yes, in principle. I don't think we need to address that here though.

> And I'd like to call fcntl here directly to unset the close-on-exec flag.

That seems fine to me. Previously, we put helpers in the `signal_safe` namespace, but having
a local helper for this case seems OK too.


- James


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


On Oct. 14, 2018, 2:05 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68644/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2018, 2:05 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9152 and MESOS-9164
>     https://issues.apache.org/jira/browse/MESOS-9152
>     https://issues.apache.org/jira/browse/MESOS-9164
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Closed all file descriptors except `whitelist_fds` in posix/subprocess.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/posix/subprocess.hpp 007058b61fdcd4716aa793516c842c3cef8c0a29

>   3rdparty/libprocess/src/subprocess.cpp c0640de2dc4278b884282dfaad98c49c3b067a5b 
> 
> 
> Diff: https://reviews.apache.org/r/68644/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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