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 and Stephen's logging patches, those are the first to go to master now.
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
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
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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 /.
@@ -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?
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@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, but I defintly think that the space after ! is redundant.
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,
if (homedir == NULL) { DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n"); ret = ENOMEM;homedir_ctx);
@@ -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 ;
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 ;
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.
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?
</para>
<para>
Default: /home
</para>
</listitem>
+</varlistentry>
Thanks for reading.
Pavel Reichl
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@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@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@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,
if (homedir == NULL) { DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n"); ret = ENOMEM;homedir_ctx);
@@ -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
On Fri, Mar 14, 2014 at 10:51:31AM +0100, Lukas Slebodnik wrote:
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) {
But also, pointer is not a bool, so if(!pointer) should not be used, if(pointer == NULL) is much more readable. See also: https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
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
Right, needs fixing, as above.
On (14/03/14 11:06), Jakub Hrozek wrote:
On Fri, Mar 14, 2014 at 10:51:31AM +0100, Lukas Slebodnik wrote:
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) {
But also, pointer is not a bool, so if(!pointer) should not be used, if(pointer == NULL) is much more readable. See also:
Please update http://www.freeipa.org/page/Coding_Style or start discussion on freeipa about updating coding style
https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
if (c == false) //it’s not clear enough: you should write if ((c == false) == true) //or, maybe if (((c == false) == true) == true)
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
Right, needs fixing, as above.
I prefer shorter version. But it looks like there is almost agreement among other developers. So, updated patches are attached
LS
Sorry Lukas, but the patches do not apply to master.
On Fri, 2014-03-14 at 13:49 +0100, Lukas Slebodnik wrote:
On (14/03/14 11:06), Jakub Hrozek wrote:
On Fri, Mar 14, 2014 at 10:51:31AM +0100, Lukas Slebodnik wrote:
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) {
But also, pointer is not a bool, so if(!pointer) should not be used, if(pointer == NULL) is much more readable. See also:
Please update http://www.freeipa.org/page/Coding_Style or start discussion on freeipa about updating coding style
https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
if (c == false) //it’s not clear enough: you should write if ((c == false) == true) //or, maybe if (((c == false) == true) == true)
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
Right, needs fixing, as above.
I prefer shorter version. But it looks like there is almost agreement among other developers. So, updated patches are attached
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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
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 then *domain specific* homedir_substring is not used.
Could you have a look?
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...
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.
... +
--- 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? ...
@@ -172,17 +172,18 @@ static const char *get_homedir_override(TALLOC_CTX *mem_ctx,
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
Do we really need orig_name? I think it is just obfuscating code here.
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?
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...
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. ... +
--- 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? ...
@@ -172,17 +172,18 @@ static const char *get_homedir_override(TALLOC_CTX *mem_ctx,
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
Do we really need orig_name? I think it is just obfuscating code here.
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
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.
ret = sss_parse_name_const(mem_ctx, dom->names, homedir_ctx->username, NULL, &homedir_ctx->username);
Pavel, thank you for review.
LS
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
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.
ret = sss_parse_name_const(mem_ctx, dom->names, homedir_ctx->username, NULL, &homedir_ctx->username);
Pavel, thank you for review.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
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?
LS
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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,
if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into " "name-value components.\n", orig_name);NULL, &homedir_ctx->username);
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
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
PR
On (27/05/14 13:03), Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
It is my fault. I should have explained it better.
The strange (confusing) solution is the next, but it was not used in patches. ret = sss_parse_name_const(mem_ctx, dom->names, homedir_ctx->username, NULL, &homedir_ctx->username)
Thank you for review.
LS
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure it's processed for translations.
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/28/2014 12:22 PM, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Grammar cleanup on the manpage:
The value of this option will be used in the expansion of the <emphasis>override_homedir</emphasis> option if the template contains the format string <emphasis>%H</emphasis>. An LDAP directory entry can directly contain this template so that this option can be used to expand the home directory path for each client machine (or operating system). It can be set per-domain or globally in the [nss] section. A value specified in a domain section will override one set in the [nss] section.
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
One more improvement might be that you don't have to allocate the homedir_ctx most of the time, since the functions that consume them are synchronous. You can just allocate them on stack: struct sss_homedir_ctx hctx; hctx.name = name;
Especially in responder we already do too many allocations..
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O
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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
since the functions that consume them are synchronous. You can just allocate them on stack: struct sss_homedir_ctx hctx; hctx.name = name;
Especially in responder we already do too many allocations..
LS
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote:
O >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.
I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
This patch is targeted for sssd-1-11 and we're close to the 1.11.6 deadline. If you think you can spin up another version quickly, then please do, otherwise let's clean up the unneeded allocation in 1.12.
On (02/06/14 10:01), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: > O > >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. I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
done
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
done
This patch is targeted for sssd-1-11 and we're close to the 1.11.6 deadline. If you think you can spin up another version quickly, then please do, otherwise let's clean up the unneeded allocation in 1.12.
new version attached
LS
On Mon, Jun 02, 2014 at 05:16:05PM +0200, Lukas Slebodnik wrote:
On (02/06/14 10:01), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote: > On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: > > O > > >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. > I missed that. > > I did some testing and all seems to be working, so ACK to all patches. >
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
done
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
done
This patch is targeted for sssd-1-11 and we're close to the 1.11.6 deadline. If you think you can spin up another version quickly, then please do, otherwise let's clean up the unneeded allocation in 1.12.
new version attached
LS
Perfect, thank you.
We can decide about ZERO_STRUCT/{ 0 } later, but I'd like to include these patches in 1.11.6 and the deadline is looming..
The patches work fine and look good to me.
ACK
On Mon, Jun 02, 2014 at 06:42:05PM +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 05:16:05PM +0200, Lukas Slebodnik wrote:
On (02/06/14 10:01), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote: >On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote: >> On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: >> > O >> > >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. >> I missed that. >> >> I did some testing and all seems to be working, so ACK to all patches. >> > >In the third patch, you need to add the file >src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure You ment homedir_substring.xml
Yes :)
>it's processed for translations. > Added
>Can you ask some native English speaker to check the contents of the >text added? > >As a side note, it would be nice to treat any refactoring as an >opportunity for adding more unit tests. Neither expand_homedir_template >nor sss_parse_name_const have any tests. The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
done
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
done
This patch is targeted for sssd-1-11 and we're close to the 1.11.6 deadline. If you think you can spin up another version quickly, then please do, otherwise let's clean up the unneeded allocation in 1.12.
new version attached
LS
Perfect, thank you.
We can decide about ZERO_STRUCT/{ 0 } later, but I'd like to include these patches in 1.11.6 and the deadline is looming..
The patches work fine and look good to me.
ACK
Pushed to master: 59af140ef81f6d0f10db9549089998f5e05631cb ae0a5011e2644eaa482ea1b9e1451eff05c676b9 5cd660aaa885bca95ac3dca660bb77e5786d5f8e be7eabee6b7eb8def2441bf5de4c6d4950c155bf
Can you also send a version that applies cleanly on top of sssd-1-11 ? (or point out patches that need backporting before yours)
On (02/06/14 19:15), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 06:42:05PM +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 05:16:05PM +0200, Lukas Slebodnik wrote:
On (02/06/14 10:01), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote: > On (27/05/14 16:32), Jakub Hrozek wrote: > >On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote: > >> On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: > >> > O > >> > >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. > >> I missed that. > >> > >> I did some testing and all seems to be working, so ACK to all patches. > >> > > > >In the third patch, you need to add the file > >src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure > You ment homedir_substring.xml
Yes :)
> > >it's processed for translations. > > > Added > > >Can you ask some native English speaker to check the contents of the > >text added? > > > >As a side note, it would be nice to treat any refactoring as an > >opportunity for adding more unit tests. Neither expand_homedir_template > >nor sss_parse_name_const have any tests. > The test for expand_homedir_template is in separate commit, > because patches with refactoring are complicated enough. > > LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
done
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
done
This patch is targeted for sssd-1-11 and we're close to the 1.11.6 deadline. If you think you can spin up another version quickly, then please do, otherwise let's clean up the unneeded allocation in 1.12.
new version attached
LS
Perfect, thank you.
We can decide about ZERO_STRUCT/{ 0 } later, but I'd like to include these patches in 1.11.6 and the deadline is looming..
The patches work fine and look good to me.
ACK
Pushed to master: 59af140ef81f6d0f10db9549089998f5e05631cb ae0a5011e2644eaa482ea1b9e1451eff05c676b9 5cd660aaa885bca95ac3dca660bb77e5786d5f8e be7eabee6b7eb8def2441bf5de4c6d4950c155bf
Can you also send a version that applies cleanly on top of sssd-1-11 ? (or point out patches that need backporting before yours)
Two simple patches was necessary to backport. 0001-Add-a-unit-test-for-sss_parse_name_for_domains.patch 0003-refactor-calls-of-sss_parse_name.patch
patchset for 1-11 is attached
LS
On Tue, Jun 03, 2014 at 11:29:21AM +0200, Lukas Slebodnik wrote:
ACK
Pushed to master: 59af140ef81f6d0f10db9549089998f5e05631cb ae0a5011e2644eaa482ea1b9e1451eff05c676b9 5cd660aaa885bca95ac3dca660bb77e5786d5f8e be7eabee6b7eb8def2441bf5de4c6d4950c155bf
Can you also send a version that applies cleanly on top of sssd-1-11 ? (or point out patches that need backporting before yours)
Two simple patches was necessary to backport. 0001-Add-a-unit-test-for-sss_parse_name_for_domains.patch 0003-refactor-calls-of-sss_parse_name.patch
patchset for 1-11 is attached
LS
ACK
On Tue, Jun 03, 2014 at 12:56:37PM +0200, Jakub Hrozek wrote:
On Tue, Jun 03, 2014 at 11:29:21AM +0200, Lukas Slebodnik wrote:
ACK
Pushed to master: 59af140ef81f6d0f10db9549089998f5e05631cb ae0a5011e2644eaa482ea1b9e1451eff05c676b9 5cd660aaa885bca95ac3dca660bb77e5786d5f8e be7eabee6b7eb8def2441bf5de4c6d4950c155bf
Can you also send a version that applies cleanly on top of sssd-1-11 ? (or point out patches that need backporting before yours)
Two simple patches was necessary to backport. 0001-Add-a-unit-test-for-sss_parse_name_for_domains.patch 0003-refactor-calls-of-sss_parse_name.patch
patchset for 1-11 is attached
LS
ACK
Pushed to sssd-1-11: 9cd08bf4b843529c14f71655d62687589301f198 00fb34246a4d1d2cb846a6e2a40d8155ee5a36a1 f9b90ac1cb9ea76f369459470097996cbbc7b343 4deef23b3a0e7da0c1ef4f45f792736c056c3123 99736adbc8da137e3ea061436e4d8baf3a015877 ac10fd4b0b31ca539f6a2ef969185a084896e81a
On Mon, 2014-06-02 at 10:01 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: > O > >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. I missed that.
I did some testing and all seems to be working, so ACK to all patches.
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
On (02/06/14 12:01), Simo Sorce wrote:
On Mon, 2014-06-02 at 10:01 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote:
On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote: > On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: > > O > > >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. > I missed that. > > I did some testing and all seems to be working, so ACK to all patches. >
In the third patch, you need to add the file src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure
You ment homedir_substring.xml
Yes :)
it's processed for translations.
Added
Can you ask some native English speaker to check the contents of the text added?
As a side note, it would be nice to treat any refactoring as an opportunity for adding more unit tests. Neither expand_homedir_template nor sss_parse_name_const have any tests.
The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
I sent patch with ZERO_STRUCT.
Feel free to file ticket to get rid of this macro from sssd source code.
LS
On Mon, 2014-06-02 at 18:06 +0200, Lukas Slebodnik wrote:
On (02/06/14 12:01), Simo Sorce wrote:
On Mon, 2014-06-02 at 10:01 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 07:26:49AM +0200, Lukas Slebodnik wrote:
On (01/06/14 19:23), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 06:22:05PM +0200, Lukas Slebodnik wrote:
On (27/05/14 16:32), Jakub Hrozek wrote: >On Tue, May 27, 2014 at 01:03:42PM +0200, Pavel Reichl wrote: >> On Tue, 2014-05-27 at 11:24 +0200, Lukas Slebodnik wrote: >> > O >> > >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. >> I missed that. >> >> I did some testing and all seems to be working, so ACK to all patches. >> > >In the third patch, you need to add the file >src/man/include/override_homedir.xml into src/man/po/po4a.cfg to make sure You ment homedir_substring.xml
Yes :)
>it's processed for translations. > Added
>Can you ask some native English speaker to check the contents of the >text added? > >As a side note, it would be nice to treat any refactoring as an >opportunity for adding more unit tests. Neither expand_homedir_template >nor sss_parse_name_const have any tests. The test for expand_homedir_template is in separate commit, because patches with refactoring are complicated enough.
LS
Thanks for the unit test!
I don't have any other comments about functionality or code, just please amend the man pages as Stephen suggested.
will do after agreement about allocation of homedir_ctx. I do not want to send patchset more than once :-).
One more improvement might be that you don't have to allocate the homedir_ctx most of the time,
It is just a *one* allocation and reason is to have a zero initialized structure. If you really want to avoid one call of talloc_zero I can replace it with structure allocated on stack and zeroing structure with memset.
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
I sent patch with ZERO_STRUCT.
Feel free to file ticket to get rid of this macro from sssd source code.
Why should we get rid of it ??
Simo.
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
On Mon, 2014-06-02 at 18:15 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
If you set "struct foo var = { 0 };" ? That would be odd.
Simo.
On (02/06/14 12:18), Simo Sorce wrote:
On Mon, 2014-06-02 at 18:15 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
If you set "struct foo var = { 0 };" ?
gcc warning-less clang has warning src/providers/krb5/krb5_utils.c:1016:28: warning: missing field 'client' initializer [-Wmissing-field-initializers] krb5_creds mcred = { 0 }; ^
That would be odd.
"struct foo var = {};" clang warning-less gcc has warning
src/providers/krb5/krb5_utils.c:1016:5: warning: missing initializer for field 'magic' of 'krb5_creds' [-Wmissing-field-initializers] krb5_creds mcred = {}; ^
LS
On Mon, Jun 02, 2014 at 06:26:54PM +0200, Lukas Slebodnik wrote:
On (02/06/14 12:18), Simo Sorce wrote:
On Mon, 2014-06-02 at 18:15 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
If you set "struct foo var = { 0 };" ?
gcc warning-less clang has warning
src/providers/krb5/krb5_utils.c:1016:28: warning: missing field 'client' initializer [-Wmissing-field-initializers] krb5_creds mcred = { 0 }; ^
That would be odd.
"struct foo var = {};" clang warning-less gcc has warning
src/providers/krb5/krb5_utils.c:1016:5: warning: missing initializer for field 'magic' of 'krb5_creds' [-Wmissing-field-initializers] krb5_creds mcred = {}; ^
LS
Right, and I've been using clang lately to get around https://bugzilla.redhat.com/show_bug.cgi?id=986923 ...
On Mon, Jun 02, 2014 at 12:18:02PM -0400, Simo Sorce wrote:
On Mon, 2014-06-02 at 18:15 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
If you set "struct foo var = { 0 };" ? That would be odd.
Yes, I explicitly have -Wno-missing-field-initializers in my CFLAGS because we have a couple of places in SSSD (IIRC mainly in Stef's sbus code) that initialize structures with { 0 } and I was getting tired about the warnings.
Here is some StackOverflow discussion on subject: https://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-thi...
As usual with SO, not all comments are worth reading, but the topmost has even some useful links.
On (02/06/14 18:27), Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:18:02PM -0400, Simo Sorce wrote:
On Mon, 2014-06-02 at 18:15 +0200, Jakub Hrozek wrote:
On Mon, Jun 02, 2014 at 12:01:14PM -0400, Simo Sorce wrote:
We have a ZERO_STRUCT call precisely for this reason.
If it is on the stack, we can simply declare it with a C99 initializer of 0 ? Or is this structure reused (hence needs resetting) more than once ?
Simo.
btw both clang and gcc complain about uninitialized struct in this case. I consider that a compiler bug, bug it's annoying anyway..
If you set "struct foo var = { 0 };" ? That would be odd.
Yes, I explicitly have -Wno-missing-field-initializers in my CFLAGS because we have a couple of places in SSSD (IIRC mainly in Stef's sbus code) that initialize structures with { 0 } and I was getting tired about the warnings.
Here is some StackOverflow discussion on subject: https://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-thi...
As usual with SO, not all comments are worth reading, but the topmost has even some useful links.
yet another link http://clang-developers.42468.n3.nabble.com/Wmissing-field-initializers-td37...
LS
sssd-devel@lists.fedorahosted.org