> On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticator.hpp, line 322 > > <https://reviews.apache.org/r/14310/diff/3/?file=356659#file356659line322> > > > > Why hard-code length when you could do: > > > > if (...) { > > *result = "in-memory-auxprop"; > > } else if (...) { > > *result = "CRAM-MD5"; > > } > > > > if (length != NULL) { > > *length = strlen(*result); > > } > > > > I couldn't find good documentation on this, can length be NULL? Should > > we add an else block to guard against unexpected option strings?
Cool thanks ... although, we don't want to set length unless we've set result. ;) The documentation is in the header: * len -- length of result (may be NULL) > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticator.hpp, line 383 > > <https://reviews.apache.org/r/14310/diff/3/?file=356659#file356659line383> > > > > Is there a sasl constant for this? > > > > I see the following in sasl.h: > > > > #define SASL_AUX_PASSWORD "*userPassword" /* User Password (of authid) > > */ > > > > The leading * is odd, and I see conversation around it. For example, > > they skip over it here: > > http://marc.info/?l=cyrus-sasl&m=104072288229946&w=2 > > > > Not sure how your > > > > I see you've used the newer SASL_AUX_PASSWORD_PROP below, should you > > use that constant here and skip the *? Yup, s/userPassword/SASL_AUX_PASSWORD_PROP/. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.hpp, lines 68-73 > > <https://reviews.apache.org/r/14310/diff/3/?file=356660#file356660line68> > > > > What made you name the second argument as api? > > > > I see: > > > > /* default name for auxprop plug-in entry point is "sasl_auxprop_init" > > * similar to sasl_server_plug_init model, except only returns one > > * sasl_auxprop_plug_t structure; > > */ > > typedef int sasl_auxprop_init_t(const sasl_utils_t *utils, > > int max_version, > > int *out_version, > > sasl_auxprop_plug_t **plug, > > const char *plugname); > > > > Referring to: > > > > /* plug-in entry point: > > * utils -- utility callback functions > > * plugname -- name of plug-in (may be NULL) > > * max_version -- highest server plug version supported > > * returns: > > * out_version -- server plug-in version of result > > * pluglist -- list of mechanism plug-ins > > * plugcount -- number of mechanism plug-ins > > * results: > > * SASL_OK -- success > > * SASL_NOMEM -- failure > > * SASL_BADVERS -- max_version too small > > * SASL_BADPARAM -- bad config string > > * ... > > */ > > typedef int sasl_server_plug_init_t(const sasl_utils_t *utils, > > int max_version, > > int *out_version, > > sasl_server_plug_t **pluglist, > > int *plugcount); > > > > I can see how max_version is effectively the api version, is that why > > you renamed it? Yes. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, line 40 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line40> > > > > Sasl didn't make this const?! Nope. :( > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, line 59 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line59> > > > > You wanted me to remind you to explain this line. Yup, thanks! > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, lines 61-63 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line61> > > > > How about: CHECK(properties != NULL) << ... How about: CHECK_NOTNULL(properties) << ... ;) > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, lines 72-84 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line72> > > > > From your formatting here, it looks as though you wanted newlines? > > > > If not, then maybe we should add quotes around the values to make this > > clearer on a single line: > > > > e.g. > > << " user: '" << user << "'" I added quotes. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, line 88 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line88> > > > > It looks like you can keep property in the for loop scope: > > > > for (const propval* property = properties; property->name != NULL; > > ++property) { > > > > Did you avoid this because you wanted the for loop to fit on a single > > line? Yup. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, lines 138-139 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line138> > > > > s/for subsequent/to the same name as previous/ Thanks! > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/auxprop.cpp, line 147 > > <https://reviews.apache.org/r/14310/diff/3/?file=356661#file356661line147> > > > > Did you mean -1? > > > > From the documentation: > > * vallen -- length of value, if < 0 then strlen(value) will be used Hmm, my header file says '<= 0'! Regardless, both say '< 0' so I'll switch to -1. Thanks! > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticator.hpp, line 245 > > <https://reviews.apache.org/r/14310/diff/3/?file=356659#file356659line245> > > > > Can you add the sasl_errstring to the log message? Done. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticator.hpp, lines 97-98 > > <https://reviews.apache.org/r/14310/diff/3/?file=356659#file356659line97> > > > > Append sasl_errstring as done elsewhere? Done. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticatee.hpp, lines 259-261 > > <https://reviews.apache.org/r/14310/diff/3/?file=356658#file356658line259> > > > > Should we follow the SASL example here? > > > > We should not be sending when data/length is NULL/0 and SASL_OK is > > returned, looking at their sample client: > > > > > > https://github.com/winlibs/cyrus-sasl/blob/master/sample/sample-client.c#L818 I updated it to check for output != NULL and length > 0. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticatee.hpp, line 215 > > <https://reviews.apache.org/r/14310/diff/3/?file=356658#file356658line215> > > > > Reading the manpage for sasl_client_start, I only see SASL_CONTINUE as > > a posible return code. What made you check for SASL_OK? The sample client. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticatee.hpp, line 59 > > <https://reviews.apache.org/r/14310/diff/3/?file=356658#file356658line59> > > > > Do you want to hide this in a namespace? Ditto for AuthenticatorProcess. It's in the mesos::internal::sasl namespace which I think is okay for now. > On Sept. 25, 2013, 7:09 a.m., Ben Mahler wrote: > > src/sasl/authenticator.hpp, line 234 > > <https://reviews.apache.org/r/14310/diff/3/?file=356659#file356659line234> > > > > Exclamation warranted? ;) > > > > Ditto on these other log lines. Downgraded my excitement. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14310/#review26367 ----------------------------------------------------------- On Sept. 24, 2013, 8:26 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14310/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2013, 8:26 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Initial SASL based authentication support. (I'll likely also be cleaning some > stuff up as I deal with compilation issues with different libsasl2* versions.) > > > Diffs > ----- > > configure.ac 96ac7a81311e9f3765bd54e078368566a3cc33ca > include/mesos/mesos.proto 8f845cc9b7cd491622cb29a69f6909170510a664 > src/Makefile.am 3eae9642b639e78f101e7fe580aa803beadbebda > src/messages/messages.proto 4d400c2d8f6ecb9f7cf8635b2a98341206bbb6c5 > src/sasl/authenticatee.hpp PRE-CREATION > src/sasl/authenticator.hpp PRE-CREATION > src/sasl/auxprop.hpp PRE-CREATION > src/sasl/auxprop.cpp PRE-CREATION > src/tests/sasl_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/14310/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
