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/03/13 00:26:02 UTC
Review Request 19162: Added optional envvar map to subprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37143
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68517>
I think the child's environment will _only_ contain the variables defined in envp and thus we'll lose the standard variables, $USER, $HOME, $PATH, etc.
environ lists the standard variables.
- Ian Downes
On March 13, 2014, 5:59 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 13, 2014, 11:38 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 137-138
> > <https://reviews.apache.org/r/19162/diff/3/?file=519501#file519501line137>
> >
> > What does "= delete" mean? Perhaps use the same comment from other places in the code:
> >
> > // Not copyable, not assignable.
> > Promise(const Promise<T>&);
> > Promise<T>& operator = (const Promise<T>&);
I wonder if it's worth having a macro to make this easier:
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
TypeName(const TypeName&); \
TypeName& operator = (const TypeName&)
> On March 13, 2014, 11:38 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 17
> > <https://reviews.apache.org/r/19162/diff/3/?file=519502#file519502line17>
> >
> > Anonymous namespaces are, as far as I know, new style in libprocess. Did you need this?
Need it? No. But it does make it so that the symbols aren't available outside the translation unit. Ie, I can ensure that noone else is calling cleanup/using Envp so I won't break people inadvertently if I change them.
> On March 13, 2014, 11:38 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 54
> > <https://reviews.apache.org/r/19162/diff/3/?file=519502#file519502line54>
> >
> > Why are you using 'malloc', instead of 'new'? That is, if you need to, a comment would be nice!
because i'm dealing with basic types that don't have constructors. new[] and delete[] could be used just as well if that's prefered.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37167
-----------------------------------------------------------
On March 13, 2014, 5:46 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37167
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68574>
How about s/env/environment/ to keep things consistent with the rest of our code, in which we try to avoid abbreviations.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68576>
Can you move this to the .cpp file and just test that subprocess does the right thing with the environment? It seems a little messy to expose this implementation detail especially given how trivial the test is.
What does 'Envp' mean? A comment would be nice!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68575>
What does "= delete" mean? Perhaps use the same comment from other places in the code:
// Not copyable, not assignable.
Promise(const Promise<T>&);
Promise<T>& operator = (const Promise<T>&);
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68578>
Anonymous namespaces are, as far as I know, new style in libprocess. Did you need this?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68583>
Why are you using 'malloc', instead of 'new'? That is, if you need to, a comment would be nice!
- Ben Mahler
On March 14, 2014, 12:46 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 12:46 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On March 20, 2014, 8 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/subprocess_tests.cpp, line 10
> > <https://reviews.apache.org/r/19162/diff/7/?file=528494#file528494line10>
> >
> > System C includes go before C++ includes, can you revert the move?
>
> Dominic Hamon wrote:
> gmock is not a system include, it's third party.
Let's keep it consistent with the rest of the code for now, and open a conversation to update the code if it makes sense. :)
> On March 20, 2014, 8 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 48-49
> > <https://reviews.apache.org/r/19162/diff/7/?file=528493#file528493line48>
> >
> > What does '// = delete' mean? Do you still need this now that you've added the comment about what you're trying to restrict?
>
> Dominic Hamon wrote:
> it's ready for C++11. When we have that required we can comment this in and explicitly delete the constructors, as opposed to just not defining them.
Ah I see, let's just keep this consistent with the rest of the code as well and make the '= delete' change in one swoop when we introduce the C++11 requirement.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37860
-----------------------------------------------------------
On March 20, 2014, 11:20 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 11:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 20, 2014, 1 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 48-49
> > <https://reviews.apache.org/r/19162/diff/7/?file=528493#file528493line48>
> >
> > What does '// = delete' mean? Do you still need this now that you've added the comment about what you're trying to restrict?
it's ready for C++11. When we have that required we can comment this in and explicitly delete the constructors, as opposed to just not defining them.
> On March 20, 2014, 1 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 71-72
> > <https://reviews.apache.org/r/19162/diff/7/?file=528493#file528493line71>
> >
> > Would be nice to avoid the need for static_cast and sizeof by just using 'new' in Envp, any reason not to clean it up?
> >
> > Ditto for calloc.
i tend to avoid new[] for basic types, but that's a hangover from too much kernel coding :P
> On March 20, 2014, 1 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/subprocess_tests.cpp, line 10
> > <https://reviews.apache.org/r/19162/diff/7/?file=528494#file528494line10>
> >
> > System C includes go before C++ includes, can you revert the move?
gmock is not a system include, it's third party.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37860
-----------------------------------------------------------
On March 20, 2014, 4:20 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37860
-----------------------------------------------------------
Thanks for pulling out the cleanup review!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment69624>
Why did you re-order these? Can you follow the same include style as elsewhere?
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment69625>
This normally goes above, but why did you need this?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69803>
Can you use os::environ here instead? Would also love to leave a TODO to add 'os::environment()' that returns a map so we can clean up Envp by merging the parent and child environment maps.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69822>
I like this abstraction!
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69626>
Can you remove the std:: qualifiers?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69627>
What does '// = delete' mean? Do you still need this now that you've added the comment about what you're trying to restrict?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69811>
We haven't been using '_' suffixes for member variables.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69810>
Can you kill this one?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69817>
Period at the end of the comment.
Could indexing make this code cleaner?
size_t count = 0;
while (environ[count++] != NULL);
or
size_t count = 0;
for (count = 0; environ[count] != NULL; ++count);
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69820>
Would be nice to avoid the need for static_cast and sizeof by just using 'new' in Envp, any reason not to clean it up?
Ditto for calloc.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69821>
Could you just index into 'environ' here as well?
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment69823>
System C includes go before C++ includes, can you revert the move?
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment69824>
Periods at the end of comments, here and below.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment69825>
I thought the point of taking a map was that we didn't need to quote? We should probably test that spaces work without the need for quoting, no?
- Ben Mahler
On March 20, 2014, 5:52 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 5:52 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 24, 2014, 8:51 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 63-64
> > <https://reviews.apache.org/r/19162/diff/9/?file=530839#file530839line63>
> >
> > What's not clear from reading this code is that our technique here is to reserve the first [0,size) envp entries for our allocated additional environment data, and the (size, ...] entries for the parent 'environ' data.
> >
> > This seems a bit confusing since I would normally expect 'size' to refer to the size of the array.
> >
> > Would it be simpler to just have envp contain our own allocated data (some copied from 'environ' rather than borrowed, some copied from 'environment'). This would more closely model what things will look like when we implement the TODO for os::environment.
> >
> > Also, you're currently placing the parent environment *after* the child environment, can you add a test that the child environment overrides what's contained in the parent environment? I'm wondering how execle deals with duplicate keys.
Let's clean all this up with https://reviews.apache.org/r/19948.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review38361
-----------------------------------------------------------
On March 26, 2014, 6:54 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 6:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-995
> https://issues.apache.org/jira/browse/MESOS-995
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review38361
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment70457>
What's not clear from reading this code is that our technique here is to reserve the first [0,size) envp entries for our allocated additional environment data, and the (size, ...] entries for the parent 'environ' data.
This seems a bit confusing since I would normally expect 'size' to refer to the size of the array.
Would it be simpler to just have envp contain our own allocated data (some copied from 'environ' rather than borrowed, some copied from 'environment'). This would more closely model what things will look like when we implement the TODO for os::environment.
Also, you're currently placing the parent environment *after* the child environment, can you add a test that the child environment overrides what's contained in the parent environment? I'm wondering how execle deals with duplicate keys.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment70453>
It looks like there is a bug here in which you're assigning environ[0] twice, right?
envp[N] = environ[0]; // Loop iteration 1.
envp[N+1] = environ[0]; // Loop iteration 2.
This is because parentEnv begins with environ[0] (*environ) and in the first loop iteration is re-assigned to environ[0] again.
- Ben Mahler
On March 20, 2014, 11:40 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review39362
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment71737>
true - also switched to strncpy in case there's an embedded NULL.
- Dominic Hamon
On April 2, 2014, 2:57 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 2:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-995
> https://issues.apache.org/jira/browse/MESOS-995
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated April 2, 2014, 2:57 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
changed dependency and respond to comments
Bugs: MESOS-995
https://issues.apache.org/jira/browse/MESOS-995
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review39360
-----------------------------------------------------------
Ship it!
Great cleanup with os::environment.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment71726>
Please add a short comment that explains that the environment is combined with the current environment (overriding as necessary) since by default doing an exec*e only uses the environment that one passes. You could even consider adding a TODO to differentiate this case.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment71728>
Might as well initialize size to 0 too!
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment71722>
Minor cleanup, but let's name the constructor argument '_environment' to match style in codebase and then s/environ/environment/ please.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment71724>
Won't you get the '\0' copied for free if you copy 'entry.size() + 1' since you did 'entry.c_str()'?
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment71729>
Newline here please.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment71731>
I'm not sure what you mean by "override OS" but calling this just 'environmentOverride' is probably sufficient.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19162/#comment71733>
// Make sure we override an existing environment variable.
os::setenv("MESSAGE", "hello");
map<string, string> environment;
environment["MESSAGE"] = "goodbye";
- Benjamin Hindman
On April 2, 2014, 8:44 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 8:44 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-995
> https://issues.apache.org/jira/browse/MESOS-995
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated April 2, 2014, 1:44 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
use os::environment
Bugs: MESOS-995
https://issues.apache.org/jira/browse/MESOS-995
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated April 2, 2014, 10:59 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added dependency
Bugs: MESOS-995
https://issues.apache.org/jira/browse/MESOS-995
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 31, 2014, 6:23 p.m., Adam B wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 102-104
> > <https://reviews.apache.org/r/19162/diff/9/?file=530839#file530839line102>
> >
> > This only deletes the strings for the child environment and leaves the parent env strings alone, right? Is this intentional? Please fix or comment.
This is intentional - The parent env strings are pointers to the global environment so not owned by this code.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review39120
-----------------------------------------------------------
On March 26, 2014, 11:54 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 11:54 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-995
> https://issues.apache.org/jira/browse/MESOS-995
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review39120
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment71507>
This only deletes the strings for the child environment and leaves the parent env strings alone, right? Is this intentional? Please fix or comment.
- Adam B
On March 26, 2014, 11:54 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 11:54 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-995
> https://issues.apache.org/jira/browse/MESOS-995
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 26, 2014, 11:54 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added reference to jira
Bugs: MESOS-995
https://issues.apache.org/jira/browse/MESOS-995
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 20, 2014, 4:40 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added back dependency. sorry for the extra mail.
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 20, 2014, 4:40 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
reverted include order changed, removed =delete comments.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 20, 2014, 4:20 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 20, 2014, 10:52 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
added dependency
Repository: mesos-git
Description
-------
See summary
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 19, 2014, 3:29 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 19, 2014, 11:51 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Split change into two patches .. this is the second now dependent on 19416.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 18, 2014, 7:05 p.m., Ben Mahler wrote:
> > It's a bit difficult to read the change to the logic here since you're moving to a .cpp file (which is great!) and updating the logic at the same time.
> >
> > Mind splitting the diff into two parts to make reviewing faster? :)
19416
i'll rebase this to sit on top
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37678
-----------------------------------------------------------
On March 20, 2014, 4:20 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37678
-----------------------------------------------------------
It's a bit difficult to read the change to the logic here since you're moving to a .cpp file (which is great!) and updating the logic at the same time.
Mind splitting the diff into two parts to make reviewing faster? :)
- Ben Mahler
On March 17, 2014, 7:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 7:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Nikita Vetoshkin <ni...@gmail.com>.
> On March 18, 2014, 10:57 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 52
> > <https://reviews.apache.org/r/19162/diff/5/?file=523512#file523512line52>
> >
> > It would only really change line 71. Given the execle call expects a char** eventually, i'd rather keep it in that form from the beginning.
>
> Ben Mahler wrote:
> If you reply from the main review page, then the thread of conversation will be kept intact.
Can't we pass &envp_[0] to execle?
- Nikita
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37657
-----------------------------------------------------------
On March 17, 2014, 7:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 7:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ben Mahler <be...@gmail.com>.
> On March 18, 2014, 10:57 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 52
> > <https://reviews.apache.org/r/19162/diff/5/?file=523512#file523512line52>
> >
> > It would only really change line 71. Given the execle call expects a char** eventually, i'd rather keep it in that form from the beginning.
If you reply from the main review page, then the thread of conversation will be kept intact.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37657
-----------------------------------------------------------
On March 17, 2014, 7:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 7:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 18, 2014, 3:57 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 52
> > <https://reviews.apache.org/r/19162/diff/5/?file=523512#file523512line52>
> >
> > It would only really change line 71. Given the execle call expects a char** eventually, i'd rather keep it in that form from the beginning.
>
> Ben Mahler wrote:
> If you reply from the main review page, then the thread of conversation will be kept intact.
>
> Nikita Vetoshkin wrote:
> Can't we pass &envp_[0] to execle?
we can, though i'm not sure of the benefit. And should someone decide later to replace the vector with a list, we'll have a tough bug to find.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37657
-----------------------------------------------------------
On March 17, 2014, 12:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 12:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37657
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment69289>
It would only really change line 71. Given the execle call expects a char** eventually, i'd rather keep it in that form from the beginning.
- Dominic Hamon
On March 17, 2014, 12:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 12:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Nikita Vetoshkin <ni...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37443
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68983>
What about using std::vector<char *> to hold prepared env? Maybe it would look cleaner?
- Nikita Vetoshkin
On March 17, 2014, 7:33 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 17, 2014, 7:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 17, 2014, 12:33 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
3rdparty/libprocess/include/process/subprocess.hpp 75b2a96b3ca62a4a59e64aba046f1d797bd8ebf6
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Till Toenshoff <to...@me.com>.
> On March 15, 2014, 8:46 a.m., Till Toenshoff wrote:
> >
Great extension, thanks Dominic. Super valuable for my current work.
> On March 15, 2014, 8:46 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 77
> > <https://reviews.apache.org/r/19162/diff/4/?file=519657#file519657line77>
> >
> > Aren't you are missing the terminating 0? How about using "calloc" or size()+1?
That certainly is for both, allocation and copying. So make it:
envp_[index] = static_cast<char*>(
malloc((entry.size() + 1) * sizeof(char)));
memcpy(envp_[index], entry.c_str(), entry.size() + 1);
or
envp_[index] = static_cast<char*>(
calloc((entry.size() + 1) * sizeof(char)));
memcpy(envp_[index], entry.c_str(), entry.size());
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37316
-----------------------------------------------------------
On March 14, 2014, 4:45 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 4:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37316
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68777>
Aren't you are missing the terminating 0? How about using "calloc" or size()+1?
- Till Toenshoff
On March 14, 2014, 4:45 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 4:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 14, 2014, 9:45 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 13, 2014, 5:46 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37137
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68484>
Please put '{' on a new line.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68485>
We don't indent these.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68486>
Add a newline.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68504>
Let's move this to .cpp now.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68489>
We indent by +2 here, only +4 for parameter and argument lists.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68494>
Indent +2 here, and both on their own line please. Also, what about adding a comment at the end of the 'size_' line that explains the +1.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68510>
s/envi/index/
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68512>
s/kv/entry/
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68490>
Newline.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68493>
Please put each of these on their own line.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68495>
s/(char *)NULL/(char*) NULL/
- Benjamin Hindman
On March 13, 2014, 5:59 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 13, 2014, 11:01 a.m., Nikita Vetoshkin wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 38
> > <https://reviews.apache.org/r/19162/diff/1/?file=517809#file517809line38>
> >
> > Why not putenv explicitly in child process or use execle?
Please see latest diff.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37074
-----------------------------------------------------------
On March 13, 2014, 10:59 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 10:59 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Nikita Vetoshkin <ni...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37074
-----------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68354>
Why not putenv explicitly in child process or use execle?
- Nikita Vetoshkin
On March 13, 2014, 5:59 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 13, 2014, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/
-----------------------------------------------------------
(Updated March 13, 2014, 10:59 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
See summary
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
Diff: https://reviews.apache.org/r/19162/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 12, 2014, 5:33 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 38
> > <https://reviews.apache.org/r/19162/diff/1/?file=517809#file517809line38>
> >
> > shouldn't we be escaping keys/values...
> >
> > setenv technically isn't async-signal-safe but that could be used in the child
> >
> > or, even better, we should use execle rather than execl
> >
If you can give me some examples of key/values that you think would be problematic i can make sure we have tests for them. I was thinking any values containing special characters can be explicitly quoted (as in the test with spaces).
using execle actually solves many of these problems and will, i think, remove the need for quotes in multiple URLs from the containerizer...
> On March 12, 2014, 5:33 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 52
> > <https://reviews.apache.org/r/19162/diff/1/?file=517808#file517808line52>
> >
> > I thought we always named all arguments in the codebase?
Apparently not consistently for friend methods - I'm following what was here, see also stout/os/process.hpp. Adding some here though.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37021
-----------------------------------------------------------
On March 12, 2014, 4:26 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 4:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19162: Added optional envvar map to subprocess.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19162/#review37021
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68279>
I thought we always named all arguments in the codebase?
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68277>
s/string &/string& /
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19162/#comment68278>
s/> &env/>& env/
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68274>
s/string &command/string& command/
and
s/> &env/>& env/
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68273>
s/string /string& /
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19162/#comment68280>
shouldn't we be escaping keys/values...
setenv technically isn't async-signal-safe but that could be used in the child
or, even better, we should use execle rather than execl
- Ian Downes
On March 12, 2014, 11:26 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19162/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 11:26 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19162/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>