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

Ondrej Kos okos at redhat.com
Mon Nov 19 07:45:35 UTC 2012


On 11/16/2012 04:40 PM, Jan Cholasta wrote:
> 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
>

patch attached.

Ondra

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: +420-532-294-558
cell:  +420-736-417-909
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Display-more-information-on-DB-version-crash.patch
Type: text/x-patch
Size: 7624 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121119/f27e5098/attachment.bin>


More information about the sssd-devel mailing list