[PATCHES] dp: free sdap domain if subdomain is removed

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/1968

[Sumit Bose]

On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/1968

The patches do not cleanly apply to current master, can you send a
rebased version?

bye,
Sumit

[Pavel Březina]

On 10/25/2013 02:44 PM, Sumit Bose wrote:
> On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/1968
>
> The patches do not cleanly apply to current master, can you send a
> rebased version?
>
> bye,
> Sumit

Hi,
those patches apply cleanly to current master. Are you sure you have the 
current master?

[Sumit Bose]

On Fri, Oct 25, 2013 at 03:09:54PM +0200, Pavel Březina wrote:
> On 10/25/2013 02:44 PM, Sumit Bose wrote:
> > On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/1968
> >
> > The patches do not cleanly apply to current master, can you send a
> > rebased version?
> >
> > bye,
> > Sumit
>
> Hi,
> those patches apply cleanly to current master. Are you sure you have
> the current master?

yes, sorry, my fault.

I have some concerns about patch [PATCH 04/10] 'dp: set request domain'.
I'm not sure if there is some code in one of the backends which expects
that be_req->domain is the parent domain.

I would suggest to either add a new member to struct be_req to save the
domain name or add some kind of garbage collection ptasks. If there are
no active requests the ptask can remove and sdap domains which do not
have a domain anymore.

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Březina]

----- Original Message -----
> From: "Sumit Bose" 
> To: [e-mail]
> Sent: Friday, October 25, 2013 4:56:58 PM
> Subject: Re: [SSSD] [PATCHES] dp: free sdap domain if subdomain is removed
>
> On Fri, Oct 25, 2013 at 03:09:54PM +0200, Pavel Březina wrote:
> > On 10/25/2013 02:44 PM, Sumit Bose wrote:
> > > On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/1968
> > >
> > > The patches do not cleanly apply to current master, can you send a
> > > rebased version?
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> > those patches apply cleanly to current master. Are you sure you have
> > the current master?

Hi,
thank you for the review.
> yes, sorry, my fault.
>
> I have some concerns about patch [PATCH 04/10] 'dp: set request domain'.
> I'm not sure if there is some code in one of the backends which expects
> that be_req->domain is the parent domain.

be_req->domain is the new member, it does not exist before this patch.
> I would suggest to either add a new member to struct be_req to save the
> domain name or add some kind of garbage collection ptasks. If there are
> no active requests the ptask can remove and sdap domains which do not
> have a domain anymore.
>
> bye,
> Sumit
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Fri, Oct 25, 2013 at 01:42:56PM -0400, Pavel Brezina wrote:
> ----- Original Message -----
> > From: "Sumit Bose" 
> > To: [e-mail]
> > Sent: Friday, October 25, 2013 4:56:58 PM
> > Subject: Re: [SSSD] [PATCHES] dp: free sdap domain if subdomain is removed
> >
> > On Fri, Oct 25, 2013 at 03:09:54PM +0200, Pavel Březina wrote:
> > > On 10/25/2013 02:44 PM, Sumit Bose wrote:
> > > > On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/1968
> > > >
> > > > The patches do not cleanly apply to current master, can you send a
> > > > rebased version?
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Hi,
> > > those patches apply cleanly to current master. Are you sure you have
> > > the current master?
>
> Hi,
> thank you for the review.
> > yes, sorry, my fault.
> >
> > I have some concerns about patch [PATCH 04/10] 'dp: set request domain'.
> > I'm not sure if there is some code in one of the backends which expects
> > that be_req->domain is the parent domain.
>
> be_req->domain is the new member, it does not exist before this patch.

ah, sorry I confused it with struct be_ctx.
> > I would suggest to either add a new member to struct be_req to save the
> > domain name or add some kind of garbage collection ptasks. If there are
> > no active requests the ptask can remove and sdap domains which do not
> > have a domain anymore.

I tested the patch set a bit with plain LDAP, AD and IPA provider and
removed the trust in IPA to simulate the removal of domains and didn't
see any issues.

ACK

bye,
Sumit
> > bye,
> > Sumit
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 09:56:05PM +0200, Sumit Bose wrote:
> On Fri, Oct 25, 2013 at 01:42:56PM -0400, Pavel Brezina wrote:
> > ----- Original Message -----
> > > From: "Sumit Bose" 
> > > To: [e-mail]
> > > Sent: Friday, October 25, 2013 4:56:58 PM
> > > Subject: Re: [SSSD] [PATCHES] dp: free sdap domain if subdomain is removed
> > >
> > > On Fri, Oct 25, 2013 at 03:09:54PM +0200, Pavel Březina wrote:
> > > > On 10/25/2013 02:44 PM, Sumit Bose wrote:
> > > > > On Thu, Oct 24, 2013 at 01:13:25PM +0200, Pavel Březina wrote:
> > > > > > https://fedorahosted.org/sssd/ticket/1968
> > > > >
> > > > > The patches do not cleanly apply to current master, can you send a
> > > > > rebased version?
> > > > >
> > > > > bye,
> > > > > Sumit
> > > >
> > > > Hi,
> > > > those patches apply cleanly to current master. Are you sure you have
> > > > the current master?
> >
> > Hi,
> > thank you for the review.
> > > yes, sorry, my fault.
> > >
> > > I have some concerns about patch [PATCH 04/10] 'dp: set request domain'.
> > > I'm not sure if there is some code in one of the backends which expects
> > > that be_req->domain is the parent domain.
> >
> > be_req->domain is the new member, it does not exist before this patch.
>
> ah, sorry I confused it with struct be_ctx.
> > > I would suggest to either add a new member to struct be_req to save the
> > > domain name or add some kind of garbage collection ptasks. If there are
> > > no active requests the ptask can remove and sdap domains which do not
> > > have a domain anymore.
>
> I tested the patch set a bit with plain LDAP, AD and IPA provider and
> removed the trust in IPA to simulate the removal of domains and didn't
> see any issues.
>
> ACK
>
> bye,
> Sumit

Pushed to master and sssd-1-11