From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4614D941E6 for ; Tue, 10 Jan 2023 13:36:24 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2E344A4E9 for ; Tue, 10 Jan 2023 13:36:24 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 10 Jan 2023 13:36:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D3EC04422A for ; Tue, 10 Jan 2023 13:36:22 +0100 (CET) Date: Tue, 10 Jan 2023 13:36:21 +0100 From: Wolfgang Bumiller To: Christian Ebner Cc: pve-devel@lists.proxmox.com Message-ID: <20230110123621.itumnpojjfjolfq3@fwblub> References: <20230109150706.446377-1-c.ebner@proxmox.com> <20230109150706.446377-3-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230109150706.446377-3-c.ebner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.213 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [tools.pm] Subject: Re: [pve-devel] [PATCH common 1/1] tools: Add callback based filtering for firewall logfiles X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Jan 2023 12:36:24 -0000 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 > --- > 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; 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