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.
be_req_terminate(be_req, DP_ERR_FATAL, EIO, NULL);
}
}
}