[pve-devel] [PATCH common 1/1] tools: Add callback based filtering for firewall logfiles

[Christian Ebner]

Thank you for the detailed review Wolfgang, I am still unsure about one of your comments, see below. Maybe you could clarify.

Thx.

> On 10.01.2023 13:36 CET Wolfgang Bumiller <w.bumiller at proxmox.com> wrote:
> 
>  
> On Mon, Jan 09, 2023 at 04:07:06PM +0100, Christian Ebner wrote:
> > This patch introduces callback based filtering functionality for firewall logs.
> > In addition, the contents of rotated logfiles are included by setting the
> > `include_rotated_logs` flag.
> > 
> > Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> > ---
> >  src/PVE/Tools.pm | 108 +++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 91 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> > index cdbee6d..cafc2f7 100644
> > --- a/src/PVE/Tools.pm
> > +++ b/src/PVE/Tools.pm
> > @@ -17,6 +17,7 @@ use IO::Handle;
> >  use IO::Pipe;
> >  use IO::Select;
> >  use IO::Socket::IP;
> > +use IO::Zlib;
> >  use IPC::Open3;
> >  use JSON;
> >  use POSIX qw(EINTR EEXIST EOPNOTSUPP);
> > @@ -1265,29 +1266,19 @@ sub split_args {
> >      return $str ? [ Text::ParseWords::shellwords($str) ] : [];
> >  }
> >  
> > -sub dump_logfile {
> > -    my ($filename, $start, $limit, $filter) = @_;
> > -
> > -    my $lines = [];
> > -    my $count = 0;
> > -
> > -    my $fh = IO::File->new($filename, "r");
> > -    if (!$fh) {
> > -	$count++;
> > -	push @$lines, { n => $count, t => "unable to open file - $!"};
> > -	return ($count, $lines);
> > -    }
> > +sub dump_logfile_by_filehandle {
> > +    my ($fh, $start, $limit, $filter, $count, $lines, $read_until_end) = @_;
> 
> ^ I think it'll be easier if we move start, limit, count and lines into
> a `$state` hash, and keep $read_until_end internal here.
> This way we don't need to pass them back and forth via paramters &
> return values.
> 
> We can pull them out as needed here and also do the defaults, eg.
> 
>     my $limit = ($state->{limit} //= 50);
>     my $count = ($state->{count} //= 0);
>     my $lines = ($state->{lines} //= []);
>     my $read_until_end = $limit == 0;
> 
I don't see why $start should only be set to a value != 0 (if given anyway) for the first file.

The line/count the output should start from might be located in any file. So
    my $start = ($state->{start} //= 0);
would make more sense to me, or am I missing something?
> and for $start I think it should only hit the first file anyway:
> 
>     my $start = delete($state->{start}) // 0;
> 
> >  
> > -    $start = $start // 0;
> > -    $limit = $limit // 50;
> > -
> > -    my $read_until_end = $limit == 0;
> >      my $line;
> >  
> >      if ($filter) {
> >  	# duplicate code, so that we do not slow down normal path
> >  	while (defined($line = <$fh>)) {
> > -	    next if $line !~ m/$filter/;
> > +	    if (ref($filter) eq 'CODE') {
> > +		next if !$filter->($line);
> > +	    } else {
> > +		next if $line !~ m/$filter/;
> > +	    }
> >  	    next if $count++ < $start;
> >  	    if (!$read_until_end) {
> >  		next if $limit <= 0;
> > @@ -1308,6 +1299,29 @@ sub dump_logfile {
> >  	}
> >      }
> 
> and then fill the numbers back into the hash here and return nothing.
> ($lines would be a reference, so no need to write it back)
> >  
> > +    return ($count, $lines, $limit);
> > +}
> > +
> > +sub dump_logfile {
> > +    my ($filename, $start, $limit, $filter) = @_;
> > +
> > +    $start = $start // 0;
> > +    $limit = $limit // 50;
> > +
> > +    my $read_until_end = $limit == 0;
> > +    my $lines = [];
> > +    my $count = 0;
> 
> ^ then the above won't be needed
> 
> > +
> > +    my $fh = IO::File->new($filename, "r");
> > +    if (!$fh) {
> > +	$count++;
> > +	push @$lines, { n => $count, t => "unable to open file - $!"};
> > +	return ($count, $lines);
> 
> ^ this doesn't really need $count or $lines since they're 0 and [], so
> this could just do
>     return (1, [{ n => 1, t => "..."}]);
> directly
> 
> > +    }
> > +
> > +    ($count, $lines, $limit) = dump_logfile_by_filehandle(
> > +	$fh, $start, $limit, $filter, $count, $lines, $read_until_end);
> > +
> >      close($fh);
> >  
> >      # HACK: ExtJS store.guaranteeRange() does not like empty array
> 
> ^ Maybe the hack part should happen in `dump_logfile_by_filehandle` by
> passing a `$final` parameter to?
> 
> > @@ -1320,6 +1334,66 @@ sub dump_logfile {
> >      return ($count, $lines);
> >  }
> >  
> > +sub dump_fw_logfile {
> 
> Because then *this* one here an actually go into pve-firewall and also
> drop the start/limit default logic.
> 
> > +    my ($filename, $start, $limit, $filter, $include_rotated_logs) = @_;
> > +
> > +    if (!$include_rotated_logs) {
> > +	return dump_logfile($filename, $start, $limit, $filter);
> > +    }
> > +
> > +    $start = $start // 0;
> > +    $limit = $limit // 50;
> > +
> > +    # Take into consideration also rotated logs                                              
> 
> ^ trailing whitespace
> 
> > +    my ($basename, $logdir, $type) = fileparse($filename);
> > +    my @files = ();                                                                          
> 
> ^ trailing whitespace
> 
> > +
> > +    opendir(LOGDIR, $logdir) || die "Cannot open $logdir";
> > +    my $entry;
> > +    while ($entry = readdir(LOGDIR)) {
> > +	my $namefilter = $basename."*";
> 
> ^ This regex looks wrong (glob vs regex?) and should be built outside
> the loop as it stays the same
> 
> Maybe our `dir_glob_foreach()` can shorten this whole block?
> 
> > +	next if $entry !~ m/$namefilter/;
> > +	push @files, $entry;
> > +    }
> > +    closedir(LOGDIR);
> > +    @files = reverse sort @files;
> > +
> > +    my $read_until_end = $limit == 0;
> > +    my $lines = [];
> > +    my $count = 0;
> > +
> > +    foreach (@files) {
> 
> IMO a bit too large to use $_.
> Use
>     for my $filename (@$files) {
> 
> > +	my ($base, $path, $type) = fileparse($_, ".gz");
> 
> You use neither $base nor $path, a simple `if ($filename =~ /\.gz$/) {`
> should suffice.
> 
> > +
> > +	my $fh;
> > +	if ($type eq '.gz') {
> > +	    $fh = IO::Zlib->new($logdir.$_, "r");
> > +	} else {
> > +	    $fh = IO::File->new($logdir.$_, "r");
> > +	}
> > +
> > +	if (!$fh) {
> > +	    $count++;
> > +	    push @$lines, { n => $count, t => "unable to open file - $!"};
> 
> I'd like this error to only happen if the error isn't ENOENT, as that
> would just be a TOCTOU race against a cleanup.
> 
> > +	    return ($count, $lines);
> 
> ^ And just break out of the loop instead of returning.
> 
> > +	}
> > +
> > +	($count, $lines, $limit) = dump_logfile_by_filehandle(
> > +	    $fh, $start, $limit, $filter, $count, $lines, $read_until_end);
> > +
> > +	close($fh);
> > +    }
> > +
> > +    # HACK: ExtJS store.guaranteeRange() does not like empty array
> > +    # so we add a line
> > +    if (!$count) {
> > +	$count++;
> > +	push @$lines, { n => $count, t => "no content"};
> > +    }
> > +
> > +    return ($count, $lines);
> > +}
> > +
> >  sub dump_journal {
> >      my ($start, $limit, $since, $until, $service) = @_;
> >  
> > -- 
> > 2.30.2