Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 7, 2016, 9:50 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 71c4308cccfa5c56b93f6c3928dd2a1cf3ba9741 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 6, 2016, 10:36 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 58)


Nit: 

`static constexpr Bytes BASIC_BLOCK_SIZE = Bytes(512);` 

for consistency just because this is how constants in Mesos are generally 
constructed (copy constructed).


- Jiang Yan Xu


On April 6, 2016, 11:42 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 6, 2016, 11:42 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 6, 2016, 6:42 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On April 5, 2016, 10:25 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 286
> > 
> >
> > `s/directory/path`.

As discussed, we only accept directories now.


> On April 5, 2016, 10:25 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.hpp, lines 61-62
> > 
> >
> > s/directory/path/, this breaks the symmetry in the `*projectId` methods 
> > but this doesn't need to be a directory and we actually use it to test 
> > files in tests.

As discussed we only accept directories now.


- James


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


On April 5, 2016, 11:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 5, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Looking good. Just a few minor comments, plus one mentioned in [another 
review](https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line140)
 and we are good to go.


src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 61 - 62)


s/directory/path/, this breaks the symmetry in the `*projectId` methods but 
this doesn't need to be a directory and we actually use it to test files in 
tests.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 72)


Kill line.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 78)


`s/__XFS_HPP__/__XFS_UTILS_HPP__`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59)


Add an empty line.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 154 - 155)


I didn't mention this earlier but it would be nice if all these internal 
arithmetics use the Bytes object and only convert to a uint when it calls 
underlying systems calls.

A TODO would be fine.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 265)


"larger than or equal to" or ">=" so it matches the condition check.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 266)


Nit: `stringify(Bytes(BASIC_BLOCK_SIZE))`. 

I have another comment about a TODO for using Bytes consistently, including 
the type of BASIC_BLOCK_SIZE. But OK to not do it in this review.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 286)


`s/directory/path`.



src/tests/environment.cpp (lines 468 - 471)


This is unrelated?


- Jiang Yan Xu


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 4, 2016, 5:27 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 45 - 47)


Sorry it didn't occur to to me earlier but looking at the tests I realized 
that we need to return a Result.

i.e.,

```
Result getProjectQuota(
const std::string& path,
prid_t projectId);
```

because a zero limit is "no limit", hence `None`. We use `setProjectQuota` 
and `clearProjectQuota` to carefully spearate the two use cases so here I think 
it's justified to make zero quota a special type.

(Plus we do this for `getProjectId()` already).


- Jiang Yan Xu


On March 31, 2016, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 31, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 1, 2016, 12:01 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updated for review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On March 31, 2016, 5:12 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 336
> > 
> >
> > Plural?
> > 
> > We often use a trailing understore: `char* directory_[]`. 
> > (s/path/directory/ is due to the suggestion on generalizing the method)
> > 
> > Also, no padding space in `{}`.
> > 
> > Therefore:
> > 
> > ```
> > char* directory_[] = {const_cast(path.c_str()), nullptr};
> > ```

This is identical to the code in ``src/linux/cgroups.cpp``.


- James


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


On March 30, 2016, 10:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 30, 2016, 10:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

> On March 31, 2016, 10:12 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, lines 366-368
> > 
> >
> > In this method we can just validate the projectId first and then 
> > 
> > ```
> > traverse(
> > path,
> > std::bind(internal::setProjectId,
> > lambda::_1,
> > projectId,
> > lambda::_2));
> > ```

The above wasn't indented correctly...

```
traverse(
path,
std::bind(
internal::setProjectId,
lambda::_1,
projectId,
lambda::_2));
```


- Jiang Yan


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


On March 30, 2016, 3:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 30, 2016, 3:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 17)


Now that this header was moved: `s/__XFS_HPP__/__XFS_UTILS_HPP__/`



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 39)


s/operator ==/operator==/ as per convention.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 61)


It would be great if we can have consitently named two set of methods:

```
[get|set|clear]ProjectQuota()
[get|set|clear]ProjectId()
```

