> On May 20, 2014, 10:01 a.m., Dominic Hamon wrote: > > src/master/master.cpp, line 2871 > > <https://reviews.apache.org/r/21665/diff/1/?file=584715#file584715line2871> > > > > is it worth differentiating between the future failing and the future > > being ready but not having a valid principal?
On the server side probably no. The client definitely differentiates the two. > On May 20, 2014, 10:01 a.m., Dominic Hamon wrote: > > src/sasl/authenticator.hpp, line 352 > > <https://reviews.apache.org/r/21665/diff/1/?file=584716#file584716line352> > > > > s/static_cast/reinterpret_cast/ > > > > std::string(input, inlen); > > > > CHECK_NOTNULL(input); > > CHECK_NOTNULL(principal); > > CHECK_NOTNULL(output); > > > > maybe also: > > > > Option<std::string>* principal = > > reinterpret_cast<Option<std::string>*>(_principal); > > CHECK_NONE(*principal); I think static_cast is fine for casting from void*. I've seem some discussions about whether reinterpret_cast is suitable in different versions of the spec but static_cast seems safe. http://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinterpret-cast-when-casting-a-void-to-whatever Changed to: CHECK_NOTNULL(input); CHECK_NOTNULL(context); CHECK_NOTNULL(output); // Save the input. Option<std::string>* principal = static_cast<Option<std::string>*>(context); CHECK(principal->isNone()); *principal = std::string(input, inputLength); > On May 20, 2014, 10:01 a.m., Dominic Hamon wrote: > > src/sasl/authenticator.hpp, line 386 > > <https://reviews.apache.org/r/21665/diff/1/?file=584716#file584716line386> > > > > unnecessary as this defaults to None(). Right. But I think the explicitness here to beneficial. > On May 20, 2014, 10:01 a.m., Dominic Hamon wrote: > > src/sasl/authenticator.hpp, line 409 > > <https://reviews.apache.org/r/21665/diff/1/?file=584716#file584716line409> > > > > this seems fragile.. could it be a std::vector<sasl_callback_t> instead? sasl_server_new expects an array: LIBSASL_API int sasl_server_new(const char *service, const char *serverFQDN, const char *user_realm, const char *iplocalport, const char *ipremoteport, const sasl_callback_t *callbacks, unsigned flags, sasl_conn_t **pconn); Are you saying that we can use "&callbacks[0]" for "vector<sasl_callback_t> callbacks" to get the array? While this works, I am going to punt on this because this is not the convention in the codebase when configuring static sized lists. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21665/#review43495 ----------------------------------------------------------- On May 20, 2014, 3:26 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21665/ > ----------------------------------------------------------- > > (Updated May 20, 2014, 3:26 p.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, and Vinod Kone. > > > Bugs: MESOS-1383 > https://issues.apache.org/jira/browse/MESOS-1383 > > > Repository: mesos-git > > > Description > ------- > > The principal returned from Authenticator will be useful in a next patch for > https://issues.apache.org/jira/browse/MESOS-1373 > > > Diffs > ----- > > src/master/master.hpp 5e0d712de997bd10079655df9b07099284f8257f > src/master/master.cpp 075755cad5c50a57c92d7d82f2466b467796f673 > src/sasl/authenticator.hpp 784ecc876bef236fdf6b83ac118c5e7e4a02f156 > src/tests/sasl_tests.cpp 945426d899e0e3c8cc32f494f26a0103f610ddc6 > > Diff: https://reviews.apache.org/r/21665/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
