On 01/15/2015 03:57 PM, Pavel Reichl wrote:
On 01/15/2015 11:21 AM, Pavel Březina wrote:
> 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?
I don't care much, maybe the second one. Should I prepare a new patch
or will you Jakub amend the patch while pushing it? Thanks.
Attached patch updates
man page with Pavel's second suggestion.
Can I get the final ack? I would like to prepare scratch build for
customer ASAP.
Thanks!
>
> Ack otherwise.
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel