You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2013/07/10 09:36:20 UTC
Review Request 12430: Added "argv" lookup for Process::command on OS X.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
Diff: https://reviews.apache.org/r/12430/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 12430: Added "argv" lookup for Process::command on OS X.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 12, 2013, 2:22 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp, line 80
> > <https://reviews.apache.org/r/12430/diff/2/?file=320713#file320713line80>
> >
> > Do you trust tokens to be non-empty always here?
Wrapped in an 'if (!tokens.empty())'.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/#review23055
-----------------------------------------------------------
On July 12, 2013, 1:23 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12430/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 1:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
>
> Diff: https://reviews.apache.org/r/12430/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 12430: Added "argv" lookup for Process::command on OS X.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/#review23055
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/12430/#comment46875>
Do you trust tokens to be non-empty always here?
- Ben Mahler
On July 12, 2013, 1:23 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12430/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 1:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
>
> Diff: https://reviews.apache.org/r/12430/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 12430: Added "argv" lookup for Process::command on OS X.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/
-----------------------------------------------------------
(Updated July 12, 2013, 1:23 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed BenM's review comments.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
Diff: https://reviews.apache.org/r/12430/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 12430: Added "argv" lookup for Process::command on OS X.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 11, 2013, 5:48 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp, line 43
> > <https://reviews.apache.org/r/12430/diff/1/?file=319784#file319784line43>
> >
> > s/priveledge/privilege/
Thanks Ben. ;)
> On July 11, 2013, 5:48 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp, line 53
> > <https://reviews.apache.org/r/12430/diff/1/?file=319784#file319784line53>
> >
> > Any pointers for this? I found a few posts and the like that seemed to show the pseudo-struct that gets returned, perhaps document that?
I added a link and some more comments.
> On July 11, 2013, 5:48 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp, lines 58-64
> > <https://reviews.apache.org/r/12430/diff/1/?file=319784#file319784line58>
> >
> > This took me a bit to understand, maybe something like the following would be simpler to follow:
> >
> > std::vector<std::string> tokens =
> > strings::tokenize(args.get(), std::string(1, '\0'));
> >
> > if (argc == 1) {
> > // When there are no arguments, all we care about is
> > // the executable path.
> > s = tokens[0];
> > } else if (argc > 1) {
> > // When there are arguments, we grab arg[0..n]
> > // assuming arg0 is the canonical executable name.
> > tokens.erase(tokens.front()); // Remove path.
> > s = strings::join(" ", tokens);
> > }
> >
> > command = s;
> >
> > Also, what about the argc == 0 case? Should that be an error? Should there be an error when tokens.size() != argc?
> >
> > What do you think?
Awesome idea! I cleaned it up. If argc == 0 then 'command' will just be none, likewise, if we get no tokens out of 'tokens.erase(tokens.begin() + argc, tokens.end())' (see the code) then 'command' will just stay none. Thanks Ben!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/#review23020
-----------------------------------------------------------
On July 12, 2013, 1:23 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12430/
> -----------------------------------------------------------
>
> (Updated July 12, 2013, 1:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
>
> Diff: https://reviews.apache.org/r/12430/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 12430: Added "argv" lookup for Process::command on OS X.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12430/#review23020
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/12430/#comment46801>
s/priveledge/privilege/
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/12430/#comment46802>
Any pointers for this? I found a few posts and the like that seemed to show the pseudo-struct that gets returned, perhaps document that?
3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp
<https://reviews.apache.org/r/12430/#comment46803>
This took me a bit to understand, maybe something like the following would be simpler to follow:
std::vector<std::string> tokens =
strings::tokenize(args.get(), std::string(1, '\0'));
if (argc == 1) {
// When there are no arguments, all we care about is
// the executable path.
s = tokens[0];
} else if (argc > 1) {
// When there are arguments, we grab arg[0..n]
// assuming arg0 is the canonical executable name.
tokens.erase(tokens.front()); // Remove path.
s = strings::join(" ", tokens);
}
command = s;
Also, what about the argc == 0 case? Should that be an error? Should there be an error when tokens.size() != argc?
What do you think?
- Ben Mahler
On July 10, 2013, 7:36 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12430/
> -----------------------------------------------------------
>
> (Updated July 10, 2013, 7:36 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp e8429b997cdefd703ef16249789750fe7d8ac9c4
> 3rdparty/libprocess/3rdparty/stout/tests/strings_tests.cpp 7ec94468df3d447a0c30ad9c82fa4c2fe91deb7e
>
> Diff: https://reviews.apache.org/r/12430/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>