Re: [PATCH 2/4] cifs: Split ntlm and ntlmv2 authentication methods off CIFS_SessSetup()

[Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, 2014-05-01 at 15:01 -0500, Shirish Pargaonkar wrote:
> On Thu, May 1, 2014 at 11:26 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Thu, 2014-05-01 at 10:31 -0500, Shirish Pargaonkar wrote:
> >
> >> > +
> >> > +out:
> >> > +       sess_data->result = rc;
> >> > +       sess_data->func = NULL;
> >> > +       sess_free_buffer(sess_data);
> >> > +       sess_establish_session(sess_data);
> >> > +       kfree(ses->auth_key.response);
> >> > +       ses->auth_key.response = NULL;
> >> > +}
> >>
> >> In all three sess_auth_* functions (ntlm, ntlmv2, and lanman (in
> >> previous patch),
> >> we will end up calling sess_establish_session() even if there is an error i.e.
> >> sess_data->result is not 0.
> >
> > We check for any errors within sess_establish_session() before we
> > attempt to establish session.
> >
> > static int
> > sess_establish_session(struct sess_data *sess_data)
> > {
> > ..
> >         if (!sess_data->result) {
> >                 //Establish Session.
> >         }
> > ..
> > }
> >
> > This isn't a technical issue but maybe considered a style issue.
> 
> Sure, would have preferred to call the function before goto lables, but will
> leave at that.
> 

Having given it another thought, I agree with you. I've made the
required modifications and tested the new patches. I'll send them to the
list shortly.

Sachin Prabhu


> >
> >
> >> > +       if (phase == NtLmChallenge) {
> >> > +               rc = decode_ntlmssp_challenge(bcc_ptr, blob_len, ses);
> >> > +               /* now goto beginning for ntlmssp authenticate phase */
> >>
> >> I think this comment should move to after if statement
> >> (it was at wrong place to begin with) i.e. go to authenticate phase
> >> only if rc is NULL.
> >
> > I agree. The comment is actually not required.
> >
> > Sachin Prabhu
> >