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




src/linux/cgroups.hpp (line 645)
<https://reviews.apache.org/r/44974/#comment186444>

    Because a selector major/minor value can either bo the '*' character or an 
unsinged integer, I'd probably change my constructors to reflect this. I.e.:
    
    ```
    Selector(Type type, unsigned int major, unsigned int minor);
    Selector(Type type, char minor, unsigned int minor);
    Selector(Type type, unsigned int major, char minor);
    Selector(Type type, char major, char minor);
    ```
    
    And then just stringify them under the hood to store them as strings for a 
common representation.



src/linux/cgroups.hpp (lines 660 - 662)
<https://reviews.apache.org/r/44974/#comment186440>

    I would probably just call these read/write/mknod instead of adding 
"device" as a prefix.



src/linux/cgroups.hpp (lines 665 - 666)
<https://reviews.apache.org/r/44974/#comment186439>

    I see you copied my comment, but changed the name of the class to 
DeviceEntry :)
    
    We should probaly update one or the other.
    
    I am personaly in favor of simply 'Device', but I'll like @bmahler and 
@nnielsen chime in on their preferences.



src/linux/cgroups.hpp (lines 671 - 673)
<https://reviews.apache.org/r/44974/#comment186438>

    We typically don't write accessor's like this.
    
    Also, we don't space things out like this.  We use single spaces between 
the different elements.



src/linux/cgroups.hpp (lines 675 - 684)
<https://reviews.apache.org/r/44974/#comment186443>

    I know we kept going back and forth about this offline, but in the end, I 
think it makes sense to just make all of this public and non-const. I'd even go 
so far as to say we just make this a struct instead of a class.
    
    The original reasoning behind making this constructor private and the 
members const was to prevent someone from inputting wrong values for the 
major/minor numbers at initialization (because these values can either be the 
'*' character or an unsigned integer).  I think this is actually better handled 
in the Selector() constructor, as mentioned above.
    
    Moreover, even if they have bad values, this will all be caught by errors 
when reading/writing to the `devices.allow`, `devices.deny`, and `devices.list` 
files.



src/linux/cgroups.hpp (lines 687 - 694)
<https://reviews.apache.org/r/44974/#comment186446>

    For the use case of the GPU, I also need `operator==` for all of these 
structs.  They are implemented in my github branch I showed you before:
    
    
https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices



src/linux/cgroups.cpp (lines 2426 - 2441)
<https://reviews.apache.org/r/44974/#comment186448>

    There's probably no need for this extra level of indirection through the 
`type` variable.  I would also consider using a `switch` statement since these 
are all enums.
    
    ```
      switch (selector.deviceType) {
        case DeviceEntry::Selector::ALL:
          stream << "a";
          break;
        case DeviceEntry::Selector::BLOCK:
          stream << "b";
          break;
        case DeviceEntry::Selector::CHARACTER:
          stream << "c";
          break;
        default:
          LOG(ERROR) << "Unable to parse device type: " + selector.deviceType;
          return stream;
      }
    
      return stream << " "
                    << selector.deviceMajor
                    << ":"
                    << selector.deviceMinor;
    ```



src/linux/cgroups.cpp (line 2459)
<https://reviews.apache.org/r/44974/#comment186451>

    The style guieds require two newlines between functions in a `.cpp` file. 
Could you do a pass to fix this up?



src/linux/cgroups.cpp (lines 2463 - 2465)
<https://reviews.apache.org/r/44974/#comment186450>

    You shouldn't have to call `stringify()` around the `selector` and `access` 
fields because you have already provided `operator<<` operators for them.



src/linux/cgroups.cpp (lines 2523 - 2526)
<https://reviews.apache.org/r/44974/#comment186452>

    I would probably move this block up to line 2509.  We typically try to 
error out on cases we know are error conditions as early in the code as 
possible.  
    
    In this case, we know we require 3 arguments as soon as we determine that 
the first token is not an "a", so we shoud error out right away.



src/linux/cgroups.cpp (line 2539)
<https://reviews.apache.org/r/44974/#comment186453>

    I would probably reverse this to say:
    
    ```
    if (deviceNumbers[0] != "*" major.isError())
    ```
    
    The reason being that the only reason we care about the error is because 
the `deviceNumber != "*"`.



src/linux/cgroups.cpp (line 2546)
<https://reviews.apache.org/r/44974/#comment186454>

    Same comment as above.



src/linux/cgroups.cpp (line 2552)
<https://reviews.apache.org/r/44974/#comment186455>

    I would suggest removing this newline here.



src/linux/cgroups.cpp (line 2556)
<https://reviews.apache.org/r/44974/#comment186457>

    Sicne we have `using stad::string` at the top of this file, I would suggest 
omitting the `std::` here (and everywhere else in this file).



src/linux/cgroups.cpp (line 2557)
<https://reviews.apache.org/r/44974/#comment186456>

    What is this line doing?  There should be no spaces in `tokens[2]` by this 
point because we originally tokenized on spaces at the beginning of the 
function.



src/linux/cgroups.cpp (lines 2567 - 2581)
<https://reviews.apache.org/r/44974/#comment186458>

    We typically prefer `foreach` blocks over standard for/while loops when 
possible. I.e., the way I showed you on the github branch we were looking at 
last night:
    
    ```
    foreach (const char& permission, tokens[2]) {
      if (permission == 'r') {
        access.read = true;
      } else if (permission == 'w') {
        access.write = true;
      } else if (permission == 'm') {
        access.mknod = true;
      } else {
        return Error("Unable to parse cgroups device entry: "
                     "invalid access arguments: " + tokens[2]);
      }
    }
    ```



src/linux/cgroups.cpp (lines 2586 - 2590)
<https://reviews.apache.org/r/44974/#comment186459>

    Why the extra level of indirection through something like 
`DeviceEntry::Selector(()`
    
    This would work just as well, and is preferred:
    ```
    DeviceEntry::DeviceEntry()
      : deviceSelector(DeviceEntry::Selector::ALL, "*", "*"),
        deviceAccess(true, true, true) {}
    ```



src/linux/cgroups.cpp (lines 2596 - 2604)
<https://reviews.apache.org/r/44974/#comment186460>

    These should probably go away, as per my previos comments.



src/linux/cgroups.cpp (lines 2637 - 2641)
<https://reviews.apache.org/r/44974/#comment186461>

    I would probably put the `stringify(devicEntry)` directly in the `write()` 
call. No need to create a remporary variable here unnecessarily.



src/linux/cgroups.cpp (line 2642)
<https://reviews.apache.org/r/44974/#comment186462>

    I would place a newline above this line.



src/linux/cgroups.cpp (line 2654)
<https://reviews.apache.org/r/44974/#comment186463>

    Same comment as above.



src/linux/cgroups.cpp (line 2659)
<https://reviews.apache.org/r/44974/#comment186464>

    Same comment as above.


- Kevin Klues


On March 17, 2016, 7:21 p.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> -----------------------------------------------------------
> 
> (Updated March 17, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> 
> 
> Bugs: MESOS-3368
>     https://issues.apache.org/jira/browse/MESOS-3368
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> There are some helper methods added to support device cgroups in cgroups 
> abstraction to aid isolators controlling access to devices.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Reply via email to