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.
Just one more nitpick: SSS_DB_CHECK_PTS and sss_db_version_check are
used only in sysdb.c, so there is no reason to have them defined
publicly in util.h+util.c. Move them both to sysdb.c please (you might
also want to rename them to better fit in sysdb.c).
Honza
--
Jan Cholasta