[SSSD] [PATCHES] Remove requests when AD subdomain goes away

Ondrej Kos okos at redhat.com
Wed Aug 28 09:01:22 UTC 2013


On 08/26/2013 11:39 AM, Pavel Březina wrote:
> On 08/08/2013 02:21 PM, Ondrej Kos wrote:
>> On 08/08/2013 01:08 AM, Pavel Březina wrote:
>>> On 08/05/2013 04:10 PM, Ondrej Kos wrote:
>>>> Hi,
>>>>
>>>> Attached are three patches,
>>>>
>>>> [PATCH 1/3] Make subdomain refresh period configurable
>>>> * Adds the ad_subdomain_refresh_period and ipa_subdomain_refresh_period
>>>> configuration options. This isn't needed to be pushed, but I think it
>>>> can be beneficial. Also, I needed to write this anyway to work with the
>>>> refresh.
>>>>
>>>> [PATCH 2/3] DP: Store list of back-end tevent requests
>>>> * Adds every created request to list, and removes every terminated.
>>>> This
>>>> is to enable iteration through active requests, to fix the issue
>>>> addressed in https://fedorahosted.org/sssd/ticket/1968
>>>>
>>>> [PATCH 3/3] Clean list of domain requests
>>>> * fixes https://fedorahosted.org/sssd/ticket/1968
>>>> * Goes through the list of tevent requests introduced in previous patch
>>>> an those, which match the vanished domain are terminated.
>>>>
>>>> Ondra
>>>
>>> Hi,
>>>
>>>> +void be_clean_dom_req_list(struct be_ctx *be_ctx)
>>>> +{
>>>> +    struct be_req *be_req;
>>>> +    struct sss_domain_info *domain;
>>>> +    char *domain_name;
>>>> +
>>>> +    domain_name = be_ctx->domain->name;
>>>> +
>>>> +    DEBUG(SSSDBG_TRACE_FUNC,
>>>> +            ("Removing requests for domain %s\n", domain_name));
>>>> +
>>>> +    DLIST_FOR_EACH(be_req, be_ctx->be_reqlist) {
>>>> +
>>>> +        domain = be_req_get_domain(be_req);
>>>> +
>>>> +        if (strcmp(domain_name, domain->name) == 0) {
>>>> +            be_req_terminate(be_req, DP_ERR_FATAL, EIO, NULL);
>>>> +        }
>>>> +    }
>>>> +}
>>>
>>>> +struct sss_domain_info *be_req_get_domain(struct be_req *be_req)
>>>> +{
>>>> +    struct be_ctx *be_ctx = be_req_get_be_ctx(be_req);
>>>> +
>>>> +    return be_ctx ? be_ctx->domain : NULL;
>>>> +}
>>>
>>>
>>> please, correct me if I'm wrong (I haven't test those patches yet), but
>>> it won't work as expected. This will always remove *all* be_req, since
>>> you're comparing be_ctx->domain with be_req->be_ctx->domain which is the
>>> same.
>>>
>>> You want to compare name of the subdomain that was removed with the
>>> actual domain (original domain or one of the subdomain) used in the
>>> request - this information is not available at the moment.
>>>
>>
>> Hi,
>>
>> I don't know how this happened, I probably sent patchset from VM. Actual
>> and rebased patches are attached, thanks for noticing this!
>>
>> Ondra
>
> Hi,
>
> Patch 1: Make subdomain refresh period configurable
>
> You are creating two options:
> - ad_subdomain_refresh_period
> - ipa_subdomain_refresh_period
>
> I'd merge it into one subdomain_refresh_period. Does anyone else have
> any opinion on this?
>
> Anyway, you put AD option into IPA manpage:
>> +++ b/src/man/sssd-ipa.5.xml
>> @@ -208,6 +208,19 @@
>>                  </varlistentry>
>>
>>                  <varlistentry>
>> +                    <term>ad_subdomain_refresh_period (integer)</term>
>
> Patch 3:
> You are calling be_clean_dom_req_list() only in ad subdomains, it needs
> to be called also in ipa.
>
> The main point of this ticket is to safely free sdap_domain structure.
> However, you don't call talloc_free() in these patches :-)
>
>
>> void be_clean_dom_req_list(struct be_ctx *be_ctx, const char
>> *domain_name)
>> {
>>     struct be_req *be_req;
>>     struct sss_domain_info *domain;
>>
>>     DEBUG(SSSDBG_TRACE_FUNC,
>>             ("Removing requests for domain %s\n", domain_name));
>>
>>     DLIST_FOR_EACH(be_req, be_ctx->be_reqlist) {
>>
>>         domain = be_req_get_domain(be_req);
>>
>>         if (domain == NULL) continue;
>>
>>         if (strcmp(domain_name, domain->name) == 0) {
>
> be_ctx->domain is always the domain from sssd.conf [domain] section.
> That means that this is a dead code. You need to associate the subdomain
> with be_req and then remove those request that works with this subdomain.

I don't see what you're implying here.

*be_ctx->domain* is not used here at all

*be_ctx->be_reqlist* should contain all request for the provider
*domain_name* is name of subdomain which is getting terminated
*domain* is subdomain info fetched from request which is being processed 
in current iteration through the request list

>
>>             be_req_terminate(be_req, DP_ERR_FATAL, EIO, NULL);
>>         }
>>     }
>> }
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


-- 
Ondrej Kos
Associate Software Engineer
Identity Management - SSSD
Red Hat Czech



More information about the sssd-devel mailing list