On Tue, Dec 08, 2015 at 04:57:02PM +0100, Petr Cech wrote:
> On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
> >On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
> >>So, I have choose the 2) option and the patch is attached.
> >
> >I think this patch goes in the right direction, but still needs more
> >work. See inline.
>
> Hello Jakub,
> thank you for careful review.
>
> >>And something more:
> >> a.bak ... log before patch
> >> b.bak ... log after patch
> >>
> >>Regards
> >>
> >>Petr
> >
> >> From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
> >>From: Petr Cech <pcech(a)redhat.com>
> >>Date: Mon, 9 Nov 2015 09:51:05 -0500
> >>Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
> >>
> >>Services for IPA subdomains aren't handled by SSSD. This patch
> >>adds quick shortcut to avoid sending an LDAP query to cache.
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2747
> >>---
> ...
> >>
> >>+
> >>+static bool is_object_overridable(struct be_acct_req *ar)
> >>+{
> >>+ bool ret = false;
> >>+
> >>+ switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> >>+ case BE_REQ_USER:
> >>+ ret = true;
> >>+ break;
> >>+ case BE_REQ_GROUP:
> >>+ ret = true;
> >>+ break;
> >
> >I know what you tried to do here, but please keep in mind that other types
> >of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
> >BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.
>
> I did cases for those REQs, but I don't know if it is all.
>
> >We can also collapse the case: statements so that the switch is more
> >compact.
> Addressed.
>
> ...
> >>diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
> >>index
472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
> >>--- a/src/providers/ipa/ipa_subdomains_id.c
> >>+++ b/src/providers/ipa/ipa_subdomains_id.c
> >>@@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
> >> NULL };
> >> char *name;
> >>
> >>+ if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES) {
> >>+ DEBUG(SSSDBG_OP_FAILURE,
> >>+ "Services for IPA subdomains aren't handled by
SSSD.\n");
> >>+ ret = ENOENT;
> >>+ goto done;
> >>+ }
> >>+
> >
> >With adding is_object_overridable(), I don't think this branch is
> >reachable.
>
> I agree. I have been surprised that this message wasn't been in logs.
One style nitpick and then we can push :)
>
>
> >> if (ar->filter_type == BE_FILTER_SECID) {
> >> ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value,
attrs,
> >> &res);
>
> Regards
>
> Petr
> From e024fd15dc9c6735b291f5566ba435da7fc496bd Mon Sep 17 00:00:00 2001
> From: Petr Cech <pcech(a)redhat.com>
> Date: Mon, 9 Nov 2015 09:51:05 -0500
> Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
>
> Function get_object_from_cache() does not handle services.
> This patch adds quick shortcut to avoid sending an LDAP query
> to cache.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/2747
> ---
> src/providers/ipa/ipa_id.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> index
e81ccb34dd6eb44618538593f5473fbe5e89d896..dd32d7cf5fb2e6b4793b0009924949801517a809 100644
> --- a/src/providers/ipa/ipa_id.c
> +++ b/src/providers/ipa/ipa_id.c
> @@ -30,6 +30,31 @@
> #include "providers/ldap/sdap_async.h"
> #include "providers/ipa/ipa_id.h"
>
> +static bool is_object_overridable(struct be_acct_req *ar)
> +{
> + bool ret = false;
> +
> + switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> + case BE_REQ_USER: ret = true;
> + break;
> + case BE_REQ_GROUP: ret = true;
> + break;
> + case BE_REQ_INITGROUPS: ret = true;
> + break;
> + case BE_REQ_BY_SECID: ret = true;
> + break;
> + case BE_REQ_USER_AND_GROUP: ret = true;
> + break;
> + case BE_REQ_BY_UUID: ret = true;
> + break;
> + case BE_REQ_BY_CERT: ret = true;
> + break;
> + default: break;
> + }
We can make the code more compact with:
case BE_REQ_USER:
case BE_REQ_GROUP:
/* And also all the others.. */
ret = true;
we can also use
return true;
> break;
>
>> +
>> + return ret;
>> +}
>> +
>> static const char *ipa_account_info_error_text(int ret, int *dp_error,
>> const char *default_text)
>> {
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org