mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilbert Song <songzihao1...@gmail.com>
Subject Re: Review Request 68019: Added a parser for the Docker Seccomp config format.
Date Sat, 19 Jan 2019 01:05:17 GMT

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


Fix it, then Ship it!





src/linux/seccomp/seccomp_parser.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/68019/#comment297809>

    Not a big issue but seems like in our code base we usually pass in a pointer instead of
reference.
    
    The positive side of passing a pointer is that it would be more explicit for code reader
when they read the caller side of this method, they know the protobuf object will be mutated.
    
    You can regard `Architecture_Parse()` as an example.
    
    cc @bmahler



src/linux/seccomp/seccomp_parser.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/68019/#comment297812>

    ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 164-167 (patched)
<https://reviews.apache.org/r/68019/#comment297816>

    does the order of the `architecture/subarchitecture` matter? asking cause I am not sure
if we need architecture before its corresponding subarchitecture in the repeated field



src/linux/seccomp/seccomp_parser.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/68019/#comment297813>

    ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 263 (patched)
<https://reviews.apache.org/r/68019/#comment297815>

    ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 283 (patched)
<https://reviews.apache.org/r/68019/#comment297817>

    index->as<uint32_t>()?



src/linux/seccomp/seccomp_parser.cpp
Lines 290 (patched)
<https://reviews.apache.org/r/68019/#comment297818>

    ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 297 (patched)
<https://reviews.apache.org/r/68019/#comment297819>

    ditto



src/linux/seccomp/seccomp_parser.cpp
Lines 325 (patched)
<https://reviews.apache.org/r/68019/#comment297814>

    ditto.



src/linux/seccomp/seccomp_parser.cpp
Lines 490-494 (patched)
<https://reviews.apache.org/r/68019/#comment297827>

    Since we somehow have to create the profile in launch.cpp, could we avoid create the `ctx`
object twice?
    
    Instead, for the default profile, I think we could add this verification in isolator::create
method after we parse it


- Gilbert Song


On Nov. 8, 2018, 7:24 a.m., Andrei Budnik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68019/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2018, 7:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9105
>     https://issues.apache.org/jira/browse/MESOS-9105
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Docker Seccomp config is a JSON file containing Seccomp filtering
> rules. This patch introduces a parser for Docker Seccomp config format.
> This parser accepts a JSON-string, parses and validates it, then
> returns a prepared `ContainerSeccompProfile` message.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt a574d449dc26b820cbef7ff0b5e94b42b6fe86cf 
>   src/Makefile.am cd785255fcdf1302a8f9fa358039e5d1f200e132 
>   src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
>   src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68019/diff/14/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


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