Re: Review Request 37032: Extend permissions.hpp to work on both Windows and POSIX.

[Artem Harutyunyan]

> On Sept. 8, 2015, 3:51 p.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp, line 57
> > 
> >
> > A comment describing why do we need this function would be great.
> 
> Alex Clemmer wrote:
> Just to make sure we're on the same page, this function is moving here -- 
> I didn't write it. It looks like it's used only in the fetcher tests and the 
> `credentials.hpp`, so my guess is it's not really _needed_. But either way, 
> I'd like to suggest that I'm not the best person to provide this 
> justification.

OK. Thanks for clarifying.


- Artem


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


On Sept. 10, 2015, 12:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37032/
> ---
> 
> (Updated Sept. 10, 2015, 12:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extend permissions.hpp to work on both Windows and POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 8853f92fcfcff81d0a3197bade02110685fa0325 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
> 196c3f5fac7c3526924f2bea03c06d1fbce25c61 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/permissions.hpp 
> 98f0b3c8e55190df87d6a581667e21b31ac044bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/permissions.hpp 
> daed4b4e9c76d6e7c043a1fa3a46ab1f3db95f48 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7ab75ece44ab4b0cc42f992daf1101d0faf80b1f 
> 
> Diff: https://reviews.apache.org/r/37032/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 37032: Extend permissions.hpp to work on both Windows and POSIX.

[Joris Van Remoortere]

> On Sept. 8, 2015, 10:51 p.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 152-159
> > 
> >
> > Do we really need to leave this up to the user to decide? What's the 
> > benefit of one versus the other? Can't we just stick to one option and just 
> > implement that one? 
> > 
> > If not, is there a build time configuration flag that can be used to 
> > change this?
> 
> Alex Clemmer wrote:
> I provided this (1) because I could see use cases for both permission 
> semantics (both for us and for downstream clients _e.g._ DCOS clients who are 
> concerned about security or something), (2) because making this configurable 
> at runtime would likely mean augmenting or refactoring the existing API for 
> things like `chown`, and (3) because a "final" solution is probably out of 
> scope for the MVP.
> 
> Happy to change this, but for now I think we should probably ship it and 
> re-evaluate later.
> 
> Also, to answer your question about specifying this at compile time, yes, 
> you can just pass a flag like `-DSTRICT_OTHER_PERMISSIONS` and it should 
> "just work." This isn't wired up in CMake but that would be trivial to do.
> 
> Joseph Wu wrote:
> I suggest for now to get rid of the flags and opt for strict-ness (no 
> fallback) by default.  Being strict means places in the codebase that try to 
> use "group/other" permissions will be more likely to error out (i.e. access 
> denied).  We can then catch those cases individually and deal with them.
> 
> We should also create a JIRA to track this, in-case anything breaks as a 
> result (on Windows).

Dropping the compiler flag for now and only using the strict versions for now.


- Joris


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


On Sept. 10, 2015, 7:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37032/
> ---
> 
> (Updated Sept. 10, 2015, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extend permissions.hpp to work on both Windows and POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 8853f92fcfcff81d0a3197bade02110685fa0325 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
> 196c3f5fac7c3526924f2bea03c06d1fbce25c61 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/permissions.hpp 
> 98f0b3c8e55190df87d6a581667e21b31ac044bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/permissions.hpp 
> daed4b4e9c76d6e7c043a1fa3a46ab1f3db95f48 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7ab75ece44ab4b0cc42f992daf1101d0faf80b1f 
> 
> Diff: https://reviews.apache.org/r/37032/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 37032: Extend permissions.hpp to work on both Windows and POSIX.

[Artem Harutyunyan]

> On Sept. 8, 2015, 3:51 p.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 152-159
> > 
> >
> > Do we really need to leave this up to the user to decide? What's the 
> > benefit of one versus the other? Can't we just stick to one option and just 
> > implement that one? 
> > 
> > If not, is there a build time configuration flag that can be used to 
> > change this?
> 
> Alex Clemmer wrote:
> I provided this (1) because I could see use cases for both permission 
> semantics (both for us and for downstream clients _e.g._ DCOS clients who are 
> concerned about security or something), (2) because making this configurable 
> at runtime would likely mean augmenting or refactoring the existing API for 
> things like `chown`, and (3) because a "final" solution is probably out of 
> scope for the MVP.
> 
> Happy to change this, but for now I think we should probably ship it and 
> re-evaluate later.
> 
> Also, to answer your question about specifying this at compile time, yes, 
> you can just pass a flag like `-DSTRICT_OTHER_PERMISSIONS` and it should 
> "just work." This isn't wired up in CMake but that would be trivial to do.
> 
> Joseph Wu wrote:
> I suggest for now to get rid of the flags and opt for strict-ness (no 
> fallback) by default.  Being strict means places in the codebase that try to 
> use "group/other" permissions will be more likely to error out (i.e. access 
> denied).  We can then catch those cases individually and deal with them.
> 
> We should also create a JIRA to track this, in-case anything breaks as a 
> result (on Windows).
> 
> Joris Van Remoortere wrote:
> Dropping the compiler flag for now and only using the strict versions for 
> now.

I agree. Alex, could you please create a JIRA to track?


- Artem


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


On Sept. 10, 2015, 12:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37032/
> ---
> 
> (Updated Sept. 10, 2015, 12:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extend permissions.hpp to work on both Windows and POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 8853f92fcfcff81d0a3197bade02110685fa0325 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
> 196c3f5fac7c3526924f2bea03c06d1fbce25c61 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/permissions.hpp 
> 98f0b3c8e55190df87d6a581667e21b31ac044bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/permissions.hpp 
> daed4b4e9c76d6e7c043a1fa3a46ab1f3db95f48 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7ab75ece44ab4b0cc42f992daf1101d0faf80b1f 
> 
> Diff: https://reviews.apache.org/r/37032/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 37032: Extend permissions.hpp to work on both Windows and POSIX.

[Alex Clemmer]

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

(Updated Sept. 10, 2015, 7:36 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Extend permissions.hpp to work on both Windows and POSIX.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
8853f92fcfcff81d0a3197bade02110685fa0325 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
196c3f5fac7c3526924f2bea03c06d1fbce25c61 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/permissions.hpp 
98f0b3c8e55190df87d6a581667e21b31ac044bc 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/permissions.hpp 
daed4b4e9c76d6e7c043a1fa3a46ab1f3db95f48 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
7ab75ece44ab4b0cc42f992daf1101d0faf80b1f 

Diff: https://reviews.apache.org/r/37032/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 37032: Extend permissions.hpp to work on both Windows and POSIX.

[Alex Clemmer]

> On Sept. 8, 2015, 11:04 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp, lines 140-143
> > 
> >
> > I don't think it's obvious from reading this the first time that you're 
> > about to describe an open question / problem. Could you make this more 
> > clear?

I'm not sure what we mean by "open problem", but I think part of the problem 
might be that the "execute permissions" section was misplaced between the two 
parts of the "flag strictness" comments. This is fixed in the upcoming review.


- Alex


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


On Sept. 10, 2015, 7:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37032/
> ---
> 
> (Updated Sept. 10, 2015, 7:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extend permissions.hpp to work on both Windows and POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 8853f92fcfcff81d0a3197bade02110685fa0325 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
> 196c3f5fac7c3526924f2bea03c06d1fbce25c61 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/permissions.hpp 
> 98f0b3c8e55190df87d6a581667e21b31ac044bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/permissions.hpp 
> daed4b4e9c76d6e7c043a1fa3a46ab1f3db95f48 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 7ab75ece44ab4b0cc42f992daf1101d0faf80b1f 
> 
> Diff: https://reviews.apache.org/r/37032/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>