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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Thank for comments.
Amended patches attached. Thanks!