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.
Ack.