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.
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; }
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?
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? The latter one can be solved... but it may be even more conditions :-)
OK then.
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?
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.
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?
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. I'm sorry for taking over your contribution :-)
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
I'm sorry for taking over your contribution :-)
Well, your code is simply better :-) thanks for proposing it.
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; ^
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.
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?= pbrezina@redhat.com Date: Tue, 28 Jul 2015 13:49:37 +0200 Subject: [PATCH] AD: Use ad_site also when site search fails
ACK
On Wed, Jul 29, 2015 at 02:30:15PM +0200, Jakub Hrozek wrote:
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?= pbrezina@redhat.com Date: Tue, 28 Jul 2015 13:49:37 +0200 Subject: [PATCH] AD: Use ad_site also when site search fails
ACK
* master: cbbd8ce524a7e1ae0a1b553c2af18fbef59a06ce
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina