> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote: > > src/authorizer/authorizer.cpp, lines 17-28 > > <https://reviews.apache.org/r/36049/diff/5/?file=998603#file998603line17> > > > > Great update! But it seems the comment is not applying, no? > > > > The built-in authorizer factory `Try<Authorizer*> > > LocalAuthorizer::create()` does not seem to test for `nullptr`. You seem to > > do those checks in master.cpp and local.cpp instead. Or am I missing the > > point here?
`LocalAuthorizer::create()` now returns an error if new fails, i.e. returns a null pointer. Still not sure if I should remove the check in main. > On July 3, 2015, 10:29 a.m., Till Toenshoff wrote: > > src/local/local.cpp, lines 227-228 > > <https://reviews.apache.org/r/36049/diff/5/?file=998604#file998604line227> > > > > You may reach this without an error-message, no? > > > > How about: > > ``` > > if (create.isError() || create.get() == nullptr) { > > EXIT(EXIT_FAILURE) << "Could not create authorizer module '" > > << flags.authorizer << "'" > > << (create.isError() ? ": " + create.error() : ""); > > ; > > } > > > > ``` > > > > Or actually, that factory can not return a `nullptr` anymore, can it? Now the comment from the previos issue is true, so create never returns a nullptr but instead sets the `Try` to an `Error`. > On July 3, 2015, 10:29 a.m., Till Toenshoff wrote: > > src/master/flags.cpp, lines 415-424 > > <https://reviews.apache.org/r/36049/diff/5/?file=998608#file998608line415> > > > > We may want to follow the pattern used for the authenticator - as in, > > call the flag "authorizers" and prepare the CLI interface for allowing the > > activation of multiple, concurrently active authorizers. > > > > What do you think? as bespoken. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/#review90317 ----------------------------------------------------------- On July 3, 2015, 3:17 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36049/ > ----------------------------------------------------------- > > (Updated July 3, 2015, 3:17 p.m.) > > > Review request for mesos and Till Toenshoff. > > > Bugs: MESOS-2945 > https://issues.apache.org/jira/browse/MESOS-2945 > > > Repository: mesos > > > Description > ------- > > Adds and integrates helper classes needed to support an `Authorizer` module. > Also adds a flag to the master, allowing the selection of an `Authorizer` > module. > > It also adds a flag which allows to select the module name. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.hpp PRE-CREATION > include/mesos/module/authorizer.hpp PRE-CREATION > src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 > src/authorizer/authorizer.cpp PRE-CREATION > src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff > src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce > src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 > src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f > src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 > src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 > src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 > > Diff: https://reviews.apache.org/r/36049/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
