[SSSD] [PATCH] Display more information on DB version crash

Ondrej Kos okos at redhat.com
Wed Nov 14 13:03:26 UTC 2012


On 11/14/2012 11:53 AM, Jan Cholasta wrote:
> On 13.11.2012 15:41, Ondrej Kos wrote:
>> On 11/12/2012 01:14 PM, Jan Cholasta wrote:
>>> On 9.11.2012 13:24, Ondrej Kos wrote:
>>>> On 11/08/2012 07:01 PM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 8.11.2012 15:05, Ondrej Kos wrote:
>>>>>> https://fedorahosted.org/sssd/ticket/1589
>>>>>>
>>>>>> patch is attached
>>>>>>
>>>>>> O.
>>>>>>
>>>>>
>>>>> 1) I think monitor and responders should use a different error
>>>>> message,
>>>>> excluding "If greater version is expected, run SSSD ...". Telling
>>>>> users
>>>>> to run SSSD when they in fact have it running might be confusing for
>>>>> someone.
>>>>>
>>>>> It might be even better to have different error messages for when the
>>>>> version is greater than expected and when the version is lesser than
>>>>> expected (not sure how hard it would be to implement that).
>>>>>
>>>>> 2) You should check for EMEDIUMTYPE in sss_cache.c and tools_util.c
>>>>> right after sysdb_init_domain_and_sysdb is called IMO.
>>>>>
>>>>> 3) I would prefer if you did not suppress logging of debug messages
>>>>> when
>>>>> ret == EMEDIUMTYPE.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> 1 - distinguished whether reporting from daemon or tool, added another
>>>> errno return value - EUCLEAN meaning database version is higher then
>>>> expected
>>>>
>>>> 2 - fixed
>>>>
>>>> 3 - fixed
>>>>
>>>> new patch attached
>>>>
>>>> O.
>>>>
>>>
>>> +errno_t
>>> +sysdb_version_diff(const char *expected,
>>> +                           const char *received)
>>> +{
>>> +    return (atof(expected) < atof(received)) ? EUCLEAN : EMEDIUMTYPE;
>>> +}
>>> +
>>>
>>> I don't think you can use atof here, as it is locale-dependent. For
>>> example, when the current locale is cs_CZ, then atof("0.13") == 0.0,
>>> because in Czech the decimal mark is ",", so atof will stop parsing when
>>> it hits the ".".
>>>
>>> I think it would be nice (though not necessary) to add a generic
>>> function for comparing version strings to util and use it for the
>>> comparison.
>>>
>>>
>>> You use this:
>>>
>>> +        if (ret == EMEDIUMTYPE || ret == EUCLEAN) {
>>> +            sss_db_version_mismatch(ret, true);
>>> +        }
>>>
>>> everywhere in the patch, I think you should make a macro out of it.
>>>
>>>
>>> Please add src/util/util.c to po/POTFILES.in, so that gettext can pick
>>> up the error messages you have added for translation.
>>>
>>>
>>> Honza
>>>
>>
>> New patch attached (without adding src/util/util.c to po/POTFILES.in
>> since there's new bug filed for it)
>>
>> O.
>>
>>
>
> I'm afraid this is not a very good solution:
>
> +double
> +sss_db_version_str_to_num(char *str)
> +{
> +    int i;
> +    char *c;
> +    bool sep_occured = false;
> +
> +    for (i = 0, c = str; c[i]; i++) {
> +        if (c[i] == ',' || c[i] == '.') {
> +            if (sep_occured) {
> +                return NAN;
> +            }
> +            c[i] = '.';
> +            sep_occured = true;
> +        } else if (!isdigit(c[i])) {
> +            return NAN;
> +        }
> +    }
> +    return atof(str);
> +}
>
> This fixes it for Czech, but what about languages that do not use "."
> and neither "," as a decimal mark? Also I don't think using floats for
> this is a particularly good idea. I think we can use something like this
> instead:
>
> errno_t
> sss_db_version_check(const char *expected,
>                       const char *received)
> {
>      int ret;
>      unsigned int exp_major, exp_minor, recv_major, recv_minor;
>
>      ret = sscanf(expected, "%u.%u", &exp_major, &exp_minor);
>      if (ret != 2) {
>          return EINVAL;
>      }
>      ret = sscanf(receibed, "%u.%u", &recv_major, &recv_minor);
>      if (ret != 2) {
>          return EINVAL;
>      }
>
>      if (exp_major < recv_major) {
>          return EUCLEAN;
>      } else if (exp_major > recv_major) {
>          return EMEDIUMTYPE;
>      }
>
>      if (exp_minor < recv_minor) {
>          return EUCLEAN;
>      } else if (exp_minor > recv_minor) {
>          return EMEDIUMTYPE;
>      }
>
>      return EOK;
> }
>
> Honza
>

I misunderstood the situation with decimal mark. Here's new patch.

O.

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: +420-532-294-558
cell:  +420-736-417-909
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Display-more-information-on-DB-version-crash.patch
Type: text/x-patch
Size: 7515 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121114/b479a6bf/attachment.bin>


More information about the sssd-devel mailing list