On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
> On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
>> On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
>>> On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
>>>>
https://fedorahosted.org/sssd/ticket/2584
>>> btw (unrelated to these patches) it seems strange to require the
>>> user of
>>> the sysdb database to connect to the db and then separately initialize
>>> subdomains...I would vote for merging that code..
>>
>> +1
>
> I'll file a ticket for michal.
>
>>
>>>> +
>>>> +#include <talloc.h>
>>>> +#include <popt.h>
>>>> +
>>>> +#include "confdb/confdb.h"
>>>> +
>>>> +struct sss_tool_ctx {
>>>> + struct confdb_ctx *confdb;
>>>> + char *confdb_path;
>>>> +
>>>> + char *default_domain;
>>>> + struct sss_domain_info *domains;
>>>
>>> I wonder if the tool_ctx must be open for consumers? Currently only
>>> domains is used, maybe we could get away with a getter and an opaque
>>> structure? We can always make the structure public in future, but if
>>> it's public from the start, then it's easier to add cruft..
>>
>> I think it is not worth the effort with such simple structure. I am
>> all for opaque structures but here is just no gain in code safety. It
>> would
>> only make code lines bigger.
>
> Fine.
>
>>
>>>> +enum sss_tool_domain {
>>>> + SSS_TOOL_DOMAIN_REQUIRED,
>>>> + SSS_TOOL_DOMAIN_OPTIONAL
>>>> +};
>>>> +
>>>> +int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
>>>> + struct sss_tool_ctx *tool_ctx,
>>>> + const char *input,
>>>> + enum sss_tool_domain require_domain,
>>>
>>> Do we need require_domain now that you added the getpwnam?
>>
>> No, removed.
>
> Thanks
>
> [...]
>
>>>
>>> Hmm looks like you should have a check for res validity here, for cases
>>> no domain match..
>>
>> Added a dom validity.
>
> Thanks.
>
>>
>>> btw it might be nice to split this large function into smaller ones.
>>> Maybe the do-while check could be a separate function returning res.
>>
>> There is no way to split it that would be actually beneficial. There are
>> three blocks:
>> a) beginning - getpwnam
>> b) middle - iteration
>> c) end - linearized dn
>>
>> If you take away the iteration you can directly return dn from the new
>> function since you don't need res at all so that would also took away
>> the
>> end. And the beginning belongs to the function were iteration is IMHO.
>
> OK
>
>
>> From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>> Date: Fri, 24 Jul 2015 09:55:28 +0200
>> Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
>>
>> Objects doesn't have to have overrideDN specified when using LOCAL view.
>> Since the view is not stored on the server we do not want to contact
>> LDAP therefore we special case LOCAL view saying that it is OK that
>> this attribute is missing.
>>
>> Preparation for:
>>
https://fedorahosted.org/sssd/ticket/2584
>> ---
>> src/db/sysdb.h | 3 +-
>> src/db/sysdb_views.c | 7 +++++
>> src/tests/cmocka/test_sysdb_views.c | 62
>> +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>> index
>>
0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22
>> 100644
>> --- a/src/db/sysdb.h
>> +++ b/src/db/sysdb.h
>> @@ -157,9 +157,10 @@
>> #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires"
>> #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
>>
>> +#define SYSDB_DEFAULT_VIEW_NAME "default"
>> +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side
>> overrides */
>> #define SYSDB_VIEW_CLASS "view"
>> #define SYSDB_VIEW_NAME "viewName"
>> -#define SYSDB_DEFAULT_VIEW_NAME "default"
>> #define SYSDB_OVERRIDE_CLASS "overrride"
>> #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID"
>> #define SYSDB_OVERRIDE_USER_CLASS "userOverride"
>> diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
>> index
>>
aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec
>> 100644
>> --- a/src/db/sysdb_views.c
>> +++ b/src/db/sysdb_views.c
>> @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct
>> sss_domain_info *domain,
>> override_dn_str = ldb_msg_find_attr_as_string(obj,
>>
>> SYSDB_OVERRIDE_DN, NULL);
>> if (override_dn_str == NULL) {
>> + if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) ==
>> 0) {
>> + /* LOCAL view doesn't have to have overrideDN
>> specified. */
>> + ret = EOK;
>> + goto done;
>> + }
>> +
>> DEBUG(SSSDBG_CRIT_FAILURE,
>> "Missing override DN for objext [%s].\n",
>> ldb_dn_get_linearized(obj->dn));
>> +
>> ret = ENOENT;
>> goto done;
>> }
>> diff --git a/src/tests/cmocka/test_sysdb_views.c
>> b/src/tests/cmocka/test_sysdb_views.c
>> index
>>
1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871
>> 100644
>> --- a/src/tests/cmocka/test_sysdb_views.c
>> +++ b/src/tests/cmocka/test_sysdb_views.c
>> @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void
>> **state)
>> assert_int_equal(ldb_val_string_cmp(&el->values[1],
>> "OVERRIDEKEY2"), 0);
>> }
>>
>> +void test_sysdb_add_overrides_to_object_local(void **state)
>> +{
>> + int ret;
>> + struct ldb_message *orig;
>> + struct ldb_message_element *el;
>> + char *tmp_str;
>> + struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
>> + struct
>> sysdb_test_ctx);
>> +
>> + orig = ldb_msg_new(test_ctx);
>> + assert_non_null(orig);
>> +
>> + tmp_str = talloc_strdup(orig, "ORIGNAME");
>
> Please put assert_non_null here and to other assignments to tmp_str in
> these two tests, otherwise ACK
>
>> + ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
>> + assert_int_equal(ret, EOK);
>> +
>> + tmp_str = talloc_strdup(orig, "ORIGGECOS");
>> + ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
>> + assert_int_equal(ret, EOK);
>> +
>> + test_ctx->domain->has_views = true;
>> + test_ctx->domain->view_name = "LOCAL";
>> +
>> + ret = sysdb_add_overrides_to_object(test_ctx->domain, orig,
>> NULL, NULL);
>> + assert_int_equal(ret, EOK);
>> +}
>
>> From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>> Date: Wed, 22 Jul 2015 10:02:02 +0200
>> Subject: [PATCH 2/3] TOOLS: add common command framework
>
> ACK
>
>
>> From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>> Date: Fri, 24 Jul 2015 09:58:11 +0200
>> Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
>>
>> Resolves:
>>
https://fedorahosted.org/sssd/ticket/2584
>
> There was one more Coverity warning:
> Error: UNINIT (CWE-457): [#def1]
> sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable
> "ret" without initializer.
> sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using
> uninitialized value "ret".
> # 250|
> # 251| done:
> # 252|-> if (ret != EOK) {
> # 253| talloc_free(attrs);
> # 254| return NULL;
>
> Otherwise looks good.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Thanks, new patches are attached. In addition to your comments:
- one new patch that adds null check in original unit test
- man page and tool change that says that zero ids are not supported
Few more bug fixed during irc review. Thanks Jakub!