[PATCH] DP: Notify propperly when removing PAC responder

[Ondrej Kos]

Hi,

During testing, I noticed, that all responders are logged propperly when 
being shut down:

[sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
[sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client

But not PAC:

[sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...

Attached patch adds pac_cli pointer, to fix this behavior.

Ondra

[Pavel Březina]

On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> Hi,
>
> During testing, I noticed, that all responders are logged propperly when
> being shut down:
>
> [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
>
> But not PAC:
>
> [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
>
> Attached patch adds pac_cli pointer, to fix this behavior.
>
> Ondra

Ack.

[Jakub Hrozek]

On Thu, Aug 08, 2013 at 12:32:37AM +0200, Pavel Březina wrote:
> On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> > Hi,
> >
> > During testing, I noticed, that all responders are logged propperly when
> > being shut down:
> >
> > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
> >
> > But not PAC:
> >
> > [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
> >
> > Attached patch adds pac_cli pointer, to fix this behavior.
> >
> > Ondra
>
> Ack.

Do the _cli pointers actually serve any purpose except for the
backchannel towards nss responder? Shouldn't we rather clean them up?

[Pavel Březina]

On 08/08/2013 12:37 AM, Jakub Hrozek wrote:
> On Thu, Aug 08, 2013 at 12:32:37AM +0200, Pavel Březina wrote:
> > On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> > > Hi,
> > >
> > > During testing, I noticed, that all responders are logged propperly when
> > > being shut down:
> > >
> > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
> > >
> > > But not PAC:
> > >
> > > [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
> > >
> > > Attached patch adds pac_cli pointer, to fix this behavior.
> > >
> > > Ondra
> >
> > Ack.
>
> Do the _cli pointers actually serve any purpose except for the
> backchannel towards nss responder? Shouldn't we rather clean them up?

That's a good question.

Only nss_cli is used, but in initgroups callback where it can be 
accessed by be_req->becli (which is used by other callbacks).

[Jakub Hrozek]

On Thu, Aug 08, 2013 at 01:06:24AM +0200, Pavel Březina wrote:
> On 08/08/2013 12:37 AM, Jakub Hrozek wrote:
> > On Thu, Aug 08, 2013 at 12:32:37AM +0200, Pavel Březina wrote:
> > > On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> > > > Hi,
> > > >
> > > > During testing, I noticed, that all responders are logged propperly when
> > > > being shut down:
> > > >
> > > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> > > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
> > > >
> > > > But not PAC:
> > > >
> > > > [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
> > > >
> > > > Attached patch adds pac_cli pointer, to fix this behavior.
> > > >
> > > > Ondra
> > >
> > > Ack.
> >
> > Do the _cli pointers actually serve any purpose except for the
> > backchannel towards nss responder? Shouldn't we rather clean them up?
>
> That's a good question.
>
> Only nss_cli is used, but in initgroups callback where it can be
> accessed by be_req->becli (which is used by other callbacks).

Sumit, do you remember why we put the explicit comment to
client_registration() ? Was it just because we didn't plan on using the
pointer explicitly? This one:

@@ -2118,7 +2121,7 @@ static int client_registration(DBusMessage *message,
     } else if (strcasecmp(cli_name, "SSH") == 0) {
         becli->bectx->ssh_cli = becli;
     } else if (strcasecmp(cli_name, "PAC") == 0) {
-        /* no need to set becli */
+        becli->bectx->pac_cli = becli;
     } else {
         DEBUG(1, ("Unknown client! [%s]\n", cli_name));
     }

I think we should commit this patch to get rid of the warning
and then file another ticket that we would solve together with
https://fedorahosted.org/sssd/ticket/2038 to review if we actually need
all these client pointers..

[Jakub Hrozek]

On Mon, Aug 19, 2013 at 09:20:10PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 08, 2013 at 01:06:24AM +0200, Pavel Březina wrote:
> > On 08/08/2013 12:37 AM, Jakub Hrozek wrote:
> > > On Thu, Aug 08, 2013 at 12:32:37AM +0200, Pavel Březina wrote:
> > > > On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> > > > > Hi,
> > > > >
> > > > > During testing, I noticed, that all responders are logged propperly when
> > > > > being shut down:
> > > > >
> > > > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> > > > > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
> > > > >
> > > > > But not PAC:
> > > > >
> > > > > [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
> > > > >
> > > > > Attached patch adds pac_cli pointer, to fix this behavior.
> > > > >
> > > > > Ondra
> > > >
> > > > Ack.
> > >
> > > Do the _cli pointers actually serve any purpose except for the
> > > backchannel towards nss responder? Shouldn't we rather clean them up?
> >
> > That's a good question.
> >
> > Only nss_cli is used, but in initgroups callback where it can be
> > accessed by be_req->becli (which is used by other callbacks).
>
> Sumit, do you remember why we put the explicit comment to
> client_registration() ? Was it just because we didn't plan on using the
> pointer explicitly? This one:
>
> @@ -2118,7 +2121,7 @@ static int client_registration(DBusMessage *message,
>      } else if (strcasecmp(cli_name, "SSH") == 0) {
>          becli->bectx->ssh_cli = becli;
>      } else if (strcasecmp(cli_name, "PAC") == 0) {
>    /* no need to set becli */
>    becli->bectx->pac_cli = becli;
> } else {
>        DEBUG(1, ("Unknown client! [%s]\n", cli_name));
>    }
>
> I think we should commit this patch to get rid of the warning
> and then file another ticket that we would solve together with
> https://fedorahosted.org/sssd/ticket/2038 to review if we actually need
> all these client pointers..

I filed https://fedorahosted.org/sssd/ticket/2054 to track whether we
need the _cli connections or whether we can get away with be_req->becli
in the initgroups case which would make them dead code.

I don't think we need to solve it now, but I'd like to have this
possible cleanup tracked with a ticket.

[Jakub Hrozek]

On Thu, Aug 08, 2013 at 12:32:37AM +0200, Pavel Březina wrote:
> On 08/05/2013 04:36 PM, Ondrej Kos wrote:
> > Hi,
> >
> > During testing, I noticed, that all responders are logged propperly when
> > being shut down:
> >
> > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed PAM client
> > [sssd[be[DOM]]] [be_client_destructor] (0x0400): Removed NSS client
> >
> > But not PAC:
> >
> > [sssd[be[DOM]]] [be_client_destructor] (0x0020): Unknown client removed ...
> >
> > Attached patch adds pac_cli pointer, to fix this behavior.
> >
> > Ondra
>
> Ack.

Pushed to master.