On Fri, Jan 30, 2015 at 05:04:09PM +0100, Pavel Březina wrote:
On 01/27/2015 02:19 PM, Pavel Březina wrote:
>Hi,
>these patches were created as part of my dbus work however they do not
>depend on it. Those patches can be applied on top of current master and
>review separately.
>
>Recently introduced cache_req interface which hides all the logic of
>search for a sysdb object in cache and data provider supported only user
>by name and initgroups request.
>
>These patches add support also for "user by id", "group by name",
"group
>by id" (for new Users and Groups IFP interfaces). It also hides
>sss_parse_inp into cache_req so the may no longer be the need to parse
>username in consumers of the API.
>
>Everything is unit tested.
>
>Happy reviewing :-)
Self nack. I was passing wrong name (input->name instead of
input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not
caught by unit tests since the return value of sss_parse_inp is mocked.
I fixed this and renamed input->name into input->safe_name to avoid another
name confusion.
From db6c8c38bd450f21a9f8d0489fc2b88b6cf3d1b9 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 23 Jan 2015 14:03:07 +0100
Subject: [PATCH 03/13] cache_req: preparations for different input type
Currently cache_req takes only user name as an input parameter. However,
this is not enough since we will need also UID and GID in the future.
This patch creates a structure to hold input parameters so it can be
simply extended to support other input types.
---
src/responder/common/responder_cache_req.c | 341 +++++++++++++++++++++--------
src/responder/common/responder_cache_req.h | 41 +++-
src/responder/ifp/ifpsrv_cmd.c | 24 +-
3 files changed, 306 insertions(+), 100 deletions(-)
[...]
static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
- enum sss_dp_acct_type dp_type,
- struct sss_domain_info *domain,
- const char *name,
+ struct cache_req_input *input,
struct ldb_result **_result)
{
struct ldb_result *result = NULL;
bool one_item_only;
Not related to this patchset per se but I noticed this function returns
ENOENT if one_item_only is set but multiple objects match. I think this
should be a hard error.
[...]
@@ -260,7 +357,7 @@ static errno_t cache_req_cache_check(struct
tevent_req *req)
if (state->result == NULL || state->result->count == 0) {
ret = ENOENT;
} else {
- if (state->dp_type == SSS_DP_INITGROUPS) {
+ if (state->input->dp_type == SSS_DP_INITGROUPS) {
Could we for example remove dp_type here and only use the internal type?
[...]
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -477,6 +477,7 @@ static void
ifp_user_get_attr_lookup(struct tevent_req *subreq)
{
struct ifp_user_get_attr_state *state = NULL;
+ struct cache_req_input *input = NULL;
struct tevent_req *req = NULL;
errno_t ret;
@@ -490,9 +491,30 @@ ifp_user_get_attr_lookup(struct tevent_req *subreq)
return;
}
+ switch (state->search_type) {
+ case SSS_DP_USER:
+ input = cache_req_input_create(state, CACHE_REQ_USER_BY_NAME,
+ state->name);
+ break;
+ case SSS_DP_INITGROUPS:
+ input = cache_req_input_create(state, CACHE_REQ_INITGROUPS,
+ state->name);
Why don't you use initgr_by_name() directly?
+ break;
+ default:
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported search type [%d]!\n",
+ state->search_type);
+ tevent_req_error(req, ERR_INTERNAL);
+ return;
+ }
+
+ if (input == NULL) {
+ tevent_req_error(req, ENOMEM);
+ return;
+ }
+
subreq = cache_req_send(state, state->rctx->ev, state->rctx,
state->ncache, state->neg_timeout, 0,
- state->search_type, state->domname, state->name);
+ state->domname, input);
if (subreq == NULL) {
tevent_req_error(req, ENOMEM);
return;
--
1.7.11.7
From d1e87b6a4adc80a99a8df5ca02dd33c3a2dd68b1 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 23 Jan 2015 15:32:55 +0100
Subject: [PATCH 04/13] cache_req: add support for user by uid
---
src/responder/common/responder_cache_req.c | 103 +++++++-
src/responder/common/responder_cache_req.h | 17 +-
src/responder/ifp/ifpsrv_cmd.c | 4 +-
src/tests/cmocka/test_responder_cache_req.c | 370 +++++++++++++++++++++++++++-
4 files changed, 483 insertions(+), 11 deletions(-)
diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
index b10dfbd783be1c8f4782f0b765644ca1083749a7..76756fcee6652ac5d524e8427dad827a9d3fa1ed
100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -33,6 +33,7 @@ struct cache_req_input {
/* Provided input. */
const char *orig_name;
+ uint32_t id;
/* Data Provider request type resolved from @type. */
enum sss_dp_acct_type dp_type;
@@ -52,7 +53,8 @@ struct cache_req_input {
struct cache_req_input *
cache_req_input_create(TALLOC_CTX *mem_ctx,
enum cache_req_type type,
- const char *name)
+ const char *name,
+ uint32_t id)
{
struct cache_req_input *input;
@@ -77,11 +79,20 @@ cache_req_input_create(TALLOC_CTX *mem_ctx,
goto fail;
}
break;
+ case CACHE_REQ_USER_BY_ID:
+ if (id == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: id cannot be 0!\n");
+ goto fail;
+ }
+
+ input->id = id;
+ break;
I'm not sure the cache_req should error out here, just return ENOENT
which would be more likely to be handled gracefully.
}
/* Resolve Data Provider request type. */
switch (type) {
case CACHE_REQ_USER_BY_NAME:
+ case CACHE_REQ_USER_BY_ID:
input->dp_type = SSS_DP_USER;
break;
@@ -138,6 +149,14 @@ cache_req_input_set_domain(struct cache_req_input *input,
}
break;
+
+ case CACHE_REQ_USER_BY_ID:
+ fqn = talloc_asprintf(tmp_ctx, "UID:%d@%s", input->id,
domain->name);
+ if (fqn == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ break;
}
input->domain = domain;
@@ -163,6 +182,9 @@ static errno_t cache_req_check_ncache(struct cache_req_input *input,
ret = sss_ncache_check_user(ncache, neg_timeout,
input->domain, input->safe_name);
break;
+ case CACHE_REQ_USER_BY_ID:
+ ret = sss_ncache_check_uid(ncache, neg_timeout, input->id);
+ break;
default:
ret = EINVAL;
DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
@@ -188,6 +210,41 @@ static void cache_req_add_to_ncache(struct cache_req_input *input,
ret = sss_ncache_set_user(ncache, false, input->domain,
input->safe_name);
break;
+ case CACHE_REQ_USER_BY_ID:
+ /* Nothing to do. Those types must be unique among all domains so
+ * the don't contain domain part. Therefore they must be set only
+ * if all domains are search and the entry is not found. */
You must initialize ret here.
+ break;
+ default:
+ ret = EINVAL;
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
+ break;
+ }
+
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "Cannot set negcache for [%s] [%d]:
%s\n",
+ input->fqn, ret, sss_strerror(ret));
+
+ /* not fatal */
+ }
+
+ return;
+}
+
+static void cache_req_add_to_ncache_global(struct cache_req_input *input,
+ struct sss_nc_ctx *ncache)
When you switch the cache_req structure into the per-domain and per-lookup
type, can you also change the negcache functions to accept per-domain
and per-lookup structures? That's exactly one case where we want to be
careful with using the right data.
+{
+ errno_t ret;
+
+ switch (input->type) {
+ case CACHE_REQ_USER_BY_NAME:
+ case CACHE_REQ_INITGROUPS:
You must initialize ret here.
[....]
The other patches looked mostly good to me, they are variants on a
similar topic, so I will just highlight one repated pattern:
struct tevent_req *
+cache_req_group_by_name_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct resp_ctx *rctx,
+ struct sss_nc_ctx *ncache,
+ int neg_timeout,
+ int cache_refresh_percent,
+ const char *domain,
+ const char *name)
+{
+ struct cache_req_input *input;
+
+ input = cache_req_input_create(mem_ctx, CACHE_REQ_GROUP_BY_NAME, name, 0);
+ if (input == NULL) {
+ return NULL;
+ }
cache_req_input_create can fail for multiple reasons, I think we should
add a new error code instead of returning NULL. When NULL is returned
from the send function, it usually is treated as ENOMEM by the caller.
So far I read through all the patches. I haven't been able to do any
testing, but I like the direction.
One additional question - are you also planning to add lookups by UPN?
It seems that's the only functionality not implemented by this shared
lookup code.