[PATCH] Make the replace-space functionality usable for other responders

[Jakub Hrozek]

Hi,

the attached patches change the replace-space-in-name functionality
after discussion on the list. In short:
    - only space can be substituted
    - for only one character, not a string
    - the option is now set in the [sssd] section because it works for
      any responder

For more details, please see the attached patches.

[Lukas Slebodnik]

On (07/08/14 19:46), Jakub Hrozek wrote:
> Hi,
>
> the attached patches change the replace-space-in-name functionality
> after discussion on the list. In short:
> only space can be substituted
> for only one character, not a string
> the option is now set in the [sssd] section because it works for
> any responder
>
> For more details, please see the attached patches.
> From 4ebeea258ec01cf08bebfa4a5a110320c3c0898d Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 5 Aug 2014 10:12:34 +0200
> Subject: [PATCH 1/6] Only replace space with the specified substitution
>
> https://fedorahosted.org/sssd/ticket/2397
> make sss_replace_whitespaces only replace space (' ') not any
>
> whitespace
> make sss_replace_whitespaces only replace a single char, not the whole
>
> string
> rename CONFDB_NSS_OVERRIDE_DEFAULT_WHITESPACE to
>
> CONFDB_NSS_OVERRIDE_DEFAULT_SPACE
> rename the override_default_whitespace option to override_space
> rename sss_replace_whitespaces() to sss_replace_space()
> rename sss_reverse_replace_whitespaces() to sss_reverse_replace_space()
> rename nctx->override_default_wsp_str to nctx->override_space
> make the return value of sss_replace_space non-const to avoid freeing
> the result without compilation warnings

//snip
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -566,11 +566,11 @@ errno_t name_to_well_known_sid(const char *dom, const char *name,
>                                const char **sid);
>
> /* from string_utils.c */
> -const char * sss_replace_whitespaces(TALLOC_CTX *mem_ctx,
>                                 const char *orig_name,
>                                 const char *replace_string);
>
> -char * sss_reverse_replace_whitespaces(TALLOC_CTX *mem_ctx,
>                                   char *orig_name,
>                                   const char *replace_string);
>
> +char * sss_replace_space(TALLOC_CTX *mem_ctx,
>                     const char *orig_name,
>                     const char *replace_string);
>
> +char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
>                             char *orig_name,
>                             const char *replace_string);
>
> #endif /* __SSSD_UTIL_H__ */
>
> 1.9.3

I don't know why did you change the same lines twice:
    1st time in this patch: functions werere renamed.
    2nt time in next patch "const char *replace_string" was replaced with
    character.
