On (11/09/16 23:49), Jakub Hrozek wrote: >On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote: >> Let me explain why wrappers are not good idea in production. >> There was introduced new wrapper(#1991) for ldb_search >> SSS_LDB_SEARCH. It should guarantee that ENONET will be >> returned and not EOK + res->count == 0. >> >> I found just a single usage of this wrapper since introducing >> but many usage of ldb_search (I stopped counting after 10). >> And there will be the same problem with newly introduced wrappers. >> It's crystal clear that review does not help. Otherwise we would use >> SSS_LDB_SEARCH everywhere. >> >> That is a reason why I am fine with using wrappers just for a for development >> but not for productions. Or try to propose some automatic way >> how to simply log ldifs for *ALL* required ldb functions. >> IMHO, it would be the best to implement it in libldb itself. > >You have a point here (and I regret adding the ENOENT retval in general, >but the difference is that ldb_search wrapper changes /functionality/, this >just adds logging. So the only thing we would miss if we forget to use >the wrapper is the extra debugging. And in that case we would have to >build a new package and commit the messages to master, but that's no >different from missing debug messages in general. > Inconsistencies are bad. it does not matter wheter it's about ENOENT or logging.
>There's been quite a few cases where I wanted to see what exactly is >being added for example with duplicate member: attributes or a member >attribute that points nowhere or 'binary' attributes. These patches >would solve it nicely. And for such few cases you can prepare custom build of sssd. I am fine with first 3 patches but rest of patches (replacement ldb_* with wrappers) have NACK or a) you can find other way how to consistently log messages from ldb_ functions BTW you will still be able to prepare test builds for debugging because wrappers will be part of upstream (but will be ununsed by default.) LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
