On 14.11.2012 14:03, Ondrej Kos wrote:
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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
