[PATCHES] Remove requests when AD subdomain goes away

[Ondrej Kos]

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

[Pavel Březina]

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.

[Ondrej Kos]

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

[Pavel Březina]

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);
>     }
> }
>
> }

[Ondrej Kos]

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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

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.

[Ondrej Kos]

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
> [e-mail]
> 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

[Pavel Březina]

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
> > [e-mail]
> > 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?