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

[maks attems]

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.
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).
 
> 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.
> 
> 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, 
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!
 
> 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.
 
> 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!

i already closed your above named bug, because of the strange
confusion in your proposed manpage. good bits like the FILES
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.

 
> 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.
 
--
maks