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 >> >> >> <fiden...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
