[SSSD] [PATCHES] sss_case = preserving

Michal Židek mzidek at redhat.com
Mon Jul 21 16:29:06 UTC 2014


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-function-confdb_set_string.patch
Type: text/x-patch
Size: 3947 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140721/f491c9f9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-case_sensitivity-preserving.patch
Type: text/x-patch
Size: 6124 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140721/f491c9f9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-MAN-case_sensitivity-man-page-update.patch
Type: text/x-patch
Size: 2439 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140721/f491c9f9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Remove-unused-function-confdb_set_bool.patch
Type: text/x-patch
Size: 3079 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140721/f491c9f9/attachment-0003.bin>


More information about the sssd-devel mailing list