On Thu, Sep 08, 2016 at 12:56:08PM +0200, Lukas Slebodnik wrote: > 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.
I'm not sure I understand..? We already have some undocumented options. I agree it's easy to discover the option if you look into the source, but if anyone is able to look at the source, I'm pretty sure they won't be suprised by LDIF messages in cache.. > 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. No, the recommended debug level would stay the same. Also, what about the environment variable? Do you think we should start recommending to also put LDB_LDIF_DEBUG=1 and debug_level=10/0xfff0? > > >> > >> 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. 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. 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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