In my opinion, it can be done in one patch.
> From ba2d408f14de29b2b1357998fb3df812e264c453 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Thu, 7 Aug 2014 15:28:11 +0200
> Subject: [PATCH 2/6] Make the space override responder-agnostic
>
> https://fedorahosted.org/sssd/ticket/2397
>
> In order to make the override_space option usable by other responders,
> we need to move the override_space option to the generic responder
> structure.
>
> src/confdb/confdb.h                     |  2 +-
> src/responder/common/responder.h        |  1 +
> src/responder/common/responder_common.c | 15 +++++++
> src/responder/nss/nsssrv.c              |  5 ---
> src/responder/nss/nsssrv.h              |  1 -
> src/responder/nss/nsssrv_cmd.c          | 17 +++++---
> src/tests/cmocka/test_nss_srv.c         |  1 +
> src/tests/cmocka/test_string_utils.c    | 76 ++++++++++++++++-----------------
> src/util/string_utils.c                 | 12 +++---
> src/util/util.h                         |  4 +-
> 10 files changed, 74 insertions(+), 60 deletions(-)
>
> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> index d5f7511d0cfe916242b5bfae506271e028a87edb..9160debd6f4b5a35ac94ec29dc863ed80a0e337e 100644
> --- a/src/confdb/confdb.h
> +++ b/src/confdb/confdb.h
> @@ -68,6 +68,7 @@
> #define CONFDB_MONITOR_TRY_INOTIFY "try_inotify"
> #define CONFDB_MONITOR_KRB5_RCACHEDIR "krb5_rcache_dir"
> #define CONFDB_MONITOR_DEFAULT_DOMAIN "default_domain_suffix"
> +#define CONFDB_MONITOR_OVERRIDE_SPACE "override_default_space"
>
> /* Both monitor and domains */
> #define CONFDB_NAME_REGEX   "re_expression"
> @@ -99,7 +100,6 @@
> #define CONFDB_MEMCACHE_TIMEOUT "memcache_timeout"
> #define CONFDB_NSS_HOMEDIR_SUBSTRING "homedir_substring"
> #define CONFDB_DEFAULT_HOMEDIR_SUBSTRING "/home"
> -#define CONFDB_NSS_OVERRIDE_SPACE "override_space"
>
> /* PAM */
> #define CONFDB_PAM_CONF_ENTRY "config/pam"
> diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
> index 167f45cb156a189deb996d28f8165529535607de..3674d13f2303d0ce248f765a638aaa83d0c16cf3 100644
> --- a/src/responder/common/responder.h
> +++ b/src/responder/common/responder.h
> @@ -102,6 +102,7 @@ struct resp_ctx {
>     uid_t *allowed_uids;
> char *default_domain;
>
> char override_space;
>
> void *pvt_ctx;
>
> diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
> index 2f9db70ce22ef224e744cad80d89639340ec1a79..3afeb3cc6cb3a9fc4bd72864d45a0d0bb0bea469 100644
> --- a/src/responder/common/responder_common.c
> +++ b/src/responder/common/responder_common.c
> @@ -781,6 +781,7 @@ int sss_process_init(TALLOC_CTX *mem_ctx,
>     struct resp_ctx *rctx;
>     struct sss_domain_info *dom;
>     int ret;
>
> char *tmp = NULL;
>
> rctx = talloc_zero(mem_ctx, struct resp_ctx);
> if (!rctx) {
>
> @@ -844,6 +845,20 @@ int sss_process_init(TALLOC_CTX *mem_ctx,
>         goto fail;
>     }
> ret = confdb_get_string(rctx->cdb, rctx, CONFDB_MONITOR_CONF_ENTRY,
>                        CONFDB_MONITOR_OVERRIDE_SPACE, NULL,
>                        &tmp);
> if (ret != EOK) {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "Cannnot get the space substitution character [%d]: %s\n",
>           ret, strerror(ret));
>    goto fail;
> }
> if (tmp != NULL) {
>    rctx->override_space = tmp[0];
> }

It would be good to write debug message if size of string is longer that 1.
> ret = sss_monitor_init(rctx, rctx->ev, monitor_intf,
>                        svc_name, svc_version, rctx,
>                        &rctx->mon_conn);
>
> diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> index 9705878fa80d33d3bddfe7be7fcc97151b44a0f0..84a6b7fedd096a7d4159b7ac6670820c1d8fd941 100644
> --- a/src/responder/nss/nsssrv.c
> +++ b/src/responder/nss/nsssrv.c
> @@ -298,11 +298,6 @@ static int nss_get_config(struct nss_ctx *nctx,
>                             &nctx->homedir_substr);
>     if (ret != EOK) goto done;
> ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
>                        CONFDB_NSS_OVERRIDE_SPACE, NULL,
>                        &nctx->override_space);
> if (ret != EOK) goto done;
> ret = 0;
>
> done:
>     return ret;
> diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
> index f5238fb822ecc376d8dca06c00fb989276949688..a5b946b7e4a38d7d8b35ec5df1b6644d01896470 100644
> --- a/src/responder/nss/nsssrv.h
> +++ b/src/responder/nss/nsssrv.h
> @@ -69,7 +69,6 @@ struct nss_ctx {
>     char **etc_shells;
>     char *shell_fallback;
>     char *default_shell;
>
> char *override_space;
>
> struct sss_mc_ctx *pwd_mc_ctx;
> struct sss_mc_ctx *grp_mc_ctx;
>
> diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> index 8e9a7dbaf22af995646738f18ccad6168c10b29e..d92882564e3f59e9927011f1797a1948553f9423 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -372,7 +372,8 @@ static int fill_pwent(struct sss_packet *packet,
>             continue;
>         }
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr, nctx->override_space);
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr,
>                               nctx->rctx->override_space);
>   if (tmpstr == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_replace_space failed, skipping\n");
>
> @@ -752,7 +753,7 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx)
>         if (!name) return ENOMEM;
>     name = sss_reverse_replace_space(dctx, name,
>                                     nctx->override_space);
>                                     nctx->rctx->override_space);
>   if (name == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_reverse_replace_space failed\n");
>
> @@ -2360,7 +2361,7 @@ static int fill_members(struct sss_packet *packet,
>             continue;
>         }
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr, nctx->override_space);
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr, nctx->rctx->override_space);
>   if (tmpstr == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_replace_space failed\n");
>
> @@ -2523,7 +2524,7 @@ static int fill_grent(struct sss_packet *packet,
>             continue;
>         }
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr, nctx->override_space);
>    tmpstr = sss_replace_space(tmp_ctx, tmpstr, nctx->rctx->override_space);
>   if (tmpstr == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_replace_space failed, skipping\n");
>
> @@ -2724,7 +2725,8 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx)
>         name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>         if (!name) return ENOMEM;
>    name = sss_reverse_replace_space(dctx, name, nctx->override_space);
>    name = sss_reverse_replace_space(dctx, name,
>                                     nctx->rctx->override_space);
>   if (name == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_reverse_replace_space failed\n");
>
> @@ -3754,7 +3756,8 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
>         name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>         if (!name) return ENOMEM;
>    name = sss_reverse_replace_space(dctx, name, nctx->override_space);
>    name = sss_reverse_replace_space(dctx, name,
>                                     nctx->rctx->override_space);
>   if (name == NULL) {
>       DEBUG(SSSDBG_CRIT_FAILURE,
>             "sss_reverse_replace_space failed\n");
>
> @@ -3921,7 +3924,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>             }
>         name = sss_reverse_replace_space(dctx, name,
>                                         nctx->override_space);
>                                         nctx->rctx->override_space);
>       if (name == NULL) {
>           DEBUG(SSSDBG_CRIT_FAILURE,
>                 "sss_reverse_replace_space failed\n");
>
> diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
> index 808a7a1854cb80b6ffb3f84f26144083e8599bb5..f5d1f60ec62056b42c52789f27a38eebdf362657 100644
> --- a/src/tests/cmocka/test_nss_srv.c
> +++ b/src/tests/cmocka/test_nss_srv.c
> @@ -889,6 +889,7 @@ void test_nss_setup(struct sss_test_conf_param params[],
>     nss_test_ctx->rctx = mock_rctx(nss_test_ctx, nss_test_ctx->tctx->ev,
>                                    nss_test_ctx->tctx->dom, nss_test_ctx->nctx);
>     assert_non_null(nss_test_ctx->rctx);
>
> nss_test_ctx->nctx->rctx = nss_test_ctx->rctx;
>
> /* Create client context */
> nss_test_ctx->cctx = mock_cctx(nss_test_ctx, nss_test_ctx->rctx);
>
> diff --git a/src/tests/cmocka/test_string_utils.c b/src/tests/cmocka/test_string_utils.c
> index dda9fac80be32b2dbbee8b365ca2f82c475a3b30..a52b9bb2180e5eb103d0a5cff4d5f939261b5414 100644
> --- a/src/tests/cmocka/test_string_utils.c
> +++ b/src/tests/cmocka/test_string_utils.c
> @@ -31,34 +31,34 @@ void test_replace_whitespaces(void **state)
>     struct {
>         const char *input;
>         const char *output;
>    const char *replace_string;
>    const char replace_string;

s/string/character/
> } data_set[] = {
>    { "", "", "-" },
>    { " ", "-", "-" },
>    { "abcd", "abcd", "-" },
>    { "a b c d", "a-b-c-d", "-" },
>    { " a b c d ", "-a-b-c-d-", "-" },
>    { " ", "^", "^" },
>    { "abcd", "abcd", "^" },
>    { "a b c d", "a^b^c^d", "^" },
>    { " a b c d ", "^a^b^c^d^", "^" },
>    { " ", "^", "^" },
>    { " ", " ", " " },
>    { "    ", "    ", " " },
>    { "abcd", "abcd", " " },
>    { "a b c d", "a b c d", " " },
>    { NULL, NULL, NULL },
>    { "", "", '-' },
>    { " ", "-", '-' },
>    { "abcd", "abcd", '-' },
>    { "a b c d", "a-b-c-d", '-' },
>    { " a b c d ", "-a-b-c-d-", '-' },
>    { " ", "^", '^' },
>    { "abcd", "abcd", '^' },
>    { "a b c d", "a^b^c^d", '^' },
>    { " a b c d ", "^a^b^c^d^", '^' },
>    { " ", "^", '^' },
>    { " ", " ", ' ' },
>    { "    ", "    ", ' ' },
>    { "abcd", "abcd", ' ' },
>    { "a b c d", "a b c d", ' ' },
>    { NULL, NULL, '\0' },
>
> };
>
> mem_ctx = talloc_new(NULL);
>   assert_non_null(mem_ctx);
>   check_leaks_push(mem_ctx);
> res = sss_replace_space(mem_ctx, input_str, NULL);
> res = sss_replace_space(mem_ctx, input_str, '\0');
> assert_string_equal(res, input_str);
> talloc_zfree(res);
> res = sss_replace_space(mem_ctx, input_str, "");
> res = sss_replace_space(mem_ctx, input_str, '\0');
> assert_string_equal(res, input_str);
> talloc_zfree(res);
>
> @@ -84,36 +84,36 @@ void test_reverse_replace_whitespaces(void **state)
>     struct {
>         const char *input;
>         const char *output;
>    const char *replace_string;
>    const char replace_string;

s/string/character/
> } data_set[] = {
>    { "", "", "-" },
>    { "-", " ", "-" },
>    { "----", "    ", "-" },
>    { "abcd", "abcd", "-" },
>    { "a-b-c-d", "a b c d", "-" },
>    { "-a-b-c-d-", " a b c d ", "-" },
>    { "^", " ", "^" },
>    { "^^^^", "    ", "^" },
>    { "abcd", "abcd", "^" },
>    { "a^b^c^d", "a b c d", "^" },
>    { "^a^b^c^d^", " a b c d ", "^" },
>    { " ", " ", " " },
>    { "    ", "    ", " " },
>    { "abcd", "abcd", " " },
>    { "a b c d", "a b c d", " " },
>    { " a b c d ", " a b c d ", " " },
>    { NULL, NULL, NULL },
>    { "", "", '-' },
>    { "-", " ", '-' },
>    { "----", "    ", '-' },
>    { "abcd", "abcd", '-' },
>    { "a-b-c-d", "a b c d", '-' },
>    { "-a-b-c-d-", " a b c d ", '-' },
>    { "^", " ", '^' },
>    { "^^^^", "    ", '^' },
>    { "abcd", "abcd", '^' },
>    { "a^b^c^d", "a b c d", '^' },
>    { "^a^b^c^d^", " a b c d ", '^' },
>    { " ", " ", ' ' },
>    { "    ", "    ", ' ' },
>    { "abcd", "abcd", ' ' },
>    { "a b c d", "a b c d", ' ' },
>    { " a b c d ", " a b c d ", ' ' },
>    { NULL, NULL, '\0' },
>
> };
>
> mem_ctx = talloc_new(NULL);
>   assert_non_null(mem_ctx);
>   check_leaks_push(mem_ctx);
> res = sss_reverse_replace_space(mem_ctx, input_str, NULL);
> res = sss_reverse_replace_space(mem_ctx, input_str, '\0');
> assert_string_equal(res, input_str);
> talloc_free(res);
> res = sss_reverse_replace_space(mem_ctx, input_str, "");
> res = sss_reverse_replace_space(mem_ctx, input_str, '\0');
> assert_string_equal(res, input_str);
> talloc_free(res);
>
> diff --git a/src/util/string_utils.c b/src/util/string_utils.c
> index d639c6de97275e53e019b60c8578f33ca22ec37f..ec4cc687e6652ee58e963869f7da3bd5f4cec2da 100644
> --- a/src/util/string_utils.c
> +++ b/src/util/string_utils.c
> @@ -46,20 +46,20 @@ static char *replace_char(TALLOC_CTX *mem_ctx,
>
> char * sss_replace_space(TALLOC_CTX *mem_ctx,
>                          const char *orig_name,
>                     const char *replace_string)
>                     const char subst)
>
> {
> if (replace_string == NULL || replace_string[0] == '\0') {
> if (subst == '\0') {
>    return talloc_strdup(mem_ctx, orig_name);
> }
> return replace_char(mem_ctx, orig_name, ' ', replace_string[0]);
> return replace_char(mem_ctx, orig_name, ' ', subst);
>
> }
>
> char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
>                                  char *orig_name,
>                             const char *replace_string)
>                             const char subst)
>
> {
> if (replace_string == NULL || replace_string[0] == '\0') {
> if (subst == '\0') {
>    return talloc_strdup(mem_ctx, orig_name);
> }
> return replace_char(mem_ctx, orig_name, replace_string[0], ' ');
> return replace_char(mem_ctx, orig_name, subst, ' ');
>
> }
> diff --git a/src/util/util.h b/src/util/util.h
> index c58e3fde62928b0f6b43698b8e20c058fd856df1..ca740d0163939f237dff0f80c5c8ab2ed183d07f 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -568,9 +568,9 @@ errno_t name_to_well_known_sid(const char *dom, const char *name,
> /* from string_utils.c */
> char * sss_replace_space(TALLOC_CTX *mem_ctx,
>                          const char *orig_name,
>                     const char *replace_string);
>                     const char replace_char);
>
> char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
>                                  char *orig_name,
>                             const char *replace_string);
>                             const char replace_char);
>
> #endif /* __SSSD_UTIL_H__ */
>
> 1.9.3

