On Thu, Mar 18, 2010 at 12:15:39PM -0400, Dmitri Pal wrote: > Ralf Haferkamp wrote: > > Am Donnerstag 18 März 2010 15:25:49 schrieb Dmitri Pal: > > > >> Ralf Haferkamp wrote: > >> > >>> Am Donnerstag 18 März 2010 12:42:23 schrieb Simo Sorce: > >>> > >>>> On Wed, 17 Mar 2010 15:33:38 +0100 > >>>> > >>>> Ralf Haferkamp <[email protected]> wrote: > >>>> > >>>>> Hi, > >>>>> > >>>>> here's another set of enhancements to the LDAP Password Policy > >>>>> support in the PAM module and the LDAP backend. The PAM module now > >>>>> issues warning when either the grace counter or the expire counter > >>>>> of the LDAP Ppolicy Control in > 0. > >>>>> > >>>>> Addtionally I changed the detection for Ppolicy support of the > >>>>> LDAP Server a bit. If the Server returned the Ppolicy Control in > >>>>> the Bind Response ppolicy support is assumed. > >>>>> > >>>>> I left the original check for "pwdAttribute" intact, though I > >>>>> think it doesn't make a lot of sense. The "pwdAttribute" LDAP > >>>>> Attribute is usually not part of the user's entry, it's part of > >>>>> the Entry that contains the Policy. Addtionally it might be > >>>>> protected by ACLs and not be returned for anonymous (without > >>>>> losing any functionality). > >>>>> > >>>> Ralf, > >>>> patch looks mostly good, but there are some heavy coding style > >>>> violations. > >>>> > >>>> if ( condition ){ is not good, it should be: is (condition) { > >>>> > >>>> same for some functions foo( ccc ); is not good, use foo(ccc); > >>>> > >>>> Ie, no space in parenthesis, a space only after keywords, and > >>>> always before the opening { > >>>> > >>>> Don't use ++x, but x++ if possible. > >>>> > >>>> Also there is at least one place where the return of talloc is not > >>>> checked. > >>>> > >>>> Finally please try to keep the 80 columns limit where possible. > >>>> > >>> Updated patch attached. I think I fixed the coding style issues. > >>> Additionally I just noticed that my orginal patch broke password > >>> resets via LDAP password policies. This should be fixed now as > >>> well. > >>> > >>> regards, > >>> > >>> Ralf > >>> > >> + data = talloc_size(pd, 2* sizeof(uint32_t)); > >> + if (*data == NULL) { > >> + DEBUG(1, ("talloc_size failed.\n")); > >> + return ENOMEM; > >> + } > >> > >> > >> The returned value check does not look right. > >> I do not know if there are other places with similar logic. > >> > > There weren't (The one above was embarrasing enough. I must have > > overlooked the compiler warning :| ). Updated patch attached. > > > > Shouldn't it be freed somewhere down within the same function? >
No, this data will be send to a client in a later call and freed by a talloc_free at the end to the client request. bye, Sumit > > > >> My eye just caught it when I was scrolling through the patch... > >> > > > > Ralf > > > > ------------------------------------------------------------------------ > > > > _______________________________________________ > > sssd-devel mailing list > > [email protected] > > https://fedorahosted.org/mailman/listinfo/sssd-devel > > > -- > Thank you, > Dmitri Pal > > Engineering Manager IPA project, > Red Hat Inc. > > > ------------------------------- > Looking to carve out IT costs? > www.redhat.com/carveoutcosts/ > > _______________________________________________ > sssd-devel mailing list > [email protected] > https://fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
