----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68018/#review211533 -----------------------------------------------------------
src/linux/seccomp/seccomp.hpp Lines 52 (patched) <https://reviews.apache.org/r/68018/#comment296838> s/Loads/Load/ src/linux/seccomp/seccomp.hpp Lines 56 (patched) <https://reviews.apache.org/r/68018/#comment296839> Suggest to rename the argument `ctx` to `_ctx`. src/linux/seccomp/seccomp.hpp Lines 58-65 (patched) <https://reviews.apache.org/r/68018/#comment296849> I would suggest to define these 3 helpers as static functions directly in seccomp.cpp rather than part of the class `SeccompFilter`. src/linux/seccomp/seccomp.hpp Lines 59 (patched) <https://reviews.apache.org/r/68018/#comment296840> s/ContainerSeccompProfile_Syscall/ContainerSeccompProfile::Syscall/ I think we usually use `message::submessage` rather than `message_submessage`. And maybe we do not need `mesos::` here? src/linux/seccomp/seccomp.hpp Lines 65 (patched) <https://reviews.apache.org/r/68018/#comment296841> Ditto. src/linux/seccomp/seccomp.cpp Lines 17 (patched) <https://reviews.apache.org/r/68018/#comment296842> This should be the last include rather than the first one. src/linux/seccomp/seccomp.cpp Lines 22 (patched) <https://reviews.apache.org/r/68018/#comment296843> Why do we need this? src/linux/seccomp/seccomp.cpp Lines 26-27 (patched) <https://reviews.apache.org/r/68018/#comment296844> I think we do not need these, you can just use `ContainerSeccompProfile::Syscall` in the code below. src/linux/seccomp/seccomp.cpp Lines 29-32 (patched) <https://reviews.apache.org/r/68018/#comment296845> These should be before those `using::mesos`. src/linux/seccomp/seccomp.cpp Lines 66 (patched) <https://reviews.apache.org/r/68018/#comment296846> Can you please mention the side effects here? src/linux/seccomp/seccomp.cpp Lines 73 (patched) <https://reviews.apache.org/r/68018/#comment296847> Should `int arch` be `ContainerSeccompProfile::Architecture arch`? src/linux/seccomp/seccomp.cpp Lines 90-91 (patched) <https://reviews.apache.org/r/68018/#comment296848> A newline between. src/linux/seccomp/seccomp.cpp Lines 125 (patched) <https://reviews.apache.org/r/68018/#comment296850> Just a question: So we will only add a rule if the process's capabilities has **all** the `includesCaps` or has **any** of the `excludesCaps`, right? src/linux/seccomp/seccomp.cpp Lines 166 (patched) <https://reviews.apache.org/r/68018/#comment296851> Better to initilize `ret` with 0 rather than -1. src/linux/seccomp/seccomp.cpp Lines 182 (patched) <https://reviews.apache.org/r/68018/#comment296852> s/Cannot/Failed to/ src/linux/seccomp/seccomp.cpp Lines 199 (patched) <https://reviews.apache.org/r/68018/#comment296853> Kill this newline. - Qian Zhang On Nov. 8, 2018, 11:24 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68018/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2018, 11:24 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang. > > > Bugs: MESOS-9034 > https://issues.apache.org/jira/browse/MESOS-9034 > > > Repository: mesos > > > Description > ------- > > `SeccompFilter` class is a wrapper for `libseccomp` API. Its main > purpose is to provide a translation of the `ContainerSeccompProfile` > message into calls of `libseccomp` API. > > > Diffs > ----- > > src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 > src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa > src/linux/seccomp/seccomp.hpp PRE-CREATION > src/linux/seccomp/seccomp.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/68018/diff/8/ > > > Testing > ------- > > > Thanks, > > Andrei Budnik > >
