On 03/09/2016 05:49 PM, Petr Cech wrote:
On 03/08/2016 05:57 PM, Petr Cech wrote:
> On 03/08/2016 05:09 PM, Pavel Reichl wrote:
>> Hello Petr,
>>
>> I just run through the code and I have some code style suggestions, feel
>> free to disagree :-).
>
> I will address some suggestions tomorrow. I am doing tests for
> sysdb_sudo_rules now. Thanks for comments.
>
> Petr
>
Hello,
all comments addressed. New patches attached. I am still working on
tests, so I will read all the patches again.
Hi, just a quick review.
Patch 1:
errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *sub_filter,
const char **attrs,
size_t *_msgs_count,
struct ldb_message ***_msgs)
{
...
if (sub_filter == NULL) {
filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
Remove ampersand please, it is not necessary. (objectClass=sudoRule) is
sufficient.
} else {
filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
SUDO_ALL_FILTER, sub_filter);
}
if (filter == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
ret = ENOMEM;
goto done;
}
DEBUG(SSSDBG_TRACE_INTERNAL,
"Search sudo rules with filter: %s\n", filter);
ret = sysdb_search_entry(tmp_ctx, domain->sysdb, dn,
LDB_SCOPE_SUBTREE, filter, attrs,
&msgs_count, &msgs);
if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
The code says the ENOENT may be returned but msgs_count and msgs remain
not set. You need to set it here explicitly and goto done.
} else if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
sss_strerror(ret));
You need to goto done here.
}
*_msgs_count = msgs_count;
*_msgs = talloc_steal(mem_ctx, msgs);
ret = EOK;
done:
talloc_zfree(tmp_ctx);
return ret;
}
static struct ldb_dn *
sysdb_sudo_rule_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *name)
{
return sysdb_custom_dn(mem_ctx, domain, name, SUDORULE_SUBDIR);
}
errno_t
sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op)
{
errno_t ret;
struct ldb_dn *dn;
TALLOC_CTX *tmp_ctx;
tmp_ctx = talloc_new(NULL);
NULL_CHECK(tmp_ctx, ret, done);
dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
Temporary context is not needed here, you can use NULL instead and
talloc_free(dn) in the end.
NULL_CHECK(dn, ret, done);
ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
done:
talloc_free(tmp_ctx);
return ret;
}
Patch 3:
case 'e':
idb = INVALIDATE_EVERYTHING;
+ idb |= INVALIDATE_SUDO_RULES;
break;
#ifdef BUILD_SUDO should be here... it is not necessary but we should
remain clear.