LS

[Jakub Hrozek]

On Fri, Aug 08, 2014 at 10:48:11AM +0200, Lukas Slebodnik wrote:
> On (07/08/14 19:46), Jakub Hrozek wrote:
> > Hi,
> >
> > the attached patches change the replace-space-in-name functionality
> > after discussion on the list. In short:
> > only space can be substituted
> > for only one character, not a string
> > the option is now set in the [sssd] section because it works for
> > any responder
> >
> > For more details, please see the attached patches.
> > From 4ebeea258ec01cf08bebfa4a5a110320c3c0898d Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 5 Aug 2014 10:12:34 +0200
> > Subject: [PATCH 1/6] Only replace space with the specified substitution
> >
> > https://fedorahosted.org/sssd/ticket/2397
> > make sss_replace_whitespaces only replace space (' ') not any
> >
> > whitespace
> > make sss_replace_whitespaces only replace a single char, not the whole
> >
> > string
> > rename CONFDB_NSS_OVERRIDE_DEFAULT_WHITESPACE to
> >
> > CONFDB_NSS_OVERRIDE_DEFAULT_SPACE
> > rename the override_default_whitespace option to override_space
> > rename sss_replace_whitespaces() to sss_replace_space()
> > rename sss_reverse_replace_whitespaces() to sss_reverse_replace_space()
> > rename nctx->override_default_wsp_str to nctx->override_space
> > make the return value of sss_replace_space non-const to avoid freeing
> > the result without compilation warnings
>
> //snip
> > --- a/src/util/util.h
> > +++ b/src/util/util.h
> > @@ -566,11 +566,11 @@ errno_t name_to_well_known_sid(const char *dom, const char *name,
> >                                const char **sid);
> >
> > /* from string_utils.c */
> > -const char * sss_replace_whitespaces(TALLOC_CTX *mem_ctx,
> >                                 const char *orig_name,
> >                                 const char *replace_string);
> >
> > -char * sss_reverse_replace_whitespaces(TALLOC_CTX *mem_ctx,
> >                                   char *orig_name,
> >                                   const char *replace_string);
> >
> > +char * sss_replace_space(TALLOC_CTX *mem_ctx,
> >                     const char *orig_name,
> >                     const char *replace_string);
> >
> > +char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
> >                             char *orig_name,
> >                             const char *replace_string);
> >
> > #endif /* __SSSD_UTIL_H__ */
> >
> > 1.9.3
>
> I don't know why did you change the same lines twice:
>     1st time in this patch: functions werere renamed.
>     2nt time in next patch "const char *replace_string" was replaced with
>     character.
> In my opinion, it can be done in one patch.

