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

Jan Cholasta jcholast at redhat.com
Thu Nov 15 14:03:31 UTC 2012


On 14.11.2012 16:20, Ondrej Kos wrote:
> On 11/14/2012 03:38 PM, Simo Sorce wrote:
>> On Wed, 2012-11-14 at 15:18 +0100, Jan Cholasta wrote:
>>>
>>> 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).
>>>
>>
>> Yes please, rename them to be sysdb_* functions and move them in sysdb
>> where they belong.
>> The util/ shouldn't have component specific functions.
>>
>> Simo.
>>
>
> moved all code to sysdb, new patch attached.
>
> O.
>
>

OK, perhaps I should have been more clear with that last nitpick: 
sysdb_version_check and SYSDB_CHECK_PTS are used only by sysdb.c 
internally, there is no need to put them in *any* header file. Please 
keep them *only* is sysdb.c and make sysdb_version_check static. 
Speaking of SYSDB_CHECK_PTS, I see no point in having it #defined, as it 
is used solely by sysdb_version_check and changing its value doesn't 
really do anything other than breaking sysdb_version_check.

Also I have done more intense testing and found a few more issues:

1) sss_cache does not print the error message:

# sss_cache -u baduser --debug 10
...
(Thu Nov 15 06:38:56:755800 2012) [sss_cache] 
[sysdb_domain_init_internal] (0x0010): Wrong DB version (got 0.13 
expected 0.12)
(Thu Nov 15 06:38:56:756043 2012) [sss_cache] [init_domains] (0x0020): 
Could not initialize connection to the sysdb
Could not open available domains
(Thu Nov 15 06:38:56:756426 2012) [sss_cache] [init_context] (0x0040): 
Initialization of sysdb connections failed
...

This is because sysdb_init is called in two places in sss_cache, but 
only one includes SYSDB_MIS_CHECK.

2) The "DB version mismatch!" error message seems redundant, as the 
"Unknown DB version ..." or "Wrong DB version ..." debug message is 
always printed right before it in both daemons and tools in all (?) 
debug levels.

3) I have noticed that the common practice for multi-statement macros in 
SSSD is to wrap them in "do { stuff } while(0)". SYSDB_MIS_CHECK should 
probably do the same.

4) The "Higher version of database is expected" error message is 
actually never printed in SSSD. While this is fine (in this case it is 
not really an error, because SSSD can handle this situation well by 
upgrading the database), the in_tool check for this in 
sysdb_version_mismatch should be fixed.

5) There are some errors in the "remove cache files" message:
    a) I think it should say "Removing cache files" instead of "Removing 
cache*d* files".
    b) The path "/var/run/sss/db" is wrong, use the DB_PATH macro here 
instead.
    c) The comma in "... be aware, that ..." does not feel right to me.

6) Can we turn the whole sysdb_version_mismatch function into a macro? 
Or perhaps two macros, one for use in daemons and the other for use in 
tools? I have something like this in mind (also fixes points 2 to 5):

#define SYSDB_VERSION_ERROR_HINT \
     ERROR("Removing cache files in "DB_PATH" should fix the issue, " \
           "but note that removing cache files will also remove all of 
your " \
           "cached credentials.\n")

#define SYSDB_VERSION_LOWER_ERROR(ret) do { \
     if (ret == EUCLEAN) { \
         ERROR("Lower version of database is expected!\n"); \
         SYSDB_VERSION_ERROR_HINT; \
     } \
} while(0)

#define SYSDB_VERSION_HIGHER_ERROR(ret) do { \
     if (ret == EMEDIUMTYPE) { \
         ERROR("Higher version of database is expected!\n"); \
         SYSDB_VERSION_ERROR_HINT; \
     } \
} while(0)

/* use this in daemons */
#define SYSDB_VERSION_ERROR_DAEMON(ret) \
     SYSDB_VERSION_LOWER_ERROR(ret)

/* use this in tools */
#define SYSDB_VERSION_ERROR(ret) \
     SYSDB_VERSION_LOWER_ERROR(ret); \
     SYSDB_VERSION_HIGHER_ERROR(ret)

Honza

-- 
Jan Cholasta



More information about the sssd-devel mailing list