[PATCHES] sss_case = preserving

[Michal Židek]

Hi,

patches for ticket
https://fedorahosted.org/sssd/ticket/2367
are in attachment.

Michal

[Michal Židek]

On 07/16/2014 03:40 PM, Michal Židek wrote:
> Hi,
>
> patches for ticket
> https://fedorahosted.org/sssd/ticket/2367
> are in attachment.
>
> Michal

I forgot to add reference to the ticket in the
patch description. New patches are attached.

Michal

[Pavel Reichl]

On Wed, 2014-07-16 at 15:40 +0200, Michal Židek wrote:
> Hi,
>
> patches for ticket
> https://fedorahosted.org/sssd/ticket/2367
> are in attachment.
>
> Michal
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hi Michal,

I haven't tested the patches yet. I just wanted to share some nitpicks
with you:
> From 071b3ca8cd9a194c8cb287f9abca2fe7c58323a2 Mon Sep 17 00:00:00 2001
> From: Michal Zidek 
> Date: Tue, 15 Jul 2014 12:00:36 -0400
> Subject: [PATCH 1/3] Add function confdb_set_string.
>
> src/confdb/confdb.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/confdb/confdb.h |  6 +++++
>  2 files changed, 76 insertions(+)
>
> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> index 15de961..79c89b7 100644
> --- a/src/confdb/confdb.c
> +++ b/src/confdb/confdb.c
> @@ -369,6 +369,76 @@ done:
>      return ret;
>  }
>
> +int confdb_set_string(struct confdb_ctx *cdb,
>                  const char *section,
>                  const char *attribute,
>                  char *val)
>
> +{
> TALLOC_CTX *tmp_ctx;
> struct ldb_dn *dn;
> char *secdn;
> struct ldb_message *msg;
> int ret, lret;
> tmp_ctx = talloc_new(NULL);
> if (!tmp_ctx)
>    return ENOMEM;

Although there's not a consensus about the form of checking allocated
pointers ( !tmp_ctx vs. tmp_ctx != NULL ) among SSSD developers, still I
think there is an agreement that 'if' should be followed by block or
condition and action should be one-liner.

Could you change the code to something like?

if (!tmp_ctx) return ENOMEM;

or

if (!tmp_ctx) { 
    return ENOMEM;
}

In the second patch you use 2 forms of testing result of strcasecmp
> !strcasecmp(tmp, "true")
> strcasecmp(domain->provider, "local") == 0

Could you please just use one of them? From my POV the second is
preferred.

[Michal Židek]

On 07/17/2014 06:12 PM, Pavel Reichl wrote:
> On Wed, 2014-07-16 at 15:40 +0200, Michal Židek wrote:
> > Hi,
> >
> > patches for ticket
> > https://fedorahosted.org/sssd/ticket/2367
> > are in attachment.
> >
> > Michal
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Hi Michal,
>
> I haven't tested the patches yet. I just wanted to share some nitpicks
> with you:
> > From 071b3ca8cd9a194c8cb287f9abca2fe7c58323a2 Mon Sep 17 00:00:00 2001
> > From: Michal Zidek 
> > Date: Tue, 15 Jul 2014 12:00:36 -0400
> > Subject: [PATCH 1/3] Add function confdb_set_string.
> >
> > src/confdb/confdb.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   src/confdb/confdb.h |  6 +++++
> >   2 files changed, 76 insertions(+)
> >
> > diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> > index 15de961..79c89b7 100644
> > --- a/src/confdb/confdb.c
> > +++ b/src/confdb/confdb.c
> > @@ -369,6 +369,76 @@ done:
> >       return ret;
> >   }
> >
> > +int confdb_set_string(struct confdb_ctx *cdb,
> >                  const char *section,
> >                  const char *attribute,
> >                  char *val)
> >
> > +{
> > TALLOC_CTX *tmp_ctx;
> > struct ldb_dn *dn;
> > char *secdn;
> > struct ldb_message *msg;
> > int ret, lret;
> > tmp_ctx = talloc_new(NULL);
> > if (!tmp_ctx)
> >    return ENOMEM;
>
> Although there's not a consensus about the form of checking allocated
> pointers ( !tmp_ctx vs. tmp_ctx != NULL ) among SSSD developers, still I
> think there is an agreement that 'if' should be followed by block or
> condition and action should be one-liner.
>
> Could you change the code to something like?
>
> if (!tmp_ctx) return ENOMEM;
>
> or
>
> if (!tmp_ctx) {
>    return ENOMEM;
> }

Fixed.
> In the second patch you use 2 forms of testing result of strcasecmp
> > !strcasecmp(tmp, "true")
> > strcasecmp(domain->provider, "local") == 0
>
> Could you please just use one of them? From my POV the second is
> preferred.

Fixed.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thank you Pavel!
New version is attached.

Michal

[Pavel Reichl]

Thanks for the quick update, I have some more concerns and questions 
about the patches.

1st patch:
> +int confdb_set_string(struct confdb_ctx *cdb,
>                  const char *section,
>                  const char *attribute,
>                  char *val)
>
> +{

[snip]
> lret = ldb_msg_add_string(msg, attribute, val);
> if (lret != LDB_SUCCESS) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "ldb_msg_add_string failed: [%s]\n", ldb_strerror(lret));
>    ret = EIO;
>    goto done;
> }

Two empty lines.
> lret = ldb_modify(cdb->ldb, msg);
> if (lret != LDB_SUCCESS) {
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "ldb_modify failed: [%s]\n", ldb_strerror(lret));
>    ret = EIO;
>    goto done;
> }
> ret = EOK;
>
> +done:
> talloc_free(tmp_ctx);
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Failed to set [%s] from [%s], error [%d] (%s)\n",
>           attribute, section, ret, strerror(ret));
> }
> return ret;
>
> +}

missing new line here
> int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
>                        const char *section, const char *attribute,
>                        const char *defstr, char **result)
> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> index ba33ea5..f81c6d4 100644
> --- a/src/confdb/confdb.h
> +++ b/src/confdb/confdb.h
> @@ -216,7 +216,7 @@ struct sss_domain_info {
>  bool cache_credentials;
>  bool legacy_passwords;
> bool case_sensitive;

Why do you remove case_sensitive here and add it back in second patch? I 
think this 'ping-pong' is confusing and needless, could you fix it, please?
> bool case_preserve;
>
> gid_t override_gid;
> const char *override_homedir;
>
> @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
>                       const char *attribute,
>                       bool val);