Done as well as all the other changes you requested. Please see the
attached patches.

[Lukas Slebodnik]

On (10/08/14 19:11), Jakub Hrozek wrote:
> On Fri, Aug 08, 2014 at 10:48:11AM +0200, Lukas Slebodnik wrote:
> > On (07/08/14 19:46), Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patches change the replace-space-in-name functionality
> > > after discussion on the list. In short:
> > > only space can be substituted
> > > for only one character, not a string
> > > the option is now set in the [sssd] section because it works for
> > > any responder
> > >
> > > For more details, please see the attached patches.
> > > From 4ebeea258ec01cf08bebfa4a5a110320c3c0898d Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 5 Aug 2014 10:12:34 +0200
> > > Subject: [PATCH 1/6] Only replace space with the specified substitution
> > >
> > > https://fedorahosted.org/sssd/ticket/2397
> > > make sss_replace_whitespaces only replace space (' ') not any
> > >
> > > whitespace
> > > make sss_replace_whitespaces only replace a single char, not the whole
> > >
> > > string
> > > rename CONFDB_NSS_OVERRIDE_DEFAULT_WHITESPACE to
> > >
> > > CONFDB_NSS_OVERRIDE_DEFAULT_SPACE
> > > rename the override_default_whitespace option to override_space
> > > rename sss_replace_whitespaces() to sss_replace_space()
> > > rename sss_reverse_replace_whitespaces() to sss_reverse_replace_space()
> > > rename nctx->override_default_wsp_str to nctx->override_space
> > > make the return value of sss_replace_space non-const to avoid freeing
> > > the result without compilation warnings
> >
> > //snip
> > > --- a/src/util/util.h
> > > +++ b/src/util/util.h
> > > @@ -566,11 +566,11 @@ errno_t name_to_well_known_sid(const char *dom, const char *name,
> > >                                const char **sid);
> > >
> > > /* from string_utils.c */
> > > -const char * sss_replace_whitespaces(TALLOC_CTX *mem_ctx,
> > >                                 const char *orig_name,
> > >                                 const char *replace_string);
> > >
> > > -char * sss_reverse_replace_whitespaces(TALLOC_CTX *mem_ctx,
> > >                                   char *orig_name,
> > >                                   const char *replace_string);
> > >
> > > +char * sss_replace_space(TALLOC_CTX *mem_ctx,
> > >                     const char *orig_name,
> > >                     const char *replace_string);
> > >
> > > +char * sss_reverse_replace_space(TALLOC_CTX *mem_ctx,
> > >                             char *orig_name,
> > >                             const char *replace_string);
> > >
> > > #endif /* __SSSD_UTIL_H__ */
> > >
> > > 1.9.3
> >
> > I don't know why did you change the same lines twice:
> >     1st time in this patch: functions werere renamed.
> >     2nt time in next patch "const char *replace_string" was replaced with
> >     character.
> > In my opinion, it can be done in one patch.
>
> Done as well as all the other changes you requested. Please see the
> attached patches.
> From 830aff639d7cfe859b977486ca41baa0ae613e0c Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Fri, 1 Aug 2014 07:31:39 +0200
> Subject: [PATCH 4/6] IFP: Use the override_default_space option
>
> https://fedorahosted.org/sssd/ticket/2397
>
> The input of the InfoPipe responder substitutes the configured character
> for space and the GetUserAttrs and GetUserGroups functions substitute
> space for the configured character in their output.