set|assign & remove|clear are basically synonymous so we can choose either 
but keep them consistent.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 62)


s/path/direcotry/ and we can do a `isdir` check inside.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 67)


4 space indetation & s/path/directory/.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 70 - 72)


Since we don't do recursive calls for `getProjectId`, I think we can just 
internally detect if the path is a directory without concerns about performance.

Therefore:

```
Result getProjectId(const std::string& path);
```



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 75 - 83)


These two methods don't need to appear in the header as they are in fact 
just two internal callbacks driven by the public version of their namesakes.

We can put them in the cpp file: 

```
namespace internal {
static Try setProjectId(
const string& path, 
prid_t projectId,
const struct stat& stat);

static Try clearProjectId(
const string& path, 
prid_t projectId,
const struct stat& stat)
}
```

In fact, I think we can further simplify by removing `clearProjectId()` as 
it is really just a special case of `setProjectId()`. 

I understand that separating them is for clarity and you go a check

```
if (projectId == NON_PROJECT_ID) {
  return NonProjectError();
}
```

in `setProjectId()` so it's less error-prone to use.

However we can do the check in the public version of `setProjectId()` so 
the private `setProjectId()` doesn't need it do it again per file/directory.

Without this check the private versions of `setProjectId()` and 
`clearProjectId()` are identical and we reduce code duplication by consolidate 
them into one.

See my comments on `xfsSetProjectQuota()` for the internal namespace 
explanation.

Also I see that in the tests you have directly used `setProjectId(path)` on 
a file directly so moving this into cpp would force you to change it. I'll 
comment on that test directly.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 19 - 20)


I think the last sentence is too specific to CentOS and the first two 
sentences have already provided sufficient infomation, i.e., "xfsprogs versions 
earlier than 4.5", so we can remove the last sentence.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59)


Nit: 512u.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 60)


Add an empty line.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 67)


This is capitalized so it works like a class?

Can we call it `nonProjectError()`?



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 69)


s/invalid/Invalid/



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 131)


It would be more idiomatic to put this in a 

```
namespace internal {
  static Try setProjectQuota(
  const string& path,
  prid_t projectId,
  Bytes limit)
  {
...
  }
}
```

So when in your public `setProjectQuota()` it'll be calling 
`internal::setProjectQuota()`, which is clear that a wrapper is calling some 
internal helper, as compared to calling `xfsSetProjectQuota()`. (Because this 
entire file is for XFS so a `xfs` prefix doesn't disambiguate as effectively. 
The same applies to using "assign vs. 

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 30, 2016, 10:18 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 29, 2016, 8:55 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and updated.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 21e2965bb7cd9b88a5c787dd3efe0673c71cdc4f 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 26, 2016, 5:06 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[haosdent huang]

> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 17
> > 
> >
> > Move this after below stout headers.
> 
> James Peach wrote:
> The Mesos C++ style guide says that this should come first.
> 
> Jie Yu wrote:
> Sampled a few files, non of them are in this style. Either the style 
> guide is not accurate, or we don't follow Mesos style guide consistently 
> (which is bad).
> 
> Fot the time being, let's just be consistent?

The rule in [our 
clang-format](https://github.com/apache/mesos/blob/master/support/clang-format#L38-L46)
 is 

```
IncludeCategories:
  - Regex:   '^<.*.h>'
Priority:1
  - Regex:   '^<.*.hpp>'
Priority:3
  - Regex:   '^<.*>'
Priority:2
  - Regex:   '.*'
Priority:4
```


- haosdent


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


On March 22, 2016, 11:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 11:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 305)


More strictly: `0u`.

Plus, it would be clearer is it is defined as a constant.

// A sentinel project ID that indicates that the file/directory is not 
associated with a valid project ID.
constexpr prid_t NON_PROJECT_ID = 0u;

As mentioned in another review, I think we can limit the use of this 
sentinel in the util.


- Jiang Yan Xu


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On March 23, 2016, 5:39 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp, lines 17-19
> > 
> >
> > So this is bug of the xfs headers?
> > 
> > I guess it's still not clear to me why this is relevant here. Is it a 
> > bug of XFS? 
> > 
> > It looks like textdomain() is for i18n support. I guess this has to do 
> > with path names being non ASCII? When ENABLE_GETTEXT is definied here and 
> > no other dependencies are checked, will XFS headers go the right thing?
> > 
> > This might be too long of a discussion for here so I was suggesting a 
> > link where the above question is answered (or source).

Yes I'd say this is a bug in the XFS headers. This happens due to the way 
xfsprogs was built by the distribution. I'm not sure where I would link to, but 
it's fairly clear from the headers what is going on.


> On March 23, 2016, 5:39 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp, line 119
> > 
> >
> > I know I asked this previoiusly but what's the difference between this 
> > and:
> > 
> > ```
> > fs_disk_quota_t quota;
> > ```
> > 
> > If it's the same, then the more common syntax is preferred because this 
> > syntax reads like the 1st member of the struct has special some 
> > meaning/usage.

This is a ``C`` zero-initialization. I can either do this or ``memset`` the 
structure to zero, to get the same result.


- James


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


On March 22, 2016, 11:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 11:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 22, 2016, 11:24 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On March 22, 2016, 4:46 p.m., Gilbert Song wrote:
> > BTW, should we also add `utils.cpp` to `CMakeList.txt`?

Cmake support is going to need a lot more than that :-/


- James


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


On March 22, 2016, 1:20 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 1:20 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Gilbert Song]

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



