> On Nov. 20, 2014, 8:31 p.m., Ben Mahler wrote:
> > Thanks for taking this on Cody! This looks like a decent hack, just some
> > minor cleanup and documentation of the issues with the existing code, and
> > we can get this committed!
> >
> > A few comments on the overall code, these are not your fault, was just
> > noticing these while reading the code:
> >
> > (1) With the path "standardization", issuing a read of /log/ would return
> > data from the /log file? Seems wrong to me.
> > (2) Currently, if one attaches a file as a directory, e.g.
> > attach(".../log", "/log/") this will work? We should validate against that.
> > (3) Looks like browse.json is missing a BadRequest when one tries to browse
> > a file instead of a directory (or symlink to a directory).
> >
> > Feel free to punt on all of these for now by dropping some TODOs in the
> > code, we could add some `DISABLED_` tests or leave TODOs in the existing
> > tests to show the issues.
So for all three, definitely things I'd like to fix but I'm trying to be much
more incremental with this patchset. I'm aiming to get to MESOS-1733 without
breaking /files again. My general plan of attack is:
1. Make the code use a common path so that I can't get paths uneven (Part 1/2
why MESOS-1878 broke)
2. Add some tests to show where there are corner cases (review 27605 + this
review)
3. Normalize the path with explicit changes in behavior showing up in the unit
tests (Part 2/2 why MESOS-1878)
4. Switch to a non-variadic path::join()
> On Nov. 20, 2014, 8:31 p.m., Ben Mahler wrote:
> > src/files/files.cpp, lines 57-60
> > <https://reviews.apache.org/r/27606/diff/2/?file=770670#file770670line57>
> >
> > Ok, for now, how about we call this 'sanitize' and have it also perform
> > flattening of slashes (per the [unix
> > spec](http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_266)).
> >
> > Let's document it:
> >
> > // Returns a sanitized version of the path, which currently includes:
> > // (1) Flattening slashes (// -> /).
> > // (2) Stripping trailing slashes.
I'll rename it to sanitize. I don't want to perform any extra sanitization here
other than what the code was doing before, because that is what got me into
trouble last time implementing MESOS-1733 causing MESOS-1878. This is simply
moving the sanitization which existed before to a common place. Later patches
will make it more thorough.
> On Nov. 20, 2014, 8:31 p.m., Ben Mahler wrote:
> > src/tests/files_tests.cpp, line 83
> > <https://reviews.apache.org/r/27606/diff/2/?file=770671#file770671line83>
> >
> > What is this? Why should it work?
It's mainly to track the oddness in behavior here so it is explicitly called out
For things like browse.json, whether the user pases a path that starts with a
'/' or not doesn't make a difference. '/a/b' and 'a/b' will resolve to the same
path. Every path which is attached is always an absolute path in the virtual
filesystem which browse builds.
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27606/#review62386
-----------------------------------------------------------
On Nov. 20, 2014, 12:34 a.m., Cody Maloney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27606/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 12:34 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: mesos-1877
> https://issues.apache.org/jira/browse/mesos-1877
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Files attach, detach, and browse now all call a single function to
> standardize the path. Before attach, browse did the normalization. Detach did
> not.
>
> Switch to strings::tokenize() for splitting apart the path into tokens, as
> that is more canonical across the codebase, used by libprocess for
> proccessing paths.
>
> Update detach to return a bool of whether or not the detach does anything
> which means that it can be tested.
>
> Add some additional testing to catch the inconsistencies in path handlign
> between attach and detach previously, some of the oddness in current path
> handling so that they can be explicitly seen when changes happen.
>
>
> Diffs
> -----
>
> src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9
> src/files/files.cpp 12e8f75aa7bd77d2e81d5d3a7a4d09dd915854aa
> src/tests/files_tests.cpp 9ad6db51873b96b3cd759523cef9748f6823fb7e
>
> Diff: https://reviews.apache.org/r/27606/diff/
>
>
> Testing
> -------
>
> make distcheck on ubuntu 14.04
>
>
> Thanks,
>
> Cody Maloney
>
>
