mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 61901: Added common validation for Volume.
Date Mon, 28 Aug 2017 19:11:31 GMT

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




src/common/validation.cpp
Lines 206 (patched)
<https://reviews.apache.org/r/61901/#comment260012>

    Seems like you could start with adding this:
    ```
    if (!path::absolute(volume.host_path()) &&
        !path::absolute(volume.container_path())) {
      ...
    }
    ```
    
    (Some form of which exists in the filesystem/linux isolator and Docker containerizer)



src/common/validation.cpp
Lines 212-217 (patched)
<https://reviews.apache.org/r/61901/#comment260010>

    Might be clearer to count the number of occurrences rather than bit-mask them.
    
    Then error if the number of occurrences is not equal to one.
    
    ---
    
    Also, I'm surprised that we don't have this validation already.  
    
    It seems like, in the existing code, if you:
    1) Specify some value for all three fields.
    2) Enable the appropriate isolators (filesystem/linux, docker/volume, volume/sandbox_path).
    3) All three isolators will do their thing, possibly running over each other in the process.



src/common/validation.cpp
Lines 249-250 (patched)
<https://reviews.apache.org/r/61901/#comment260007>

    The default case should be an error on its own.  You could leave out the default case
so that this code won't compile when a new Source type is defined.


- Joseph Wu


On Aug. 24, 2017, 4:58 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61901/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2017, 4:58 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Bugs: MESOS-7306
>     https://issues.apache.org/jira/browse/MESOS-7306
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added common validation for Volume.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5157b2baac4ff6c4493faaeb7a311381e8e76fe7 
>   src/common/validation.hpp 8c92436eb6c8aa87ffd6d6cf3f42079d6c90e525 
>   src/common/validation.cpp 33b501bfa6613dab70a663f552a424ffa8a15b0a 
>   src/tests/CMakeLists.txt 6c05ea93c3288fb9608c191234f95e270390d89e 
>   src/tests/common_validation_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61901/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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