You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Cody Maloney <co...@mesosphere.io> on 2014/10/15 20:46:32 UTC
Review Request 26767: MESOS-1878: Fix files/files.hpp
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/
-----------------------------------------------------------
Review request for mesos and Timothy Chen.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
Diffs
-----
src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
Diff: https://reviews.apache.org/r/26767/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/#review56797
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26766, 26767]
All tests passed.
- Mesos ReviewBot
On Oct. 15, 2014, 6:46 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26767/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
>
> This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
>
> The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
>
> This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
>
> Diff: https://reviews.apache.org/r/26767/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 17, 2014, 7:33 a.m., Timothy Chen wrote:
> > src/files/files.cpp, line 59
> > <https://reviews.apache.org/r/26767/diff/1/?file=722434#file722434line59>
> >
> > path asAbsolute as I remember is just sticking a / in front if it doesn't exist. It seems like it's only relevant in this query string path case?
> > I don't see how asAbsolute is ever useful anywhere else?
For files, they all take the query string and really should process it identically (Previously they weren't, files::attach("/foo/bar/"); Files::detach("/foo/bar/"); would not cause /foo/bar to be removed... They need to be identical and reverse.
As far as usefulness goes, it is fairly common when writing code that deals with paths passed in by users whether on the command line or through an HTTP API to normalize them so that they are always absolute paths (Particuarly when you change the current working directory of the program). It's not something we use a lot of places, but it is a common operation on filesystem paths.
Most of the api in paths which I added is similar to functionality in python's os.path library
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/#review57121
-----------------------------------------------------------
On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26767/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
>
> This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
>
> The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
>
> This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
>
> Diff: https://reviews.apache.org/r/26767/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/#review57121
-----------------------------------------------------------
src/files/files.cpp
<https://reviews.apache.org/r/26767/#comment97596>
path asAbsolute as I remember is just sticking a / in front if it doesn't exist. It seems like it's only relevant in this query string path case?
I don't see how asAbsolute is ever useful anywhere else?
- Timothy Chen
On Oct. 16, 2014, 8:53 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26767/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2014, 8:53 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
>
> This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
>
> The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
>
> This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
>
> Diff: https://reviews.apache.org/r/26767/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/#review58025
-----------------------------------------------------------
Patch looks great!
Reviews applied: [26766, 26767]
All tests passed.
- Mesos ReviewBot
On Oct. 23, 2014, 4:52 p.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26767/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 4:52 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Bugs: MESOS-1878
> https://issues.apache.org/jira/browse/MESOS-1878
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
>
> This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
>
> The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
>
> This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
>
> Diff: https://reviews.apache.org/r/26767/diff/
>
>
> Testing
> -------
>
> make distcheck
>
>
> Thanks,
>
> Cody Maloney
>
>
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 4:52 p.m.)
Review request for mesos, Ben Mahler and Timothy Chen.
Changes
-------
Update for naming changes in stout::path helpers.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
Diffs (updated)
-----
src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
Diff: https://reviews.apache.org/r/26767/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney
Re: Review Request 26767: MESOS-1878: Fix files/files.hpp
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26767/
-----------------------------------------------------------
(Updated Oct. 16, 2014, 8:53 p.m.)
Review request for mesos, Ben Mahler and Timothy Chen.
Bugs: MESOS-1878
https://issues.apache.org/jira/browse/MESOS-1878
Repository: mesos-git
Description
-------
The new path::join logic introduced changed how paths showed up in files/files.hpp. Specifically, it made it so that paths more rigorously had / didn't have a leading '/' (were absolute).
This caused some inconsistincies when trying to attach, detach, and resolve paths, resulting in the files not being found, and therefore not showing up in the webui.
The fix is primarily to standardize all the path manipulation routines (See review 26766), testing the round trip path thoroughly, then updating files to use the helpers to always clean / sanitize the incoming paths so the fit into the same pattern.
This should resolve some lurking bugs around detach() as well. Unfortunately I couldn't really add more tests there without changing the detach() return, which was out of scope for this bugfix.
Diffs
-----
src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
src/tests/files_tests.cpp a696aa22d56b37ee70c6e64c81a849da6d436451
Diff: https://reviews.apache.org/r/26767/diff/
Testing
-------
make distcheck
Thanks,
Cody Maloney