mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 53555: Moved xattr utilities into stout/os/linux.hpp.
Date Mon, 07 Nov 2016 23:37:26 GMT

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



While it is clear how the current filesystem test can break under Windows, I do not understand
why we need to wipe these utilities from all BSDs. The existing code already cleanly disabled
the stout functions under Windows, and created useful wrappers for OS X which uses different
parameters.

What you suggest here seems to remove potentially useful functionality, and also force users
of this library to `ifdef` code instead of us solving this in the library. I would prefer
us writing mostly `ifdef`-free user-code as it is easier to understand and maintain, and exposes
code to more setups.

If you troubled by these functions living in the `stout/posix/` hierarchy, once could move
all xattr related code to e.g., `stout/os/xattr.hpp`, and remove `stout/os/windows/xattr.hpp`
and `stout/os/posix/xattr.hpp`. If there are issues under OS X we should fix them, and created
related tests (the current test passes).


3rdparty/stout/tests/os/filesystem_tests.cpp (line 445)
<https://reviews.apache.org/r/53555/#comment225048>

    This could just be `#ifndef __WINDOWS__`. It doesn't call into any Linux-specific functions
to confirm the behavior of our wrapper, but instead uses observable effects that should work
well under BSDs as well.


- Benjamin Bannier


On Nov. 7, 2016, 11:49 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53555/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6360
>     https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `xattr` utilities are Linux specific syscalls.  Although these are
> defined in some form on some flavours of Posix (such as OSX and BSD),
> we do not intend to add the utility for all flavours of Posix.
> 
> Hence, this moves the `xattr` functions into `stout/os/linux.hpp`
> and removes any Posix-flavour-specific #ifdef-ing.
> 
> This also fixes the Windows build by properly #ifdef-ing the related
> filesystem test.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/linux.hpp bf12e0b0577810c64cc8276ff0d987524327ffcd

>   3rdparty/stout/include/stout/os/posix/xattr.hpp 518940fdffab38ad97cf229078c4494fa944e1d8

>   3rdparty/stout/include/stout/os/windows/xattr.hpp 3157f72c5a0d5fdf59dccb3a34b154e0bb719bfa

>   3rdparty/stout/include/stout/os/xattr.hpp b86b50addc9bfeb5ddefa757ea74d357df46fbeb

>   3rdparty/stout/tests/os/filesystem_tests.cpp 5c8f422443d0ea8e24b6a2bb506324966ce2e2ab

> 
> Diff: https://reviews.apache.org/r/53555/diff/
> 
> 
> Testing
> -------
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message