On Fri, 2010-04-23 at 11:28 +0200, Sumit Bose wrote:
> On Thu, Apr 22, 2010 at 11:58:10PM +0200, Martin Nagy wrote:
> > On 04/16/2010 12:22 PM, Sumit Bose wrote:
> > > Hi,
> > >
> > > to support the current effort to make the LDAP provider more robust this
> > > patch removes all the #ifdef HAVE_LDAP_CONNCB calls from the main code
> > > into a separate file which provides generic helper functions.
> > >
> > > bye,
> > > Sumit
> >
> > Nitpick:
> > -static void sdap_ldap_result(struct tevent_context *ev,
> > +void sdap_ldap_result(struct tevent_context *ev,
> > struct tevent_fd *fde,
> > uint16_t flags, void *pvt)
> > Please indent the last two lines so that "struct" and "uint16_t" begin
> > where the "struct" on the first line begins. This is in sdap_async.c and
> > sdap_async_private.h.
>
> done
>
> >
> > In setup_ldap_connection_callbacks():
> > ret = ldap_set_option(sh->ldap, LDAP_OPT_CONNECT_CB,
> > sh->sdap_fd_events->conncb);
> > if (ret != LDAP_OPT_SUCCESS) {
> > DEBUG(1, ("Failed to set connection callback\n"));
> > goto fail;
> > }
> > Mixing of errno and ldap return values.
>
> done
>
> >
> > sdap_set_connected():
> > I'm not sure if the way you defined the inline function is very the
> > right one [1]. I'd recommend either using "static inline" and define the
> > function in the header file, or simply not make it inline.
>
> I removed the inline.
>
> >
> > Other than this, the patch looks very good.
>
> Thanks for the review, new version attached.
>
> bye,
> Sumit
Ah, sorry, I was too quick. I just noticed that you only use enum
sdap_fd_event_type as a member of struct sdap_fd_events, but you only
assign to the member, you never read it. Can you please remove it?
Martin
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
