You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2015/06/17 16:28:51 UTC
Review Request 35561: Updated process::subprocess to replace
environment.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/
-----------------------------------------------------------
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
Diff: https://reviews.apache.org/r/35561/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88419
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp (lines 329 - 340)
<https://reviews.apache.org/r/35561/#comment140931>
As till pointed out, you're leaking this memory in the parent process.
- Ben Mahler
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review89268
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp (line 367)
<https://reviews.apache.org/r/35561/#comment141861>
Nice little concern break :)
- Till Toenshoff
On June 18, 2015, 6:48 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 6:48 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/
-----------------------------------------------------------
(Updated June 18, 2015, 6:48 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
Diff: https://reviews.apache.org/r/35561/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/
-----------------------------------------------------------
(Updated June 18, 2015, 6:39 p.m.)
Review request for mesos and Till Toenshoff.
Bugs: MESOS-2832
https://issues.apache.org/jira/browse/MESOS-2832
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am cb531806c4ec29c4909f831bc9faa4cad95de119
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp a289f8303740394ec1a0900e643d33e3c18a831d
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 7584cb871d02ad01021f0c3439ea205736d4f6b4
3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp c2c6a6ac97044f2317418295f48d75e94de4112b
3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
docs/home.md 0f848f151e896c0ec8c6bb5f0a3596253f34ffab
docs/mesos-testing-patterns.md 6383786419fec23ad49289b594979f0657e1fe83
Diff: https://reviews.apache.org/r/35561/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Ben Mahler <be...@gmail.com>.
> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > <https://reviews.apache.org/r/35561/diff/1/?file=986458#file986458line332>
> >
> > Aren't we leaking this one in the parent process?
>
> Benjamin Hindman wrote:
> We'll be exec'ing, so they'll get "freed" then. Same applies for the 'char** argv' above. Want an extra comment?
Then why do we `delete[] _argv;` below? :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
-----------------------------------------------------------
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Till Toenshoff <to...@me.com>.
> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 335
> > <https://reviews.apache.org/r/35561/diff/1/?file=986458#file986458line335>
> >
> > Should we be using copies here instead?
> >
> > ```
> > foreachpair (const string key, const string value, environment.get()) {
> > ...
> > }
> > ```
Covered by the styleguide.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
-----------------------------------------------------------
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > <https://reviews.apache.org/r/35561/diff/1/?file=986458#file986458line332>
> >
> > Aren't we leaking this one in the parent process?
>
> Benjamin Hindman wrote:
> We'll be exec'ing, so they'll get "freed" then. Same applies for the 'char** argv' above. Want an extra comment?
>
> Ben Mahler wrote:
> Then why do we `delete[] _argv;` below? :)
Yes, of course, thanks Till and Ben. Fix coming.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
-----------------------------------------------------------
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > <https://reviews.apache.org/r/35561/diff/1/?file=986458#file986458line332>
> >
> > Aren't we leaking this one in the parent process?
We'll be exec'ing, so they'll get "freed" then. Same applies for the 'char** argv' above. Want an extra comment?
> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/tests/subprocess_tests.cpp, line 756
> > <https://reviews.apache.org/r/35561/diff/1/?file=986459#file986459line756>
> >
> > I can see that Clock::pause is pretty much everywhere at the top of these tests. However, I find it not ideal. It should be right before the preparation of any arguments for, and the invocation of the subprocess.
> >
> > In this particular test, I would put it after the preparation of the parent's environment.
I agree, cleaned it up.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
-----------------------------------------------------------
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 35561: Updated process::subprocess to replace
environment.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35561/#review88265
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/subprocess.cpp (line 330)
<https://reviews.apache.org/r/35561/#comment140700>
Aren't we leaking this one in the parent process?
3rdparty/libprocess/src/subprocess.cpp (line 333)
<https://reviews.apache.org/r/35561/#comment140693>
Should we be using copies here instead?
```
foreachpair (const string key, const string value, environment.get()) {
...
}
```
3rdparty/libprocess/src/tests/subprocess_tests.cpp (line 756)
<https://reviews.apache.org/r/35561/#comment140692>
I can see that Clock::pause is pretty much everywhere at the top of these tests. However, I find it not ideal. It should be right before the preparation of any arguments for, and the invocation of the subprocess.
In this particular test, I would put it after the preparation of the parent's environment.
- Till Toenshoff
On June 17, 2015, 2:28 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35561/
> -----------------------------------------------------------
>
> (Updated June 17, 2015, 2:28 p.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-2832
> https://issues.apache.org/jira/browse/MESOS-2832
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 425b119e5eb122fa21c3e54fe070c7d553958f2c
> 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp b5cfc8d3daff489b332b6bfef2872cef9abecefe
>
> Diff: https://reviews.apache.org/r/35561/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>