On 03/10/2016 02:36 PM, Pavel Březina wrote:
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.
Addressed.
> } 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.
Addressed.
> } else if (ret != EOK) {
> DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
> sss_strerror(ret));
You need to goto done here.
Adrressed.
> }
>
> *_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.
I am not sure about it. I have tried it but it didn't work for me.
> 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.
Addressed.
There is whole patch set attached. I worked on tests (003 patch) last
couple days. I know that there are some test cases missing.
My big challenge is test_filter_rules_by_time(). I recognize that we
don't have function for load sudo rule by name. I mean how to obtain
struct sysdb_attrs *rule, for example by name of rule.
If I understand it rigt, we just prepare filter and we say (in attrs)
which fields of rule we want load. Is it right?
Of course, if you see any missing important test case, please write me :-)
Regards
--
Petr^4 Čech