In my opinion, it is not necessary to replace spaces in infopipe responder.
dbus utilities already returned quoted strings.
# gdbus call --system --dest org.freedesktop.sssd.infopipe --object-path /org/freedesktop/sssd/infopipe --method
org.freedesktop.sssd.infopipe.GetUserGroups 'usersssd25'
(['biggroup 1', 'small group 1', 'big group 2'],)

There will not be such problem with parsing output as with other
commands: id, groups ...
> src/responder/ifp/ifpsrv_cmd.c | 53 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
> index 50483933188cfe9cf1a855bf3505f7e24a91b975..15ecaef65fb865b98d16566aa35c79c4b29ea96c 100644
> --- a/src/responder/ifp/ifpsrv_cmd.c
> +++ b/src/responder/ifp/ifpsrv_cmd.c
> @@ -174,6 +174,26 @@ static void ifp_user_get_attr_process(struct tevent_req *req)
> }
>
> static errno_t
> +ifp_user_get_attr_replace_space(TALLOC_CTX *mem_ctx,
>                            struct ldb_message_element *el,
>                            const char sub)
>
> +{
> int i;
> for (i = 0; i < el->num_values; i++) {
>    el->values[i].data = (uint8_t *) sss_replace_space(mem_ctx,
>                                         (const char *) el->values[i].data,
>                                         sub);
>    if (el->values[i].data == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "sss_replace_space failed, skipping\n");
>        return ENOMEM;
>    }
> }
> return EOK;
>
> +}
>
> +static errno_t
> ifp_user_get_attr_handle_reply(struct ifp_req *ireq,
>                                const char **attrs, struct ldb_result *res)
> {
> @@ -214,6 +234,18 @@ ifp_user_get_attr_handle_reply(struct ifp_req *ireq,
>                 continue;
>             }
>        /* Normalize white space in user names */
>        if (ireq->ifp_ctx->rctx->override_space != '\0' &&
>                strcmp(attrs[ai], SYSDB_NAME) == 0) {
>            ret = ifp_user_get_attr_replace_space(ireq, el,
>                                    ireq->ifp_ctx->rctx->override_space);
>            if (ret != EOK) {
>                DEBUG(SSSDBG_MINOR_FAILURE, "Cannot normalize %s\n",
>                      attrs[ai]);
>                continue;
>            }
>        }
>       ret = ifp_add_ldb_el_to_dict(&iter_dict, el);
>       if (ret != EOK) {
>           DEBUG(SSSDBG_MINOR_FAILURE,
>
> @@ -343,8 +375,17 @@ ifp_user_get_groups_reply(struct ifp_req *ireq, struct ldb_result *res)
>             continue;
>         }
>    DEBUG(SSSDBG_TRACE_FUNC, "Adding group %s\n", name);
>    groupnames[i] = name;
>    if (ireq->ifp_ctx->rctx->override_space != '\0') {
>        groupnames[i] = sss_replace_space(groupnames, name,
>                                      ireq->ifp_ctx->rctx->override_space);
>        if (groupnames[i] == NULL) {
>            DEBUG(SSSDBG_MINOR_FAILURE, "Cannot normalize %s\n", name);
>            continue;
>        }
>    } else {
>        groupnames[i] = name;
>    }
>    DEBUG(SSSDBG_TRACE_FUNC, "Adding group %s\n", groupnames[i]);
>
> }
>
> return infopipe_iface_GetUserGroups_finish(ireq->dbus_req,
>
> @@ -495,6 +536,14 @@ static errno_t ifp_user_get_attr_search(struct tevent_req *req)
>         name = sss_get_cased_name(state, state->name, dom->case_sensitive);
>         if (!name) return ENOMEM;
>    state->name = sss_reverse_replace_space(state, name,
>                                     state->rctx->override_space);

^^^^^^^^^^^
    sanitized string is sored in variable state->name, but variable 'name'
    is tested for NULL and used further incode.
>    if (name == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "sss_reverse_replace_space failed\n");
>        return ENOMEM;
>    }
>   /* verify this user has not yet been negatively cached,
>    * or has been permanently filtered */
>   ret = sss_ncache_check_user(state->ncache,

It is your decision. You can:
    a) fix problem in function ifp_user_get_attr_search
    b) remove changes in IFP responder.

