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

Ondrej Kos okos at redhat.com
Fri Nov 16 14:25:03 UTC 2012


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.

-- 
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: 7681 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121116/bb330da4/attachment.bin>


More information about the sssd-devel mailing list