----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89630 -----------------------------------------------------------
Looks great. I think you're covering all the places where roles are specified now. But I'd like to see the validate() method move onto a Role/Roles class somewhere like Resources, rather than freestanding in an internal namespace. src/common/validation.hpp (lines 27 - 34) <https://reviews.apache.org/r/35711/#comment142465> I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource protobuf message and a Resources class with parse() and validate(), we should also have a Roles::validate(). src/common/validation.cpp (lines 20 - 26) <https://reviews.apache.org/r/35711/#comment142489> Unused? src/common/validation.cpp (lines 43 - 44) <https://reviews.apache.org/r/35711/#comment142285> I suppose role name ".." would also be invalid by similar logic (cannot `mkdir ..`) src/common/validation.cpp (lines 50 - 55) <https://reviews.apache.org/r/35711/#comment142468> s/space characters/whitespace/ src/common/validation.cpp (line 58) <https://reviews.apache.org/r/35711/#comment142491> What is `\x20`? You didn't comment it. src/common/validation.cpp (line 61) <https://reviews.apache.org/r/35711/#comment142467> s/space/whitespace/ src/master/flags.cpp (line 189) <https://reviews.apache.org/r/35711/#comment142493> This could be a quick Roles::parse() src/master/flags.cpp (lines 194 - 196) <https://reviews.apache.org/r/35711/#comment142471> Check for the `*` first, then the other check can just `return Role::validate(role);`. src/slave/flags.cpp (line 52) <https://reviews.apache.org/r/35711/#comment142474> s/value/resourcesString/ since 'value' is referenced as something else in the help string above. src/slave/flags.cpp (lines 61 - 62) <https://reviews.apache.org/r/35711/#comment142477> How about `return Resources::validate(parsed.get());` src/tests/common/validation_tests.cpp (lines 38 - 40) <https://reviews.apache.org/r/35711/#comment142482> Where are these being used? src/tests/common/validation_tests.cpp (line 50) <https://reviews.apache.org/r/35711/#comment142484> s/name/names/ s/contains/contain/ src/tests/common/validation_tests.cpp (lines 59 - 60) <https://reviews.apache.org/r/35711/#comment142480> Please add tests for strings starting with and strings containing a `-`. src/tests/common/validation_tests.cpp (line 63) <https://reviews.apache.org/r/35711/#comment142481> Test `\n` as well? - Adam B On June 27, 2015, 4:35 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35711/ > ----------------------------------------------------------- > > (Updated June 27, 2015, 4:35 a.m.) > > > Review request for mesos, Adam B and Jie Yu. > > > Bugs: MESOS-2210 > https://issues.apache.org/jira/browse/MESOS-2210 > > > Repository: mesos > > > Description > ------- > > Disallow special characters in role name. > > > Diffs > ----- > > src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5 > src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 > src/common/validation.hpp PRE-CREATION > src/common/validation.cpp PRE-CREATION > src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8 > src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0 > src/tests/common/validation_tests.cpp PRE-CREATION > src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e > > Diff: https://reviews.apache.org/r/35711/diff/ > > > Testing > ------- > > make -j8 check > > > Thanks, > > haosdent huang > >