LS

[Jakub Hrozek]

On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> It is your decision. You can:
>     a) fix problem in function ifp_user_get_attr_search
>     b) remove changes in IFP responder.
>
> LS

Thanks for the quick review. I would prefer to keep the ifp changes in
-- I don't realistically expect anyone being bitten by spaces when using
InfoPipe, but it's more about being consistent. When we say we want to
replace the spaces, we should replace them everywhere..

[Pavel Březina]

On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
> On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> > It is your decision. You can:
> >      a) fix problem in function ifp_user_get_attr_search
> >      b) remove changes in IFP responder.
> >
> > LS
>
> Thanks for the quick review. I would prefer to keep the ifp changes in
> -- I don't realistically expect anyone being bitten by spaces when using
> InfoPipe, but it's more about being consistent. When we say we want to
> replace the spaces, we should replace them everywhere..

Patch 1:
Ack.

Patch 2:
Man page says "override_space", but you renamed the option to 
"override_default_space". Actually I like the first name better, can you 
switch it back? Or what is the reason for using "default" word?

Patch 3:
Ack.

However, I'm not really sure it is a good idea to allow both logins. 
Wouldn't it be better to allow only one depending whether the option is 
set or not?

Patch 4:
Ack.

Patch 5:
Ack.

Path 6:
Ack.

The code is good, however, I still see the problem that the 
transformation is not symmetrical. If an input contains the replacement 
character, we cannot perform reverse transformation correctly. Either we 
have to very loudly warn that the character must be unused all over the 
database or fix it.

For example I have two users: user-1, "user 4" and I choose '-' as 
replacement character. user-1 is not able to login.

[Michal Židek]

