[SSSD] [PATCH] Four small patches related to GC lookups

Jakub Hrozek jhrozek at redhat.com
Tue May 28 13:13:27 UTC 2013


On Tue, May 28, 2013 at 11:14:33AM +0200, Sumit Bose wrote:
> On Tue, May 28, 2013 at 10:56:06AM +0200, Jakub Hrozek wrote:
> > On Tue, May 28, 2013 at 10:26:54AM +0200, Sumit Bose wrote:
> > > > @@ -1371,14 +1382,11 @@ fo_set_port_status(struct fo_server *server, enum port_status status)
> > > >       * into fo_server structures. Find the duplicates and set the same
> > > >       * status */
> > > >      DLIST_FOR_EACH(siter, server->service->server_list) {
> > > > -        if (siter == server) continue;
> > > > -        if (!siter->common || !siter->common->name) continue;
> > > > -
> > > > -        if (siter->port == server->port &&
> > > > -            (strcasecmp(siter->common->name, server->common->name) == 0)) {
> > > > -            DEBUG(7, ("Marking port %d of duplicate server '%s' as '%s'\n",
> > > > -                      siter->port, SERVER_NAME(siter),
> > > > -                      str_port_status(status)));
> > > > +        if (fo_server_cmp(siter, server)) {
> > > 
> > > You removed the NULL check on siter->common->name but siter is used in
> > > fo_server_cmp() as first argument to fo_server_match() where it is not
> > > properly checked if siter->common->name is not NULL.
> > > 
> > > I guess it's good to remove the check here and add it to
> > > fo_server_match().
> > > 
> > 
> > Sorry, I'm not sure I completely understood. I moved the
> > server->common->name check to fo_server_match(), is that what you meant?
> 
> yes
> 
> > 
> > > > From fae5ac11bc7c48684625b5b43d9c5856b11fa9c4 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek <jhrozek at redhat.com>
> > > > Date: Fri, 17 May 2013 14:33:26 +0200
> > > > Subject: [PATCH 4/4] AD dyndns: extract the host name from URI
> > > > 
> > > > ---
> > > 
> ...
> 
> > > maybe you can check if strcasecmp( lud->lud_scheme, "ldapi" ) == 0
> > > because in this case lud->lud_host contains a local path and not a host
> > > name. Please note that lud->lud_scheme might be NULL .
> > 
> > You're right, I was thinking about the schemes, too, but wasn't sure if
> > there is a way to actually have an AD server with ldapi ? But it's
> > better to be safe.
> 
> I was more thinking in terms of code reuse, since this part of the code
> is pretty generic chances are that it get extracted and reused in other
> places as well.
> 
> All my concerns are addressed, ACK.
> 
> bye,
> Sumit

Thank you for the review.

Pushed to master.



More information about the sssd-devel mailing list