On (27/05/14 11:15), Pavel Reichl wrote: >On Tue, 2014-05-27 at 10:42 +0200, Lukas Slebodnik wrote: >> On (27/05/14 10:31), Pavel Reichl wrote: >> >On Tue, 2014-05-27 at 09:32 +0200, Lukas Slebodnik wrote: >> >> On (26/05/14 18:10), Pavel Reichl wrote: >> >> >On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote: >> >> >> On (22/05/14 17:08), Pavel Reichl wrote: >> >> >> >Sorry Lukas, but the patches do not apply to master. >> >> >> > >> >> >> rebased patches are attached. >> >> >> >> >> >> LS >> >> >> _______________________________________________ >> >> >> sssd-devel mailing list >> >> >> sssd-devel@lists.fedorahosted.org >> >> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >> >> > >> >> >Sorry for broken formatting in prev mail. >> >> > >> >> >Hello Lukas, >> >> > >> >> >I'm having trouble with one of my testing configuration: >> >> > >> >> >sssd is client of ipa which has AD trust. >> >> > >> >> >When I ask on users from AD trust domain specific homedir_substring is >> >> >not used. >> >> > >> >> >Could you have a look? >> >> Thank you for catching this issue. >> >> Function new_subdomain is appropriately updated. >> >> >> >> >I have also noticed following nitpicks, could you have a look as well >> >> > >> >> >> diff --git a/src/config/etc/sssd.api.conf >> >> >> b/src/config/etc/sssd.api.conf >> >> >> index >> >> >> c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 >> >> >> 100644 >> >> >> --- a/src/config/etc/sssd.api.conf >> >> >> +++ b/src/config/etc/sssd.api.conf >> >> >> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false >> >> >> pwfield = str, None, false >> >> >> override_homedir = str, None, false >> >> >> fallback_homedir = str, None, false >> >> >> +homedir_substring = str, None, false, /home >> >> >> >> >> >I'm sorry, but I am not aware of the meaning of this file, could you >> >> >explain to me? >> >> >And I'm also curious about the '/home' value, as all other lines are >> >> >having just 3 values... >> >> > >> >> sh-4.2$ head -n3 src/config/etc/sssd.api.conf >> >> # Format: >> >> # option = type, subtype, mandatory[, default] >> >> >> >> > >> >> >> static const char *get_shell_override(TALLOC_CTX *mem_ctx, >> >> >> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c >> >> >> index 66b0e59..2698936 100644 >> >> >> --- a/src/util/sss_nss.c >> >> >> +++ b/src/util/sss_nss.c >> >> >> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX >> >> >> *mem_ctx, const char *template, >> >> >> >> >> >> homedir_ctx->flatname); >> >> >> break; >> >> >> >> >> >> + case 'H': >> >> >> + if (homedir_ctx->config_homedir_substr == NULL) { >> >> >> + DEBUG(SSSDBG_CRIT_FAILURE, >> >> >> + ("Cannot expand home directory substring >> >> >> template " >> >> >> ^ >> >> >> + "substring is empty.\n")); >> >> >> ^ >> >> >I think that the parenthesis are relic here. >> >> It is lefover after multiple rebases in last few months. >> >> Acctually, it is not problem, but it needn't be here. >> >> I fixed the same issue also in 2nd patch. >> >> >> >> >> --- a/src/providers/ipa/ipa_s2n_exop.c >> >> >> +++ b/src/providers/ipa/ipa_s2n_exop.c >> >> >> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct >> >> >> tevent_req *subreq) >> >> >> struct resp_attrs *simple_attrs = NULL; >> >> >> time_t now; >> >> >> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */ >> >> >> + struct sss_nss_homedir_ctx *homedir_ctx; >> >> >> const char *homedir = NULL; >> >> >> struct sysdb_attrs *user_attrs = NULL; >> >> >> struct sysdb_attrs *group_attrs = NULL; >> >> >> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct >> >> >> tevent_req *subreq) >> >> >> switch (attrs->response_type) { >> >> >> case RESP_USER: >> >> >> if (state->dom->subdomain_homedir) { >> >> >> + homedir_ctx = talloc_zero(state, struct >> >> >> sss_nss_homedir_ctx); >> >> >> + if (homedir_ctx == NULL) { >> >> >> + ret = ENOMEM; >> >> >> + goto done; >> >> >> + } >> >> >> + homedir_ctx->username = attrs->a.user.pw_name; >> >> >> + homedir_ctx->uid = attrs->a.user.pw_uid; >> >> >> + homedir_ctx->domain = state->dom->name; >> >> >> + homedir_ctx->flatname = state->dom->flat_name; >> >> >> + >> >> >> homedir = expand_homedir_template(state, >> >> >> ^ >> >> >could you remove the the extra whitespace if already changing nearby >> >> >code? >> >> >... >> >> > >> >> sure, done >> >> >> >> >> struct ldb_message *msg, >> >> >> struct nss_ctx *nctx, >> >> >> struct sss_domain_info *dom, >> >> >> - const char *orig_name, >> >> >> - uint32_t uid) >> >> >> + struct sss_nss_homedir_ctx >> >> >> *homedir_ctx) >> >> >> { >> >> >> const char *homedir; >> >> >> - char *name; >> >> >> + const char *orig_name = homedir_ctx->username; >> >> >> errno_t ret; >> >> >> >> >> >> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL); >> >> >> + homedir_ctx->original = homedir; >> >> >> >> >> >> /* Subdomain users store FQDN in their name attribute */ >> >> >> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL, &name); >> >> >> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name, >> >> >> + NULL, &homedir_ctx->username); >> >> >> if (ret != EOK) { >> >> >> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " >> >> >> "name-value components.\n", orig_name); >> >> >Do we really need orig_name? I think it is just obfuscating code here. >> >> Yes, >> >> >> >> orig_name was parameter of function get_homedir_override, >> >> which was converted into "struct sss_nss_homedir_ctx" >> >> >> >> I could use "homedir_ctx->username" for two purposes >> >> (input and outtupt argument). It seems strange to me. >> >Yes, it is strange and it's a potential source of bugs if order of >> >reading to and writing from would change in implementation of >> >sss_parse_name(). But hiding this potential issue via orig_name is not >> >helping anything. >> Could you ellaborate? What kind of potential bugs? >The fact of passing pointer to the same area in memory to 2 separate >arguments of sss_parse_name() is what I called potential source of bugs. >You said "It seems strange to me" so I hope you know what I mean. It is strange, but it isn't wrong. * orig_name refers to old string * homedir_ctx->username will refer to new string. I need to use old string in debug message if function fails. If you want I can explicitly free old string (orig_name)
>I'm not saying you can't do it, I just say I don't see any reason why to >hide it with usage of orig_name. > Do you have any suggestion of better solution? LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
