You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/08/07 23:25:03 UTC
Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Bugs: MESOS-534
https://issues.apache.org/jira/browse/MESOS-534
Repository: mesos-git
Description
-------
- Removed os::read(int fd) because it's not used elsewhere.
- Separated os::read()s into os/read.hpp.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/13387/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Aug. 9, 2013, 6:36 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/Makefile.am, line 44
> > <https://reviews.apache.org/r/13387/diff/2/?file=339147#file339147line44>
> >
> > Thanks for moving this out! Would be great to add read_tests.cpp while you're at it :)
So I modified the OsTest.readWriteString to test multi-line text.
Couldn't have moved it into a separate file but this method depends on the OsTest fixture to create tmp dir/file. Would be nice to put the fixture base class in a header under stout/tests/os/ but punt it for now?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/#review24934
-----------------------------------------------------------
On Aug. 9, 2013, 3:13 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13387/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2013, 3:13 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-534
> https://issues.apache.org/jira/browse/MESOS-534
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Removed os::read(int fd) because it's not used elsewhere.
> - Separated os::read()s into os/read.hpp.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13387/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/#review24934
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/Makefile.am
<https://reviews.apache.org/r/13387/#comment49084>
Thanks for moving this out! Would be great to add read_tests.cpp while you're at it :)
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49085>
s/text//
s/content/contents/
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49086>
s/Error/ErrnoError/
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49092>
std::string(line, read), otherwise if the input has null bytes you'll miss them and anything after them in the input.
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49094>
newline?
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49093>
kill '&& errno != 0', since the documentation states that errno will be set if an error occurs
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49095>
Isn't the delimiter included in the buffer? Does this produce additional newlines?
If so, then let's just s/lines/result/ and use result.append(line, read) to build up the result.
On this note, can you add some tests?
- Ben Mahler
On Aug. 9, 2013, 3:13 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13387/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2013, 3:13 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-534
> https://issues.apache.org/jira/browse/MESOS-534
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Removed os::read(int fd) because it's not used elsewhere.
> - Separated os::read()s into os/read.hpp.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13387/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/#review24946
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment49117>
No longer needed?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/13387/#comment49118>
thanks!
- Ben Mahler
On Aug. 9, 2013, 9:48 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13387/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2013, 9:48 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-534
> https://issues.apache.org/jira/browse/MESOS-534
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Removed os::read(int fd) because it's not used elsewhere.
> - Separated os::read()s into os/read.hpp.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp f926056239b9cd86041e497c7c3b8eadc45f44c0
>
> Diff: https://reviews.apache.org/r/13387/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/
-----------------------------------------------------------
(Updated Aug. 9, 2013, 9:48 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Comments.
Bugs: MESOS-534
https://issues.apache.org/jira/browse/MESOS-534
Repository: mesos-git
Description
-------
- Removed os::read(int fd) because it's not used elsewhere.
- Separated os::read()s into os/read.hpp.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp f926056239b9cd86041e497c7c3b8eadc45f44c0
Diff: https://reviews.apache.org/r/13387/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/
-----------------------------------------------------------
(Updated Aug. 9, 2013, 3:13 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-534
https://issues.apache.org/jira/browse/MESOS-534
Repository: mesos-git
Description
-------
- Removed os::read(int fd) because it's not used elsewhere.
- Separated os::read()s into os/read.hpp.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/13387/diff/
Testing
-------
make check
Thanks,
Jiang Yan Xu
Re: Review Request 13387: (Stout) Rewrote os::read(string path) because the
existing implementation does not work well under the inconsistency of /proc
pseudo filesystem.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13387/#review24834
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48988>
Add a newline please.
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48990>
s/fp/file/
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48989>
s/len/length/
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48991>
Please document these semantics, in particular, focus on the malloc and realloc that will occur for 'line' given 'length'.
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48992>
Let's move free(line) just below the while and put a newline after it.
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48993>
s/err/error/
3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp
<https://reviews.apache.org/r/13387/#comment48994>
s/;;/;/
- Benjamin Hindman
On Aug. 7, 2013, 9:25 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13387/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 9:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-534
> https://issues.apache.org/jira/browse/MESOS-534
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> - Removed os::read(int fd) because it's not used elsewhere.
> - Separated os::read()s into os/read.hpp.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 770378d11d30e1e222e418c1839affd9d4ffe8d3
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 448739e313bf5127e7e5469dd4090445dc5c5405
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13387/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jiang Yan Xu
>
>