On 01/07/2015 03:28 PM, Pavel Reichl wrote:
Please see attached patches.
Thanks.
Hi,
I know there is a design page I did not comment on but seeing the patch
now, does the option really have to be names ad_site_override? Wouldn't
simple ad_site be a better name?
I'd change the description to something like:
"Specify AD site client should try to connect to. If this option is not
set, the AD site will be auto-discovered."
We are using talloc_zero so the following code:
@@ -591,6 +593,14 @@ ad_srv_plugin_ctx_init(TALLOC_CTX *mem_ctx,
goto fail;
}
+ if (ad_site_override == NULL) {
+ ctx->ad_site_override = NULL;
+ } else {
+ ctx->ad_site_override = talloc_strdup(ctx, ad_site_override);
+ if (ctx->ad_site_override == NULL) {
+ goto fail;
+ }
+ }
return ctx;
can be simplified to:
if (ad_site_override != NULL) {
ctx->ad_site_override = talloc_strdup(ctx, ad_site_override);
if (ctx->ad_site_override == NULL) {
goto fail;
}
}
(please add an empty line here)
return ctx;
@@ -756,6 +766,19 @@ static void ad_srv_plugin_site_done(struct
tevent_req *subreq)
ret = ad_get_client_site_recv(state, subreq, &state->site,
&state->forest);
talloc_zfree(subreq);
+ /* Ignore AD site found by dns discovery if specific site is set in
+ * configuration file. */
+ 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 = talloc_strdup(state, state->ctx->ad_site_override);
+ if (state->site == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ }
There is no need to strdup the site name if you make state->site const.