Sumit Bose <sbose(a)redhat.com> wrote:
On Mon, Oct 04, 2010 at 12:02:50PM +0200, Jan Zelený wrote:
> I'm sending a patch solving ticket #599 for review. After I tried some
> other solutions, this one seemed to be the most simple one which didn't
> cause any problems.
>
> My only concern was whether monitoring the LDAP socket will be restored
> once the backend goes online. Looking at the code, it should, because
> sdap_install_callbacks is called every time sh->connected is set to
> true. But still I wanted to be sure. From testing performed by Dmitri,
> it seems that I was right and the backend is correctly restored.
>
> Here is a complete test case:
> 1. Disconnect from the network
> 2. Try to log in
> -- the data provider should now go offline
> 3. Connect to the network
> 4. Wait for the log filling message to appear in the log
> -- ! The messsage should appear only once !
> 5. If the message doesn't appear, repeat from step 1
> 6. If the message appeared, try to log in
> 7. If successful, check that the login was performed online and not from
> cached credentials
> 8. Step 7 might need to be repeated a couple times, it takes some time
> for SSSD to go online again
>
> Note: the reproduction of this issue might not be that easy. To achieve
> the message appearing in the log, you would have to try different
> scenarios (NB connected using VPN -> suspended -> woke up in the company
> network -> suspended -> ...)
>
> Jan
>
> From 9446ce9061cdabe7685fff9995543c5916339584 Mon Sep 17 00:00:00 2001
> From: Jan Zeleny <jzeleny(a)redhat.com>
> Date: Mon, 4 Oct 2010 11:03:08 +0200
> Subject: [PATCH] Disable events on ldap fd when offline.
>
> Erase events on LDAP socket when backend is offline and an event appears
> on the socket. Normally this would lead to infinite loop, because event
> is present on the fd, but instead of being processed, an error log is
> written and the program continues to wait for the event.
>
> Ticket: #599
> ---
>
> src/providers/ldap/sdap.h | 8 ++++++++
> src/providers/ldap/sdap_async.c | 3 +++
> src/providers/ldap/sdap_fd_events.c | 8 --------
> 3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
> index 4426dac..276891a 100644
> --- a/src/providers/ldap/sdap.h
> +++ b/src/providers/ldap/sdap.h
> @@ -82,6 +82,14 @@ struct sup_list {
>
> char **vals;
>
> };
>
> +struct sdap_fd_events {
> +#ifdef HAVE_LDAP_CONNCB
> + struct ldap_conncb *conncb;
> +#else
> + struct tevent_fd *fde;
> +#endif
> +};
> +
I would prefer a solution where this struct definition stays in
sdap_fd_events.c. Would you mind to add a function to sdap_fd_events.c
which does the talloc_free on fde similar to
remove_ldap_connection_callbacks(). I haven't checked where
remove_ldap_connection_callbacks() is used, maybe it makes sense to
rename remove_ldap_connection_callbacks() and call
talloc_zfree(sh->sdap_fd_events->fde) from here, too?
Thanks for the feedback. Actually using remove_ldap_connection_callbacks() was
my original idea. I don't remember why, but I dropped it for some reason.
Anyway I just tested it builds fine and it also doesn't seem to break anything.
I'm attaching the patch.
Jan