Re: Review Request 60621: Added new stout capability: os::copyfile(source, dest).

[Jeff Coffler <je...@taltos.com>]

> On Nov. 16, 2017, 5:50 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 45 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864336#file1864336line45>
> >
> >     The backslash at the end check seems a bit odd. Doesn't `isdir` return `true` if `/foo/bar/` is a directory? We also wouldn't be catching other cases like `/foo/bar/.` here, right?

isdir will return true, but it's a stat:: operation, so it will only return true if a directory existed.

The goal here was to try to limit Posix operations to be as follows:

1. Limits should not interfere with what Mesos was already doing, and
2. Limits should roughly put same restrictions in place as the Windows API code did. This probably wasn't perfect, but I felt it was a good stab, and generally good enough.

No, I think you're right, we wouldn't catch all cases (like /foo/bar/.), but I wasn't after every single case. Just to do basic limits. The crux of the problem here is that, on Posix, we use 'cp'. If you think about 'cp' behavior, it's pretty wierd from an API perspective (you can't determine what it will do without knowing state of things). I was trying to make it more deterministic, but I never intended to be "perfect" about it.


> On Nov. 16, 2017, 5:50 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/copyfile.hpp
> > Lines 65 (patched)
> > <https://reviews.apache.org/r/60621/diff/6/?file=1864336#file1864336line65>
> >
> >     I believe this is equivalent to
> >     ```
> >     if (!WSUCCEEDED(status))
> >     ```

Why, yes, it is. The problem is that WSUCCEEDED is defined in src/common/status_utils.hpp, and this is stout, so I can't include that here.


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60621/#review191201
-----------------------------------------------------------


On Nov. 6, 2017, 6:08 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60621/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, Li Li, and Michael Park.
> 
> 
> Bugs: MESOS-6705
>     https://issues.apache.org/jira/browse/MESOS-6705
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The os::copyfile() method provides an API to copy a file from one
> location to another. Note that copying of directories are not
> supported, and that the destination directory must exist before a
> file can be copied into a directory.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
>   3rdparty/stout/include/Makefile.am bdd3e9908ebfc682458a3babc34cbee36ad3f751 
>   3rdparty/stout/include/stout/os/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/posix/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
>   3rdparty/stout/tests/os/copyfile_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60621/diff/6/
> 
> 
> Testing
> -------
> 
> See upstream
> 
> Note that Joe made some changes to this, I ended up taking his changes as is.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>