On Thu, Oct 08, 2015 at 01:21:29PM +0200, Pavel Březina wrote:
> On 10/07/2015 04:28 PM, Sumit Bose wrote:
> >On Wed, Oct 07, 2015 at 04:22:31PM +0200, Sumit Bose wrote:
> >>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.
> >
> >and now with patches ...
>
> Ack. Thank you!
I pushed both sets to master:
* 8ded8b2f4a57d1833fd230307218d8b07a571785
* 374268c5eda35e8bbc2fef30752299199439cffe
* 391b81f2a78a812a87530e0c50c70d59150f49eb
* 2fce47f2dadd10d2a2c8bf9f03ab7094bc6c6b3a
* 3688374991afb34bbaf2b7843683fc13dd77879d
* 28ebfa4373d1e7ce45b5d70a3619df1c074a661e
* d8125f0e0d38c6939887a0849a44859d6c498c57
and sssd-1-13:
* b1b0abe6223348083be552531f195518b0df3ba5
* f2c3994c6ffd6e715621d7d7ee580db3cd3e85a9
* 055248cf20c1ad9dfba06903789e71b88752b810
* 6bb2a01333bca8a338cb0be6e8d513040b227cb8
* 44415c5a8ad054555a5e0cc016eec8a125c927ab
* f04ead531a4e941de1a5bcb695326ca11a3dd145
* ae3896a12fb4818aac11ccc5d3a57d2cdf61cd43
I hope it's OK to push these all to sssd-1-13 since Sumit's patches
depend on Pavel's and in 1-13 the cache_req is mostly used for UPN.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
