You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/08/14 02:02:19 UTC
Review Request 24679: Added os::execvpe to stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Repository: mesos-git
Description
-------
The idea here is to use the 'p' (search PATH) version consistently in the code base.
Mac does not have os::execvpe defined, so I end up implementing it per this thread:
http://stackoverflow.com/questions/7789750/execve-with-path-search
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d671869754ba7d83ca86d76eacdbb67c7828ee7
Diff: https://reviews.apache.org/r/24679/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 24679: Added os::execvpe to stout.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 14, 2014, 3:42 a.m., Ben Mahler wrote:
> > Thanks! Looks like the s/rc/result/ change was missed?
whoops.. will fix that!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/#review50545
-----------------------------------------------------------
On Aug. 14, 2014, 2:39 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24679/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 2:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The idea here is to use the 'p' (search PATH) version consistently in the code base.
>
> Mac does not have os::execvpe defined, so I end up implementing it per this thread:
> http://stackoverflow.com/questions/7789750/execve-with-path-search
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d67186
>
> Diff: https://reviews.apache.org/r/24679/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 24679: Added os::execvpe to stout.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/#review50545
-----------------------------------------------------------
Ship it!
Thanks! Looks like the s/rc/result/ change was missed?
- Ben Mahler
On Aug. 14, 2014, 2:39 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24679/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 2:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The idea here is to use the 'p' (search PATH) version consistently in the code base.
>
> Mac does not have os::execvpe defined, so I end up implementing it per this thread:
> http://stackoverflow.com/questions/7789750/execve-with-path-search
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d67186
>
> Diff: https://reviews.apache.org/r/24679/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 24679: Added os::execvpe to stout.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/
-----------------------------------------------------------
(Updated Aug. 14, 2014, 2:39 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
BenM's comments.
Repository: mesos-git
Description
-------
The idea here is to use the 'p' (search PATH) version consistently in the code base.
Mac does not have os::execvpe defined, so I end up implementing it per this thread:
http://stackoverflow.com/questions/7789750/execve-with-path-search
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d67186
Diff: https://reviews.apache.org/r/24679/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 24679: Added os::execvpe to stout.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 14, 2014, 1:06 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 596-604
> > <https://reviews.apache.org/r/24679/diff/1/?file=659693#file659693line596>
> >
> > This comment is great, I wonder if we can get away with just saying that this is a _portable_ version of execvpe (which does not exist on all systems)? Up to you.
Done.
> On Aug. 14, 2014, 1:06 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 607-618
> > <https://reviews.apache.org/r/24679/diff/1/?file=659693#file659693line607>
> >
> > I think your code is self-explanatory, and will be even more so with os::environp(), feel free to remove these code comments if you like :)
> >
> > s/ **/** /
> > s/rc/result/
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/#review50534
-----------------------------------------------------------
On Aug. 14, 2014, 12:02 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24679/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 12:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The idea here is to use the 'p' (search PATH) version consistently in the code base.
>
> Mac does not have os::execvpe defined, so I end up implementing it per this thread:
> http://stackoverflow.com/questions/7789750/execve-with-path-search
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d671869754ba7d83ca86d76eacdbb67c7828ee7
>
> Diff: https://reviews.apache.org/r/24679/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 24679: Added os::execvpe to stout.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24679/#review50534
-----------------------------------------------------------
As per your suggestion, exposing an os::environp() sounds good to me.
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/24679/#comment88397>
This seems a bit tricky since the caller needs to be aware of the fact that the 'environ' memory can be swapped without a leak occurring?
Per your suggestion in our conversation, seems safer to expose a portable 'os::environp()'. Less prone to mistakes :)
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/24679/#comment88398>
This comment is great, I wonder if we can get away with just saying that this is a _portable_ version of execvpe (which does not exist on all systems)? Up to you.
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/24679/#comment88399>
I think your code is self-explanatory, and will be even more so with os::environp(), feel free to remove these code comments if you like :)
s/ **/** /
s/rc/result/
- Ben Mahler
On Aug. 14, 2014, 12:02 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24679/
> -----------------------------------------------------------
>
> (Updated Aug. 14, 2014, 12:02 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The idea here is to use the 'p' (search PATH) version consistently in the code base.
>
> Mac does not have os::execvpe defined, so I end up implementing it per this thread:
> http://stackoverflow.com/questions/7789750/execve-with-path-search
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 4d671869754ba7d83ca86d76eacdbb67c7828ee7
>
> Diff: https://reviews.apache.org/r/24679/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>