BTW, should we also add `utils.cpp` to `CMakeList.txt`?


src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 94)


Just some small formatting issues:

We try to add Apostrophe for the variables in error/failure messages, e.g.,
```
return ErrnoError("Unable to access '" + path + "'");
```

Could we update the others below :) ?



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 99)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 104)


one newline above?



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 139)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 177)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 228)


ditto here. Could we do:

```return Error("Failed to open '" + path + "': " + fd.error());
```



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 235)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 256)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 264)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 275)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 291)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 299)


ditto.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 310)


ditto.


- Gilbert Song


On March 21, 2016, 6:20 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 21, 2016, 6:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 22, 2016, 1:20 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 21, 2016, 9:46 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and fixed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 21, 2016, 6:30 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Fixed review issues.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 53
> > 
> >
> > Instead of relying on parameter, can we use os::stat::isdir here?
> 
> James Peach wrote:
> ``isdir`` always follows symlinks. From the caller of this we always make 
> sure what kind of inode we have so we don't need to ``stat(2)`` again and 
> potentially get it wrong.
> 
> Jie Yu wrote:
> From the API perspective, i just feel it strange that we need to pass 
> around 'isdir' which we can always derive.
> 
> Regarding the symlink semantics, we should clearly document the semantics 
> for those public functions (i.e., do we follow symlink or not).
> 
> I am wondering if we can do a symlink (`os::stat::islink`) check in those 
> public functions. For instance, return Nothing if 'path' passed to 
> clearProjectId is a symlink.

Yeh I can do this. We'll just end up ``stat(2)``ing the path multiple times at 
different layers.


- James


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


On March 21, 2016, 6:30 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 21, 2016, 6:30 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/linux/xfs.hpp PRE-CREATION 
>   src/linux/xfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 21, 2016, 6:18 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased. Added --enable-xfs-disk-isolator.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jie Yu]

> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 17
> > 
> >
> > Move this after below stout headers.
> 
> James Peach wrote:
> The Mesos C++ style guide says that this should come first.

Sampled a few files, non of them are in this style. Either the style guide is 
not accurate, or we don't follow Mesos style guide consistently (which is bad).

Fot the time being, let's just be consistent?


> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 53
> > 
> >
> > Instead of relying on parameter, can we use os::stat::isdir here?
> 
> James Peach wrote:
> ``isdir`` always follows symlinks. From the caller of this we always make 
> sure what kind of inode we have so we don't need to ``stat(2)`` again and 
> potentially get it wrong.