Would you consider adding doxygen comment here? All functions with 
exception of confdb_set_bool() have one.
But maybe it's not worth it, what do you think?
> +int confdb_set_string(struct confdb_ctx *cdb,
>                  const char *section,
>                  const char *attribute,
>                  char *val);
>
> /**
> @brief Convenience function to retrieve a single-valued attribute as a
> null-terminated array of strings
>
> -- 
> 1.9.3

Generally, I think there's custom do remove any unused function from 
code-base when they aren't called. So I think you should remove 
confdb_set_string(). I guess you will need a new patch for that to keep 
every commit compilable.

2nd patch:
> @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct 
> confdb_ctx *cdb,
>          }
>      }
> ret = get_entry_as_bool(res->msgs[0], &domain->case_sensitive,
>                        CONFDB_DOMAIN_CASE_SENSITIVE, true);
> if(ret != EOK) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
>    goto done;
> tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> CONFDB_DOMAIN_CASE_SENSITIVE, "true");
> if (tmp != NULL) {
>    if (strcasecmp(tmp, "true") == 0) {
>        domain->case_sensitive = true;
>        domain->case_preserve = true;
>    } else if (strcasecmp(tmp, "false") == 0) {
>        domain->case_sensitive = false;
>        domain->case_preserve = false;
>    } else if (strcasecmp(tmp, "preserving") == 0) {
>        domain->case_sensitive = false;
>        domain->case_preserve = true;
>    } else {
>        DEBUG(SSSDBG_FATAL_FAILURE,
>              "Invalid value for %s\n", 
>
> CONFDB_DOMAIN_CASE_SENSITIVE);
>        goto done;
>    }
> } else {
>    /* default */
>    domain->case_sensitive = true;
>    domain->case_preserve = true;
> }
>    if (domain->case_sensitive == false &&
>        strcasecmp(domain->provider, "local") == 0) {
>
> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> index f81c6d4..8a642b3 100644
> --- a/src/confdb/confdb.h
> +++ b/src/confdb/confdb.h
> @@ -216,6 +216,7 @@ struct sss_domain_info {
>  bool cache_credentials;
>  bool legacy_passwords;
>
> bool case_sensitive;
> bool case_preserve;
>
> gid_t override_gid;
>
> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> index 67ded36..672a1e1 100644
> --- a/src/providers/ad/ad_common.c
> +++ b/src/providers/ad/ad_common.c
> @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>      char *realm;
>      char *ad_hostname;
>      char hostname[HOST_NAME_MAX + 1];
> char *tmp;

I personally dislike general variable names, it's my opinion that if you 
are creating variable for specific use then you should name it accordingly.
Of course there are exceptions from this rule, do you see any not to 
name it 'case_sensitive_str' or something like this?
>  opts = talloc_zero(mem_ctx, struct ad_options);
>  if (!opts) return ENOMEM;
>
> @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>      }
>  /* Active Directory is always case-insensitive */
> dom->case_sensitive = false;
>
> ret = confdb_get_string(cdb, mem_ctx, conf_path,
>                        CONFDB_DOMAIN_CASE_SENSITIVE, "false",
>                        &tmp);
>
> if (ret != EOK) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
>    goto done;
>
> }
>
> if (strcasecmp(tmp, "true") == 0) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Warning: AD domain can not be set as case-sensitive.\n");
>    dom->case_sensitive = false;
>    dom->case_preserve = false;
>
> } else if (strcasecmp(tmp, "false") == 0) {
>    dom->case_sensitive = false;
>    dom->case_preserve = false;
>
> } else if (strcasecmp(tmp, "preserving") == 0) {
>    dom->case_sensitive = false;
>    dom->case_preserve = true;
>
> } else {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
>    goto done;
>
> }
>
> /* Set this in the confdb so that the responders pick it
> up when they start up.
>
> */
> ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
>                      dom->case_sensitive);
> ret = confdb_set_string(cdb, conf_path, "case_sensitive",
>                        tmp);
> if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "Could not set domain case-sensitive: [%s]\n",
>
> diff --git a/src/providers/ipa/ipa_selinux.c 
> b/src/providers/ipa/ipa_selinux.c
> index 927e545..3cd1cc1 100644
> --- a/src/providers/ipa/ipa_selinux.c
> +++ b/src/providers/ipa/ipa_selinux.c
> @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char 
> *orig_name,
>      /* pam_selinux needs the username in the same format getpwnam() would
>       * return it
>       */
> username = sss_get_cased_name(tmp_ctx, orig_name,
>
> dom->case_sensitive);
> username = sss_get_cased_name(tmp_ctx, orig_name,
>
> dom->case_preserve);
>      if (username == NULL) {
>          ret = ENOMEM;
>          goto done;
> diff --git a/src/responder/nss/nsssrv_cmd.c 
> b/src/responder/nss/nsssrv_cmd.c
> index a168a3e..c201d3a 100644
> --- a/src/responder/nss/nsssrv_cmd.c
> +++ b/src/responder/nss/nsssrv_cmd.c
> @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
>              packet_initialized = true;
>          }
>    tmpstr = sss_get_cased_name(tmp_ctx, orig_name, 
>
> dom->case_sensitive);
>    tmpstr = sss_get_cased_name(tmp_ctx, orig_name, 
>
> dom->case_preserve);
>          if (tmpstr == NULL) {
>              DEBUG(SSSDBG_CRIT_FAILURE,
>                    "sss_get_cased_name failed, skipping\n");
> @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
>              }
>          }
>    tmpstr = sss_get_cased_name(tmp_ctx, orig_name, 
>
> dom->case_sensitive);
>    tmpstr = sss_get_cased_name(tmp_ctx, orig_name, 
>
> dom->case_preserve);
>          if (tmpstr == NULL) {
>              DEBUG(SSSDBG_CRIT_FAILURE,
>                    "sss_get_cased_name failed, skipping\n");
> -- 
> 1.9.3

I don't like very much the code duplication around parsing the option 
value case_sensitive.  Would you consider creating some utility function 
that would do so and call it from ad_get_common_options() and 
confdb_get_domain_internal()?
Something like:

int sss_parse_case_sensitivity_option(const char* str)
{
         if (strcasecmp(str, "true") == 0) {
             return CASE_INSENSITIVE;
         } else if (strcasecmp(str, "false") == 0) {
             return CASE_INSENSITIVE;
....

3rd patch:
> @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
>                  </varlistentry>
>              <varlistentry>
>                <term>case_sensitive (boolean)</term>
>                <term>case_sensitive (string)</term>
>                <listitem>
>                    <para>
>                        Treat user and group names as case 
>
> sensitive. At
>                              the moment, this option is not supported in
>                        the local provider.
>                        the local provider. Possible options are:
>                        True, False, Preserving. Preserving is the
>                        same as False (case insensitive), but does
>                        not lowercase names in the output of getpwnam
>                        and getgrnam.
>                    </para>
>                    <para>
>                        Default: True

I wonder if it were better to list each option value on its own line. 
Something like:

<para>
                              Treat user and group names as case 
sensitive. At
                              the moment, this option is not supported in
-                            the local provider. Possible options are:
-                            True, False, Preserving. Preserving is the
-                            same as False (case insensitive), but does
+                            the local provider.
+                        </para>
+                        <para>
+                            Supported values:
+                        </para>
+                        <para>
+                            True: case insensitive
+                        </para>
+                        <para>
+                            True: case sensitive
+                        </para>
+                        <para>
+                            Preserving: case insensitive, but does
                              not lowercase names in the output of getpwnam
                              and g

But I think this is a matter of personal taste so I leave it up to you. 
Anyway, I think that man page changes must be acked-by native speaker 
(Stephen),

Thanks,

[Michal Židek]

On 07/18/2014 03:41 PM, Pavel Reichl wrote:
> Thanks for the quick update, I have some more concerns and questions
> about the patches.
>
> 1st patch:
> > +int confdb_set_string(struct confdb_ctx *cdb,
> >                  const char *section,
> >                  const char *attribute,
> >                  char *val)
> >
> > +{
>
> [snip]
> > lret = ldb_msg_add_string(msg, attribute, val);
> > if (lret != LDB_SUCCESS) {
> >    DEBUG(SSSDBG_MINOR_FAILURE,
> >          "ldb_msg_add_string failed: [%s]\n", ldb_strerror(lret));
> >    ret = EIO;
> >    goto done;
> > }
>
> Two empty lines.

Fixed.
> > lret = ldb_modify(cdb->ldb, msg);
> > if (lret != LDB_SUCCESS) {
> >    DEBUG(SSSDBG_MINOR_FAILURE,
> >          "ldb_modify failed: [%s]\n", ldb_strerror(lret));
> >    ret = EIO;
> >    goto done;
> > }
> > ret = EOK;
> >
> > +done:
> > talloc_free(tmp_ctx);
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "Failed to set [%s] from [%s], error [%d] (%s)\n",
> >           attribute, section, ret, strerror(ret));
> > }
> > return ret;
> >
> > +}
>
> missing new line here

Fixed.
> > int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
> >                        const char *section, const char *attribute,
> >                        const char *defstr, char **result)
> > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > index ba33ea5..f81c6d4 100644
> > --- a/src/confdb/confdb.h
> > +++ b/src/confdb/confdb.h
> > @@ -216,7 +216,7 @@ struct sss_domain_info {
> >  bool cache_credentials;
> >  bool legacy_passwords;
> > bool case_sensitive;
>
> Why do you remove case_sensitive here and add it back in second patch? I
> think this 'ping-pong' is confusing and needless, could you fix it, please?

Sorry, bad rebasing. Fixed.
> > bool case_preserve;
> >
> > gid_t override_gid;
> > const char *override_homedir;
> >
> > @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
> >                       const char *attribute,
> >                       bool val);
>
> Would you consider adding doxygen comment here? All functions with
> exception of confdb_set_bool() have one.
> But maybe it's not worth it, what do you think?

