[SSSD] [PATCHES] AD: support for AD site override

Pavel Březina pbrezina at redhat.com
Thu Jan 15 10:21:11 UTC 2015


On 01/09/2015 04:56 PM, Pavel Reichl wrote:
>
> On 01/08/2015 08:36 PM, Pavel Březina wrote:
>> 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.

man page:
Specify AD site client should try to connect to.
Specify AD site to which client should try to connect.

Which one sounds better?

Ack otherwise.






More information about the sssd-devel mailing list