Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to handle `os::execvpe` error.

2017-08-30 Thread Alexander Rojas 

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


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 _EXIT in `childMain` to handle `os::execvpe` error.

2017-08-29 Thread Benjamin Mahler 


> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > 
> >
> > 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.

2017-08-29 Thread Andrei Budnik 


> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > 
> >
> > 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 _EXIT in `childMain` to handle `os::execvpe` error.

2017-08-29 Thread James Peach 

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


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 _EXIT in `childMain` to handle `os::execvpe` error.

2017-08-25 Thread Andrei Budnik 


> On Aug. 24, 2017, 10:59 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Lines 200 (patched)
> > 
> >
> > 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.

2017-08-24 Thread James Peach 

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


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.

2017-08-24 Thread Andrei Budnik 


> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > 
> >
> > 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.

2017-08-24 Thread Alexander Rukletsov 


> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > 
> >
> > 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.

2017-08-23 Thread Andrei Budnik 


> On Aug. 22, 2017, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195-197 (patched)
> > 
> >
> > 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.

2017-08-22 Thread Alexander Rukletsov 

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


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