Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6484002:
Authenticate user/password with PAM in BeginSessionRequest() (Closed)
|
Created:
9 years, 10 months ago by Lambros Modified:
9 years, 7 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Visibility:
Public. |
DescriptionChromoting: UserAuthenticator interface and its implementation for PAM.
BUG=none
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75069
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixes for review comments #
Total comments: 2
Patch Set 3 : Removed host_stub_fake.cc, fixed style nit. #Patch Set 4 : Add UserAuthenticator interface #
Total comments: 3
Patch Set 5 : Fixed nit #
Total comments: 10
Patch Set 6 : Removed Pimpl, renamed Auth->Authenticator #Patch Set 7 : Add free() back in, and remove unused scoped_ptr #include. #
Total comments: 22
Patch Set 8 : Some nits fixed #
Total comments: 2
Patch Set 9 : Reformat lib_list package list #
Total comments: 2
Messages
Total messages: 18 (0 generated)
Looks very good. Most of my comments are just nits. The only real issue, is that we should not add real auth in HostStubReal. You can add host_stub_linux.cc, and use it on linux - this can be done in a separate CL. BTW, you don't need to manually send mail for code reviews. Upload your CL, then click "Publish+Mail Comments" on the left, enter reviewers, and click "Publish All My Drafts" - this will send mail to all reviewers. http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc File remoting/host/host_stub_fake.cc (right): http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... remoting/host/host_stub_fake.cc:30: protocol::LocalLoginStatus* status = new protocol::LocalLoginStatus(); We shouldn't add pam auth in HostStubFake. It is fake, and shouldn't do any real auth. I think we need host_stub_<platform>.cc which will be platform specific. This can be done in a separate CL - up to you. http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... remoting/host/host_stub_fake.cc:51: // ??? I think it should be enough to log the failure here: LOG(ERROR) << "Login failed for user " << username; http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:24: static int convFn(int num_msg, const struct pam_message** msg, ConvFunction? http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:43: const std::string& password) { nit: wrapped arguments should be aligned with the arguments on the previous line. int fun(int arg1, int arg2, int arg3, int arg4) Alternatively you can wrap first argument and indent it with 4 spaces: int fun( int arg1, int arg2, int arg3, int arg4) http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:49: // TODO(lambroslambrou): allow PAM service name to be configurable nit: comments should be full sentences: start with capital letter and end with period http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:55: // TODO(lambroslambrou): move to separate thread nit: same here http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:63: struct pam_response** resp, void* appdata_ptr) { nit: indent properly http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:75: int count; This can be inside of for() http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... remoting/host/user_auth_pam.cc:77: // Alias for readability nit: period at the end of the sentence. http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h File remoting/host/user_auth_pam.h (right): http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... remoting/host/user_auth_pam.h:30: private: nit: empty line before private: http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... remoting/host/user_auth_pam.h:33: DISALLOW_COPY_AND_ASSIGN(UserAuthPam); nit: Empty line before this one
Hi, Thanks for the feedback. Sorry about any spam! :-) What should I trim the "To:" list down to? Just one thing I don't quite understand: I'm not sure what you mean by "not add real auth in HostStubReal" and "[not] add pam auth in HostStubFake". I don't remember seeing any class called HostStubReal anywhere. Do you think HostStubFake should stay as it was (not doing any auth), and then have HostStubWin, HostStubLinux, HostStubMac? Or would you prefer a single HostStub (or HostStubReal) class, that implements different schemes using #if (OS_LINUX) .. #elif (OS_WIN) ... I think I'd prefer to handle the renaming/refactoring in a separate CL anyway. Cheers, Lambros On Thu, Feb 10, 2011 at 9:14 PM, <sergeyu@chromium.org> wrote: > Looks very good. Most of my comments are just nits. The only real issue, is > that > we should not add real auth in HostStubReal. You can add > host_stub_linux.cc, and > use it on linux - this can be done in a separate CL. > > BTW, you don't need to manually send mail for code reviews. Upload your CL, > then > click "Publish+Mail Comments" on the left, enter reviewers, and click > "Publish > All My Drafts" - this will send mail to all reviewers. > > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc > File remoting/host/host_stub_fake.cc (right): > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... > remoting/host/host_stub_fake.cc:30: protocol::LocalLoginStatus* status = > new protocol::LocalLoginStatus(); > We shouldn't add pam auth in HostStubFake. It is fake, and shouldn't do > any real auth. I think we need host_stub_<platform>.cc which will be > platform specific. This can be done in a separate CL - up to you. > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... > remoting/host/host_stub_fake.cc:51: // ??? > I think it should be enough to log the failure here: > LOG(ERROR) << "Login failed for user " << username; > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc > File remoting/host/user_auth_pam.cc (right): > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:24: static int convFn(int num_msg, const > struct pam_message** msg, > ConvFunction? > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:43: const std::string& password) { > nit: wrapped arguments should be aligned with the arguments on the > previous line. > int fun(int arg1, int arg2, > int arg3, int arg4) > > Alternatively you can wrap first argument and indent it with 4 spaces: > int fun( > int arg1, int arg2, > int arg3, int arg4) > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:49: // TODO(lambroslambrou): allow PAM > service name to be configurable > nit: comments should be full sentences: start with capital letter and > end with period > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:55: // TODO(lambroslambrou): move to > separate thread > nit: same here > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:63: struct pam_response** resp, void* > appdata_ptr) { > nit: indent properly > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:75: int count; > This can be inside of for() > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... > remoting/host/user_auth_pam.cc:77: // Alias for readability > nit: period at the end of the sentence. > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h > File remoting/host/user_auth_pam.h (right): > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... > remoting/host/user_auth_pam.h:30: private: > nit: empty line before private: > > > http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... > remoting/host/user_auth_pam.h:33: DISALLOW_COPY_AND_ASSIGN(UserAuthPam); > nit: Empty line before this one > > http://codereview.chromium.org/6484002/ >
On Thu, Feb 10, 2011 at 1:55 PM, Lambros Lambrou <lambroslambrou@google.com>wrote: > Hi, > > Thanks for the feedback. Sorry about any spam! :-) What should I trim the > "To:" list down to? > > Just one thing I don't quite understand: > I'm not sure what you mean by "not add real auth in HostStubReal" and > "[not] add pam auth in HostStubFake". I don't remember seeing any class > called HostStubReal anywhere. > That was a typo. You should read it HostStubFake. Sorry. > > Do you think HostStubFake should stay as it was (not doing any auth), and > then have HostStubWin, HostStubLinux, HostStubMac? > Yes > Or would you prefer a single HostStub (or HostStubReal) class, that > implements different schemes using #if (OS_LINUX) .. #elif (OS_WIN) ... > > I think I'd prefer to handle the renaming/refactoring in a separate CL > anyway. Ok > > Cheers, > Lambros > > > > On Thu, Feb 10, 2011 at 9:14 PM, <sergeyu@chromium.org> wrote: > >> Looks very good. Most of my comments are just nits. The only real issue, >> is that >> we should not add real auth in HostStubReal. You can add >> host_stub_linux.cc, and >> use it on linux - this can be done in a separate CL. >> >> BTW, you don't need to manually send mail for code reviews. Upload your >> CL, then >> click "Publish+Mail Comments" on the left, enter reviewers, and click >> "Publish >> All My Drafts" - this will send mail to all reviewers. >> >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc >> File remoting/host/host_stub_fake.cc (right): >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... >> remoting/host/host_stub_fake.cc:30: protocol::LocalLoginStatus* status = >> new protocol::LocalLoginStatus(); >> We shouldn't add pam auth in HostStubFake. It is fake, and shouldn't do >> any real auth. I think we need host_stub_<platform>.cc which will be >> platform specific. This can be done in a separate CL - up to you. >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/host_stub_fake.cc... >> remoting/host/host_stub_fake.cc:51: // ??? >> I think it should be enough to log the failure here: >> LOG(ERROR) << "Login failed for user " << username; >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc >> File remoting/host/user_auth_pam.cc (right): >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:24: static int convFn(int num_msg, const >> struct pam_message** msg, >> ConvFunction? >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:43: const std::string& password) { >> nit: wrapped arguments should be aligned with the arguments on the >> previous line. >> int fun(int arg1, int arg2, >> int arg3, int arg4) >> >> Alternatively you can wrap first argument and indent it with 4 spaces: >> int fun( >> int arg1, int arg2, >> int arg3, int arg4) >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:49: // TODO(lambroslambrou): allow PAM >> service name to be configurable >> nit: comments should be full sentences: start with capital letter and >> end with period >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:55: // TODO(lambroslambrou): move to >> separate thread >> nit: same here >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:63: struct pam_response** resp, void* >> appdata_ptr) { >> nit: indent properly >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:75: int count; >> This can be inside of for() >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.cc#... >> remoting/host/user_auth_pam.cc:77: // Alias for readability >> nit: period at the end of the sentence. >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h >> File remoting/host/user_auth_pam.h (right): >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... >> remoting/host/user_auth_pam.h:30: private: >> nit: empty line before private: >> >> >> http://codereview.chromium.org/6484002/diff/1/remoting/host/user_auth_pam.h#n... >> remoting/host/user_auth_pam.h:33: DISALLOW_COPY_AND_ASSIGN(UserAuthPam); >> nit: Empty line before this one >> >> http://codereview.chromium.org/6484002/ >> > >
I too think we should have separate implementation files for difference platforms. Before that I'd prefer we define a DesktopEnvironment class that contains capturer, event executor and this authenticator. Reason being that there's strong connection between these two components. Also you should consider doing the authentication on the main thread rather than on the network thread.
Lambros, thanks for addressing my nits. I have couple more comments. Also, please remove changes in host_stub_fake.cc from this CL. You can add DesktopEnvironment as Alpha suggested in a separate CL. http://codereview.chromium.org/6484002/diff/1006/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/1006/remoting/host/user_auth_pam.... remoting/host/user_auth_pam.cc:24: static int convFunction(int num_msg, Capital C. Function names and class names must always be CamelCase except accessors_and_mutators: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names http://codereview.chromium.org/6484002/diff/1006/remoting/host/user_auth_pam.h File remoting/host/user_auth_pam.h (right): http://codereview.chromium.org/6484002/diff/1006/remoting/host/user_auth_pam.... remoting/host/user_auth_pam.h:24: class UserAuthPam { I think we should have generic interface called UserAuthenticator, or UserAuth (I would prefer UserAuthenticator, but feel free to disagree). And this class should be an implementation of that interface.
FWIW this is similar to the licensed code layout (SDesktop interface is implemented per-platform and provides platform-specific PixelBuffer and InputHandler implementations), although we took the view that authentication was orthogonal to actual desktop integration, which may not be relevant for Chromoting. On 11 February 2011 19:51, <hclam@chromium.org> wrote: > I too think we should have separate implementation files for difference > platforms. > > Before that I'd prefer we define a DesktopEnvironment class that contains > capturer, event executor and this authenticator. Reason being that there's > strong connection between these two components. Also you should consider > doing > the authentication on the main thread rather than on the network thread. > > > > http://codereview.chromium.org/6484002/ >
Couple more nits. I've started try jobs for this CL. http://codereview.chromium.org/6484002/diff/10/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/10/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:82: for (count = 0; count < num_msg; count++) { nit: "for (int count = 0; count < num_msg; count++)" and remove previous line http://codereview.chromium.org/6484002/diff/10/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:125: : pimpl_(new UserAuthPamPimpl(this)) { nit: This can fit on the previous line.
http://codereview.chromium.org/6484002/diff/10/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/10/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:82: for (count = 0; count < num_msg; count++) { On 2011/02/14 18:58:25, sergeyu wrote: > nit: "for (int count = 0; count < num_msg; count++)" and remove previous line The "count" is used outside the loop (on line 111), in case the loop exited early, so it only cleans as far as the loop got. I could have used "calloc" and made an assumption that all data fields would become NULL or 0. That way, the cleanup loop can always run all the way to num_msgs. Not sure that's a valid assumption :-)
Hey Lambros, Looks pretty solid. I have some nitpicks about style and structure. http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:65: const struct pam_message** msg, Since this is c++, we don't need to explicitly tag types with "struct". Remove here and below? http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:74: struct pam_response* resp_tmp = reinterpret_cast<struct pam_response*>( Use a scoped_ptr_malloc<> to hold the result? http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:84: struct pam_response& r = resp_tmp[count]; Please avoid single-letter variable names. Also, people generally avoid non-const references in Chromium code. http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:99: // No response needed, continue with next prompt. Explain why no response is needed... http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.h File remoting/host/user_auth_pam.h (right): http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.h#... remoting/host/user_auth_pam.h:25: class UserAuthPam : public UserAuthenticator { In one class, you use the "Auth" shorting, and in the other, you use "Authenticator." Make it consistent. Call UserAuthenticatorPam? http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.h#... remoting/host/user_auth_pam.h:33: friend class UserAuthPamPimpl; Since this class is only for Pam, and we already have a hard interface in UserAuthenticator, I don't think we need to additionally use a Pimpl pattern here. Merge UserAuthPamPimpl with UserAuthPam?
http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc File remoting/host/user_auth_pam.cc (right): http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:65: const struct pam_message** msg, On 2011/02/15 01:05:28, awong wrote: > Since this is c++, we don't need to explicitly tag types with "struct". Remove > here and below? Done. http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:74: struct pam_response* resp_tmp = reinterpret_cast<struct pam_response*>( On 2011/02/15 01:05:28, awong wrote: > Use a scoped_ptr_malloc<> to hold the result? I had a go at this, but I found it more trouble than it's worth. The main problem is that the smart ptr class can only do half the cleanup job - it cannot clean up the strdup()ed strings, so we still need some dirty cleanup code anyway. Then it makes sense to have the cleanup in one place, rather than hiding bits of it away inside a C++ destructor. Although the smart ptr saves you from forgetting a "free()" in the cleanup code, it introduces another thing you have to remember: calling release() before handing it back to PAM. Also, it doesn't really work as an array with [] syntax. So I don't think it fits this situation. http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:84: struct pam_response& r = resp_tmp[count]; On 2011/02/15 01:05:28, awong wrote: > Please avoid single-letter variable names. > > Also, people generally avoid non-const references in Chromium code. Done. http://codereview.chromium.org/6484002/diff/11/remoting/host/user_auth_pam.cc... remoting/host/user_auth_pam.cc:99: // No response needed, continue with next prompt. On 2011/02/15 01:05:28, awong wrote: > Explain why no response is needed... Done. I've added a little bit more to the comment.
> > > Although the smart ptr saves you from forgetting a "free()" in the > cleanup code, it introduces another thing you have to remember: calling > release() before handing it back to PAM. Also, it doesn't really work > as an array with [] syntax. So I don't think it fits this situation. Having reverted my scoped_ptr_malloc attempt, I've gone and forgotten to put the "free()" back in! D'oh!!! :-) Lambros http://codereview.chromium.org/6484002/ >
New patch ready for review.
Thanks for merging the classes and renaming! I've added a more thorough scrub of the style. The main comment is that the style guide discourages abbreviations if the longer version doesn't make the name unwieldy. There's a bit of interpretation in this, so push back if you disagree with the suggested names. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... File remoting/host/user_authenticator_pam.cc (right): http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:8: #include <stdlib.h> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... I think this should be #include <stdlib.h> #include <string> #include <security/pam_appl.h> http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:25: conversation.appdata_ptr = static_cast<void*>(this); Do you need a static_cast in order to upcast to void*? http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:27: pam_handle_t* pam_h; pam_h -> pam_handle. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:28: if (pam_start("chromoting", username_.c_str(), Pull this out into a file constant. kPamServiceName or something. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:40: int UserAuthenticatorPam::ConvFunction(int num_msg, num_msg -> num_messages http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:41: const pam_message** msg, msg -> message http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:42: pam_response** resp, resp -> response http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:43: void* appdata_ptr) { Usually we don't type-tag things with _ptr. Just call appdata? http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:53: return PAM_CONV_ERR; Add a newline here? This block of code is a bit too dense, and I think there's a natural break between setting up the outer "resp" object and its fields. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:55: // On exit from the loop, 'count' will hold the number of initialised items initialised -> initialized. (kidding...ignore this comment ;)) http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:90: free(resp_tmp[n].resp); Let's NULL out the pointer as well just to be OCD about this.
http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... File remoting/host/user_authenticator_pam.cc (right): http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:8: #include <stdlib.h> On 2011/02/15 16:36:06, awong wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > I think this should be > > #include <stdlib.h> > > #include <string> > > #include <security/pam_appl.h> Done. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:25: conversation.appdata_ptr = static_cast<void*>(this); On 2011/02/15 16:36:06, awong wrote: > Do you need a static_cast in order to upcast to void*? You don't need it. But since void* is a "strange" type, I thought it best to make the cast explicit here. I''ll remove it if that's what people prefer. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:27: pam_handle_t* pam_h; On 2011/02/15 16:36:06, awong wrote: > pam_h -> pam_handle. Strange! That's the name I originally picked, but I got compilation errors coming from PAM. I thought it was because PAM defines a type also called pam_handle. But I've made this change now and everything seems OK. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:28: if (pam_start("chromoting", username_.c_str(), On 2011/02/15 16:36:06, awong wrote: > Pull this out into a file constant. kPamServiceName or something. Done. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:40: int UserAuthenticatorPam::ConvFunction(int num_msg, On 2011/02/15 16:36:06, awong wrote: > num_msg -> num_messages I didn't pick those names. They come from a PAM callback interface documented in "man pam_conv". I don't know if I should be changing the names to follow our own conventions, or sticking with the PAM conventions here? Is it OK to fiddle with parameter names in third-party interfaces? If it is, I will happily change these! :-) http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:43: void* appdata_ptr) { On 2011/02/15 16:36:06, awong wrote: > Usually we don't type-tag things with _ptr. Just call appdata? Specifically, "appdata_ptr" is also a field-name of "struct pam_conv", which is self-documenting where the parameter originates. So I'm reluctant to change this particular name. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:53: return PAM_CONV_ERR; On 2011/02/15 16:36:06, awong wrote: > Add a newline here? This block of code is a bit too dense, and I think there's > a natural break between setting up the outer "resp" object and its fields. Done. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:55: // On exit from the loop, 'count' will hold the number of initialised items On 2011/02/15 16:36:06, awong wrote: > initialised -> initialized. > > (kidding...ignore this comment ;)) Hehe, never noticed that one! I'm just giving the code some international flavour (sorry, "flavor")! http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:90: free(resp_tmp[n].resp); On 2011/02/15 16:36:06, awong wrote: > Let's NULL out the pointer as well just to be OCD about this. Is there any point, when we free() the whole resp_tmp block immediately afterwards?
LGTM if the last comment is fixed. Albert, if it looks good to you, I'll submit it. http://codereview.chromium.org/6484002/diff/19008/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/6484002/diff/19008/build/install-build-deps.sh... build/install-build-deps.sh:121: libpng12-0 Can you please reformat this string, so that there is more than one packet on this line.
http://codereview.chromium.org/6484002/diff/19008/build/install-build-deps.sh File build/install-build-deps.sh (right): http://codereview.chromium.org/6484002/diff/19008/build/install-build-deps.sh... build/install-build-deps.sh:121: libpng12-0 On 2011/02/15 18:27:21, sergeyu wrote: > Can you please reformat this string, so that there is more than one packet on > this line. Done.
LGTM http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... File remoting/host/user_authenticator_pam.cc (right): http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:40: int UserAuthenticatorPam::ConvFunction(int num_msg, On 2011/02/15 18:15:51, Lambros wrote: > On 2011/02/15 16:36:06, awong wrote: > > num_msg -> num_messages > I didn't pick those names. They come from a PAM callback interface documented > in "man pam_conv". I don't know if I should be changing the names to follow our > own conventions, or sticking with the PAM conventions here? Is it OK to fiddle > with parameter names in third-party interfaces? If it is, I will happily change > these! :-) Ah, I see. Yeah, in these APIs that interface with external code, the conventions are a bit mixed. Let's stick with what you have. http://codereview.chromium.org/6484002/diff/20004/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:90: free(resp_tmp[n].resp); On 2011/02/15 18:15:51, Lambros wrote: > On 2011/02/15 16:36:06, awong wrote: > > Let's NULL out the pointer as well just to be OCD about this. > Is there any point, when we free() the whole resp_tmp block immediately > afterwards? No, not really. You're right. http://codereview.chromium.org/6484002/diff/23002/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:16: UserAuthenticatorPam::~UserAuthenticatorPam() { Personally, for these, I prefere static const char [], and not an anonymous namespace. Up to you though. http://codereview.chromium.org/6484002/diff/23002/remoting/host/user_authenti... remoting/host/user_authenticator_pam.cc:17: } It's a bit degenerate with a 1-line namespace, but you're supposed to end your name spaces with a comment like this: } // namespace [namespace name] For an anonymous namespace, it's just: } // namespace
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
