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

Jan Cholasta jcholast at redhat.com
Wed Nov 14 10:53:02 UTC 2012


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

-- 
Jan Cholasta



More information about the sssd-devel mailing list