On Thu, Sep 17, 2015 at 11:06:30AM +0200, Pavel Březina wrote:
> On 09/17/2015 10:32 AM, Petr Cech wrote:
> >Hi Pavel!
> >
> >There is some code between my last end and this continuation. I was read
> >it and did't find anything wrong.
>
> Hi Petr,
> thank you for you review. New patches are attached. All comments from the
> previous mail should be addressed.
>
[PATCH 2/3] cache_req: add support for UPN
> diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> index
> 5f33b225a31e1c1add7f776215d500fb09e127ca..a72a8bac594dc10ff9aa337399f27d9936830a8e
> 100644
> --- a/src/db/sysdb_search.c
> +++ b/src/db/sysdb_search.c
> @@ -295,6 +295,59 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
> return filter;
> }
>
> +int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
> + struct sss_domain_info *domain,
> + const char *upn,
> + struct ldb_result **_res)
> +{
> + TALLOC_CTX *tmp_ctx;
> + struct ldb_message *msg;
> + struct ldb_result *res;
> + static const char *attrs[] = SYSDB_PW_ATTRS;
> + errno_t ret;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> + return ENOMEM;
> + }
> +
> + ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, &msg);
> + if (ret != EOK && ret != ENOENT) {
> + DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn() failed.\n");
> + goto done;
> + }
The call stack here would be
sysdb_getpwupn()->sysdb_search_user_by_upn()->sysdb_search_entry()->ldb_search()
and the returned results, starting from ldb_search()
ldb_result -> ldb_message (array) -> ldb_message -> ldb_result
I think it would be easier and safe some allocations if it would be
sysdb_search_user_by_upn()->sysdb_getpwupn()->ldb_search() .
Do you think this change makes sense?
> +
> + res = talloc_zero(tmp_ctx, struct ldb_result);
> + if (res == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_zero() failed.\n");
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + if (ret == ENOENT) {
> + res->count = 0;
> + res->msgs = NULL;
> + } else {
> + res->count = 1;
> + res->msgs = talloc_array(res, struct ldb_message *, 1);
> + if (res->msgs == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "talloc_array failed.\n");
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + res->msgs[0] = talloc_steal(res->msgs, msg);
> + }
> +
> + *_res = talloc_steal(mem_ctx, res);
> + ret = EOK;
> +
> +done:
> + talloc_free(tmp_ctx);
> + return ret;
> +}
> +
...
> diff --git a/src/responder/common/responder_cache_req.c
> b/src/responder/common/responder_cache_req.c
> index
> fba5001476481040ed962dbbd9b01cc16fe0ba74..70a3010eb697cf14874bd1a950cb7a874bc060b4
> 100644
> --- a/src/responder/common/responder_cache_req.c
> +++ b/src/responder/common/responder_cache_req.c
> @@ -70,6 +70,7 @@ struct cache_req_input {
> enum cache_req_type type;
>
> /* Provided input. */
> + const char *raw_name;
> const char *orig_name;
Instead of adding a new member which again will hold the originally
provided name I would suggest to refactor the change added by
e87b2a6e94c1066b3044fe683825ff5b4f8716c2 (cache_req: parse input name if
needed) by introducing a member called e.g. parsed_name which keeps the
name returned by sss_parse_inp_recv() and return it in cache_req_recv().
In e87b2a6e94c1066b3044fe683825ff5b4f8716c2 it made sense to overwrite
orig_name because no additional member was needed. But now I think the
code would be more clear if orig_name will be kept unmodified.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
