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
There's been quite a few cases where I wanted to see what exactly
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.)