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