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

[Pavel Reichl]

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.

Ack otherwise.



_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel