Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Nov. 20, 2016, 4:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Nov. 20, 2016, 4:50 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> 
> Diff: https://reviews.apache.org/r/52980/diff/2/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Anindya Sinha]

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

(Updated Nov. 20, 2016, 4:50 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


Bugs: MESOS-4893
https://issues.apache.org/jira/browse/MESOS-4893


Repository: mesos


Description
---

Mode still defaults to 755, but we now enable the caller to specify
a different mode for creation of directories.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/mkdir.hpp 
fe86864c8b480993c8f052f39b2fd3ece23798da 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Anindya Sinha]

> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > 
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.
> 
> Anindya Sinha wrote:
> So, er either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on `chmod()` calls.
> 
> Anindya Sinha wrote:
> s/er/we. So to restate my comments once again:
> 
> So, we either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on chmod() calls.
> 
> Greg Mann wrote:
> Anindya, do you see a downside to the recursive `chmod` approach? Going 
> with that would allow us to avoid adding the posix-specific `mode` to another 
> function signature.
> 
> Alex, do you have any thoughts at this time on how file permissions 
> support for Windows might evolve? If we think that we'll end up defining a 
> set of generic file permission states that can be converted to either 
> posix-style or Windows-style permissions, then perhaps adding the `mode` 
> parameter here isn't such a big deal; we could simply convert it to some 
> generic `Mode` type that we introduce later on.

No downside functionally to use a recursive `chmod()` over a mode-specific 
`mkdir()`, but we would need to add support for a recursive `chmod()` in that 
case also the current `chmod()` does not handle recursive processing. So, that 
would be a change to the existing api nevertheless. Also, `chmod()` for windows 
is also punted currently.

So, I am not sure if one approach is better than the other.


- Anindya


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Greg Mann]

> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > 
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.
> 
> Anindya Sinha wrote:
> So, er either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on `chmod()` calls.
> 
> Anindya Sinha wrote:
> s/er/we. So to restate my comments once again:
> 
> So, we either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on chmod() calls.

Anindya, do you see a downside to the recursive `chmod` approach? Going with 
that would allow us to avoid adding the posix-specific `mode` to another 
function signature.

Alex, do you have any thoughts at this time on how file permissions support for 
Windows might evolve? If we think that we'll end up defining a set of generic 
file permission states that can be converted to either posix-style or 
Windows-style permissions, then perhaps adding the `mode` parameter here isn't 
such a big deal; we could simply convert it to some generic `Mode` type that we 
introduce later on.


- Greg


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Anindya Sinha]

> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > 
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.

So, er either extend `mkdir()` to include a `mode` (which as you indicated 
would need a change in the windows api also), OR call `chmod()` recursively 
after the call to the existing `mkdir()` [ie. without `mode`]. Currently, linux 
implementation in stout does not have a recursive `chmod()` which needs to be 
added, and I think windows just punts on `chmod()` calls.


- Anindya


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Anindya Sinha]

> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > 
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.
> 
> Anindya Sinha wrote:
> So, er either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on `chmod()` calls.

s/er/we. So to restate my comments once again:

So, we either extend `mkdir()` to include a `mode` (which as you indicated 
would need a change in the windows api also), OR call `chmod()` recursively 
after the call to the existing `mkdir()` [ie. without `mode`]. Currently, linux 
implementation in stout does not have a recursive `chmod()` which needs to be 
added, and I think windows just punts on chmod() calls.


- Anindya


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Alex Clemmer]

> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > 
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?

Also, sorry, I should have been more clear: I mean that permissions _on 
Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
API also has to support Windows.


- Alex


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Alex Clemmer]

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




3rdparty/stout/include/stout/os/mkdir.hpp (line 43)


Permissions is a bit of a subtle issue in Mesos, which we've largely punted 
on. I'm a bit worried that adding a pluggable `mode` here will cause us to add 
code that only works on Unix. How do you see this API evolving to accommodate 
both?


- Alex Clemmer


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 52980: Updated os::mkdir() to take the mode as an input.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


Bugs: MESOS-4893
https://issues.apache.org/jira/browse/MESOS-4893


Repository: mesos


Description
---

Mode still defaults to 755, but we now enable the caller to specify
a different mode for creation of directories.


Diffs
-

  3rdparty/stout/include/stout/os/mkdir.hpp 
fe86864c8b480993c8f052f39b2fd3ece23798da 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha