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

Jan Cholasta jcholast at redhat.com
Fri Nov 16 15:40:41 UTC 2012


On 16.11.2012 15:25, Ondrej Kos wrote:
> On 11/15/2012 03:03 PM, Jan Cholasta wrote:
>> 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
>>
> great idea.
>
> new patch attached
>
> O.
>

Thank you.

Replace this:

+        ERROR("Higher version of database is expected!\n"); \
+        SYSDB_VERSION_ERROR_HINT; \
+        ERROR("In this case, you could also try running SSSD, " \
+              "which should handle the database upgrade.\n"); \

with this:

+        ERROR("Higher version of database is expected!\n"); \
+        ERROR("In order to upgrade the database, you must run SSSD.\n"); \
+        SYSDB_VERSION_ERROR_HINT; \

and it's an ACK.

(Also it wouldn't hurt if a native English speaker checked the error 
messages for correct grammar.)

Honza

-- 
Jan Cholasta



More information about the sssd-devel mailing list