mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 44974: Added device support in cgroups abstraction.
Date Thu, 17 Mar 2016 21:30:57 GMT

-----------------------------------------------------------
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
> 
>


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