On Mon, Nov 19, 2012 at 08:45:35AM +0100, Ondrej Kos wrote:
> 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
Ack
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
