On Wed, Jul 29, 2015 at 11:07:09AM +0200, Pavel Březina wrote:
> On 07/28/2015 08:55 PM, Jakub Hrozek wrote:
> >On Tue, Jul 28, 2015 at 08:51:22PM +0200, Jakub Hrozek wrote:
> >>On Tue, Jul 28, 2015 at 01:51:13PM +0200, Pavel Březina wrote:
> >>>On 07/28/2015 01:42 PM, Jakub Hrozek wrote:
> >>>>On Tue, Jul 28, 2015 at 12:13:31PM +0200, Pavel Březina wrote:
> >>>>>On 07/27/2015 09:35 PM, Jakub Hrozek wrote:
> >>>>>>On Mon, Jul 27, 2015 at 07:38:12PM +0200, Pavel Březina wrote:
> >>>>>>>On 07/27/2015 05:38 PM, Jakub Hrozek wrote:
> >>>>>>>>On Mon, Jul 27, 2015 at 12:32:59PM +0200, Pavel Březina wrote:
> >>>>>>>>>On 07/21/2015 09:45 PM, Jakub Hrozek wrote:
> >>>>>>>>>>Hi,
> >>>>>>>>>>
> >>>>>>>>>>the attached patch helps AD clients that have trouble detecting the
> >>>>>>>>>>right site. Please see the patch and the commit message for more
> >>>>>>>>>>details.
> >>>>>>>>>
> >>>>>>>>>Hmm... nack. I think this change makes better job:
> >>>>>>>>>
> >>>>>>>>> if (state->ctx->ad_site_override != NULL) {
> >>>>>>>>> DEBUG(SSSDBG_TRACE_INTERNAL,
> >>>>>>>>> "Ignoring AD site found by DNS discovery: '%s', "
> >>>>>>>>> "using configured value: '%s' instead.\n",
> >>>>>>>>> state->site, state->ctx->ad_site_override);
> >>>>>>>>> state->site = state->ctx->ad_site_override;
> >>>>>>>>>+ ret = EOK;
> >>>>>>>>> }
> >>>>>>>>
> >>>>>>>>That was my first version, too, but it appears that the EOK branch
> >>>>>>>>tries
> >>>>>>>>to access state->forest that is not known -- keep in mind that this is
> >>>>>>>>for cases where ad_get_client_site_recv() returned ENOENT so its
> >>>>>>>>output
> >>>>>>>>parameters might not be available.
> >>>>>>>>
> >>>>>>>>Can we infer the forest somehow?
> >>>>>>>
> >>>>>>>Is forest not known or is it just not set because _recv returned error?
> >>>>>>
> >>>>>>I don't think we can make any assumptions unless we change the _recv to
> >>>>>>return a special error code for the case where the request was only able
> >>>>>>to read the forest. But looking at the request, both are read at the
> >>>>>>same time using ad_get_client_site_parse_ndr().
> >>>>>>
> >>>>>>>The
> >>>>>>>latter one can be solved... but it may be even more conditions :-)
> >>>>>>>
> >>>>>>>OK then.
> >>>>>>
> >>>>>>Can you review the patch, please?
> >>>>>
> >>>>>How about this:
> >>>>>
> >>>>>diff --git a/src/providers/ad/ad_srv.c b/src/providers/ad/ad_srv.c
> >>>>>index f3a4e64..dabcf9f 100644
> >>>>>--- a/src/providers/ad/ad_srv.c
> >>>>>+++ b/src/providers/ad/ad_srv.c
> >>>>>@@ -774,6 +774,12 @@ static void ad_srv_plugin_site_done(struct
> >>>>>tevent_req
> >>>>>*subreq)
> >>>>> "using configured value: '%s' instead.\n",
> >>>>> state->site, state->ctx->ad_site_override);
> >>>>> state->site = state->ctx->ad_site_override;
> >>>>>+
> >>>>>+ if (state->forest == NULL) {
> >>>>>+ state->forest = state->discovery_domain;
> >>>>>+ }
> >>>>>+
> >>>>>+ ret = EOK;
> >>>>> }
> >>>>>
> >>>>>With some debug message probably.
> >>>>
> >>>>Yes, that's better, do you want to send this as a git-formatted patch?
> >>>
> >>>Attached.
> >>
> >>ACK
> >
> >Actually sorry, I was being careless in the review. The code works, but
> >there is a warning:
> >
> >/home/remote/jhrozek/devel/sssd/src/providers/ad/ad_srv.c: In function
> >'ad_srv_plugin_site_done':
> >/home/remote/jhrozek/devel/sssd/src/providers/ad/ad_srv.c:781:27:
> >warning: assignment discards 'const' qualifier from pointer target type
> >[-Wdiscarded-qualifiers]
> > state->forest = state->discovery_domain;
> > ^
>
> Ah, sorry about that. I made state->forest const as that is what we did with
> state->site recently. So we are consistent there.
>
> From 4cd7ffe4d4275889ac5dfaf68f4608e515fab0d5 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Tue, 28 Jul 2015 13:49:37 +0200
> Subject: [PATCH] AD: Use ad_site also when site search fails
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
