On 08/28/2013 11:01 AM, Ondrej Kos wrote:
> 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);
>>> }
>>> }
>>> }
FYI we talked about this offline and it turned out that I was right :-)
be_req_get_domain(be_req) returns be_req->be_ctx->domain, which is
always the main domain.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel New patchset attached.
With this, we store the domain name to the request, where available. I'd
suggest to file a ticket and store it for every request, but for this
purpose it's enough.
Ondra
--
Ondrej Kos
Associate Software Engineer
Identity Management - SSSD
Red Hat Czech