>From the API perspective, i just feel it strange that we need to pass around 
>'isdir' which we can always derive.

Regarding the symlink semantics, we should clearly document the semantics for 
those public functions (i.e., do we follow symlink or not).

I am wondering if we can do a symlink (`os::stat::islink`) check in those 
public functions. For instance, return Nothing if 'path' passed to 
clearProjectId is a symlink.


> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 187
> > 
> >
> > This interface looks weird. I understand that project quota is set on 
> > the device level. Can we make it explicit? and expose another public method 
> > to get device by path?
> 
> James Peach wrote:
> This is not a general-purpose interface; it's just here for the XFS 
> isolator. If we require a device path here, then we just foist that work off 
> onto the caller. I think that it is simpler to hide the device requirement 
> from that level and just do it implicitly (at least until we can show there's 
> a performance win to allowing the caller to cache it).

I made this comment due to the confusion I got while reviewing the code. My 
first reaction was that quota for a given project id is tied to a given 'path'. 
I am pretty sure other readers will have the same confusion until they read the 
code more.

I just thougth we should make the API semantics more clear to improve 
readability.


- Jie


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


On March 21, 2016, 2 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 21, 2016, 2 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/linux/xfs.hpp PRE-CREATION 
>   src/linux/xfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 17
> > 
> >
> > Move this after below stout headers.

The Mesos C++ style guide says that this should come first.


> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 53
> > 
> >
> > Instead of relying on parameter, can we use os::stat::isdir here?

``isdir`` always follows symlinks. From the caller of this we always make sure 
what kind of inode we have so we don't need to ``stat(2)`` again and 
potentially get it wrong.


> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 187
> > 
> >
> > This interface looks weird. I understand that project quota is set on 
> > the device level. Can we make it explicit? and expose another public method 
> > to get device by path?

This is not a general-purpose interface; it's just here for the XFS isolator. 
If we require a device path here, then we just foist that work off onto the 
caller. I think that it is simpler to hide the device requirement from that 
level and just do it implicitly (at least until we can show there's a 
performance win to allowing the caller to cache it).


> On March 19, 2016, 10:10 p.m., Jie Yu wrote:
> > src/linux/xfs.cpp, line 195
> > 
> >
> > is block size always 512 bytes in xfs? If not, worth adding a TODO.

Basic blocks not related to the underlying filesystem block size, they are just 
a 512 byte unit used in the quota API.


- James


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


On March 21, 2016, 2 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 21, 2016, 2 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/linux/xfs.hpp PRE-CREATION 
>   src/linux/xfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 17, 2016, 10:43 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jie Yu]

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




src/linux/xfs.cpp (line 17)


Move this after below stout headers.



src/linux/xfs.cpp (line 49)


Kill one line here.



src/linux/xfs.cpp (line 53)


Instead of relying on parameter, can we use os::stat::isdir here?



src/linux/xfs.cpp (line 79)


Let's try to be consistent here. Either use "xfsctl" or "::xfsctl", but 
consistently.



src/linux/xfs.cpp (line 118)


isError



src/linux/xfs.cpp (line 149)


A nits: can you move all helpers above public functions.



src/linux/xfs.cpp (line 185)


2 lines apart.



src/linux/xfs.cpp (line 187)


This interface looks weird. I understand that project quota is set on the 
device level. Can we make it explicit? and expose another public method to get 
device by path?



src/linux/xfs.cpp (line 191)


Can you return Error instead of CHECK here?



src/linux/xfs.cpp (line 195)


is block size always 512 bytes in xfs? If not, worth adding a TODO.



src/linux/xfs.cpp (line 196)


indentation.


- Jie Yu


On March 18, 2016, 3:46 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 18, 2016, 3:46 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/linux/xfs.hpp PRE-CREATION 
>   src/linux/xfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated March 18, 2016, 3:46 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach