On Mon, Jan 30, 2012 at 09:26:21AM -0500, Stephen Gallagher wrote:
On Mon, 2012-01-30 at 15:04 +0100, Jakub Hrozek wrote:
> On Sat, Jan 28, 2012 at 02:10:19PM -0500, Stephen Gallagher wrote:
> > On Sat, 2012-01-28 at 13:52 -0500, Stephen Gallagher wrote:
> > > On Sat, 2012-01-28 at 12:08 -0500, Stephen Gallagher wrote:
> > > > I'm still working on the enumeration support, but I wanted to get
the
> > > > direct lookup on the list early.
> > > >
> > > > Patch 0001: extend sysdb_store_service() to accept additional
attributes
> > > > This is necessary to be able to store provider-specific data in the
> > > > entry (such as the entryUSN).
>
> Can you move the NULL check of msg->dn outside the switch so that there
> is only one and all cases are checked? Currently the service case does
> not have a NULL check.
>
Fixed. Thanks, I meant to do that in the first place and then... didn't.
> You should also pass primary_name, not name into the call of
> sysdb_remove_attrs() because name points to ldb_message structure that
> was already freed.
>
Good catch, thanks!
> > > >
> > > > Patch 0002: Add a helper function to get an integer value from a
> > > > sysdb_attrs object directly as a uint16_t.
> > > >
>
> Ack
>
> > > > Patch 0003: The big one. This patch adds support for looking up
services
> > > > in LDAP. There shouldn't be any big "gotchas" here.
It's very
> > > > straightforward (if long).
> > > >
>
> Setting errno to 0 before strtouint16() is not needed, strtouint16()
> does it internally.
>
Removed.
> Otherwise Ack.
>
> Patch 0004: LDAP: Add enumeration support for services
> Ack
>
> > > > Patch 0004: Extend this support to the IPA Provider (even though IPA
> > > > itself does not yet support the services map natively).
> > >
>
> (This is actually patch 0005 in the most recent set)
> Ack
>
> > > I over-estimated the additional effort necessary to add the enumeration
> > > support. New patch set includes the enumeration work (before the IPA
> > > provider) as well as fixing a minor typo in patch 0003 (bad copy-paste
> > > in a comment).
> >
> > One more set. I forgot to include updates to the SSSDConfig API and the
> > manpages in the previous set. I added it as a new, sixth patch. This
> > should now be complete.
>
> Patch 0006: I don't understand this line:
> #replaced by ldap_entry_usn# 'ldap_service_entry_usn' : _('Service
entryUSN attribute'),
>
> It looks like SDAP_AT_SERVICE_USN is used in the sdap services code?
If you look at the rest of the SSSDConfig.py list, we make the same
comment elsewhere for users, groups, etc. The reason is that there's
never actually a reason to use the specific ones, because no LDAP server
is going to have different entryUSN attributes for different maps.
Furthermore, we generally prefer that these options are unset, since we
have very good auto-detection for this attribute.
So I'm leaving that as-is.
>
> Also "ldap_service_search_base" is missing from the manual page.
Fixed.
New patches attached.
Ack to all the patches.
I noticed one more bug in the responder - for a service that has both
"tcp" and "udp" values for the "ipServiceProtocol"
attribute, the first
one is always returned unconditionally even if the user specifically
asked for a protocol.
So getservbyname("genericservice", "udp"); would always retun
"tcp" if
it's the first service in the cache even if "udp" exists.
We can obviously fix this post-beta, though.