----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54297/#review157925 -----------------------------------------------------------
Fix it, then Ship it! I would file a follow up issue to add tests for the validation code itself. src/slave/containerizer/mesos/io/switchboard.cpp (lines 492 - 493) <https://reviews.apache.org/r/54297/#comment228587> How about: ```cpp // TODO(klueska): Move this to `src/slave/validation.hpp` and make the agent validate all the calls before forwarding them to the switchboard. ```` src/slave/containerizer/mesos/io/switchboard.cpp (lines 847 - 849) <https://reviews.apache.org/r/54297/#comment228582> Kill this, you already have an explicit `CHECK` for this before you invoke this. src/slave/containerizer/mesos/io/switchboard.cpp (lines 852 - 866) <https://reviews.apache.org/r/54297/#comment228584> For this handler, it should *only* validate that the second record is a `ProcessIO` record. It shouldn't validate the other types and just return an error if the record is of some other type. Can you just do: ```cpp case agent::Call::AttachContainerInput::UNKNOWN: case agent::Call::AttachContainerInput::CONTAINER_ID: { return Error("Expecting 'attach_container_input.type' to be 'PROCESS_IO'" " instead of: '" + call.type() + "'"); ``` src/slave/containerizer/mesos/io/switchboard.cpp (lines 934 - 936) <https://reviews.apache.org/r/54297/#comment228581> We rely on the compiler failing if a `condition` is missed in the case statements `-Wswitch`. Kill this and just have `UNREACHABLE()` in the outer scope. src/slave/containerizer/mesos/io/switchboard.cpp (lines 939 - 941) <https://reviews.apache.org/r/54297/#comment228585> Kill this and replace with unreachable outside. src/slave/containerizer/mesos/io/switchboard.cpp (lines 984 - 985) <https://reviews.apache.org/r/54297/#comment228576> Kill this invariant check. The agent does not validate if the second record type is `PROCESS_IO`. We need to validate it in the handler itself for now as per our earlier discussion. src/slave/containerizer/mesos/io/switchboard.cpp (lines 1044 - 1046) <https://reviews.apache.org/r/54297/#comment228580> Kill this and just have a `UNREACHABLE()`. - Anand Mazumdar On Dec. 4, 2016, 6:55 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54297/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2016, 6:55 a.m.) > > > Review request for mesos, Anand Mazumdar and Jie Yu. > > > Bugs: MESOS-6467 > https://issues.apache.org/jira/browse/MESOS-6467 > > > Repository: mesos > > > Description > ------- > > Added support for ATTACH_CONTAINER_INPUT to the io switchboard. > > > Diffs > ----- > > src/slave/containerizer/mesos/io/switchboard.cpp > 778367a268ec350ed438bc9fe9d359d63bdb5503 > src/tests/containerizer/io_switchboard_tests.cpp > d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 > > Diff: https://reviews.apache.org/r/54297/diff/ > > > Testing > ------- > > make -j check > > > Thanks, > > Kevin Klues > >
