[SSSD] Support for subdomains - stage 1

Jakub Hrozek jhrozek at redhat.com
Mon Apr 2 18:54:08 UTC 2012


On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
> Once more, this time rebased on top of current master.
> 
> Thanks
> Jan

Patch 0001: Sysdb routines for subdomains
Ack, can you just add a DEBUG message in case sysdb_transaction_commit()
fails?

Patch 0002: Add some utility functions for subdomains
Ack

Patch 0003: Add conn_name to allow different names for domains and
connections
Ack

Patch 0004: Responder part of the subdomain retrieval work
"get_domains_timeout" is not mentioned in neither configAPI nor manpages

+        if (ret == EOK) {
+            DEBUG(SSSDBG_TRACE_FUNC,
+                  ("Last call was too recent, nothing to do!\n"));
+            ret = EOK;
        ^^ this is redundant
+            goto done;
+        } else if (ret != EAGAIN) {

Please stick to either goto or return in get_domains_next()

Two review items from the original review are not addressed yet:
There is no explanation of the "hint" parameter (and without one, it's
hard to say what to do with the FIXME) and there is no
sss_dp_get_domains_recv()

Patch 0005: Modified responder_get_domain()
It is a convention to put a TALLOC_CTX we allocate on as the first
parameter (responder_get_domain())

I think that it might be useful to store
CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from
confdb on every request. The case where confdb_get_int either errors out
or the timeout is negative should at least trigger a DEBUG message.

As a general question, why are the domain names case sensitive but
subdomain names seem to be case insensitive? For example check_last_request()
is doing a case-sensitive strcmp but responder_get_domain is doing a
strcasecmp.

Also please split the long DEBUG() statement in responder_get_domain()
into two lines, it doesn't fit on my screen :-)

Patch 0006: Retrieve subdomains if there is a request for fully
qualified user
Looks OK, will just need amendment when sss_dp_get_domains_recv() is
implemented. Currently the patch supports users and netgroups, are there
plans to also support services and the SSH responder? I think we can
leave out the sudo responder for now, sudo doesn't support domains
(yet?). Autofs in theory can send a request for a fully-qualified map,
but it doesn't so the work can be done later, although I would prefer a
ticket to show that we are aware of the issue.

Patch 0007: Ask for subdomains in responder in the first request after
startup
Ack, with the same comment about services and SSH as with patch #6.

Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
Ack

Patch 0009: data provider: added subdomains
If the bandaid patch I posted on the list the other day is accepted,
there should be a check for the result of the sbus_get_connection() call.

The comment about fast reply in be_get_subdomains() seems misleading to
me, there is no check for BE_REQ_FAST and the whole code seems like it
can handle online requests only.

As stated in the original review, there needs to be a check for
"!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before
calling be_file_request, otherwise you'll get crashes on domains with no
subdomain support.

That's about halfway into the patchset. I'll continue the review later.



More information about the sssd-devel mailing list