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.