4:58 p.m.
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.
6:35 a.m.
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
3:29 a.m.
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
8:52 a.m.
On Tue, 2014-03-04 at 10:29 +0100, Lukas Slebodnik wrote:
On (03/03/14 13:35), Lukas Slebodnik wrote:
>On (31/01/14 23:58), Jakub Hrozek wrote:
>>On Fri, Jan 31, 2014 at 11:55:25AM +0100, Pavel Březina wrote:
>>> On 12/02/2013 04:12 PM, Lukas Slebodnik wrote:
>>> >ehlo,
>>> >
>>> >attached patches address ticket #1853
>>> >
>>> >LS
>>>
>>> Hi,
>>> these patches no longer apply.
>>
>>for patches targeting 1.12, i would prefer to rebase them on top of
Nikolai's
> ^^^^
> 1.11
>>and Stephen's logging patches, those are the first to go to master now.
>
>Rebased patches are attached,
>
>LS
Sending rebased patches on top of "ipa-server-mode: use lower-case user name
for home dir"
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Lukas,
I'm afraid I have some troubles witch patches:
ipa user-find test_home
--------------
1 user matched
--------------
User login: test_home
First name: test
Last name: home
Home directory: %H/test_home
Login shell: /bin/bash
Email address: test_home(a)ipa.work
UID: 1994000007
GID: 1994000007
Account disabled: False
Password: False
Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------
# getent passwd test_home
test_home:*:1994000007:1994000007:test home:/:/bin/bash]
I suppose homedir should rather be /home/test_home then /.
@@ -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(a)redhat.com>
Date: Mon, 2 Dec 2013 13:03:33 +0100
Subject: [PATCH 2/3] NSS: Refactor expand_homedir_template
Function expand_homedir_template had lot of parameters.
After adding new expand option, all function call should be rewritten,
(usually argument NULL will be added)
This patch wraps all necessary arguments to structure.
---
src/providers/ipa/ipa_s2n_exop.c | 17 ++++++++++----
src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
src/responder/nss/nsssrv_cmd.c | 35
++++++++++++++++++----------
src/responder/pac/pacsrv_utils.c | 14 ++++++++++--
src/util/sss_nss.c | 43
++++++++++++++++++++---------------
src/util/sss_nss.h | 12 +++++++---
6 files changed, 99 insertions(+), 49 deletions(-)
diff --git a/src/providers/ipa/ipa_s2n_exop.c
b/src/providers/ipa/ipa_s2n_exop.c
index
59a99061ac3ad6f24a8af50e9c1d574282301a1d..02e87f86b75f4b5603b8be2e11e25c0a9fcdec7c
100644
--- a/src/providers/ipa/ipa_s2n_exop.c
+++ b/src/providers/ipa/ipa_s2n_exop.c
@@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
struct resp_attrs *simple_attrs = NULL;
time_t now;
uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
+ struct sss_nss_homedir_ctx *homedir_ctx;
const char *homedir = NULL;
struct sysdb_attrs *user_attrs = NULL;
struct sysdb_attrs *group_attrs = NULL;
@@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
switch (attrs->response_type) {
case RESP_USER:
if (state->dom->subdomain_homedir) {
+ homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
+ if (! homedir_ctx) {
+ ret = ENOMEM;
+ goto done;
+ }
I think it is now prefered to use if (homedir_ctx == NULL) to highlight
homedir_ctx is a pointer, 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,
+ homedir_ctx);
if (homedir == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n");
ret = ENOMEM;
@@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
} else {
to_sized_string(&gecos, tmpstr);
}
- tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
+
+ homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
+ if (! homedir_ctx) {
+ num = 0;
+ ret = ENOMEM;
+ goto done;;
+ }
same as above and please remove the redundant ;
> +
> + 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
4:51 a.m.
On (12/03/14 14:52), Pavel Reichl wrote:
On Tue, 2014-03-04 at 10:29 +0100, Lukas Slebodnik wrote:
> On (03/03/14 13:35), Lukas Slebodnik wrote:
> >On (31/01/14 23:58), Jakub Hrozek wrote:
> >>On Fri, Jan 31, 2014 at 11:55:25AM +0100, Pavel Březina wrote:
> >>> On 12/02/2013 04:12 PM, Lukas Slebodnik wrote:
> >>> >ehlo,
> >>> >
> >>> >attached patches address ticket #1853
> >>> >
> >>> >LS
> >>>
> >>> Hi,
> >>> these patches no longer apply.
> >>
> >>for patches targeting 1.12, i would prefer to rebase them on top of
Nikolai's
> > ^^^^
> > 1.11
> >>and Stephen's logging patches, those are the first to go to master now.
> >
> >Rebased patches are attached,
> >
> >LS
>
> Sending rebased patches on top of "ipa-server-mode: use lower-case user name
> for home dir"
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Please configure
your email agent. It breaks lines longer than 80 lines
but quoted line prefix is missing on next line.
It is difficult to find out what is reply and what is wrapped line.
Example:
---
src/providers/ipa/ipa_s2n_exop.c | 17 ++++++++++----
src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
src/responder/nss/nsssrv_cmd.c | 35
++++++++++++++++++----------
src/responder/pac/pacsrv_utils.c | 14 ++++++++++--
src/util/sss_nss.c | 43
++++++++++++++++++++---------------
src/util/sss_nss.h | 12 +++++++---
6 files changed, 99 insertions(+), 49 deletions(-)
Hello Lukas,
I'm afraid I have some troubles witch patches:
ipa user-find test_home
--------------
1 user matched
--------------
User login: test_home
First name: test
Last name: home
Home directory: %H/test_home
Login shell: /bin/bash
Email address: test_home(a)ipa.work
UID: 1994000007
GID: 1994000007
Account disabled: False
Password: False
Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------
# getent passwd test_home
test_home:*:1994000007:1994000007:test home:/:/bin/bash]
I suppose homedir should rather be /home/test_home then /.
There were lot of changes in last three months. I spend some time with rebasing
patches but unfortunately git didn't help me with merging the most important
part of the last patch. (and I didn't test rebased patches before sending
to the sssd-devel)
>
> @@ -318,6 +316,7 @@ static int fill_pwent(struct sss_packet *packet,
> bool packet_initialized = false;
> int ncret;
> TALLOC_CTX *tmp_ctx = NULL;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> to_sized_string(&pwfield, nctx->pwfield);
>
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
> } else {
> to_sized_string(&gecos, tmpstr);
> }
> - tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> + homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + num = 0;
> + ret = ENOMEM;
> + goto done;;
> + }
> +
> + homedir_ctx->username = name.str;
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
Should not be homedir_ctx->config_homedir_substr set to
nctx->homedir_substring or rather domain->homedir_substring if not
empty?
It was in function get_homedir_override (in the 1st version of patch)
Fixed in attacehd patches.
> +
> + tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
> if (!tmpstr) {
> to_sized_string(&homedir, "/");
> } else {
> }
I have noticed a few interesting construction regarding our code style:
patch 2:
> From f06de6cf3130d0b01b04152e11933d1c4d875ea6 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn(a)redhat.com>
> Date: Mon, 2 Dec 2013 13:03:33 +0100
> Subject: [PATCH 2/3] NSS: Refactor expand_homedir_template
>
> Function expand_homedir_template had lot of parameters.
> After adding new expand option, all function call should be rewritten,
> (usually argument NULL will be added)
>
> This patch wraps all necessary arguments to structure.
> ---
> src/providers/ipa/ipa_s2n_exop.c | 17 ++++++++++----
> src/providers/ipa/ipa_subdomains_id.c | 27 ++++++++++++++--------
> src/responder/nss/nsssrv_cmd.c | 35
++++++++++++++++++----------
> src/responder/pac/pacsrv_utils.c | 14 ++++++++++--
> src/util/sss_nss.c | 43
++++++++++++++++++++---------------
> src/util/sss_nss.h | 12 +++++++---
> 6 files changed, 99 insertions(+), 49 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_s2n_exop.c
b/src/providers/ipa/ipa_s2n_exop.c
> index
59a99061ac3ad6f24a8af50e9c1d574282301a1d..02e87f86b75f4b5603b8be2e11e25c0a9fcdec7c 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
> struct resp_attrs *simple_attrs = NULL;
> time_t now;
> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> + struct sss_nss_homedir_ctx *homedir_ctx;
> const char *homedir = NULL;
> struct sysdb_attrs *user_attrs = NULL;
> struct sysdb_attrs *group_attrs = NULL;
> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
> switch (attrs->response_type) {
> case RESP_USER:
> if (state->dom->subdomain_homedir) {
> + homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + ret = ENOMEM;
> + goto done;
> + }
I think it is now prefered to use if (homedir_ctx == NULL) to highlight
homedir_ctx is a pointer,
I cannot find anything about prefered way
http://www.freeipa.org/page/Coding_Style
but I defintly think that the space after ! is redundant.
It *IS NOT*
'!' is very similar to 'l'
You can compare following three lines.
if (link) {
if (!ink) {
if (! ink) {
> + homedir_ctx->username =
attrs->a.user.pw_name;
> + homedir_ctx->uid = attrs->a.user.pw_uid;
> + homedir_ctx->domain = state->dom->name;
> + homedir_ctx->flatname = state->dom->flat_name;
> +
> homedir = expand_homedir_template(state,
>
state->dom->subdomain_homedir,
> -
attrs->a.user.pw_name,
> -
attrs->a.user.pw_uid,
> - NULL,
> - state->dom->name,
> -
state->dom->flat_name);
> + homedir_ctx);
> if (homedir == NULL) {
> ret = ENOMEM;
> goto done;
> diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
> index
0adf806066ec34b8c42ddf4d0cbd2d23f05caa50..1766e33708cd29da18c3ac4d0b94ccc33583a851 100644
> --- a/src/providers/ipa/ipa_subdomains_id.c
> +++ b/src/providers/ipa/ipa_subdomains_id.c
> @@ -357,10 +357,10 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
> const char **_homedir)
> {
> errno_t ret;
> - char *name;
> - char *lc_name;
> + const char *name;
> const char *homedir;
> TALLOC_CTX *tmp_ctx;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> tmp_ctx = talloc_new(mem_ctx);
> if (tmp_ctx == NULL) {
> @@ -368,22 +368,31 @@ get_subdomain_homedir_of_user(TALLOC_CTX
*mem_ctx, struct sss_domain_info *dom,
> goto done;
> }
>
> - ret = sss_parse_name(tmp_ctx, dom->names, fqname, NULL, &name);
> + homedir_ctx = talloc_zero(tmp_ctx, struct sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + ret = ENOMEM;
> + goto done;
> + }
same as above
> +
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
> + homedir_ctx->flatname = dom->flat_name;
> + ret = sss_parse_name_const(tmp_ctx, dom->names, fqname,
> + NULL, &name);
> if (ret != EOK) {
> goto done;
> }
>
> /* To be compatible with the old winbind based user lookups and
IPA
> * clients the user name in the home directory path will be
lower-case. */
> - lc_name = sss_tc_utf8_str_tolower(tmp_ctx, name);
> - if (lc_name == NULL) {
> - ret =ENOMEM;
> + homedir_ctx->username = sss_tc_utf8_str_tolower(tmp_ctx, name);
> + if (homedir_ctx->username == NULL) {
> + ret = ENOMEM;
> goto done;
> }
>
> - homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir, lc_name,
> - uid, NULL, dom->name,
dom->flat_name);
> -
> + homedir = expand_homedir_template(tmp_ctx,
dom->subdomain_homedir,
> + homedir_ctx);
> if (homedir == NULL) {
> DEBUG(SSSDBG_OP_FAILURE, "expand_homedir_template failed\n");
> ret = ENOMEM;
> @@ -373,7 +372,19 @@ static int fill_pwent(struct sss_packet *packet,
> } else {
> to_sized_string(&gecos, tmpstr);
> }
> - tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
name.str, uid);
> +
> + homedir_ctx = talloc_zero(tmp_ctx, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + num = 0;
> + ret = ENOMEM;
> + goto done;;
> + }
same as above and please remove the redundant ;
Fixed
> +
> + homedir_ctx->username = name.str;
> + homedir_ctx->uid = uid;
> + homedir_ctx->domain = dom->name;
> +
> + tmpstr = get_homedir_override(tmp_ctx, msg, nctx, dom,
homedir_ctx);
> if (!tmpstr) {
> to_sized_string(&homedir, "/");
> } else {
> diff --git a/src/responder/pac/pacsrv_utils.c
b/src/responder/pac/pacsrv_utils.c
> index
035fe84c0f39aaa40a403a8ac5f19527b2191267..4bf5585e8de667850ef520fa372d7e114f2cab1a 100644
> --- a/src/responder/pac/pacsrv_utils.c
> +++ b/src/responder/pac/pacsrv_utils.c
> @@ -353,6 +353,7 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
> char *upn;
> hash_key_t key;
> hash_value_t value;
> + struct sss_nss_homedir_ctx *homedir_ctx;
>
> pwd = talloc_zero(mem_ctx, struct passwd);
> if (pwd == NULL) {
> @@ -439,9 +440,18 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
> /* Check if there is a special homedir template for sub-domains.
If not a
> * fallback will be added by the NSS responder. */
> if (IS_SUBDOMAIN(dom) && dom->subdomain_homedir) {
> + homedir_ctx = talloc_zero(pwd, struct
sss_nss_homedir_ctx);
> + if (! homedir_ctx) {
> + ret = ENOMEM;
> + goto done;;
> + }
same as above and please remove the redundant ;
Fixed
> + homedir_ctx->username = lname;
> + homedir_ctx->uid = pwd->pw_uid;
> + homedir_ctx->domain = dom->name;
> + homedir_ctx->flatname = dom->flat_name;
> +
> pwd->pw_dir = expand_homedir_template(pwd,
dom->subdomain_homedir,
> - lname, pwd->pw_uid,
NULL,
> - dom->name,
dom->flat_name);
> + homedir_ctx);
> if (pwd->pw_dir == NULL) {
> ret = ENOMEM;
> goto done;
> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> index
406c95cd0f76c9e73705bc49b4261160d26bfc58..efc6e3e2c66944c236a5d62df9e573b15a9f5840 100644
> --- a/src/util/sss_nss.c
> +++ b/src/util/sss_nss.c
> @@ -23,9 +23,7 @@
> #include "util/sss_nss.h"
>
> char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char
*template,
> - const char *username, uint32_t uid,
> - const char *original, const char
*domain,
> - const char *flatname)
> + struct sss_nss_homedir_ctx
*homedir_ctx)
> {
> char *copy;
> char *p;
> @@ -40,6 +38,11 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx,
const char *template,
> return NULL;
> }
>
> + if (! homedir_ctx) {
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Missing home directory
data.\n"));
> + return NULL;
> + }
same as above
patch 3
> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> index
9a13f723de601413205cbb580a92791015b94aa1..cc8b2691262ed9b893bcb693eb1cbb2c05446a60 100644
> --- a/src/confdb/confdb.c
> +++ b/src/confdb/confdb.c
> @@ -1118,6 +1118,16 @@ static int confdb_get_domain_internal(struct
confdb_ctx *cdb,
> }
>
> tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> + CONFDB_NSS_HOMEDIR_SUBSTRING,
NULL);
> + if (tmp != NULL) {
> + domain->homedir_substr = talloc_strdup(domain, tmp);
> + if (!domain->homedir_substr) {
> + ret = ENOMEM;
> + goto done;
> + }
> + }
Please use uniform form of testing to NULL IMO the first one is
prefered.
I prefer short version.
Anyway, we are not consistent. There is place for discussion.
> diff --git a/src/man/include/homedir_substring.xml
b/src/man/include/homedir_substring.xml
> new file mode 100644
> index
0000000000000000000000000000000000000000..8ac83b7d665cfa8bc9b81bcbea780b40cb64ef62
> --- /dev/null
> +++ b/src/man/include/homedir_substring.xml
> @@ -0,0 +1,16 @@
> +<varlistentry>
> + <term>homedir_substring (string)</term>
> + <listitem>
> + <para>
> + Value of this option will be used in the expansion of
> + <emphasis>override_homedir</emphasis> option if template
contains
> + format string <emphasis>%H</emphasis>. LDAP directory can
directly
> + contain template and this option can be used to expand
home
> + directory for each machine (or Operating system).
> + It can be set in the [nss] section or per-domain.
If this option is set in both [nss] and domain section which value is
then used?
Updated.
> + </para>
> + <para>
> + Default: /home
> + </para>
> + </listitem>
> +</varlistentry>
Updated verion of patches are attached.
LS
5:06 a.m.
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.
7:49 a.m.
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
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
10:08 a.m.
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
11:06 a.m.
On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
homedir_ctx->flatname);
On (22/05/14 17:08), Pavel Reichl wrote:
>Sorry Lukas, but the patches do not apply to master.
rebased patches are attached.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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,
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,
+ NULL, &homedir_ctx->username);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
"name-value components.\n", orig_name);
Do we really need orig_name? I think it is just obfuscating code here.
11:10 a.m.
On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
On (22/05/14 17:08), Pavel Reichl wrote:
>Sorry Lukas, but the patches do not apply to master.
>
rebased patches are attached.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sorry for broken formatting in prev mail.
Hello Lukas,
I'm having trouble with one of my testing configuration:
sssd is client of ipa which has AD trust.
When I ask on users from AD trust domain specific homedir_substring is
not used.
Could you have a look?
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,
+ NULL, &homedir_ctx->username);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
"name-value components.\n", orig_name);
Do we really need
orig_name? I think it is just obfuscating code here.
2:32 a.m.
On (26/05/14 18:10), Pavel Reichl wrote:
On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
> On (22/05/14 17:08), Pavel Reichl wrote:
> >Sorry Lukas, but the patches do not apply to master.
> >
> rebased patches are attached.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Sorry for broken formatting in prev mail.
Hello Lukas,
I'm having trouble with one of my testing configuration:
sssd is client of ipa which has AD trust.
When I ask on users from AD trust domain specific homedir_substring is
not used.
Could you have a look?
Thank you for catching this issue.
Function new_subdomain is appropriately updated.
I have also noticed following nitpicks, could you have a look as well
> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> index
c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
> --- a/src/config/etc/sssd.api.conf
> +++ b/src/config/etc/sssd.api.conf
> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
> pwfield = str, None, false
> override_homedir = str, None, false
> fallback_homedir = str, None, false
> +homedir_substring = str, None, false, /home
>
I'm sorry, but I am not aware of the meaning of this file, could you explain to me?
And I'm also curious about the '/home' value, as all other lines are having
just 3 values...
sh-4.2$ head -n3 src/config/etc/sssd.api.conf
# Format:
# option = type, subtype, mandatory[, default]
> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> index 66b0e59..2698936 100644
> --- a/src/util/sss_nss.c
> +++ b/src/util/sss_nss.c
> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx, const char
*template,
> homedir_ctx->flatname);
> break;
>
> + case 'H':
> + if (homedir_ctx->config_homedir_substr == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + ("Cannot expand home directory substring template
"
> ^
> + "substring is empty.\n"));
> ^
I think that the parenthesis are relic here.
It is lefover after multiple rebases in
last few months.
Acctually, it is not problem, but it needn't be here.
I fixed the same issue also in 2nd patch.
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> struct resp_attrs *simple_attrs = NULL;
> time_t now;
> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> + struct sss_nss_homedir_ctx *homedir_ctx;
> const char *homedir = NULL;
> struct sysdb_attrs *user_attrs = NULL;
> struct sysdb_attrs *group_attrs = NULL;
> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct tevent_req *subreq)
> switch (attrs->response_type) {
> case RESP_USER:
> if (state->dom->subdomain_homedir) {
> + homedir_ctx = talloc_zero(state, struct sss_nss_homedir_ctx);
> + if (homedir_ctx == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> + homedir_ctx->username = attrs->a.user.pw_name;
> + homedir_ctx->uid = attrs->a.user.pw_uid;
> + homedir_ctx->domain = state->dom->name;
> + homedir_ctx->flatname = state->dom->flat_name;
> +
> homedir = expand_homedir_template(state,
> ^
could you remove the the extra whitespace if already changing nearby code?
...
sure, done
> struct ldb_message
*msg,
> struct nss_ctx *nctx,
> struct sss_domain_info *dom,
> - const char *orig_name,
> - uint32_t uid)
> + struct sss_nss_homedir_ctx *homedir_ctx)
> {
> const char *homedir;
> - char *name;
> + const char *orig_name = homedir_ctx->username;
> errno_t ret;
>
> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL);
> + homedir_ctx->original = homedir;
>
> /* Subdomain users store FQDN in their name attribute */
> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL, &name);
> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
> + NULL, &homedir_ctx->username);
> if (ret != EOK) {
> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
> "name-value components.\n", orig_name);
Do we really need orig_name? I think it is just obfuscating code here.
Yes,
orig_name was parameter of function get_homedir_override,
which was converted into "struct sss_nss_homedir_ctx"
I could use "homedir_ctx->username" for two purposes
(input and outtupt argument). It seems strange to me.
ret = sss_parse_name_const(mem_ctx, dom->names, homedir_ctx->username,
NULL, &homedir_ctx->username);
Pavel, thank you for review.
LS
3:31 a.m.
On Tue, 2014-05-27 at 09:32 +0200, Lukas Slebodnik wrote:
On (26/05/14 18:10), Pavel Reichl wrote:
>On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
>> On (22/05/14 17:08), Pavel Reichl wrote:
>> >Sorry Lukas, but the patches do not apply to master.
>> >
>> rebased patches are attached.
>>
>> LS
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel(a)lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>Sorry for broken formatting in prev mail.
>
>Hello Lukas,
>
>I'm having trouble with one of my testing configuration:
>
>sssd is client of ipa which has AD trust.
>
>When I ask on users from AD trust domain specific homedir_substring is
>not used.
>
>Could you have a look?
Thank you for catching this issue.
Function new_subdomain is appropriately updated.
>I have also noticed following nitpicks, could you have a look as well
>
>> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
>> index
c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
>> --- a/src/config/etc/sssd.api.conf
>> +++ b/src/config/etc/sssd.api.conf
>> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
>> pwfield = str, None, false
>> override_homedir = str, None, false
>> fallback_homedir = str, None, false
>> +homedir_substring = str, None, false, /home
>>
>I'm sorry, but I am not aware of the meaning of this file, could you explain to
me?
>And I'm also curious about the '/home' value, as all other lines are
having just 3 values...
>
sh-4.2$ head -n3 src/config/etc/sssd.api.conf
# Format:
# option = type, subtype, mandatory[, default]
>
>> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
>> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
>> index 66b0e59..2698936 100644
>> --- a/src/util/sss_nss.c
>> +++ b/src/util/sss_nss.c
>> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx, const
char *template,
>> homedir_ctx->flatname);
>> break;
>>
>> + case 'H':
>> + if (homedir_ctx->config_homedir_substr == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + ("Cannot expand home directory substring
template "
>> ^
>> + "substring is empty.\n"));
>> ^
>I think that the parenthesis are relic here.
It is lefover after multiple rebases in last few months.
Acctually, it is not problem, but it needn't be here.
I fixed the same issue also in 2nd patch.
>> --- a/src/providers/ipa/ipa_s2n_exop.c
>> +++ b/src/providers/ipa/ipa_s2n_exop.c
>> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct tevent_req
*subreq)
>> struct resp_attrs *simple_attrs = NULL;
>> time_t now;
>> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
>> + struct sss_nss_homedir_ctx *homedir_ctx;
>> const char *homedir = NULL;
>> struct sysdb_attrs *user_attrs = NULL;
>> struct sysdb_attrs *group_attrs = NULL;
>> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct tevent_req
*subreq)
>> switch (attrs->response_type) {
>> case RESP_USER:
>> if (state->dom->subdomain_homedir) {
>> + homedir_ctx = talloc_zero(state, struct sss_nss_homedir_ctx);
>> + if (homedir_ctx == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> + homedir_ctx->username = attrs->a.user.pw_name;
>> + homedir_ctx->uid = attrs->a.user.pw_uid;
>> + homedir_ctx->domain = state->dom->name;
>> + homedir_ctx->flatname = state->dom->flat_name;
>> +
>> homedir = expand_homedir_template(state,
>> ^
>could you remove the the extra whitespace if already changing nearby code?
>...
>
sure, done
>> struct ldb_message *msg,
>> struct nss_ctx *nctx,
>> struct sss_domain_info *dom,
>> - const char *orig_name,
>> - uint32_t uid)
>> + struct sss_nss_homedir_ctx
*homedir_ctx)
>> {
>> const char *homedir;
>> - char *name;
>> + const char *orig_name = homedir_ctx->username;
>> errno_t ret;
>>
>> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL);
>> + homedir_ctx->original = homedir;
>>
>> /* Subdomain users store FQDN in their name attribute */
>> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL, &name);
>> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
>> + NULL, &homedir_ctx->username);
>> if (ret != EOK) {
>> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
>> "name-value components.\n", orig_name);
>Do we really need orig_name? I think it is just obfuscating code here.
Yes,
orig_name was parameter of function get_homedir_override,
which was converted into "struct sss_nss_homedir_ctx"
I could use "homedir_ctx->username" for two purposes
(input and outtupt argument). It seems strange to me.
Yes, it is strange and
it's a potential source of bugs if order of
reading to and writing from would change in implementation of
sss_parse_name(). But hiding this potential issue via orig_name is not
helping anything.
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
3:42 a.m.
On (27/05/14 10:31), Pavel Reichl wrote:
On Tue, 2014-05-27 at 09:32 +0200, Lukas Slebodnik wrote:
> On (26/05/14 18:10), Pavel Reichl wrote:
> >On Fri, 2014-05-23 at 10:09 +0200, Lukas Slebodnik wrote:
> >> On (22/05/14 17:08), Pavel Reichl wrote:
> >> >Sorry Lukas, but the patches do not apply to master.
> >> >
> >> rebased patches are attached.
> >>
> >> LS
> >> _______________________________________________
> >> sssd-devel mailing list
> >> sssd-devel(a)lists.fedorahosted.org
> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> >Sorry for broken formatting in prev mail.
> >
> >Hello Lukas,
> >
> >I'm having trouble with one of my testing configuration:
> >
> >sssd is client of ipa which has AD trust.
> >
> >When I ask on users from AD trust domain specific homedir_substring is
> >not used.
> >
> >Could you have a look?
> Thank you for catching this issue.
> Function new_subdomain is appropriately updated.
>
> >I have also noticed following nitpicks, could you have a look as well
> >
> >> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
> >> index
c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
> >> --- a/src/config/etc/sssd.api.conf
> >> +++ b/src/config/etc/sssd.api.conf
> >> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
> >> pwfield = str, None, false
> >> override_homedir = str, None, false
> >> fallback_homedir = str, None, false
> >> +homedir_substring = str, None, false, /home
> >>
> >I'm sorry, but I am not aware of the meaning of this file, could you explain
to me?
> >And I'm also curious about the '/home' value, as all other lines are
having just 3 values...
> >
> sh-4.2$ head -n3 src/config/etc/sssd.api.conf
> # Format:
> # option = type, subtype, mandatory[, default]
>
> >
> >> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
> >> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> >> index 66b0e59..2698936 100644
> >> --- a/src/util/sss_nss.c
> >> +++ b/src/util/sss_nss.c
> >> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx,
const char *template,
> >> homedir_ctx->flatname);
> >> break;
> >>
> >> + case 'H':
> >> + if (homedir_ctx->config_homedir_substr == NULL) {
> >> + DEBUG(SSSDBG_CRIT_FAILURE,
> >> + ("Cannot expand home directory substring
template "
> >> ^
> >> + "substring is empty.\n"));
> >> ^
> >I think that the parenthesis are relic here.
> It is lefover after multiple rebases in last few months.
> Acctually, it is not problem, but it needn't be here.
> I fixed the same issue also in 2nd patch.
>
> >> --- a/src/providers/ipa/ipa_s2n_exop.c
> >> +++ b/src/providers/ipa/ipa_s2n_exop.c
> >> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct tevent_req
*subreq)
> >> struct resp_attrs *simple_attrs = NULL;
> >> time_t now;
> >> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
> >> + struct sss_nss_homedir_ctx *homedir_ctx;
> >> const char *homedir = NULL;
> >> struct sysdb_attrs *user_attrs = NULL;
> >> struct sysdb_attrs *group_attrs = NULL;
> >> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct tevent_req
*subreq)
> >> switch (attrs->response_type) {
> >> case RESP_USER:
> >> if (state->dom->subdomain_homedir) {
> >> + homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
> >> + if (homedir_ctx == NULL) {
> >> + ret = ENOMEM;
> >> + goto done;
> >> + }
> >> + homedir_ctx->username = attrs->a.user.pw_name;
> >> + homedir_ctx->uid = attrs->a.user.pw_uid;
> >> + homedir_ctx->domain = state->dom->name;
> >> + homedir_ctx->flatname = state->dom->flat_name;
> >> +
> >> homedir = expand_homedir_template(state,
> >> ^
> >could you remove the the extra whitespace if already changing nearby code?
> >...
> >
> sure, done
>
> >> struct ldb_message *msg,
> >> struct nss_ctx *nctx,
> >> struct sss_domain_info *dom,
> >> - const char *orig_name,
> >> - uint32_t uid)
> >> + struct sss_nss_homedir_ctx
*homedir_ctx)
> >> {
> >> const char *homedir;
> >> - char *name;
> >> + const char *orig_name = homedir_ctx->username;
> >> errno_t ret;
> >>
> >> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL);
> >> + homedir_ctx->original = homedir;
> >>
> >> /* Subdomain users store FQDN in their name attribute */
> >> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL,
&name);
> >> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
> >> + NULL, &homedir_ctx->username);
> >> if (ret != EOK) {
> >> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into "
> >> "name-value components.\n", orig_name);
> >Do we really need orig_name? I think it is just obfuscating code here.
> Yes,
>
> orig_name was parameter of function get_homedir_override,
> which was converted into "struct sss_nss_homedir_ctx"
>
> I could use "homedir_ctx->username" for two purposes
> (input and outtupt argument). It seems strange to me.
Yes, it is strange and it's a potential source of bugs if order of
reading to and writing from would change in implementation of
sss_parse_name(). But hiding this potential issue via orig_name is not
helping anything.
Could you ellaborate? What kind of potential bugs?
LS
4:15 a.m.
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(a)lists.fedorahosted.org
>> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> >
>> >Sorry for broken formatting in prev mail.
>> >
>> >Hello Lukas,
>> >
>> >I'm having trouble with one of my testing configuration:
>> >
>> >sssd is client of ipa which has AD trust.
>> >
>> >When I ask on users from AD trust domain specific homedir_substring is
>> >not used.
>> >
>> >Could you have a look?
>> Thank you for catching this issue.
>> Function new_subdomain is appropriately updated.
>>
>> >I have also noticed following nitpicks, could you have a look as well
>> >
>> >> diff --git a/src/config/etc/sssd.api.conf
b/src/config/etc/sssd.api.conf
>> >> index
c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
>> >> --- a/src/config/etc/sssd.api.conf
>> >> +++ b/src/config/etc/sssd.api.conf
>> >> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
>> >> pwfield = str, None, false
>> >> override_homedir = str, None, false
>> >> fallback_homedir = str, None, false
>> >> +homedir_substring = str, None, false, /home
>> >>
>> >I'm sorry, but I am not aware of the meaning of this file, could you
explain to me?
>> >And I'm also curious about the '/home' value, as all other lines
are having just 3 values...
>> >
>> sh-4.2$ head -n3 src/config/etc/sssd.api.conf
>> # Format:
>> # option = type, subtype, mandatory[, default]
>>
>> >
>> >> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
>> >> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
>> >> index 66b0e59..2698936 100644
>> >> --- a/src/util/sss_nss.c
>> >> +++ b/src/util/sss_nss.c
>> >> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX *mem_ctx,
const char *template,
>> >>
homedir_ctx->flatname);
>> >> break;
>> >>
>> >> + case 'H':
>> >> + if (homedir_ctx->config_homedir_substr == NULL) {
>> >> + DEBUG(SSSDBG_CRIT_FAILURE,
>> >> + ("Cannot expand home directory
substring template "
>> >> ^
>> >> + "substring is empty.\n"));
>> >> ^
>> >I think that the parenthesis are relic here.
>> It is lefover after multiple rebases in last few months.
>> Acctually, it is not problem, but it needn't be here.
>> I fixed the same issue also in 2nd patch.
>>
>> >> --- a/src/providers/ipa/ipa_s2n_exop.c
>> >> +++ b/src/providers/ipa/ipa_s2n_exop.c
>> >> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct tevent_req
*subreq)
>> >> struct resp_attrs *simple_attrs = NULL;
>> >> time_t now;
>> >> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout ! */
>> >> + struct sss_nss_homedir_ctx *homedir_ctx;
>> >> const char *homedir = NULL;
>> >> struct sysdb_attrs *user_attrs = NULL;
>> >> struct sysdb_attrs *group_attrs = NULL;
>> >> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
>> >> switch (attrs->response_type) {
>> >> case RESP_USER:
>> >> if (state->dom->subdomain_homedir) {
>> >> + homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
>> >> + if (homedir_ctx == NULL) {
>> >> + ret = ENOMEM;
>> >> + goto done;
>> >> + }
>> >> + homedir_ctx->username = attrs->a.user.pw_name;
>> >> + homedir_ctx->uid = attrs->a.user.pw_uid;
>> >> + homedir_ctx->domain = state->dom->name;
>> >> + homedir_ctx->flatname =
state->dom->flat_name;
>> >> +
>> >> homedir = expand_homedir_template(state,
>> >> ^
>> >could you remove the the extra whitespace if already changing nearby code?
>> >...
>> >
>> sure, done
>>
>> >> struct ldb_message *msg,
>> >> struct nss_ctx *nctx,
>> >> struct sss_domain_info *dom,
>> >> - const char *orig_name,
>> >> - uint32_t uid)
>> >> + struct sss_nss_homedir_ctx
*homedir_ctx)
>> >> {
>> >> const char *homedir;
>> >> - char *name;
>> >> + const char *orig_name = homedir_ctx->username;
>> >> errno_t ret;
>> >>
>> >> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR, NULL);
>> >> + homedir_ctx->original = homedir;
>> >>
>> >> /* Subdomain users store FQDN in their name attribute */
>> >> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL,
&name);
>> >> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
>> >> + NULL, &homedir_ctx->username);
>> >> if (ret != EOK) {
>> >> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s] into
"
>> >> "name-value components.\n", orig_name);
>> >Do we really need orig_name? I think it is just obfuscating code here.
>> Yes,
>>
>> orig_name was parameter of function get_homedir_override,
>> which was converted into "struct sss_nss_homedir_ctx"
>>
>> I could use "homedir_ctx->username" for two purposes
>> (input and outtupt argument). It seems strange to me.
>Yes, it is strange and it's a potential source of bugs if order of
>reading to and writing from would change in implementation of
>sss_parse_name(). But hiding this potential issue via orig_name is not
>helping anything.
Could you ellaborate? What kind of potential bugs?
The fact of passing pointer to
the same area in memory to 2 separate
arguments of sss_parse_name() is what I called potential source of bugs.
You said "It seems strange to me" so I hope you know what I mean.
I'm not saying you can't do it, I just say I don't see any reason why to
hide it with usage of orig_name.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
4:24 a.m.
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(a)lists.fedorahosted.org
> >> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >> >
> >> >Sorry for broken formatting in prev mail.
> >> >
> >> >Hello Lukas,
> >> >
> >> >I'm having trouble with one of my testing configuration:
> >> >
> >> >sssd is client of ipa which has AD trust.
> >> >
> >> >When I ask on users from AD trust domain specific homedir_substring is
> >> >not used.
> >> >
> >> >Could you have a look?
> >> Thank you for catching this issue.
> >> Function new_subdomain is appropriately updated.
> >>
> >> >I have also noticed following nitpicks, could you have a look as well
> >> >
> >> >> diff --git a/src/config/etc/sssd.api.conf
b/src/config/etc/sssd.api.conf
> >> >> index
c7c1232c378d7e641da678d853247ddfd31bb736..5e5a9284e9ad80d822fe1db4269353aeb7925682 100644
> >> >> --- a/src/config/etc/sssd.api.conf
> >> >> +++ b/src/config/etc/sssd.api.conf
> >> >> @@ -36,6 +36,7 @@ filter_users_in_groups = bool, None, false
> >> >> pwfield = str, None, false
> >> >> override_homedir = str, None, false
> >> >> fallback_homedir = str, None, false
> >> >> +homedir_substring = str, None, false, /home
> >> >>
> >> >I'm sorry, but I am not aware of the meaning of this file, could you
explain to me?
> >> >And I'm also curious about the '/home' value, as all other
lines are having just 3 values...
> >> >
> >> sh-4.2$ head -n3 src/config/etc/sssd.api.conf
> >> # Format:
> >> # option = type, subtype, mandatory[, default]
> >>
> >> >
> >> >> static const char *get_shell_override(TALLOC_CTX *mem_ctx,
> >> >> diff --git a/src/util/sss_nss.c b/src/util/sss_nss.c
> >> >> index 66b0e59..2698936 100644
> >> >> --- a/src/util/sss_nss.c
> >> >> +++ b/src/util/sss_nss.c
> >> >> @@ -136,6 +136,17 @@ char *expand_homedir_template(TALLOC_CTX
*mem_ctx, const char *template,
> >> >>
homedir_ctx->flatname);
> >> >> break;
> >> >>
> >> >> + case 'H':
> >> >> + if (homedir_ctx->config_homedir_substr == NULL)
{
> >> >> + DEBUG(SSSDBG_CRIT_FAILURE,
> >> >> + ("Cannot expand home directory
substring template "
> >> >> ^
> >> >> + "substring is empty.\n"));
> >> >> ^
> >> >I think that the parenthesis are relic here.
> >> It is lefover after multiple rebases in last few months.
> >> Acctually, it is not problem, but it needn't be here.
> >> I fixed the same issue also in 2nd patch.
> >>
> >> >> --- a/src/providers/ipa/ipa_s2n_exop.c
> >> >> +++ b/src/providers/ipa/ipa_s2n_exop.c
> >> >> @@ -648,6 +648,7 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
> >> >> struct resp_attrs *simple_attrs = NULL;
> >> >> time_t now;
> >> >> uint64_t timeout = 10*60*60; /* FIXME: find a better timeout !
*/
> >> >> + struct sss_nss_homedir_ctx *homedir_ctx;
> >> >> const char *homedir = NULL;
> >> >> struct sysdb_attrs *user_attrs = NULL;
> >> >> struct sysdb_attrs *group_attrs = NULL;
> >> >> @@ -738,13 +739,19 @@ static void ipa_s2n_get_user_done(struct
tevent_req *subreq)
> >> >> switch (attrs->response_type) {
> >> >> case RESP_USER:
> >> >> if (state->dom->subdomain_homedir) {
> >> >> + homedir_ctx = talloc_zero(state, struct
sss_nss_homedir_ctx);
> >> >> + if (homedir_ctx == NULL) {
> >> >> + ret = ENOMEM;
> >> >> + goto done;
> >> >> + }
> >> >> + homedir_ctx->username =
attrs->a.user.pw_name;
> >> >> + homedir_ctx->uid = attrs->a.user.pw_uid;
> >> >> + homedir_ctx->domain = state->dom->name;
> >> >> + homedir_ctx->flatname =
state->dom->flat_name;
> >> >> +
> >> >> homedir = expand_homedir_template(state,
> >> >> ^
> >> >could you remove the the extra whitespace if already changing nearby
code?
> >> >...
> >> >
> >> sure, done
> >>
> >> >> struct ldb_message *msg,
> >> >> struct nss_ctx *nctx,
> >> >> struct sss_domain_info
*dom,
> >> >> - const char *orig_name,
> >> >> - uint32_t uid)
> >> >> + struct sss_nss_homedir_ctx
*homedir_ctx)
> >> >> {
> >> >> const char *homedir;
> >> >> - char *name;
> >> >> + const char *orig_name = homedir_ctx->username;
> >> >> errno_t ret;
> >> >>
> >> >> homedir = ldb_msg_find_attr_as_string(msg, SYSDB_HOMEDIR,
NULL);
> >> >> + homedir_ctx->original = homedir;
> >> >>
> >> >> /* Subdomain users store FQDN in their name attribute */
> >> >> - ret = sss_parse_name(mem_ctx, dom->names, orig_name, NULL,
&name);
> >> >> + ret = sss_parse_name_const(mem_ctx, dom->names, orig_name,
> >> >> + NULL,
&homedir_ctx->username);
> >> >> if (ret != EOK) {
> >> >> DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse [%s]
into "
> >> >> "name-value components.\n", orig_name);
> >> >Do we really need orig_name? I think it is just obfuscating code here.
> >> Yes,
> >>
> >> orig_name was parameter of function get_homedir_override,
> >> which was converted into "struct sss_nss_homedir_ctx"
> >>
> >> I could use "homedir_ctx->username" for two purposes
> >> (input and outtupt argument). It seems strange to me.
> >Yes, it is strange and it's a potential source of bugs if order of
> >reading to and writing from would change in implementation of
> >sss_parse_name(). But hiding this potential issue via orig_name is not
> >helping anything.
> Could you ellaborate? What kind of potential bugs?
The fact of passing pointer to the same area in memory to 2 separate
arguments of sss_parse_name() is what I called potential source of bugs.
You said "It seems strange to me" so I hope you know what I mean.
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
6:03 a.m.
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
9:29 a.m.
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
9:32 a.m.
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.
11:22 a.m.
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
8:34 a.m.
-----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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlOIiMgACgkQeiVVYja6o6OSpACfYeF7bamazJJP0bXfBhSuN7dy
OgsAoI5NqsmjFcFHdmMUsj9J+0BnMfAm
=VBZ6
-----END PGP SIGNATURE-----
12:23 p.m.
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..
12:26 a.m.
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
3:01 a.m.
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.
10:16 a.m.
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
11:42 a.m.
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
12:15 p.m.
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)
4:29 a.m.
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
5:56 a.m.
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
6:01 a.m.
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
11:01 a.m.
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.
--
Simo Sorce * Red Hat, Inc * New York
11:06 a.m.
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
11:11 a.m.
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.
--
Simo Sorce * Red Hat, Inc * New York
11:15 a.m.
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..
11:18 a.m.
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.
--
Simo Sorce * Red Hat, Inc * New York
11:26 a.m.
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
11:31 a.m.
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 ...
11:27 a.m.
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-...
As usual with SO, not all comments are worth reading, but the topmost
has even some useful links.
11:30 a.m.
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-...
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-t...
LS
3612
days inactive
3795
days old
sssd-devel@lists.fedorahosted.org
40 comments
6 participants
participants (6)
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Pavel Reichl -
Simo Sorce -
Stephen Gallagher