On Wed, Oct 07, 2015 at 02:42:18PM +0200, Pavel Březina wrote:
> On 10/06/2015 03:16 PM, Pavel Březina wrote:
> >On 10/01/2015 05:06 PM, Sumit Bose wrote:
> >>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?
> >
> >Hi, thanks for the review. Done.
> >
> >Although I created a new function sysdb_search_user_by_upn_res since you
> >can't pass attrs into sysdb_getpwnupn (unless we are inconsistent with
> >api).
> >
> >>
> > 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.
> >
> >Done in separate patch so the changes are more visible. It can be
> >squashed to the second patch.
>
> Sumit found that test won't pass. New patchset is attached.
>
>
Thank you, the tests pass now. I've seen a compiler warning which is
seen with the first patch I attached.
During testing I found that UPN lookups for sub-domain users do not
work even with the original nss responder code. I attached fixes for
your patches and the nss responder since the sysdb change depends on you
patches.
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
