You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/02/27 01:30:48 UTC
Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review36314
-----------------------------------------------------------
Ship it!
I'm going to commit this now!
- Benjamin Hindman
On March 6, 2014, 1 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated March 6, 2014, 1 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated March 5, 2014, 5 p.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated March 5, 2014, 5 p.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated March 5, 2014, 4:28 p.m.)
Review request for mesos and Benjamin Hindman.
Summary (updated)
-----------------
Introduced CHECK_* for Future and use of ABORT_MSG
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated March 5, 2014, 2:03 p.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated Feb. 27, 2014, 11:19 a.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/
-----------------------------------------------------------
(Updated Feb. 27, 2014, 11:18 a.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1041
https://issues.apache.org/jira/browse/MESOS-1041
Repository: mesos-git
Description
-------
see summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
3rdparty/libprocess/include/process/check.hpp PRE-CREATION
3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
Diff: https://reviews.apache.org/r/18550/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 27, 2014, 11:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 1089
> > <https://reviews.apache.org/r/18550/diff/1/?file=505252#file505252line1089>
> >
> > We're losing information with this, did you want 'CHECK_READY(*this)' instead?
>
> Benjamin Hindman wrote:
> This doesn't look resolved?
i had a CHECK_READY, but that introduced a circular dependency between check.hpp and future.hpp.
I'll put back the full version (unless you can think of a way to avoid the dependency).
> On Feb. 27, 2014, 11:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 151-152
> > <https://reviews.apache.org/r/18550/diff/1/?file=505253#file505253line151>
> >
> > The 'strings::join' is likely not async signal safe since it does allocation. If you'd like to clean up this code it seems like we need to improve ABORT. One suggestion: provide an overload to '_abort' which takes 10 const char* parameters with defaults set to NULL, then be able to do:
> >
> > ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n");
> >
> > Also, please kill the whitespace here.
> >
>
> Dominic Hamon wrote:
> wouldn't i have to do something like http://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros ?
>
> maybe in this case it's better to lose the information and just abort with 'failed to execl'. maybe log the command attempt to info before the attempt?
>
> Benjamin Hindman wrote:
> You could get away with doing something simpler with variadic macros (which we already assume other places). Consider:
>
> #define ABORT(...) _Abort(_ABORT_PREFIX)(__VA_ARGS__)
>
> struct _Abort
> {
> _Abort(const char* _prefix) : prefix(_prefix) {}
>
> void operator () (
> const char* arg0 = "",
> const char* arg1 = "",
> const char* arg2 = "",
> const char* arg3 = "",
> const char* arg4 = "",
> const char* arg5 = "",
> const char* arg6 = "",
> const char* arg7 = "",
> const char* arg8 = "",
> const char* arg9 = "")
> {
> // Write the failure message in an async-signal safe manner,
> // assuming strlen is async-signal safe or optimized out.
> // In fact, it is highly unlikely that strlen would be
> // implemented in an unsafe manner:
> // http://austingroupbugs.net/view.php?id=692
> while (write(STDERR_FILENO, prefix, strlen(prefix)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg1, strlen(arg1)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg2, strlen(arg2)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg3, strlen(arg3)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg4, strlen(arg4)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg5, strlen(arg5)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg6, strlen(arg6)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg7, strlen(arg7)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg8, strlen(arg8)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg9, strlen(arg9)) == -1 && errno == EINTR);
> abort();
> }
>
> const char* prefix;
> };
>
> Alternatively you could make the defaults be NULL and only write if non-NULL.
>
> How about throwing this in a review assuming 18549 and depend on it in this review?
i can just extend the existing abort in 18549.. it seems a reasonable extension.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review35644
-----------------------------------------------------------
On March 5, 2014, 4:28 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated March 5, 2014, 4:28 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 1089
> > <https://reviews.apache.org/r/18550/diff/1/?file=505252#file505252line1089>
> >
> > We're losing information with this, did you want 'CHECK_READY(*this)' instead?
>
> Benjamin Hindman wrote:
> This doesn't look resolved?
>
> Dominic Hamon wrote:
> i had a CHECK_READY, but that introduced a circular dependency between check.hpp and future.hpp.
>
> I'll put back the full version (unless you can think of a way to avoid the dependency).
Aha, bummer. Yeah, let's revert but also a TODO to replace after we move the definitions from check into a .cpp file. Thanks!
> On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 151-152
> > <https://reviews.apache.org/r/18550/diff/1/?file=505253#file505253line151>
> >
> > The 'strings::join' is likely not async signal safe since it does allocation. If you'd like to clean up this code it seems like we need to improve ABORT. One suggestion: provide an overload to '_abort' which takes 10 const char* parameters with defaults set to NULL, then be able to do:
> >
> > ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n");
> >
> > Also, please kill the whitespace here.
> >
>
> Dominic Hamon wrote:
> wouldn't i have to do something like http://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros ?
>
> maybe in this case it's better to lose the information and just abort with 'failed to execl'. maybe log the command attempt to info before the attempt?
>
> Benjamin Hindman wrote:
> You could get away with doing something simpler with variadic macros (which we already assume other places). Consider:
>
> #define ABORT(...) _Abort(_ABORT_PREFIX)(__VA_ARGS__)
>
> struct _Abort
> {
> _Abort(const char* _prefix) : prefix(_prefix) {}
>
> void operator () (
> const char* arg0 = "",
> const char* arg1 = "",
> const char* arg2 = "",
> const char* arg3 = "",
> const char* arg4 = "",
> const char* arg5 = "",
> const char* arg6 = "",
> const char* arg7 = "",
> const char* arg8 = "",
> const char* arg9 = "")
> {
> // Write the failure message in an async-signal safe manner,
> // assuming strlen is async-signal safe or optimized out.
> // In fact, it is highly unlikely that strlen would be
> // implemented in an unsafe manner:
> // http://austingroupbugs.net/view.php?id=692
> while (write(STDERR_FILENO, prefix, strlen(prefix)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg1, strlen(arg1)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg2, strlen(arg2)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg3, strlen(arg3)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg4, strlen(arg4)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg5, strlen(arg5)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg6, strlen(arg6)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg7, strlen(arg7)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg8, strlen(arg8)) == -1 && errno == EINTR);
> while (write(STDERR_FILENO, arg9, strlen(arg9)) == -1 && errno == EINTR);
> abort();
> }
>
> const char* prefix;
> };
>
> Alternatively you could make the defaults be NULL and only write if non-NULL.
>
> How about throwing this in a review assuming 18549 and depend on it in this review?
>
> Dominic Hamon wrote:
> i can just extend the existing abort in 18549.. it seems a reasonable extension.
Okay, I'll hold off on committing that one for a second. ;)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review35644
-----------------------------------------------------------
On March 6, 2014, 12:28 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated March 6, 2014, 12:28 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18550: Introduced CHECK_* for Future and use of ABORT_MSG
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 1089
> > <https://reviews.apache.org/r/18550/diff/1/?file=505252#file505252line1089>
> >
> > We're losing information with this, did you want 'CHECK_READY(*this)' instead?
This doesn't look resolved?
> On Feb. 27, 2014, 7:03 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 151-152
> > <https://reviews.apache.org/r/18550/diff/1/?file=505253#file505253line151>
> >
> > The 'strings::join' is likely not async signal safe since it does allocation. If you'd like to clean up this code it seems like we need to improve ABORT. One suggestion: provide an overload to '_abort' which takes 10 const char* parameters with defaults set to NULL, then be able to do:
> >
> > ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n");
> >
> > Also, please kill the whitespace here.
> >
>
> Dominic Hamon wrote:
> wouldn't i have to do something like http://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros ?
>
> maybe in this case it's better to lose the information and just abort with 'failed to execl'. maybe log the command attempt to info before the attempt?
You could get away with doing something simpler with variadic macros (which we already assume other places). Consider:
#define ABORT(...) _Abort(_ABORT_PREFIX)(__VA_ARGS__)
struct _Abort
{
_Abort(const char* _prefix) : prefix(_prefix) {}
void operator () (
const char* arg0 = "",
const char* arg1 = "",
const char* arg2 = "",
const char* arg3 = "",
const char* arg4 = "",
const char* arg5 = "",
const char* arg6 = "",
const char* arg7 = "",
const char* arg8 = "",
const char* arg9 = "")
{
// Write the failure message in an async-signal safe manner,
// assuming strlen is async-signal safe or optimized out.
// In fact, it is highly unlikely that strlen would be
// implemented in an unsafe manner:
// http://austingroupbugs.net/view.php?id=692
while (write(STDERR_FILENO, prefix, strlen(prefix)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg0, strlen(arg0)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg1, strlen(arg1)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg2, strlen(arg2)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg3, strlen(arg3)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg4, strlen(arg4)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg5, strlen(arg5)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg6, strlen(arg6)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg7, strlen(arg7)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg8, strlen(arg8)) == -1 && errno == EINTR);
while (write(STDERR_FILENO, arg9, strlen(arg9)) == -1 && errno == EINTR);
abort();
}
const char* prefix;
};
Alternatively you could make the defaults be NULL and only write if non-NULL.
How about throwing this in a review assuming 18549 and depend on it in this review?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review35644
-----------------------------------------------------------
On March 6, 2014, 12:28 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated March 6, 2014, 12:28 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Feb. 27, 2014, 11:03 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 151-152
> > <https://reviews.apache.org/r/18550/diff/1/?file=505253#file505253line151>
> >
> > The 'strings::join' is likely not async signal safe since it does allocation. If you'd like to clean up this code it seems like we need to improve ABORT. One suggestion: provide an overload to '_abort' which takes 10 const char* parameters with defaults set to NULL, then be able to do:
> >
> > ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n");
> >
> > Also, please kill the whitespace here.
> >
wouldn't i have to do something like http://stackoverflow.com/questions/3046889/optional-parameters-with-c-macros ?
maybe in this case it's better to lose the information and just abort with 'failed to execl'. maybe log the command attempt to info before the attempt?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review35644
-----------------------------------------------------------
On Feb. 27, 2014, 11:18 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 11:18 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 18550: Introduce CHECK_* for Futures and use of ABORT_MSG
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18550/#review35644
-----------------------------------------------------------
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66317>
We use <> for headers that would get installed.
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66278>
Let's include the failure too:
return Some("is Failed: " + f.failure());
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66277>
Let's include the failure too:
return Some("is Failed: " + f.failure());
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66274>
Please kill all the white spaces in this file.
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66276>
Let's include the failure too:
return Some("is Failed: " + f.failure());
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66280>
s/PENDING/PROCESS/
3rdparty/libprocess/include/process/check.hpp
<https://reviews.apache.org/r/18550/#comment66279>
Kill newline at end of file.
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/18550/#comment66325>
We're losing information with this, did you want 'CHECK_READY(*this)' instead?
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/18550/#comment66340>
The 'strings::join' is likely not async signal safe since it does allocation. If you'd like to clean up this code it seems like we need to improve ABORT. One suggestion: provide an overload to '_abort' which takes 10 const char* parameters with defaults set to NULL, then be able to do:
ABORT("Failed to execl '/bin/sh -c ", command.c_str(), "'\n");
Also, please kill the whitespace here.
- Benjamin Hindman
On Feb. 27, 2014, 12:30 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18550/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 12:30 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1041
> https://issues.apache.org/jira/browse/MESOS-1041
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f6b90fee0927ac33351939ddaf2e5a2f6a
> 3rdparty/libprocess/include/process/check.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/collect.hpp 2a73bc946fc4fa65229e9c979d841f2657a7cd2d
> 3rdparty/libprocess/include/process/future.hpp e45f4f79faeefbffc28d855d2f74e8df69099f18
> 3rdparty/libprocess/include/process/subprocess.hpp 452eeeae0a9f2353db2d9302be594a578db52026
>
> Diff: https://reviews.apache.org/r/18550/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>