mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 17306: Added an asynchronous subprocess utility.
Date Mon, 03 Feb 2014 19:05:16 GMT


> On Feb. 3, 2014, 9:10 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> >     Hey Ben,
> >     
> >     These lines fail to compile on GCC 4.8.1:
> >     error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’,
declared with attribute warn_unused_result [-Werror=unused-result]
> 
> Ian Downes wrote:
>     This is the same thing I missed when using write. I just explicitly ignored the return
value with (void) write(...). Is that an acceptable solution?
> 
> Niklas Nielsen wrote:
>     Unfortunately not; still breaks.
>     
>     Think you need to read it:
>     
>     ssize_t written = ...
>     (void)written;
> 
> Ben Mahler wrote:
>     Interesting, I don't have this issue with gcc-4.8.2 on OS X 10.8. Perhaps write is
only declared with the warn_unused_result attribute on OS X 10.9?
>     
>     I will likely _exit(1) if write returns -1, as opposed to ignoring the return value,
if that sounds good to you guys.
> 
> Niklas Nielsen wrote:
>     I ran into the problem on "gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9)"
> 
> Ben Mahler wrote:
>     What about this?
>     
>     diff --git a/3rdparty/libprocess/include/process/subprocess.hpp b/3rdparty/libprocess/include/process/subprocess.hpp
>     index db9c96b..452eeea 100644
>     --- a/3rdparty/libprocess/include/process/subprocess.hpp
>     +++ b/3rdparty/libprocess/include/process/subprocess.hpp
>     @@ -154,9 +154,12 @@ inline Try<Subprocess> subprocess(const std::string&
command)
>          // implemented in an unsafe manner:
>          // http://austingroupbugs.net/view.php?id=692
>          const char* message = "Failed to execl '/bin sh -c ";
>     -    write(STDERR_FILENO, message, strlen(message));
>     -    write(STDERR_FILENO, command.c_str(), command.size());
>     -    write(STDERR_FILENO, "'\n", strlen("'\n"));
>     +    while (write(STDERR_FILENO, message, strlen(message)) == -1 &&
>     +           errno == EINTR);
>     +    while (write(STDERR_FILENO, command.c_str(), command.size()) == -1 &&
>     +           errno == EINTR);
>     +    while (write(STDERR_FILENO, "'\n", strlen("'\n")) == -1 &&
>     +           errno == EINTR);
>     
>          _exit(1);
>        }

That should work. Thanks!


- Niklas


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


On Jan. 29, 2014, 4:28 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2014, 4:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Bugs: MESOS-943
>     https://issues.apache.org/jira/browse/MESOS-943
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This adds an asynchronous mechanism for subprocess execution, per MESOS-943.
> 
> What started simple was made a little more complex due to the following issues:
> 
> 1. Who is responsible for closing the input / output descriptors?
> 
>    Placing this burden onto the caller of subprocess() seems likely to yield leaked open
file descriptors. This introduced the notion of a shared_ptr / destructor / copy constructor
/ assignment constructor to ensure that the file descriptors are closed when the handle to
the file descriptors are lost. However, even with my implementation, one may copy these file
descriptors, at which point they may be deleted from underneath them.
> 
> 2. What does discarding the status entail? Does it kill the process?
> 
>    The current implementation kills the process, which requires the use of an explicit
Promise to deal with the discard from the caller not affecting the reaper's future. If discard()
is a no-op, we must still use an explicit Promise to preserve the notification from the Reaper
(so that we can know when to delete the Reaper).
> 
> 
> That's about it, I've added tests that demonstrate the ability to communicate with the
subprocess through stdin / stout / stderr.
> 
> Please let me know if you find any simplifications that can be made! (Other than C++11
lambdas, of course :))
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459 
>   3rdparty/libprocess/include/process/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17306/diff/
> 
> 
> Testing
> -------
> 
> Tests were added and ran in repetition.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


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