Re: Review Request 35561: Updated process::subprocess to replace environment.

2015-06-24 Thread Till Toenshoff 

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



3rdparty/libprocess/src/subprocess.cpp (line 367)


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.

2015-06-18 Thread Benjamin Hindman 

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

2015-06-18 Thread Benjamin Hindman 

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

2015-06-18 Thread Benjamin Hindman 


> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > 
> >
> > 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.

2015-06-18 Thread Ben Mahler 


> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > 
> >
> > 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.

2015-06-18 Thread Ben Mahler 

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


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.

2015-06-18 Thread Benjamin Hindman 


> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 332
> > 
> >
> > 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
> > 
> >
> > 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.

2015-06-17 Thread Till Toenshoff 


> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 335
> > 
> >
> > 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.

2015-06-17 Thread Till Toenshoff 

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


Aren't we leaking this one in the parent process?



3rdparty/libprocess/src/subprocess.cpp (line 333)


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)


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

Review Request 35561: Updated process::subprocess to replace environment.

2015-06-17 Thread Benjamin Hindman 

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