I do not think it is necessary in this case, but I added it
anyway.
> > +int confdb_set_string(struct confdb_ctx *cdb,
> >                  const char *section,
> >                  const char *attribute,
> >                  char *val);
> >
> > /**
> > @brief Convenience function to retrieve a single-valued attribute
> >
> > as a
> > null-terminated array of strings
> >
> > --
> > 1.9.3
>
> Generally, I think there's custom do remove any unused function from
> code-base when they aren't called. So I think you should remove
> confdb_set_string(). I guess you will need a new patch for that to keep
> every commit compilable.

You mean confdb_set_bool? I added new patch to remove
that function.
> 2nd patch:
> > @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct
> > confdb_ctx *cdb,
> >          }
> >      }
> > ret = get_entry_as_bool(res->msgs[0], &domain->case_sensitive,
> >                        CONFDB_DOMAIN_CASE_SENSITIVE, true);
> > if(ret != EOK) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
> >    goto done;
> > tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> > CONFDB_DOMAIN_CASE_SENSITIVE, "true");
> > if (tmp != NULL) {
> >    if (strcasecmp(tmp, "true") == 0) {
> >        domain->case_sensitive = true;
> >        domain->case_preserve = true;
> >    } else if (strcasecmp(tmp, "false") == 0) {
> >        domain->case_sensitive = false;
> >        domain->case_preserve = false;
> >    } else if (strcasecmp(tmp, "preserving") == 0) {
> >        domain->case_sensitive = false;
> >        domain->case_preserve = true;
> >    } else {
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >              "Invalid value for %s\n",
> >
> > CONFDB_DOMAIN_CASE_SENSITIVE);
> >        goto done;
> >    }
> > } else {
> >    /* default */
> >    domain->case_sensitive = true;
> >    domain->case_preserve = true;
> > }
> >    if (domain->case_sensitive == false &&
> >        strcasecmp(domain->provider, "local") == 0) {
> >
> > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > index f81c6d4..8a642b3 100644
> > --- a/src/confdb/confdb.h
> > +++ b/src/confdb/confdb.h
> > @@ -216,6 +216,7 @@ struct sss_domain_info {
> >  bool cache_credentials;
> >  bool legacy_passwords;
> >
> > bool case_sensitive;
> > bool case_preserve;
> >
> > gid_t override_gid;
> >
> > diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> > index 67ded36..672a1e1 100644
> > --- a/src/providers/ad/ad_common.c
> > +++ b/src/providers/ad/ad_common.c
> > @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> >      char *realm;
> >      char *ad_hostname;
> >      char hostname[HOST_NAME_MAX + 1];
> > char *tmp;
>
> I personally dislike general variable names, it's my opinion that if you
> are creating variable for specific use then you should name it accordingly.
> Of course there are exceptions from this rule, do you see any not to
> name it 'case_sensitive_str' or something like this?

I wanted to have the same name as in confdb.c, but ok, I will
change the name in the ad specific code to case_sensitive_opt.
> >  opts = talloc_zero(mem_ctx, struct ad_options);
> >  if (!opts) return ENOMEM;
> >
> > @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> >      }
> >  /* Active Directory is always case-insensitive */
> > dom->case_sensitive = false;
> >
> > ret = confdb_get_string(cdb, mem_ctx, conf_path,
> >                        CONFDB_DOMAIN_CASE_SENSITIVE, "false",
> >                        &tmp);
> >
> > if (ret != EOK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
> >    goto done;
> >
> > }
> >
> > if (strcasecmp(tmp, "true") == 0) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "Warning: AD domain can not be set as case-sensitive.\n");
> >    dom->case_sensitive = false;
> >    dom->case_preserve = false;
> >
> > } else if (strcasecmp(tmp, "false") == 0) {
> >    dom->case_sensitive = false;
> >    dom->case_preserve = false;
> >
> > } else if (strcasecmp(tmp, "preserving") == 0) {
> >    dom->case_sensitive = false;
> >    dom->case_preserve = true;
> >
> > } else {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
> >    goto done;
> >
> > }
> >
> > /* Set this in the confdb so that the responders pick it
> > up when they start up.
> >
> > */
> > ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
> >                      dom->case_sensitive);
> > ret = confdb_set_string(cdb, conf_path, "case_sensitive",
> >                        tmp);
> > if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "Could not set domain case-sensitive: [%s]\n",
> >
> > diff --git a/src/providers/ipa/ipa_selinux.c
> > b/src/providers/ipa/ipa_selinux.c
> > index 927e545..3cd1cc1 100644
> > --- a/src/providers/ipa/ipa_selinux.c
> > +++ b/src/providers/ipa/ipa_selinux.c
> > @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char
> > *orig_name,
> >      /* pam_selinux needs the username in the same format getpwnam()
> > would
> >       * return it
> >       */
> > username = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_sensitive);
> > username = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_preserve);
> >      if (username == NULL) {
> >          ret = ENOMEM;
> >          goto done;
> > diff --git a/src/responder/nss/nsssrv_cmd.c
> > b/src/responder/nss/nsssrv_cmd.c
> > index a168a3e..c201d3a 100644
> > --- a/src/responder/nss/nsssrv_cmd.c
> > +++ b/src/responder/nss/nsssrv_cmd.c
> > @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
> >              packet_initialized = true;
> >          }
> >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_sensitive);
> >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_preserve);
> >          if (tmpstr == NULL) {
> >              DEBUG(SSSDBG_CRIT_FAILURE,
> >                    "sss_get_cased_name failed, skipping\n");
> > @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
> >              }
> >          }
> >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_sensitive);
> >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> >
> > dom->case_preserve);
> >          if (tmpstr == NULL) {
> >              DEBUG(SSSDBG_CRIT_FAILURE,
> >                    "sss_get_cased_name failed, skipping\n");
> > --
> > 1.9.3
>
> I don't like very much the code duplication around parsing the option
> value case_sensitive.  Would you consider creating some utility function
> that would do so and call it from ad_get_common_options() and
> confdb_get_domain_internal()?
> Something like:
>
> int sss_parse_case_sensitivity_option(const char* str)
> {
>          if (strcasecmp(str, "true") == 0) {
>              return CASE_INSENSITIVE;
>          } else if (strcasecmp(str, "false") == 0) {
>              return CASE_INSENSITIVE;
> ....

I do not think this will help. If we added such function
than the only think that would change would be, that
the
'if (strcasecmp(tmp, "something") == 0)'
lines would be replaced with
'if (ret == SSS_SOMETHING)'.
Or maybe switch-case might be used, but the
lines would still have to be there. See that
I set different values in the AD specific code
and print additional debug message in one case
so it is not really duplication and would still
have to be handled outside of this utility
function, which is the reason why IMO such
function would only add code but would not be
very helpful -- what do you think?).
> 3rd patch:
> > @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
> >                  </varlistentry>
> >              <varlistentry>
> >                <term>case_sensitive (boolean)</term>
> >                <term>case_sensitive (string)</term>
> >                <listitem>
> >                    <para>
> >                        Treat user and group names as case
> >
> > sensitive. At
> >                              the moment, this option is not supported in
> >                        the local provider.
> >                        the local provider. Possible options are:
> >                        True, False, Preserving. Preserving is the
> >                        same as False (case insensitive), but does
> >                        not lowercase names in the output of
> >
> > getpwnam
> >                        and getgrnam.
> >                    </para>
> >                    <para>
> >                        Default: True
>
> I wonder if it were better to list each option value on its own line.
> Something like:
>                        <para>
>                           Treat user and group names as case
>
> sensitive. At
>                               the moment, this option is not supported in
>                        the local provider. Possible options are:
>                        True, False, Preserving. Preserving is the
>                        same as False (case insensitive), but does
>                        the local provider.
>                    </para>
>                    <para>
>                        Supported values:
>                    </para>
>                    <para>
>                        True: case insensitive
>                    </para>
>                    <para>
>                        True: case sensitive
>                    </para>
>                    <para>
>                        Preserving: case insensitive, but does
>                         not lowercase names in the output of getpwnam
>                         and g
>
> But I think this is a matter of personal taste so I leave it up to you.

Ok, I added a list of possible options.
> Anyway, I think that man page changes must be acked-by native speaker
> (Stephen),
>
> Thanks,

[Michal Židek]

On 07/21/2014 05:54 PM, Michal Židek wrote:
> On 07/18/2014 03:41 PM, Pavel Reichl wrote:
> > Thanks for the quick update, I have some more concerns and questions
> > about the patches.
> >
> > 1st patch:
> > > +int confdb_set_string(struct confdb_ctx *cdb,
> > >                  const char *section,
> > >                  const char *attribute,
> > >                  char *val)
> > >
> > > +{
> >
> > [snip]
> > > lret = ldb_msg_add_string(msg, attribute, val);
> > > if (lret != LDB_SUCCESS) {
> > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > >          "ldb_msg_add_string failed: [%s]\n", ldb_strerror(lret));
> > >    ret = EIO;
> > >    goto done;
> > > }
> >
> > Two empty lines.
>
> Fixed.
> > > lret = ldb_modify(cdb->ldb, msg);
> > > if (lret != LDB_SUCCESS) {
> > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > >          "ldb_modify failed: [%s]\n", ldb_strerror(lret));
> > >    ret = EIO;
> > >    goto done;
> > > }
> > > ret = EOK;
> > >
> > > +done:
> > > talloc_free(tmp_ctx);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE,
> > >          "Failed to set [%s] from [%s], error [%d] (%s)\n",
> > >           attribute, section, ret, strerror(ret));
> > > }
> > > return ret;
> > >
> > > +}
> >
> > missing new line here
>
> Fixed.
> > > int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
> > >                        const char *section, const char *attribute,
> > >                        const char *defstr, char **result)
> > > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > > index ba33ea5..f81c6d4 100644
> > > --- a/src/confdb/confdb.h
> > > +++ b/src/confdb/confdb.h
> > > @@ -216,7 +216,7 @@ struct sss_domain_info {
> > >  bool cache_credentials;
> > >  bool legacy_passwords;
> > > bool case_sensitive;
> >
> > Why do you remove case_sensitive here and add it back in second patch? I
> > think this 'ping-pong' is confusing and needless, could you fix it,
> > please?
>
> Sorry, bad rebasing. Fixed.
> > > bool case_preserve;
> > >
> > > gid_t override_gid;
> > > const char *override_homedir;
> > >
> > > @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
> > >                       const char *attribute,
> > >                       bool val);
> >
> > Would you consider adding doxygen comment here? All functions with
> > exception of confdb_set_bool() have one.
> > But maybe it's not worth it, what do you think?
>
> I do not think it is necessary in this case, but I added it
> anyway.
> > > +int confdb_set_string(struct confdb_ctx *cdb,
> > >                  const char *section,
> > >                  const char *attribute,
> > >                  char *val);
> > >
> > > /**
> > > @brief Convenience function to retrieve a single-valued attribute
> > >
> > > as a
> > > null-terminated array of strings
> > >
> > > --
> > > 1.9.3
> >
> > Generally, I think there's custom do remove any unused function from
> > code-base when they aren't called. So I think you should remove
> > confdb_set_string(). I guess you will need a new patch for that to keep
> > every commit compilable.
>
> You mean confdb_set_bool? I added new patch to remove
> that function.
> > 2nd patch:
> > > @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct
> > > confdb_ctx *cdb,
> > >          }
> > >      }
> > > ret = get_entry_as_bool(res->msgs[0], &domain->case_sensitive,
> > >                        CONFDB_DOMAIN_CASE_SENSITIVE, true);
> > > if(ret != EOK) {
> > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > >          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
> > >    goto done;
> > > tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> > > CONFDB_DOMAIN_CASE_SENSITIVE, "true");
> > > if (tmp != NULL) {
> > >    if (strcasecmp(tmp, "true") == 0) {
> > >        domain->case_sensitive = true;
> > >        domain->case_preserve = true;
> > >    } else if (strcasecmp(tmp, "false") == 0) {
> > >        domain->case_sensitive = false;
> > >        domain->case_preserve = false;
> > >    } else if (strcasecmp(tmp, "preserving") == 0) {
> > >        domain->case_sensitive = false;
> > >        domain->case_preserve = true;
> > >    } else {
> > >        DEBUG(SSSDBG_FATAL_FAILURE,
> > >              "Invalid value for %s\n",
> > >
> > > CONFDB_DOMAIN_CASE_SENSITIVE);
> > >        goto done;
> > >    }
> > > } else {
> > >    /* default */
> > >    domain->case_sensitive = true;
> > >    domain->case_preserve = true;
> > > }
> > >    if (domain->case_sensitive == false &&
> > >        strcasecmp(domain->provider, "local") == 0) {
> > >
> > > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > > index f81c6d4..8a642b3 100644
> > > --- a/src/confdb/confdb.h
> > > +++ b/src/confdb/confdb.h
> > > @@ -216,6 +216,7 @@ struct sss_domain_info {
> > >  bool cache_credentials;
> > >  bool legacy_passwords;
> > >
> > > bool case_sensitive;
> > > bool case_preserve;
> > >
> > > gid_t override_gid;
> > >
> > > diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> > > index 67ded36..672a1e1 100644
> > > --- a/src/providers/ad/ad_common.c
> > > +++ b/src/providers/ad/ad_common.c
> > > @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> > >      char *realm;
> > >      char *ad_hostname;
> > >      char hostname[HOST_NAME_MAX + 1];
> > > char *tmp;
> >
> > I personally dislike general variable names, it's my opinion that if you
> > are creating variable for specific use then you should name it
> > accordingly.
> > Of course there are exceptions from this rule, do you see any not to
> > name it 'case_sensitive_str' or something like this?
>
> I wanted to have the same name as in confdb.c, but ok, I will
> change the name in the ad specific code to case_sensitive_opt.
> > >  opts = talloc_zero(mem_ctx, struct ad_options);
> > >  if (!opts) return ENOMEM;
> > >
> > > @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> > >      }
> > >  /* Active Directory is always case-insensitive */
> > > dom->case_sensitive = false;
> > > ret = confdb_get_string(cdb, mem_ctx, conf_path,
> > >                        CONFDB_DOMAIN_CASE_SENSITIVE, "false",
> > >                        &tmp);
> > > if (ret != EOK) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
> > >    goto done;
> > > }
> > > if (strcasecmp(tmp, "true") == 0) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE,
> > >          "Warning: AD domain can not be set as
> > >
> > > case-sensitive.\n");
> > >    dom->case_sensitive = false;
> > >    dom->case_preserve = false;
> > >
> > > } else if (strcasecmp(tmp, "false") == 0) {
> > >    dom->case_sensitive = false;
> > >    dom->case_preserve = false;
> > >
> > > } else if (strcasecmp(tmp, "preserving") == 0) {
> > >    dom->case_sensitive = false;
> > >    dom->case_preserve = true;
> > >
> > > } else {
> > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > >          "Invalid value for %s\n", CONFDB_DOMAIN_CASE_SENSITIVE);
> > >    goto done;
> > >
> > > }
> > >
> > > /* Set this in the confdb so that the responders pick it
> > > up when they start up.
> > >
> > > */
> > > ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
> > >                      dom->case_sensitive);
> > > ret = confdb_set_string(cdb, conf_path, "case_sensitive",
> > >                        tmp);
> > > if (ret != EOK) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "Could not set domain case-sensitive: [%s]\n",
> > >
> > > diff --git a/src/providers/ipa/ipa_selinux.c
> > > b/src/providers/ipa/ipa_selinux.c
> > > index 927e545..3cd1cc1 100644
> > > --- a/src/providers/ipa/ipa_selinux.c
> > > +++ b/src/providers/ipa/ipa_selinux.c
> > > @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char
> > > *orig_name,
> > >      /* pam_selinux needs the username in the same format getpwnam()
> > > would
> > >       * return it
> > >       */
> > > username = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_sensitive);
> > > username = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_preserve);
> > >      if (username == NULL) {
> > >          ret = ENOMEM;
> > >          goto done;
> > > diff --git a/src/responder/nss/nsssrv_cmd.c
> > > b/src/responder/nss/nsssrv_cmd.c
> > > index a168a3e..c201d3a 100644
> > > --- a/src/responder/nss/nsssrv_cmd.c
> > > +++ b/src/responder/nss/nsssrv_cmd.c
> > > @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
> > >              packet_initialized = true;
> > >          }
> > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_sensitive);
> > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_preserve);
> > >          if (tmpstr == NULL) {
> > >              DEBUG(SSSDBG_CRIT_FAILURE,
> > >                    "sss_get_cased_name failed, skipping\n");
> > > @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
> > >              }
> > >          }
> > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_sensitive);
> > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > >
> > > dom->case_preserve);
> > >          if (tmpstr == NULL) {
> > >              DEBUG(SSSDBG_CRIT_FAILURE,
> > >                    "sss_get_cased_name failed, skipping\n");
> > > --
> > > 1.9.3
> >
> > I don't like very much the code duplication around parsing the option
> > value case_sensitive.  Would you consider creating some utility function
> > that would do so and call it from ad_get_common_options() and
> > confdb_get_domain_internal()?
> > Something like:
> >
> > int sss_parse_case_sensitivity_option(const char* str)
> > {
> >          if (strcasecmp(str, "true") == 0) {
> >              return CASE_INSENSITIVE;
> >          } else if (strcasecmp(str, "false") == 0) {
> >              return CASE_INSENSITIVE;
> > ....
>
> I do not think this will help. If we added such function
> than the only think that would change would be, that
> the
> 'if (strcasecmp(tmp, "something") == 0)'
> lines would be replaced with
> 'if (ret == SSS_SOMETHING)'.
> Or maybe switch-case might be used, but the
> lines would still have to be there. See that
> I set different values in the AD specific code
> and print additional debug message in one case
> so it is not really duplication and would still
> have to be handled outside of this utility
> function, which is the reason why IMO such
> function would only add code but would not be
> very helpful -- what do you think?).
> > 3rd patch:
> > > @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
> > >                  </varlistentry>
> > >              <varlistentry>
> > >                <term>case_sensitive (boolean)</term>
> > >                <term>case_sensitive (string)</term>
> > >                <listitem>
> > >                    <para>
> > >                        Treat user and group names as case
> > >
> > > sensitive. At
> > >                              the moment, this option is not supported in
> > >                        the local provider.
> > >                        the local provider. Possible options are:
> > >                        True, False, Preserving. Preserving is the
> > >                        same as False (case insensitive), but does
> > >                        not lowercase names in the output of
> > >
> > > getpwnam
> > >                        and getgrnam.
> > >                    </para>
> > >                    <para>
> > >                        Default: True
> >
> > I wonder if it were better to list each option value on its own line.
> > Something like:
> >                        <para>
> >                           Treat user and group names as case
> >
> > sensitive. At
> >                               the moment, this option is not supported in
> >                        the local provider. Possible options are:
> >                        True, False, Preserving. Preserving is the
> >                        same as False (case insensitive), but does
> >                        the local provider.
> >                    </para>
> >                    <para>
> >                        Supported values:
> >                    </para>
> >                    <para>
> >                        True: case insensitive
> >                    </para>
> >                    <para>
> >                        True: case sensitive
> >                    </para>
> >                    <para>
> >                        Preserving: case insensitive, but does
> >                         not lowercase names in the output of
> >
> > getpwnam
> >                               and g
> >
> > But I think this is a matter of personal taste so I leave it up to you.
>
> Ok, I added a list of possible options.
> > Anyway, I think that man page changes must be acked-by native speaker
> > (Stephen),
> >
> > Thanks,

The 4th patch had wrong author in description. Sending the
patches again.

Michal

[Pavel Reichl]

On 07/21/2014 06:29 PM, Michal Židek wrote:
> On 07/21/2014 05:54 PM, Michal Židek wrote:
> > On 07/18/2014 03:41 PM, Pavel Reichl wrote:
> > > Thanks for the quick update, I have some more concerns and questions
> > > about the patches.
> > >
> > > 1st patch:
> > > > +int confdb_set_string(struct confdb_ctx *cdb,
> > > >                  const char *section,
> > > >                  const char *attribute,
> > > >                  char *val)
> > > >
> > > > +{
> > >
> > > [snip]
> > > > lret = ldb_msg_add_string(msg, attribute, val);
> > > > if (lret != LDB_SUCCESS) {
> > > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > > >          "ldb_msg_add_string failed: [%s]\n", 
> > > >
> > > > ldb_strerror(lret));
> > > >    ret = EIO;
> > > >    goto done;
> > > > }
> > >
> > > Two empty lines.
> >
> > Fixed.
> > > > lret = ldb_modify(cdb->ldb, msg);
> > > > if (lret != LDB_SUCCESS) {
> > > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > > >          "ldb_modify failed: [%s]\n", ldb_strerror(lret));
> > > >    ret = EIO;
> > > >    goto done;
> > > > }
> > > > ret = EOK;
> > > >
> > > > +done:
> > > > talloc_free(tmp_ctx);
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_CRIT_FAILURE,
> > > >          "Failed to set [%s] from [%s], error [%d] (%s)\n",
> > > >           attribute, section, ret, strerror(ret));
> > > > }
> > > > return ret;
> > > >
> > > > +}
> > >
> > > missing new line here
> >
> > Fixed.
> > > > int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
> > > >                        const char *section, const char *attribute,
> > > >                        const char *defstr, char **result)
> > > > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > > > index ba33ea5..f81c6d4 100644
> > > > --- a/src/confdb/confdb.h
> > > > +++ b/src/confdb/confdb.h
> > > > @@ -216,7 +216,7 @@ struct sss_domain_info {
> > > >  bool cache_credentials;
> > > >  bool legacy_passwords;
> > > > bool case_sensitive;
> > >
> > > Why do you remove case_sensitive here and add it back in second 
> > > patch? I
> > > think this 'ping-pong' is confusing and needless, could you fix it,
> > > please?
> >
> > Sorry, bad rebasing. Fixed.
> > > > bool case_preserve;
> > > >
> > > > gid_t override_gid;
> > > > const char *override_homedir;
> > > >
> > > > @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
> > > >                       const char *attribute,
> > > >                       bool val);
> > >
> > > Would you consider adding doxygen comment here? All functions with
> > > exception of confdb_set_bool() have one.
> > > But maybe it's not worth it, what do you think?
> >
> > I do not think it is necessary in this case, but I added it
> > anyway.

