[SSSD] [PATCH] Make the handling of fd events opaque

Martin Nagy mnagy at redhat.com
Fri Apr 23 12:37:01 UTC 2010


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




More information about the sssd-devel mailing list