On 08/11/2014 02:25 PM, Pavel Březina wrote:
> On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
> > On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> > > It is your decision. You can:
> > >      a) fix problem in function ifp_user_get_attr_search
> > >      b) remove changes in IFP responder.
> > >
> > > LS
> >
> > Thanks for the quick review. I would prefer to keep the ifp changes in
> > -- I don't realistically expect anyone being bitten by spaces when using
> > InfoPipe, but it's more about being consistent. When we say we want to
> > replace the spaces, we should replace them everywhere..
>
> Patch 1:
> Ack.
>
> Patch 2:
> Man page says "override_space", but you renamed the option to
> "override_default_space". Actually I like the first name better, can you
> switch it back? Or what is the reason for using "default" word?
>
> Patch 3:
> Ack.
>
> However, I'm not really sure it is a good idea to allow both logins.
> Wouldn't it be better to allow only one depending whether the option is
> set or not?
>
> Patch 4:
> Ack.
>
> Patch 5:
> Ack.
>
> Path 6:
> Ack.
>
> The code is good, however, I still see the problem that the
> transformation is not symmetrical. If an input contains the replacement
> character, we cannot perform reverse transformation correctly. Either we
> have to very loudly warn that the character must be unused all over the
> database or fix it.

We should properly document that the character can not be used
in the database.
There is no way to properly "fix" this issue. For example there
is no way to fix the following:

Space replacement character is "-".
Two users: "pirate-king luffy"
            "pirate king-luffy"
Then you try to authenticate as "pirate-king-luffy".
Which user are you trying to authenticate as?

So, if the replacement character is used in the database
it is a configuration bug IMO.
> For example I have two users: user-1, "user 4" and I choose '-' as
> replacement character. user-1 is not able to login.

[Lukas Slebodnik]

On (11/08/14 16:21), Michal Židek wrote:
> On 08/11/2014 02:25 PM, Pavel Březina wrote:
> > On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
> > > On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> > > > It is your decision. You can:
> > > >     a) fix problem in function ifp_user_get_attr_search
> > > >     b) remove changes in IFP responder.
> > > >
> > > > LS
> > >
> > > Thanks for the quick review. I would prefer to keep the ifp changes in
> > > -- I don't realistically expect anyone being bitten by spaces when using
> > > InfoPipe, but it's more about being consistent. When we say we want to
> > > replace the spaces, we should replace them everywhere..
> >
> > Patch 1:
> > Ack.
> >
> > Patch 2:
> > Man page says "override_space", but you renamed the option to
> > "override_default_space". Actually I like the first name better, can you
> > switch it back? Or what is the reason for using "default" word?
> >
> > Patch 3:
> > Ack.
> >
> > However, I'm not really sure it is a good idea to allow both logins.
> > Wouldn't it be better to allow only one depending whether the option is
> > set or not?
> >
> > Patch 4:
> > Ack.
> >
> > Patch 5:
> > Ack.
> >
> > Path 6:
> > Ack.
> >
> > The code is good, however, I still see the problem that the
> > transformation is not symmetrical. If an input contains the replacement
> > character, we cannot perform reverse transformation correctly. Either we
> > have to very loudly warn that the character must be unused all over the
> > database or fix it.
>
> We should properly document that the character can not be used
> in the database.
> There is no way to properly "fix" this issue. For example there
> is no way to fix the following:

+1

LS

[Jakub Hrozek]

On Mon, Aug 11, 2014 at 05:21:27PM +0200, Lukas Slebodnik wrote:
> On (11/08/14 16:21), Michal Židek wrote:
> > On 08/11/2014 02:25 PM, Pavel Březina wrote:
> > > On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
> > > > On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> > > > > It is your decision. You can:
> > > > >     a) fix problem in function ifp_user_get_attr_search
> > > > >     b) remove changes in IFP responder.
> > > > >
> > > > > LS
> > > >
> > > > Thanks for the quick review. I would prefer to keep the ifp changes in
> > > > -- I don't realistically expect anyone being bitten by spaces when using
> > > > InfoPipe, but it's more about being consistent. When we say we want to
> > > > replace the spaces, we should replace them everywhere..
> > >
> > > Patch 1:
> > > Ack.
> > >
> > > Patch 2:
> > > Man page says "override_space", but you renamed the option to
> > > "override_default_space". Actually I like the first name better, can you
> > > switch it back? Or what is the reason for using "default" word?

That was a mistake that crept in when I was reshuffling the patches
about Lukas' changes. Thanks for catching it!
> > > Patch 3:
> > > Ack.
> > >
> > > However, I'm not really sure it is a good idea to allow both logins.
> > > Wouldn't it be better to allow only one depending whether the option is
> > > set or not?

I don't see the benefit, sorry.
> > > Patch 4:
> > > Ack.
> > >
> > > Patch 5:
> > > Ack.
> > >
> > > Path 6:
> > > Ack.
> > >
> > > The code is good, however, I still see the problem that the
> > > transformation is not symmetrical. If an input contains the replacement
> > > character, we cannot perform reverse transformation correctly. Either we
> > > have to very loudly warn that the character must be unused all over the
> > > database or fix it.
> >
> > We should properly document that the character can not be used
> > in the database.
> > There is no way to properly "fix" this issue. For example there
> > is no way to fix the following:
>
> +1
>
> LS

I agree as well and I amended the man page hunk in the second patch.

Thank you for the review.

[Pavel Březina]

