You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2017/08/21 20:54:28 UTC
Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin Mahler, and James Peach.
Bugs: MESOS-7791
https://issues.apache.org/jira/browse/MESOS-7791
Repository: mesos
Description
-------
Using ABORT causes coredump when path to an executable is invalid.
In addition, using of _EXIT allows us to get rid of string
concatenation, thereby making `childMain` more async-signal safe.
Diffs
-----
3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
Diff: https://reviews.apache.org/r/61799/diff/1/
Testing
-------
sudo make check (mac os x, fedora 25)
internal CI
Thanks,
Andrei Budnik
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 24, 2017, 10:59 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/61799/diff/2/?file=1802876#file1802876line200>
> >
> > Is this required to be async-signal-safe?
After latest changes `_EXIT` supports formatted output.
I've eliminated signal unsafe dependency to `os::strerror`.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183758
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/2/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183758
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess_posix.hpp
Lines 200 (patched)
<https://reviews.apache.org/r/61799/#comment259814>
Is this required to be async-signal-safe?
- James Peach
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/2/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > <https://reviews.apache.org/r/61799/diff/3/?file=1803750#file1803750line195>
> >
> > Hmm. I thought about this some more and I think we should retain the `strerror` here.
> >
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> >
> > Whay do you think?
>
> Andrei Budnik wrote:
> Ok, reverted.
> In practice on Linux, malloc after fork is safe.
Is this true? I seem to recall seeing potential deadlocks deadlocks due to this. One example was [MESOS-7858](https://issues.apache.org/jira/browse/MESOS-7858), where some code in glibc was doing an bad assert. Sometimes when this fails, the malloc needed for assert to print would deadlock.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184060
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/4/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > <https://reviews.apache.org/r/61799/diff/3/?file=1803750#file1803750line195>
> >
> > Hmm. I thought about this some more and I think we should retain the `strerror` here.
> >
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> >
> > Whay do you think?
Ok, reverted.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184060
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/4/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with SAFE_EXIT in childhook
in `subprocess_posix.hpp`.
Posted by James Peach <jp...@apache.org>.
> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > <https://reviews.apache.org/r/61799/diff/3/?file=1803750#file1803750line195>
> >
> > Hmm. I thought about this some more and I think we should retain the `strerror` here.
> >
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> >
> > Whay do you think?
>
> Andrei Budnik wrote:
> Ok, reverted.
>
> Benjamin Mahler wrote:
> > In practice on Linux, malloc after fork is safe.
>
> Is this true? I seem to recall seeing potential deadlocks deadlocks due to this. One example was [MESOS-7858](https://issues.apache.org/jira/browse/MESOS-7858), where some code in glibc was doing an bad assert. Sometimes when this fails, the malloc needed for assert to print would deadlock.
>
> Andrei Budnik wrote:
> Taking into account previous comment from Benjamin, I've removed usage of unsafe `strerror`.
> Is this true? I seem to recall seeing potential deadlocks deadlocks due to this. One example was MESOS-7858, where some code in glibc was doing an bad assert. Sometimes when this fails, the malloc needed for assert to print would deadlock.
It's true in practice but clearly you can get bitten by bugs and funky atfork handlers. IMHO `strerror` here is fine for the reasons given above.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184060
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of SAFE_EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/5/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with SAFE_EXIT in childhook
in `subprocess_posix.hpp`.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > <https://reviews.apache.org/r/61799/diff/3/?file=1803750#file1803750line195>
> >
> > Hmm. I thought about this some more and I think we should retain the `strerror` here.
> >
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> >
> > Whay do you think?
>
> Andrei Budnik wrote:
> Ok, reverted.
>
> Benjamin Mahler wrote:
> > In practice on Linux, malloc after fork is safe.
>
> Is this true? I seem to recall seeing potential deadlocks deadlocks due to this. One example was [MESOS-7858](https://issues.apache.org/jira/browse/MESOS-7858), where some code in glibc was doing an bad assert. Sometimes when this fails, the malloc needed for assert to print would deadlock.
Taking into account previous comment from Benjamin, I've removed usage of unsafe `strerror`.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184060
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of SAFE_EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/5/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184060
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/src/subprocess_posix.hpp
Line 194 (original), 195 (patched)
<https://reviews.apache.org/r/61799/#comment260061>
Hmm. I thought about this some more and I think we should retain the `strerror` here.
1. We still used it in other async-signal-safe situations.
2. In glibc it only mallocs when you give it an unknown error number.
3. In practice on Linux, malloc after fork is safe.
4. The error message is pretty useful.
Whay do you think?
- James Peach
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/3/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with SAFE_EXIT in childhook
in `subprocess_posix.hpp`.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 30, 2017, 7:10 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Lines 198-200 (patched)
> > <https://reviews.apache.org/r/61799/diff/4/?file=1807242#file1807242line198>
> >
> > Sometimes for debugging is nice to have the errno, could you do it like:
> >
> > ```c++
> > "Failed to os::exvpe on path '%s': (%d) %s",
> > path.c_str(),
> > error,
> > os::strerror(error).c_str()
> > ```
As `strerror` isn't allowed due to its async-signal unsafety,
```cpp
SAFE_EXIT(errno, "Failed to os::execvpe on path '%s': %d", path.c_str(), errno);
```
is used instead.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184132
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of SAFE_EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/5/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184132
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess_posix.hpp
Lines 198-200 (patched)
<https://reviews.apache.org/r/61799/#comment260195>
Sometimes for debugging is nice to have the errno, could you do it like:
```c++
"Failed to os::exvpe on path '%s': (%d) %s",
path.c_str(),
error,
os::strerror(error).c_str()
```
- Alexander Rojas
On Aug. 21, 2017, 10:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/4/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with SAFE_EXIT in childhook
in `subprocess_posix.hpp`.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review184783
-----------------------------------------------------------
Ship it!
Ship It!
- James Peach
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of SAFE_EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/6/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > <https://reviews.apache.org/r/61799/diff/1/?file=1801034#file1801034line195>
> >
> > Is this what `clang-format` suggests?
>
> Andrei Budnik wrote:
> `clang-format` suggests
> ```cpp
> _EXIT(
> errno, "Failed to os::execvpe on path '", path, "': ", os::strerror(errno));
> ```
> which is wrong, because code style obliges us to put 4 spaces after `(`. Also, I can't add 2 more spaces, because of 80-charactes limit.
>
> Alexander Rukletsov wrote:
> Then you should go for
> ```
> _EXIT(
> errno,
> "Failed to os::execvpe on path '",
> path,
> "': ",
> os::strerror(errno));
> ```
Fixed.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183463
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/2/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > <https://reviews.apache.org/r/61799/diff/1/?file=1801034#file1801034line195>
> >
> > Is this what `clang-format` suggests?
>
> Andrei Budnik wrote:
> `clang-format` suggests
> ```cpp
> _EXIT(
> errno, "Failed to os::execvpe on path '", path, "': ", os::strerror(errno));
> ```
> which is wrong, because code style obliges us to put 4 spaces after `(`. Also, I can't add 2 more spaces, because of 80-charactes limit.
Then you should go for
```
_EXIT(
errno,
"Failed to os::execvpe on path '",
path,
"': ",
os::strerror(errno));
```
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183463
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/1/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > <https://reviews.apache.org/r/61799/diff/1/?file=1801034#file1801034line195>
> >
> > Is this what `clang-format` suggests?
`clang-format` suggests
```cpp
_EXIT(
errno, "Failed to os::execvpe on path '", path, "': ", os::strerror(errno));
```
which is wrong, because code style obliges us to put 4 spaces after `(`. Also, I can't add 2 more spaces, because of 80-charactes limit.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183463
-----------------------------------------------------------
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/1/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to
handle `os::execvpe` error.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61799/#review183463
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess_posix.hpp
Line 194 (original), 195-197 (patched)
<https://reviews.apache.org/r/61799/#comment259477>
Is this what `clang-format` suggests?
- Alexander Rukletsov
On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:54 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin Mahler, and James Peach.
>
>
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/subprocess_posix.hpp b478beccff22e2ffa08b9b91cfd5b9c6ada9b697
>
>
> Diff: https://reviews.apache.org/r/61799/diff/1/
>
>
> Testing
> -------
>
> sudo make check (mac os x, fedora 25)
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>