On (12/03/14 14:52), Pavel Reichl wrote:
On Tue, 2014-03-04 at 10:29 +0100, Lukas Slebodnik wrote:
> On (03/03/14 13:35), Lukas Slebodnik wrote:
> >On (31/01/14 23:58), Jakub Hrozek wrote:
> >>On Fri, Jan 31, 2014 at 11:55:25AM +0100, Pavel Březina wrote:
> >>> On 12/02/2013 04:12 PM, Lukas Slebodnik wrote:
> >>> >ehlo,
> >>> >
> >>> >attached patches address ticket #1853
> >>> >
> >>> >LS
> >>>
> >>> Hi,
> >>> these patches no longer apply.
> >>
> >>for patches targeting 1.12, i would prefer to rebase them on top of
Nikolai's
> > ^^^^
> > 1.11
> >>and Stephen's logging patches, those are the first to go to master now.
> >
> >Rebased patches are attached,
> >
> >LS
>
> Sending rebased patches on top of "ipa-server-mode: use lower-case user name
> for home dir"
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Please configure
your email agent. It breaks lines longer than 80 lines
but quoted line prefix is missing on next line.
It is difficult to find out what is reply and what is wrapped line.
Example:
---
src/providers/ipa/ipa_s2n_exop.c | 17 ++++++++++----
src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
src/responder/nss/nsssrv_cmd.c | 35
++++++++++++++++++----------
src/responder/pac/pacsrv_utils.c | 14 ++++++++++--
src/util/sss_nss.c | 43
++++++++++++++++++++---------------
src/util/sss_nss.h | 12 +++++++---
6 files changed, 99 insertions(+), 49 deletions(-)
Hello Lukas,
I'm afraid I have some troubles witch patches:
ipa user-find test_home
--------------
1 user matched
--------------
User login: test_home
First name: test
Last name: home
Home directory: %H/test_home
Login shell: /bin/bash
Email address: test_home(a)ipa.work
UID: 1994000007
GID: 1994000007
Account disabled: False
Password: False
Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------
# getent passwd test_home
test_home:*:1994000007:1994000007:test home:/:/bin/bash]
I suppose homedir should rather be /home/test_home then /.
There were lot of changes in last three months. I spend some time with rebasing
patches but unfortunately git didn't help me with merging the most important
part of the last patch. (and I didn't test rebased patches before sending
to the sssd-devel)
>
> @@ -318,6 +316,7 @@ static int fill_pwent(struct sss_packet *packet,
> bool packet_initialized = false;
> int ncret;
> TALLOC_CTX *tmp_ctx = NULL;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> to_sized_string(&pwfield, nctx->pwfield);
>
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
> } else {
> to_sized_string(&gecos, tmpstr);
> }
> - tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> + homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + num = 0;
> + ret = ENOMEM;
> + goto done;;
> + }
> +
> + homedir_ctx->username = name.str;
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
Should not be homedir_ctx->config_homedir_substr set to
nctx->homedir_substring or rather domain->homedir_substring if not
empty?
It was in function get_homedir_override (in the 1st version of patch)
Fixed in attacehd patches.
> +
> + tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
> if (!tmpstr) {
> to_sized_string(&homedir, "/");
> } else {
> }
I have noticed a few interesting construction regarding our code style:
patch 2:
> From f06de6cf3130d0b01b04152e11933d1c4d875ea6 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn(a)redhat.com>
> Date: Mon, 2 Dec 2013 13:03:33 +0100
> Subject: [PATCH 2/3] NSS: Refactor expand_homedir_template
>
> Function expand_homedir_template had lot of parameters.
> After adding new expand option, all function call should be rewritten,
> (usually argument NULL will be added)
>
> This patch wraps all necessary arguments to structure.
> ---
> src/providers/ipa/ipa_s2n_exop.c | 17 ++++++++++----
> src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
> src/responder/nss/nsssrv_cmd.c | 35
++++++++++++++++++----------
> src/responder/pac/pacsrv_utils.c | 14 ++++++++++--
> src/util/sss_nss.c | 43
++++++++++++++++++++---------------
> src/util/sss_nss.h | 12 +++++++---
> 6 files changed, 99 insertions(+), 49 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_s2n_exop.c
b/src/providers/ipa/ipa_s2n_exop.c
> index
59a99061ac3ad6f24a8af50e9c1d574282301a1d..02e87f86b75f4b5603b8be2e11e25c0a9fcdec7c 100644
> --- 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) {
> + ret = ENOMEM;
> + goto done;
> + }
I think it is now prefered to use if (homedir_ctx == NULL) to highlight
homedir_ctx is a pointer,
I cannot find anything about prefered way
http://www.freeipa.org/page/Coding_Style
but I defintly think that the space after ! is redundant.
It *IS NOT*
'!' is very similar to 'l'
You can compare following three lines.
if (link) {
if (!ink) {
if (! ink) {
> + 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,
>
state->dom->subdomain_homedir,
> -
attrs->a.user.pw_name,
> -
attrs->a.user.pw_uid,
> - NULL,
> - state->dom->name,
> -
state->dom->flat_name);
> + homedir_ctx);
> if (homedir == NULL) {
> ret = ENOMEM;
> goto done;
> diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
> index
0adf806066ec34b8c42ddf4d0cbd2d23f05caa50..1766e33708cd29da18c3ac4d0b94ccc33583a851 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -357,10 +357,10 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
> const char **_homedir)
> {
> errno_t ret;
> - char *name;
> - char *lc_name;
> + const char *name;
> const char *homedir;
> TALLOC_CTX *tmp_ctx;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> tmp_ctx = talloc_new(mem_ctx);
> if (tmp_ctx == NULL) {
> @@ -368,22 +368,31 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
> goto done;
> }
>
> - ret = sss_parse_name(tmp_ctx, dom->names, fqname, NULL, &name);
> + homedir_ctx = talloc_zero(tmp_ctx, struct sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + ret = ENOMEM;
> + goto done;
> + }
same as above
> +
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
> + homedir_ctx->flatname = dom->flat_name;
> + ret = sss_parse_name_const(tmp_ctx, dom->names, fqname,
> + NULL, &name);
> if (ret != EOK) {
> goto done;
> }
>
> /* To be compatible with the old winbind based user lookups and
IPA
> * clients the user name in the home directory path will be
lower-case. */
> - lc_name = sss_tc_utf8_str_tolower(tmp_ctx, name);
> - if (lc_name == NULL) {
> - ret =ENOMEM;
> + homedir_ctx->username = sss_tc_utf8_str_tolower(tmp_ctx, name);
> + if (homedir_ctx->username == NULL) {
> + ret = ENOMEM;
> goto done;
> }
>
> - homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir, lc_name,
> - uid, NULL, dom->name,
dom->flat_name);
> -
> + homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir,
> + homedir_ctx);
> if (homedir == NULL) {
> DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n");
> ret = ENOMEM;
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
> } else {
> to_sized_string(&gecos, tmpstr);
> }
> - tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> + homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + num = 0;
> + ret = ENOMEM;
> + goto done;;
> + }
same as above and please remove the redundant ;
Fixed
> +
> + homedir_ctx->username = name.str;
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
> +
> + tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
> if (!tmpstr) {
> to_sized_string(&homedir, "/");
> } else {
> diff --git a/src/responder/pac/pacsrv_utils.c
b/src/responder/pac/pacsrv_utils.c
> index
035fe84c0f39aaa40a403a8ac5f19527b2191267..4bf5585e8de667850ef520fa372d7e114f2cab1a 100644
> --- a/src/responder/pac/pacsrv_utils.c
> +++ b/src/responder/pac/pacsrv_utils.c
> @@ -353,6 +353,7 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
> char *upn;
> hash_key_t key;
> hash_value_t value;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> pwd = talloc_zero(mem_ctx, struct passwd);
> if (pwd == NULL) {
> @@ -439,9 +440,18 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
> /* Check if there is a special homedir template for sub-domains.
If not a
> * fallback will be added by the NSS responder. */
> if (IS_SUBDOMAIN(dom) && dom->subdomain_homedir) {
> + homedir_ctx = talloc_zero(pwd, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + ret = ENOMEM;
> + goto done;;
> + }
same as above and please remove the redundant ;
Fixed
> + homedir_ctx->username = lname;
> + homedir_ctx->uid = pwd->pw_uid;
> + homedir_ctx->domain = dom->name;
> + homedir_ctx->flatname = dom->flat_name;
> +
> pwd->pw_dir = expand_homedir_template(pwd,
dom->subdomain_homedir,
> - lname, pwd->pw_uid,
NULL,
> - dom->name,
dom->flat_name);
> + homedir_ctx);
> if (pwd->pw_dir == NULL) {
> ret = ENOMEM;
> goto done;
> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> index
406c95cd0f76c9e73705bc49b4261160d26bfc58..efc6e3e2c66944c236a5d62df9e573b15a9f5840 100644
> --- a/src/util/sss_nss.c
> +++ b/src/util/sss_nss.c
> @@ -23,9 +23,7 @@
> #include "util/sss_nss.h"
>
> char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char
*template,
> - const char *username, uint32_t uid,
> - const char *original, const char
*domain,
> - const char *flatname)
> + struct sss_nss_homedir_ctx
*homedir_ctx)
> {
> char *copy;
> char *p;
> @@ -40,6 +38,11 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx,
const char *template,
> return NULL;
> }
>
> + if (! homedir_ctx) {
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Missing home directory
data.\n"));
> + return NULL;
> + }
same as above
patch 3
> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> index
9a13f723de601413205cbb580a92791015b94aa1..cc8b2691262ed9b893bcb693eb1cbb2c05446a60 100644
> --- a/src/confdb/confdb.c
> +++ b/src/confdb/confdb.c
> @@ -1118,6 +1118,16 @@ static int confdb_get_domain_internal(struct
confdb_ctx *cdb,
> }
>
> tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> + CONFDB_NSS_HOMEDIR_SUBSTRING,
NULL);
> + if (tmp != NULL) {
> + domain->homedir_substr = talloc_strdup(domain, tmp);
> + if (!domain->homedir_substr) {
> + ret = ENOMEM;
> + goto done;
> + }
> + }
Please use uniform form of testing to NULL IMO the first one is
prefered.
I prefer short version.
Anyway, we are not consistent. There is place for discussion.
> diff --git a/src/man/include/homedir_substring.xml
b/src/man/include/homedir_substring.xml
> new file mode 100644
> index
0000000000000000000000000000000000000000..8ac83b7d665cfa8bc9b81bcbea780b40cb64ef62
> --- /dev/null
> +++ b/src/man/include/homedir_substring.xml
> @@ -0,0 +1,16 @@
> +<varlistentry>
> + <term>homedir_substring (string)</term>
> + <listitem>
> + <para>
> + Value of this option will be used in the expansion of
> + <emphasis>override_homedir</emphasis> option if template
contains
> + format string <emphasis>%H</emphasis>. LDAP directory can
directly
> + contain template and this option can be used to expand
home
> + directory for each machine (or Operating system).
> + It can be set in the [nss] section or per-domain.
If this option is set in both [nss] and domain section which value is
then used?
Updated.
> + </para>
> + <para>
> + Default: /home
> + </para>
> + </listitem>
> +</varlistentry>
Updated verion of patches are attached.
LS