On 08/13/2014 01:34 PM, Jakub Hrozek wrote:
> On Mon, Aug 11, 2014 at 05:21:27PM +0200, Lukas Slebodnik wrote:
> > On (11/08/14 16:21), Michal Židek wrote:
> > > On 08/11/2014 02:25 PM, Pavel Březina wrote:
> > > > On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
> > > > > On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
> > > > > > It is your decision. You can:
> > > > > >      a) fix problem in function ifp_user_get_attr_search
> > > > > >      b) remove changes in IFP responder.
> > > > > >
> > > > > > LS
> > > > >
> > > > > Thanks for the quick review. I would prefer to keep the ifp changes in
> > > > > -- I don't realistically expect anyone being bitten by spaces when using
> > > > > InfoPipe, but it's more about being consistent. When we say we want to
> > > > > replace the spaces, we should replace them everywhere..
> > > >
> > > > Patch 1:
> > > > Ack.
> > > >
> > > > Patch 2:
> > > > Man page says "override_space", but you renamed the option to
> > > > "override_default_space". Actually I like the first name better, can you
> > > > switch it back? Or what is the reason for using "default" word?
>
> That was a mistake that crept in when I was reshuffling the patches
> about Lukas' changes. Thanks for catching it!
> > > > Patch 3:
> > > > Ack.
> > > >
> > > > However, I'm not really sure it is a good idea to allow both logins.
> > > > Wouldn't it be better to allow only one depending whether the option is
> > > > set or not?
>
> I don't see the benefit, sorry.
> > > > Patch 4:
> > > > Ack.
> > > >
> > > > Patch 5:
> > > > Ack.
> > > >
> > > > Path 6:
> > > > Ack.
> > > >
> > > > The code is good, however, I still see the problem that the
> > > > transformation is not symmetrical. If an input contains the replacement
> > > > character, we cannot perform reverse transformation correctly. Either we
> > > > have to very loudly warn that the character must be unused all over the
> > > > database or fix it.
> > >
> > > We should properly document that the character can not be used
> > > in the database.
> > > There is no way to properly "fix" this issue. For example there
> > > is no way to fix the following:
> >
> > +1
> >
> > LS
>
> I agree as well and I amended the man page hunk in the second patch.
>
> Thank you for the review.

Ok, I'm fine with that. Ack.

Just fix the option name also in commit message prior pushing please.

[Jakub Hrozek]

On Wed, Aug 13, 2014 at 02:41:49PM +0200, Pavel Březina wrote:
> Ok, I'm fine with that. Ack.
>
> Just fix the option name also in commit message prior pushing please.

Done and pushed to master:
    ef49e1d709c3cbb3eccbc22710964e1ffe2612c9
    3b96d478851fbbe391ab30e3d6a0afdb9ecdd4a0
    fcfd1cb69762c49ba56326dfc85008c1d83333b2
    0fcc9ed1c66bed7ef3a0bcd6c517280a82391d2b
    f3a5ac1a50c1fccd0801023658e42d2093e1a33a
    1f3127e88a87953f059c9a70d3582ae1719594b1
and sssd-1-11:
    3fa31631d74d09ce6b52b6b8cd3c994c9f2a2db4
    6bbb1da6dd9365592b4be309cf3bd7f245d844a8
    f9cab654c8217a4f1b983bcf92dc36acffffe58f
    61cba55a3e44a937703a690254ccb3e0dd65dbb6
    5b4c81cab999c1f5a385754badd9e88600cbf3fa

I didn't commit the tests to sssd-1-11 b/c they apply on previous
patches by Pallavi and the rebase would be tedious -- I don't think it's
worth doing for unit tests, honestly. If you disagree, ping me and I'll
prepare the test for sssd-1-11 as well.

[Pavel Březina]

On 08/13/2014 03:32 PM, Jakub Hrozek wrote:
> On Wed, Aug 13, 2014 at 02:41:49PM +0200, Pavel Březina wrote:
> > Ok, I'm fine with that. Ack.
> >
> > Just fix the option name also in commit message prior pushing please.
>
> Done and pushed to master:
>      ef49e1d709c3cbb3eccbc22710964e1ffe2612c9
>      3b96d478851fbbe391ab30e3d6a0afdb9ecdd4a0
>      fcfd1cb69762c49ba56326dfc85008c1d83333b2
>      0fcc9ed1c66bed7ef3a0bcd6c517280a82391d2b
>      f3a5ac1a50c1fccd0801023658e42d2093e1a33a
>      1f3127e88a87953f059c9a70d3582ae1719594b1
> and sssd-1-11:
>      3fa31631d74d09ce6b52b6b8cd3c994c9f2a2db4
>      6bbb1da6dd9365592b4be309cf3bd7f245d844a8
>      f9cab654c8217a4f1b983bcf92dc36acffffe58f
>      61cba55a3e44a937703a690254ccb3e0dd65dbb6
>      5b4c81cab999c1f5a385754badd9e88600cbf3fa
>
> I didn't commit the tests to sssd-1-11 b/c they apply on previous
> patches by Pallavi and the rebase would be tedious -- I don't think it's
> worth doing for unit tests, honestly. If you disagree, ping me and I'll
> prepare the test for sssd-1-11 as well.

I'm fine with it.