Bug#272047: [Logcheck-devel] Bug#272047: logcheck's greplogoutput code needs cleaning

[Ross Boylan]

On Sat, Sep 18, 2004 at 11:17:50PM +0200, maks attems wrote:
> severity 272047 wishlist
> thanks
> 
> On Thu, 16 Sep 2004, Ross Boylan wrote:
> 
> > Package: logcheck
> > Version: 1.2.25
> > Severity: normal
> > 
> > Unless my understanding of shell scripts is off, there seem to be
> > quite a few problems with the greplogoutput routine.  First,
> > individual sections of the code contain comments that clearly do not
> > match what the code is doing.  Second, the overall sequencing and flow
> > of the tests (as written--perhaps less so as commented) doesn't make
> > sense.
> 
> greplogoutput is completly _b0rked_, that's the only point i agree with you.

Good.  That's the main thing.

> i tried to rewrite it at some point in june, but failed somehow.
> with sarge approaching we opted for a surprisingly working redundancy.
> (we is meant for the debian logcheck team).

Sounds like a reasonable decision.  It would be good to fix it up for
later releases.

>  
> > Consider
> > 		# Now ignore all entries from the logcheck-<package> files
> > 		if [ -n "$ignore" ]; then
> > 		    debug "Applying Logcheck override files"
> > 		    for file in $(ls -1 $ignore/) ; do
> > 		        debug "clean logcheck-<package>: $file"
> > 			cleanchecked "$ignore/$file"
> > 		    done
> > 		else
> > The code is actually using every single ignore file, though the
> > comments say it should be using only the logcheck-* files.
> > 
Although you say you don't agree on any points, I assume you  agree
with the previous one :)

> > Given that, the subsequent efforts to pick out various subsets of the
> > ignore files are completely redundant.
> > 
> > Now even if only logcheck-* were run, the subsequent section
> > 
> > 		debug "Cleaning logcheck"
> > 		# Remove any  entries already reported
> > 		for file in $(ls $raise/ | grep -v '^logcheck') ; do
> > 		    debug "Cleaning logcheck: $file"
> > 		    cleanchecked "$raise/$file"
> > 		done
> > doesn't make much sense.  The very first action of greplogoutput is to
> > look for an ignore file of the same name, which in this case must be
> > logcheck.  The previous code, if it followed the comments, would have
> > got all the logcheck-* files.  So this section redoes all those things
> > that have already been done.  It's only "contribution" is to pick up
> > logcheck* files that aren't logcheck-*, and it's not clear to me
> > that's desirable (or that there are likely to be any such files).
> 
> hmm i've longer no more looked at this function, 
I'm not sure what he preceding sentence means; I assume it's that you
haven't looked at the function in a while.

> but your analysis is wrong!
> the first for loop picks file out of $ignore and the second out of $raise
> that's a big difference!
Agreed.  Sorry; I was sleepy.

>  
> > Finally, the overall logic according to the comments seems
> > unnecessarily complex.  For every package foo, the ignore files named
> > foo
> > logcheck-foo
> > local
> > local-*
> > are ignored.  Why not local-foo instead of local-*?
> 
> there could be strings raised by logcheck-postfix
> that are ignored by local-postdrop or whatever.

As the next sentence in the original indicated, my comments were
addressed to the non-logcheck-* cases.  In particular, if you're
dealing with package foo, why use the ignore patterns in local-bar?

>  
> > Then there is the added wrinkle of the separate handling when the
> > "raise" file name is logcheck.
> > 
> > I encourage you to decide what you want this code to do  and then make
> > the code and the comments match that behavior.  Finally, *please*
> > expand the manual page to document what the behavior is.  My patch in
> > bug #215640 provides such a revised man page.  I believe it reflected
> > the actual behavior of the code at one point; there may have been
> > changes since, and it will need to reflect whatever decisions are made
> > dealing with the issues raised above.
> 
> you are welcome to send patches to clean off greplogoutput,
> seperated patches for comment fixes and code cleanup are welcome.
> needless to say that they should be well tested 
> and work on different setups!

It's impossible to fix it without knowing what the desired behavior
is, and I don't know what it is.  Maybe the file you cite at the
bottom of your message gives that.

The other thing to be considered is that a lot of systems are probably
relying on the current behavior, so there are migration and
compatibility issues.

> 
> i already closed your above named bug, because of the strange
> confusion in your proposed manpage. good bits like the FILES
What confusion?
> section and discussion of *all* logchek options is already done!
> after reading this bug report i'm even less inclined to look
> at your misinterpretations.

Well, this probably belongs in the thread about that bug, but even if
you don't like my patches, please don't close it until the
underlying problem is solved.  That problem is that you can't tell
from the manpage how the program operates.  And there are issues with
other docs as well.

Historically, note that some of the bits that are done are done
because you (maks) incorporated them from 215640 into the official
release.  However, there are substantial parts that are not there--in
fact, almost all of the most significant parts.  For that reason, bug
215640 was already closed and reopened by a logcheck maintainer once
before.

> 
>  
> > I really would like to avoid the necessity of rereading the code every
> > time I want to figure out how to fill in the configuration files :)
> 
>  /usr/share/doc/logcheck-database/README.logcheck-database.gz
>  should be enough.
>  

Thanks for the pointer; it looks really useful.  Perhaps that's the
specification logcheck should try to match.  Given the issues above, I
doubt it accurately describes the current behavior.  In terms of
documentation, minimally there should be a pointer to this from the
logcheck manpage.  Since it describes the behavior of the program, it
might be good to move it to the main logcheck package.  But I'm
wandering from the original subject of this bug.