URL:
https://github.com/SSSD/sssd/pull/492
Title: #492: SYSDB: Use sysdb_domain_dn instead of raw ldb_dn_new_fmt
jhrozek commented:
"""
Thank you for the review, please see comments inline.
On Thu, Jan 18, 2018 at 10:45:24AM +0000, lslebodn wrote:
I think I found few more. IMHO tests are not important (nice to have)
but we should fix it at least in `src/db/sysdb_subdomains.c`
```
src/db/sysdb.c:265: return ldb_dn_new_fmt(mem_ctx, dom->sysdb->ldb,
SYSDB_DOM_BASE, dom->name);
src/db/sysdb.c-266-}
src/db/sysdb.c-267-
This is the actual implementation.
src/db/sysdb.c-268-struct ldb_dn *sysdb_base_dn(struct sysdb_ctx
*sysdb, TALLOC_CTX *mem_ctx)
--
src/db/sysdb_init.c:185: msg->dn = ldb_dn_new_fmt(msg, ldb, SYSDB_DOM_BASE,
domain_name);
src/db/sysdb_init.c-186- if (!msg->dn) {
src/db/sysdb_init.c-187- ret = ENOMEM;
src/db/sysdb_init.c-188- goto done;
This is internal sysdb operation where I think the raw call makes sense
because the domain doesn't exist yet, so there is no sss_domain_info
structure. We could create a dummy, but I think it would decrease the
readability.
--
src/db/sysdb_services.c-805- goto fail;
--
src/db/sysdb_subdomains.c:760: msg->dn = ldb_dn_new_fmt(tmp_ctx,
domain->sysdb->ldb,
src/db/sysdb_subdomains.c-761- SYSDB_DOM_BASE,
domain->name);
src/db/sysdb_subdomains.c-762- if (msg->dn == NULL) {
src/db/sysdb_subdomains.c-763- ret = EIO;
src/db/sysdb_subdomains.c-764- goto done;
src/db/sysdb_subdomains.c-765- }
I fixed this one.
--
src/db/sysdb_subdomains.c:936: dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
SYSDB_DOM_BASE, name);
src/db/sysdb_subdomains.c-937- if (dn == NULL) {
src/db/sysdb_subdomains.c-938- ret = EIO;
src/db/sysdb_subdomains.c-939- goto done;
src/db/sysdb_subdomains.c-940- }
src/db/sysdb_subdomains.c-941- ret = ldb_search(sysdb->ldb, tmp_ctx, &res,
--
src/db/sysdb_subdomains.c:1204: dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
SYSDB_DOM_BASE, name);
src/db/sysdb_subdomains.c-1205- if (dn == NULL) {
src/db/sysdb_subdomains.c-1206- ret = ENOMEM;
src/db/sysdb_subdomains.c-1207- goto done;
src/db/sysdb_subdomains.c-1208- }
src/db/sysdb_subdomains.c-1209-
--
src/db/sysdb_subdomains.c:1236: dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
SYSDB_DOM_BASE, domain_name);
src/db/sysdb_subdomains.c-1237- if (dn == NULL) {
src/db/sysdb_subdomains.c-1238- ret = ENOMEM;
src/db/sysdb_subdomains.c-1239- goto done;
src/db/sysdb_subdomains.c-1240- }
src/db/sysdb_subdomains.c-1241-
--
src/db/sysdb_subdomains.c:1265: dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
SYSDB_DOM_BASE, domain_name);
src/db/sysdb_subdomains.c-1266- if (dn == NULL) {
src/db/sysdb_subdomains.c-1267- ret = ENOMEM;
src/db/sysdb_subdomains.c-1268- goto done;
In all the above, a name is used. In the functions, we could use a
sss_domain_info pointer instead and I think it would be even nicer, but
the only problem is tests - there are tests that use a hardcoded
(sometimes even non-existent) domain name.
We could work around it by using a dummy and I think here it would be
OK, because a slight inconvenience in the tests wuld make nicer deamon
code.
Another option might be to add another private call like
sysdb_domain_dn_from_name() and then we could use it also in the upgrade
codepaths..what do you think?
--
src/db/sysdb_upgrade.c-229- }
--
src/db/sysdb_upgrade.c:432: domain_dn = ldb_dn_new_fmt(tmp_ctx, sysdb->ldb,
src/db/sysdb_upgrade.c-433- SYSDB_DOM_BASE,
dom->name);
src/db/sysdb_upgrade.c-434- if (!domain_dn) {
src/db/sysdb_upgrade.c-435- ret = ENOMEM;
src/db/sysdb_upgrade.c-436- goto done;
I think I only didn't fix this one because the sysdb upgrade code uses
raw ldb calls anyway all over the place..
--
src/db/sysdb_upgrade.c-1221- goto done;
src/db/sysdb_upgrade.c-1222- }
--
src/db/sysdb_upgrade.c:1389: basedn = ldb_dn_new_fmt(ctx, sysdb->ldb,
SYSDB_DOM_BASE, tmp_str);
src/db/sysdb_upgrade.c-1390- if (!basedn) {
src/db/sysdb_upgrade.c-1391- DEBUG(SSSDBG_OP_FAILURE,
src/db/sysdb_upgrade.c-1392- "Failed to build base dn for subdomain
%s\n", tmp_str);
--
src/providers/ipa/ipa_hbac_common.c-414- }
--
src/tests/cmocka/test_sysdb_domain_resolution_order.c:88: dn =
ldb_dn_new_fmt(test_ctx, test_ctx->tctx->dom->sysdb->ldb,
src/tests/cmocka/test_sysdb_domain_resolution_order.c-89-
SYSDB_DOM_BASE, test_ctx->tctx->dom->name);
src/tests/cmocka/test_sysdb_domain_resolution_order.c-90-
src/tests/cmocka/test_sysdb_domain_resolution_order.c-91- /* Adding
domainResolutionOrder for the first time */
src/tests/cmocka/test_sysdb_domain_resolution_order.c-92- domains_in =
"foo:bar:foobar";
--
src/tests/sysdb-tests.c-503- }
src/tests/sysdb-tests.c-504-
--
src/tests/sysdb-tests.c:516: dn = ldb_dn_new_fmt(data, data->ctx->sysdb->ldb,
SYSDB_DOM_BASE,
src/tests/sysdb-tests.c-517- "LOCAL");
Here is an example of the hardcoded name in tests.
src/tests/sysdb-tests.c-518- if (!dn) {
src/tests/sysdb-tests.c-519- return ENOMEM;
src/tests/sysdb-tests.c-520- }
src/tests/sysdb-tests.c-521-
--
src/tests/sysdb-tests.c:3609: ne_dn = ldb_dn_new_fmt(test_ctx,
test_ctx->sysdb->ldb, SYSDB_DOM_BASE,
src/tests/sysdb-tests.c-3610- "non-existing domain");
And here is another.
src/tests/sysdb-tests.c-3611- fail_unless(ne_dn != NULL);
src/tests/sysdb-tests.c-3612- ret = sysdb_get_bool(test_ctx->sysdb, ne_dn,
attr_val,
```
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/SSSD/sssd/pull/492#issuecomment-358608984
"""
See the full comment at
https://github.com/SSSD/sssd/pull/492#issuecomment-358619637