On 09/04/2013 11:09 AM, Ondrej Kos wrote:
On 08/30/2013 10:47 AM, Pavel Březina wrote:
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@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
0001: Make subdomain refresh period configurable
I don't really like the sentence "Optional." in the option description,
since basically every option is optional :-) However, I can see that it
is used in a lot of ipa and ad options. Opinions?
I'm also wondering if we should put this option into man sssd.conf and
treat it similar to 'entry_cache_timeout'. What do you think?
0002: DP: Store list of back-end tevent requests
Nack.
be_req should be inserted into the be_req list in be_req_create(). This
will make sure that we don't miss anything.
Also create a destructor in be_req_create() on be_req that will remove
it from the list (instead of doing so in be_req_terminate).
struct sss_domain_info *be_req_get_domain(struct be_req *be_req)
This function is unused.
0003: DP: Save request domain name, if available
Nack.
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.
No. It definitely has to be set unconditionally, otherwise we are
risking troubles in further development. It is not that much work since
you have to change all be_req_create() calls anyway.
Also keep be_req structure opaque. I.e. create getter and - if needed -
setter, for domain_name.
0004: Clean list of domain requests
Nack.
Please, create a new error code instead of passing EIO. And pass some
nice string description of the error instead of NULL.
0005: LDAP: free subdomain structure
Will it be possible with these patches to also free sss_domain_info?
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
