On Thu, Nov 13, 2014 at 11:58:13AM +0100, Sumit Bose wrote:
Hi,
Coverity run was clean, CI run succeeded and testing against a recent
IPA server went OK. Before I ack, I also want to test against an old
server, but I'm running out of time today.
See some comments inline.
> From 9f807e92b8db786c331842117e1481dbb494a1f7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Thu, 6 Nov 2014 13:13:27 +0100
> Subject: [PATCH 1/7] ipa: add split_ipa_anchor()
ACK, see one remark inline.
> +errno_t split_ipa_anchor(TALLOC_CTX *mem_ctx, const char *anchor,
> + char **_anchor_domain, char **_ipa_uuid)
> +{
> + const char *sep;
> +
> + if (anchor == NULL) {
> + return EINVAL;
> + }
> + if (strncmp(OVERRIDE_ANCHOR_IPA_PREFIX, anchor,
> + OVERRIDE_ANCHOR_IPA_PREFIX_LEN) != 0) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "No IPA anchor [%s].\n", anchor);
> + return ENOMSG;
This is fine since we're only using the error as an..error..so the caller
will only test ret != EOK, but if we used it to differentiate between
different conditions, we should use a custom error code.
> + }
> +
> + sep = strchr(anchor + OVERRIDE_ANCHOR_IPA_PREFIX_LEN, ':');
> + if (sep == NULL || sep[1] == '\0') {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Broken IPA anchor [%s].\n", anchor);
> + return EINVAL;
> + }
> From ec4cf492945cb8f30fcb2467e9d26836de7ff6d8 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Fri, 7 Nov 2014 13:55:01 +0100
> Subject: [PATCH 2/7] LDAP: add support for lookups by UUID
>
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---
> src/providers/data_provider.h | 2 ++
> src/providers/ldap/ldap_id.c | 58
> +++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
> index e1cb4be..5df493e 100644
> --- a/src/providers/data_provider.h
> +++ b/src/providers/data_provider.h
> @@ -127,6 +127,7 @@
> #define BE_FILTER_IDNUM 2
> #define BE_FILTER_ENUM 3
> #define BE_FILTER_SECID 4
> +#define BE_FILTER_UUID 5
>
> #define BE_REQ_USER 0x0001
> #define BE_REQ_GROUP 0x0002
> @@ -139,6 +140,7 @@
> #define BE_REQ_HOST 0x0010
> #define BE_REQ_BY_SECID 0x0011
> #define BE_REQ_USER_AND_GROUP 0x0012
> +#define BE_REQ_BY_UUID 0x0013
> #define BE_REQ_TYPE_MASK 0x00FF
> #define BE_REQ_FAST 0x1000
>
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index e8b3a0e..2e58f4e 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -179,6 +179,20 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
> goto done;
> }
> break;
> + case BE_FILTER_UUID:
> + attr_name = ctx->opts->user_map[SDAP_AT_USER_UUID].name;
> + if (attr_name == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "UUID search not configured for this backend.\n");
> + ret = EINVAL;
I wonder if failing is appropriate? I guess yes, but on the other hand,
calling an unconfigured back end handler (maybe an SSH handler with a
pure LDAP backend) is mostly a noop, right?
But I can see the point, just wanted to double check with you..
Otherwise ACK
> From 15ea0a84cc1d1ded27c6f84ac563b7902f203526 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Fri, 7 Nov 2014 21:33:36 +0100
> Subject: [PATCH 3/7] LDAP: always store UUID if available
>
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---
> src/providers/ldap/sdap_async_groups.c | 20 ++++++++++++++++++++
> src/providers/ldap/sdap_async_users.c | 19 +++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async_groups.c
> b/src/providers/ldap/sdap_async_groups.c
> index a82d2aa..dc1b60d 100644
> --- a/src/providers/ldap/sdap_async_groups.c
> +++ b/src/providers/ldap/sdap_async_groups.c
> @@ -511,6 +511,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> bool posix_group;
> bool use_id_mapping;
> char *sid_str;
> + const char *uuid;
> struct sss_domain_info *subdomain;
> int32_t ad_group_type;
>
> @@ -547,6 +548,25 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> sid_str = NULL;
> }
>
> + /* Always store UUID if available */
> + ret = sysdb_attrs_get_string(attrs,
> +
> opts->group_map[SDAP_AT_GROUP_UUID].sys_name,
> + &uuid);
> + if (ret == EOK) {
> + ret = sysdb_attrs_add_string(group_attrs, SYSDB_UUID, uuid);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE, "Could not add UUID string: [%s]\n",
> + strerror(ret));
Please use sss_strerror() in new code (also elsewhere in this patch).
Otherwise ACK
> From 3adf966d906cf491f7ca852bb36921fc05542dbf Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Fri, 7 Nov 2014 15:05:41 +0100
> Subject: [PATCH 4/7] ipa: add get_be_acct_req_for_uuid()
ACK
> From 9047069105444545a2f5dc0be3ef30d4c43689d1 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Fri, 7 Nov 2014 21:34:55 +0100
> Subject: [PATCH 5/7] IPA: make get_object_from_cache() public
ACK
I just noticed one oddity when reading the code. The function mixes if
and if-else statements, I think it would be nice to add another simple
patch that would converge to if-else style.
> From 9286597c2ed35db04403f72caa75bbfb966cae9b Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Wed, 5 Nov 2014 15:58:04 +0100
> Subject: [PATCH 6/7] IPA: check overrrides for IPA users as well
>
> Currently overrides were only available for sub-domains, e.g. trusted AD
> domains. With this patch overrides can be used for IPA users as well.
>
> Related to https://fedorahosted.org/sssd/ticket/2481
> ---
[...]
> +static errno_t ipa_id_get_account_info_get_original_step(struct tevent_req
> *req,
> + struct be_acct_req
> *ar)
> +{
> + struct ipa_id_get_account_info_state *state = tevent_req_data(req,
> + struct
> ipa_id_get_account_info_state);
> + struct tevent_req *subreq;
> + int ret;
ret is unused and triggers a compilation warning.
> +
> + subreq = sdap_handle_acct_req_send(state, state->be_req, ar,
> + state->ipa_ctx->sdap_id_ctx,
> +
> state->ipa_ctx->sdap_id_ctx->opts->sdom,
> + state->ipa_ctx->sdap_id_ctx->conn,
> true);
> + if (subreq == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "sdap_handle_acct_req_send failed.\n");
> + return ENOMEM;
> + }
> + tevent_req_set_callback(subreq, ipa_id_get_account_info_orig_done, req);
> +
> + return EOK;
> +}
> +
The rest looks good.
> From d3d3d51a1d49936c9ad1a3733388bfd384fd07c4 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Fri, 7 Nov 2014 21:36:12 +0100
> Subject: [PATCH 7/7] Enable views for all domains
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
