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(a)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.
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.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel