On (07/09/16 09:53), Jakub Hrozek wrote:
On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote:
> On (05/09/16 16:07), Jakub Hrozek wrote:
> >On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote:
> >> On (05/09/16 15:24), Jakub Hrozek wrote:
> >> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano FidĂȘncio wrote:
> >> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano FidĂȘncio
<fidencio(a)redhat.com> wrote:
> >> >> > Petr,
> >> >> >
> >> >> > I went through your patches and in general they look good to
me.
> >> >> > However, I haven't done any tests yet with your patches
(and I'll do
> >> >> > it after lunch).
> >> >>
> >> >> I've done some tests and I've been able to see the ldif
changes in the
> >> >> domain log. So, I assume it's working.
> >> >> For sure it's a good improvement! Would be worth to link some
> >> >> documentation about ldiff as it may be confusing for someone who
is
> >> >> not used to it.
> >> >>
> >> >> I'll wait for a new version of the patches and go through them
again.
> >> >>
> >> >> I really would like to have someone's else opinion on this
series.
> >> >
> >> >I quickly scrolled through the patches and the primary thing I
don't
> >> >understand is why are the wrappers used only in sysdb? I think we
should
> >> >just use them everywhere..
> >> I do not like wrappers.
> >> We should not log ldif by default.
> >
> >That's why there is a separate log level, you need to turn these on
> >separately (yes, logging LDIFs by default would be too much..)
> >
> Even though it is a separate debug level users still might
> enable them by a chance.
How, except reading the code? This new debug level is not documented
anywhere and even starts off at a different base so neither debug_level=10
nor debug_level=0xFFF0 will trigger this.
> IMH0 it will be confusing for them.
> There are many users which are confused when try to analyze
> sudo logs. They can see some "LDAP like" filters which
> are used for internal searching. Users think we use wrong attribute
> due to sudoRule -> sudoRole.
really? Someone who will discover a magic constant on SSSD will then be
confused by more logging?
btw what if this extra debugging was controlled by an environment
variable, do you think then it would be safer?
Eviroment variable and new debug_level are just a way how to enable logging.
But my main concern is that we should not log them.
Or if we prepend the LDIF with something like "SSSD cache
modification
message" ?
Our long standing goal is to make log files much better for users.
We have a big mess in debug_level and we log irelevant/confusing
messages with hight debug level.
Maybe I still miss the point but how this debug_level improves it.
And remeber that nothing is *undocumented* in open-source.
We(developers) are used to recomend full debul level for debugging
issues. So it would very soon to become a standart to require
debug_level 0xffff0. And if you coombine it with debug logs from AD
with enabled logging from nsupdate it will be a real mess.
>
> These wrappers should not be used in sssd upstream.
> They can be prepared for debugging purposes in development process.
...which means we will have to rebase the patches, build the correct
version, pass it on to the person and care about correct versioning...
Why should we rebase patches?
It rarely happen that we provides custom builds with extra debugging.
And everythime it is a different custom build for different user.
And it would be the same case with using custom builds with using
wrappers.
Sorry, I just don't agree with any of the arguments, but I'm
curious
to hear more.
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.
LS