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?= <pbrez...@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?= <pbrez...@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?= <pbrez...@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