Michal you didn't have to do that if you didn't like it.  I was just 
suggesting the idea. Sorry if you felt like I made you to do that.
> > > > +int confdb_set_string(struct confdb_ctx *cdb,
> > > >                  const char *section,
> > > >                  const char *attribute,
> > > >                  char *val);
> > > >
> > > > /**
> > > > @brief Convenience function to retrieve a single-valued attribute
> > > >
> > > > as a
> > > > null-terminated array of strings
> > > >
> > > > -- 
> > > > 1.9.3
> > >
> > > Generally, I think there's custom do remove any unused function from
> > > code-base when they aren't called. So I think you should remove
> > > confdb_set_string(). I guess you will need a new patch for that to keep
> > > every commit compilable.
> >
> > You mean confdb_set_bool? I added new patch to remove
> > that function.

Yes, thanks.
> > > 2nd patch:
> > > > @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct
> > > > confdb_ctx *cdb,
> > > >          }
> > > >      }
> > > > ret = get_entry_as_bool(res->msgs[0], &domain->case_sensitive,
> > > >                        CONFDB_DOMAIN_CASE_SENSITIVE, true);
> > > > if(ret != EOK) {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          "Invalid value for %s\n", 
> > > >
> > > > CONFDB_DOMAIN_CASE_SENSITIVE);
> > > >    goto done;
> > > > tmp = ldb_msg_find_attr_as_string(res->msgs[0],
> > > > CONFDB_DOMAIN_CASE_SENSITIVE, "true");
> > > > if (tmp != NULL) {
> > > >    if (strcasecmp(tmp, "true") == 0) {
> > > >        domain->case_sensitive = true;
> > > >        domain->case_preserve = true;
> > > >    } else if (strcasecmp(tmp, "false") == 0) {
> > > >        domain->case_sensitive = false;
> > > >        domain->case_preserve = false;
> > > >    } else if (strcasecmp(tmp, "preserving") == 0) {
> > > >        domain->case_sensitive = false;
> > > >        domain->case_preserve = true;
> > > >    } else {
> > > >        DEBUG(SSSDBG_FATAL_FAILURE,
> > > >              "Invalid value for %s\n",
> > > >
> > > > CONFDB_DOMAIN_CASE_SENSITIVE);
> > > >        goto done;
> > > >    }
> > > > } else {
> > > >    /* default */
> > > >    domain->case_sensitive = true;
> > > >    domain->case_preserve = true;
> > > > }
> > > >    if (domain->case_sensitive == false &&
> > > >        strcasecmp(domain->provider, "local") == 0) {
> > > >
> > > > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
> > > > index f81c6d4..8a642b3 100644
> > > > --- a/src/confdb/confdb.h
> > > > +++ b/src/confdb/confdb.h
> > > > @@ -216,6 +216,7 @@ struct sss_domain_info {
> > > >  bool cache_credentials;
> > > >  bool legacy_passwords;
> > > >
> > > > bool case_sensitive;
> > > > bool case_preserve;
> > > >
> > > > gid_t override_gid;
> > > >
> > > > diff --git a/src/providers/ad/ad_common.c 
> > > > b/src/providers/ad/ad_common.c
> > > > index 67ded36..672a1e1 100644
> > > > --- a/src/providers/ad/ad_common.c
> > > > +++ b/src/providers/ad/ad_common.c
> > > > @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> > > >      char *realm;
> > > >      char *ad_hostname;
> > > >      char hostname[HOST_NAME_MAX + 1];
> > > > char *tmp;
> > >
> > > I personally dislike general variable names, it's my opinion that if 
> > > you
> > > are creating variable for specific use then you should name it
> > > accordingly.
> > > Of course there are exceptions from this rule, do you see any not to
> > > name it 'case_sensitive_str' or something like this?
> >
> > I wanted to have the same name as in confdb.c, but ok, I will
> > change the name in the ad specific code to case_sensitive_opt.
> > > >  opts = talloc_zero(mem_ctx, struct ad_options);
> > > >  if (!opts) return ENOMEM;
> > > >
> > > > @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
> > > >      }
> > > >  /* Active Directory is always case-insensitive */
> > > > dom->case_sensitive = false;
> > > > ret = confdb_get_string(cdb, mem_ctx, conf_path,
> > > >                        CONFDB_DOMAIN_CASE_SENSITIVE, "false",
> > > >                        &tmp);
> > > > if (ret != EOK) {
> > > >    DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
> > > >    goto done;
> > > > }
> > > > if (strcasecmp(tmp, "true") == 0) {
> > > >    DEBUG(SSSDBG_CRIT_FAILURE,
> > > >          "Warning: AD domain can not be set as
> > > >
> > > > case-sensitive.\n");
> > > >    dom->case_sensitive = false;
> > > >    dom->case_preserve = false;
> > > > } else if (strcasecmp(tmp, "false") == 0) {
> > > >    dom->case_sensitive = false;
> > > >    dom->case_preserve = false;
> > > > } else if (strcasecmp(tmp, "preserving") == 0) {
> > > >    dom->case_sensitive = false;
> > > >    dom->case_preserve = true;
> > > > } else {
> > > >    DEBUG(SSSDBG_FATAL_FAILURE,
> > > >          "Invalid value for %s\n", 
> > > >
> > > > CONFDB_DOMAIN_CASE_SENSITIVE);
> > > >    goto done;
> > > >
> > > > }
> > > >
> > > > /* Set this in the confdb so that the responders pick it
> > > > up when they start up.
> > > >
> > > > */
> > > > ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
> > > >                      dom->case_sensitive);
> > > > ret = confdb_set_string(cdb, conf_path, "case_sensitive",
> > > >                        tmp);
> > > > if (ret != EOK) {
> > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > >              "Could not set domain case-sensitive: [%s]\n",
> > > >
> > > > diff --git a/src/providers/ipa/ipa_selinux.c
> > > > b/src/providers/ipa/ipa_selinux.c
> > > > index 927e545..3cd1cc1 100644
> > > > --- a/src/providers/ipa/ipa_selinux.c
> > > > +++ b/src/providers/ipa/ipa_selinux.c
> > > > @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char
> > > > *orig_name,
> > > >      /* pam_selinux needs the username in the same format getpwnam()
> > > > would
> > > >       * return it
> > > >       */
> > > > username = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_sensitive);
> > > > username = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_preserve);
> > > >      if (username == NULL) {
> > > >          ret = ENOMEM;
> > > >          goto done;
> > > > diff --git a/src/responder/nss/nsssrv_cmd.c
> > > > b/src/responder/nss/nsssrv_cmd.c
> > > > index a168a3e..c201d3a 100644
> > > > --- a/src/responder/nss/nsssrv_cmd.c
> > > > +++ b/src/responder/nss/nsssrv_cmd.c
> > > > @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
> > > >              packet_initialized = true;
> > > >          }
> > > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_sensitive);
> > > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_preserve);
> > > >          if (tmpstr == NULL) {
> > > >              DEBUG(SSSDBG_CRIT_FAILURE,
> > > >                    "sss_get_cased_name failed, skipping\n");
> > > > @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
> > > >              }
> > > >          }
> > > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_sensitive);
> > > >    tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
> > > >
> > > > dom->case_preserve);
> > > >          if (tmpstr == NULL) {
> > > >              DEBUG(SSSDBG_CRIT_FAILURE,
> > > >                    "sss_get_cased_name failed, skipping\n");
> > > > -- 
> > > > 1.9.3
> > >
> > > I don't like very much the code duplication around parsing the option
> > > value case_sensitive.  Would you consider creating some utility 
> > > function
> > > that would do so and call it from ad_get_common_options() and
> > > confdb_get_domain_internal()?
> > > Something like:
> > >
> > > int sss_parse_case_sensitivity_option(const char* str)
> > > {
> > >          if (strcasecmp(str, "true") == 0) {
> > >              return CASE_INSENSITIVE;
> > >          } else if (strcasecmp(str, "false") == 0) {
> > >              return CASE_INSENSITIVE;
> > > ....
> >
> > I do not think this will help. If we added such function
> > than the only think that would change would be, that
> > the
> > 'if (strcasecmp(tmp, "something") == 0)'
> > lines would be replaced with
> > 'if (ret == SSS_SOMETHING)'.
> > Or maybe switch-case might be used, but the
> > lines would still have to be there. See that
> > I set different values in the AD specific code
> > and print additional debug message in one case
> > so it is not really duplication and would still
> > have to be handled outside of this utility
> > function, which is the reason why IMO such
> > function would only add code but would not be
> > very helpful -- what do you think?).

I agree with your concerns about my proposal, however I think you could 
reduce number of lines by setting default values for case_sensitive and 
case_preserve and then update them just for case where default value is 
not the right value.

But generally I don't think this is crucial for the patch. I'm fine if 
you don't do any further changes with this part of code. But thanks for 
considering.
> > > 3rd patch:
> > > > @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
> > > >                  </varlistentry>
> > > >              <varlistentry>
> > > >                <term>case_sensitive (boolean)</term>
> > > >                <term>case_sensitive (string)</term>
> > > >                <listitem>
> > > >                    <para>
> > > >                        Treat user and group names as case
> > > >
> > > > sensitive. At
> > > >                              the moment, this option is not 
> > > > supported in
> > > >                        the local provider.
> > > >                        the local provider. Possible options are:
> > > >                        True, False, Preserving. Preserving is 
> > > >
> > > > the
> > > >                        same as False (case insensitive), but 
> > > >
> > > > does
> > > >                        not lowercase names in the output of
> > > >
> > > > getpwnam
> > > >                        and getgrnam.
> > > >                    </para>
> > > >                    <para>
> > > >                        Default: True
> > >
> > > I wonder if it were better to list each option value on its own line.
> > > Something like:
> > >                        <para>
> > >                           Treat user and group names as case
> > >
> > > sensitive. At
> > >                               the moment, this option is not 
> > > supported in
> > >                        the local provider. Possible options are:
> > >                        True, False, Preserving. Preserving is the
> > >                        same as False (case insensitive), but does
> > >                        the local provider.
> > >                    </para>
> > >                    <para>
> > >                        Supported values:
> > >                    </para>
> > >                    <para>
> > >                        True: case insensitive
> > >                    </para>
> > >                    <para>
> > >                        True: case sensitive
> > >                    </para>
> > >                    <para>
> > >                        Preserving: case insensitive, but does
> > >                         not lowercase names in the output of
> > >
> > > getpwnam
> > >                               and g
> > >
> > > But I think this is a matter of personal taste so I leave it up to you.
> >
> > Ok, I added a list of possible options.

Thanks, I hope you agree it's better now.
>                        the local provider. Possible options are:

Could you change it to something like: "the local provider. Possible 
option values are:"? Hmm I'm not really sure with wording, I just think 
word 'value' should be part of the sentence.
> <variablelist>
>                        <varlistentry>

Could you also mention AD specifics for this option?
> > > Anyway, I think that man page changes must be acked-by native speaker
> > > (Stephen),
> > >
> > > Thanks,
>
> The 4th patch had wrong author in description. Sending the
> patches again.
>
> Michal
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I finally tested the patches and it seems to me to be working with AD 
and LDAP provider, but does not seem to work with IPA provider.

I was not able to create an user in IPA with some upper case so I had to 
change it directly in LDAP.

This is what I see when using ipa:

ipa user-find max
--------------
1 user matched
--------------
   User login: MaX
   First name: xx
   Last name: AAA
   Home directory: /home/max
   Login shell: /bin/sh
   Email address: [e-mail]
   UID: 1994000038
   GID: 1994000038
   Account disabled: False
   Password: False
   Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------

This is what I see when using sssd as ipa client:

case_sensitive = preserving:
getent group [e-mail]
[e-mail]:*:1994000038:

case_sensitive = false:
getent group [e-mail]
[e-mail]:*:1994000038:

case_sensitive = true:
does not return anything

Was the patch working with IPA provider to you?

Thanks!

[Pavel Reichl]

On 07/22/2014 02:03 PM, Pavel Reichl wrote:
> I finally tested the patches and it seems to me to be working with AD 
> and LDAP provider, but does not seem to work with IPA provider.
>
> I was not able to create an user in IPA with some upper case so I had 
> to change it directly in LDAP.
>
> This is what I see when using ipa:
> ipa user-find max
> 1 user matched
>
> User login: MaX
>   First name: xx
>   Last name: AAA
>   Home directory: /home/max
>   Login shell: /bin/sh
>   Email address: [e-mail]
>   UID: 1994000038
>   GID: 1994000038
>   Account disabled: False
>   Password: False
>   Kerberos keys available: False
> Number of entries returned 1
>
> This is what I see when using sssd as ipa client:
>
> case_sensitive = preserving:
> getent group [e-mail]
> [e-mail]:*:1994000038:
>
> case_sensitive = false:
> getent group [e-mail]
> [e-mail]:*:1994000038:
>
> case_sensitive = true:
> does not return anything
>
> Was the patch working with IPA provider to you?
>
> Thanks!

Michal this seems irrelevant now, I can't replicate it anymore. It must 
have been some quirk on my side. Sorry for that.

ACK to all patches except the 3rd (the man page update) which I can't ack.

Thanks!

[Michal Židek]

On 07/22/2014 02:49 PM, Pavel Reichl wrote:
> On 07/22/2014 02:03 PM, Pavel Reichl wrote:
> > I finally tested the patches and it seems to me to be working with AD
> > and LDAP provider, but does not seem to work with IPA provider.
> >
> > I was not able to create an user in IPA with some upper case so I had
> > to change it directly in LDAP.
> >
> > This is what I see when using ipa:
> > ipa user-find max
> > 1 user matched
> >
> > User login: MaX
> >   First name: xx
> >   Last name: AAA
> >   Home directory: /home/max
> >   Login shell: /bin/sh
> >   Email address: [e-mail]
> >   UID: 1994000038
> >   GID: 1994000038
> >   Account disabled: False
> >   Password: False
> >   Kerberos keys available: False
> > Number of entries returned 1
> >
> > This is what I see when using sssd as ipa client:
> >
> > case_sensitive = preserving:
> > getent group [e-mail]
> > [e-mail]:*:1994000038:
> >
> > case_sensitive = false:
> > getent group [e-mail]
> > [e-mail]:*:1994000038:
> >
> > case_sensitive = true:
> > does not return anything
> >
> > Was the patch working with IPA provider to you?
> >
> > Thanks!
>
> Michal this seems irrelevant now, I can't replicate it anymore. It must
> have been some quirk on my side. Sorry for that.
>
> ACK to all patches except the 3rd (the man page update) which I can't ack.
>
> Thanks!

Thanks, I updated the man page changes (added the "value" word
and AD comment). CC-ing Stephen for the man page review (3rd patch).

Michal

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/22/2014 09:32 AM, Michal Židek wrote:
> On 07/22/2014 02:49 PM, Pavel Reichl wrote:
> > On 07/22/2014 02:03 PM, Pavel Reichl wrote:
> > > I finally tested the patches and it seems to me to be working
> > > with AD and LDAP provider, but does not seem to work with IPA
> > > provider.
> > >
> > > I was not able to create an user in IPA with some upper case so
> > > I had to change it directly in LDAP.
> > >
> > > This is what I see when using ipa:
> > >
> > > ipa user-find max -------------- 1 user matched -------------- 
> > > User login: MaX First name: xx Last name: AAA Home directory:
> > > /home/max Login shell: /bin/sh Email address: [e-mail] UID:
> > > 1994000038 GID: 1994000038 Account disabled: False Password:
> > > False Kerberos keys available: False
> > > ---------------------------- Number of entries returned 1
> > >
> > > This is what I see when using sssd as ipa client:
> > >
> > > case_sensitive = preserving: getent group [e-mail] 
> > > [e-mail]:*:1994000038:
> > >
> > > case_sensitive = false: getent group [e-mail] 
> > > [e-mail]:*:1994000038:
> > >
> > > case_sensitive = true: does not return anything
> > >
> > > Was the patch working with IPA provider to you?
> > >
> > > Thanks!
> >
> > Michal this seems irrelevant now, I can't replicate it anymore.
> > It must have been some quirk on my side. Sorry for that.
> >
> > ACK to all patches except the 3rd (the man page update) which I
> > can't ack.
> >
> > Thanks!
>
> Thanks, I updated the man page changes (added the "value" word and
> AD comment). CC-ing Stephen for the man page review (3rd patch).
>
> Michal

Ack to the manpage language.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlPOaGEACgkQeiVVYja6o6NhvwCeIQ7upLLCa2PdfWha7tfssuSC
> hSoAn1blLO+hqsJi4vSiD7NCr692u5jo
> =uPQw
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

On Tue, Jul 22, 2014 at 09:34:25AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/22/2014 09:32 AM, Michal Židek wrote:
> > On 07/22/2014 02:49 PM, Pavel Reichl wrote:
> > > On 07/22/2014 02:03 PM, Pavel Reichl wrote:
> > > > I finally tested the patches and it seems to me to be working
> > > > with AD and LDAP provider, but does not seem to work with IPA
> > > > provider.
> > > >
> > > > I was not able to create an user in IPA with some upper case so
> > > > I had to change it directly in LDAP.
> > > >
> > > > This is what I see when using ipa:
> > > >
> > > > ipa user-find max -------------- 1 user matched -------------- 
> > > > User login: MaX First name: xx Last name: AAA Home directory:
> > > > /home/max Login shell: /bin/sh Email address: [e-mail] UID:
> > > > 1994000038 GID: 1994000038 Account disabled: False Password:
> > > > False Kerberos keys available: False
> > > > ---------------------------- Number of entries returned 1
> > > >
> > > > This is what I see when using sssd as ipa client:
> > > >
> > > > case_sensitive = preserving: getent group [e-mail] 
> > > > [e-mail]:*:1994000038:
> > > >
> > > > case_sensitive = false: getent group [e-mail] 
> > > > [e-mail]:*:1994000038:
> > > >
> > > > case_sensitive = true: does not return anything
> > > >
> > > > Was the patch working with IPA provider to you?
> > > >
> > > > Thanks!
> > >
> > > Michal this seems irrelevant now, I can't replicate it anymore.
> > > It must have been some quirk on my side. Sorry for that.
> > >
> > > ACK to all patches except the 3rd (the man page update) which I
> > > can't ack.
> > >
> > > Thanks!

Pushed to master:
    2b94ab415b30861f42b68725d9231905baf8c3bd
    abbf4f494f57c2b0a7ad0ac758db24a1c05df9be
    ff22e829fd73fc53027d1e6ca005a9ac334086dd
    5328aaeea84268b6d4e26cd33a2b3e8ea89bc349