----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33295/#review80582 -----------------------------------------------------------
This review was pending a couple of days in my outbox, hence it may be partially or entirely invalid by now given that you had updated the RR - sry for that. Feel free to drop any outdated comments without further ado. 3rdparty/libprocess/include/process/firewall.hpp <https://reviews.apache.org/r/33295/#comment130638> That is the wrong header, we have to use the "Apache License V.2" header for stout and libprocess. The "ASF" header you are using is for mesos only. The correct header would be: ``` /** * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License */ ``` 3rdparty/libprocess/include/process/firewall.hpp <https://reviews.apache.org/r/33295/#comment130640> I forgot the verbally proposed name for this function, but maybe a negated result and the name `reject()` would fit better. 3rdparty/libprocess/include/process/firewall.hpp <https://reviews.apache.org/r/33295/#comment130639> Even though this is implementation is tiny, would it still make sense to move it into a cpp-file? 3rdparty/libprocess/include/process/process.hpp <https://reviews.apache.org/r/33295/#comment130648> Could you elaborate on the term "__top__ firewall rule"? 3rdparty/libprocess/include/process/process.hpp <https://reviews.apache.org/r/33295/#comment130649> The doxygen rendered output IMHO looks better when using a capital letter on the start of description -- however, it seems the other argument descriptions start with a lower case. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/33295/#comment130643> `<< "': firewall rule forbids connection";` might be a bit more mesos'esque. 3rdparty/libprocess/src/tests/process_tests.cpp <https://reviews.apache.org/r/33295/#comment130647> const string 3rdparty/libprocess/src/tests/process_tests.cpp <https://reviews.apache.org/r/33295/#comment130646> const string 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/33295/#comment131629> Given that you simply take a copy of the firewallRule instance var, the "right" name would be `firewallRule_`, no? - Till Toenshoff On April 22, 2015, 2:35 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33295/ > ----------------------------------------------------------- > > (Updated April 22, 2015, 2:35 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff. > > > Bugs: MESOS-2620 > https://issues.apache.org/jira/browse/MESOS-2620 > > > Repository: mesos > > > Description > ------- > > Introduces the interface `FirewallRule` which will be matched against > incoming connections in order to allow them to be served or being blocked. > For details, check the [design > doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit). > > > Diffs > ----- > > 3rdparty/libprocess/include/Makefile.am > 3da3e6cef1b5cb66c223425744d84741846ea730 > 3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION > 3rdparty/libprocess/include/process/process.hpp > 392c74df3e8a122aecd3633dffdeec4bcbd1f097 > 3rdparty/libprocess/src/process.cpp > 97ac09fd10b767bc46387abc3657d005a00830c8 > 3rdparty/libprocess/src/tests/process_tests.cpp > eb38edce2c483ba7f963a826893a15a075238618 > > Diff: https://reviews.apache.org/r/33295/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
