I never would have found that. Nice job. Any chance you can create a regression test for the issue ?
On Thu, Nov 28, 2019, 05:09 Henrik K <h...@hege.li> wrote: > > Fixed: > http://svn.apache.org/viewvc?view=revision&revision=1870552 > > On Thu, Nov 28, 2019 at 11:29:19AM +0200, Henrik K wrote: > > > > Trunk has already many revamps and fixes for these codes, works there. > It > > seems 3.4 askdns has trouble using those, investigating minimal fix.. > > > > > > On Thu, Nov 28, 2019 at 10:20:39AM +0100, Tobi <jahli...@gmx.ch> wrote: > > > Henrik > > > > > > But my current testing clearly shows that without the explicit set_tag > > > _LASTEXTERNALRDNS_ and _LASTEXTERNALHELO_ won't be set. > > > I'm testing this using spamassassin in debug mode to scan a > testmessage. > > > As soon as I re-add the set_tag to PerMsgStatus.pm the tags are set and > > > tests based on that tags are run. Removing the lines from pm and debug > > > shows that the tests are **not** run anymore. > > > > > > So I somewhat doubt that the set_tag "code is redundant and should be > > > removed" :-) > > > > > > Using: SA 3.4.2 on centos7 / perl 5.16.3 > > > > > > > > > Cheers > > > > > > tobi > > > > > > Am 28.11.19 um 08:36 schrieb Henrik K: > > > > > > > > There is nothing missing. All the LASTEXT* tags are already > dynamically > > > > returning functions. If anything, that if ($lasthop) set_tag code is > > > > completely redundant and should be removed. > > > > > > > > > > > > BEGIN { > > > > LASTEXTERNALIP => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{ip} : ''; > > > > }, > > > > > > > > LASTEXTERNALRDNS => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{rdns} : ''; > > > > }, > > > > > > > > LASTEXTERNALHELO => sub { > > > > my $pms = shift; > > > > my $lasthop = $pms->{relays_external}->[0]; > > > > $lasthop ? $lasthop->{helo} : ''; > > > > }, > > > > > > > > > > > > On Wed, Nov 27, 2019 at 10:18:20AM -0500, Kevin A. McGrail wrote: > > > >> After a 10 minute or so study of the issue and comparing 3.4 and > trunk, > > > >> it definitely looks like the code is missing. I am not 100% sure > your > > > >> fix works but it's better than it currently exists :-) > > > >> > > > >> Have you been using that change in production? > > > >> > > > >> Regards, > > > >> > > > >> KAM > > > >> > > > >> On 11/27/2019 6:59 AM, Tobi <jahli...@gmx.ch> wrote: > > > >>> Hi, > > > >>> > > > >>> is there any specific reason why the two tags mentioned in subject > are > > > >>> not set in SA? It took me a while to find out why an askdns test > was not > > > >>> running. The test relies on _LASTEXTERNALRDNS_ but after running > with > > > >>> --debug I found that those tags are not set by SA. Although > > > >>> _LASTEXTERNALIP_ is set. Also the man page states that the two tags > > > >>> should exist. > > > >>> > > > >>> In PerMsgStatus.pm I saw that everything is prepared for the two > tags > > > >>> but they finally not set (around line 1721). So I changed to > > > >>> > > > >>>> if ($lasthop) { > > > >>>> $self->set_tag('LASTEXTERNALIP', $lasthop->{ip}); > > > >>>> $self->set_tag('LASTEXTERNALRDNS', $lasthop->{rdns}); > > > >>>> $self->set_tag('LASTEXTERNALHELO', $lasthop->{helo}); > > > >>>> } > > > >>> > > > >>> and --debug shows that the tags are now set and the askdns test is > run. > > > >>> > > > >>> So I wonder if the code has just been forgotten or if there are > deeper > > > >>> reasons to not set the tags? > > > >>> If no deeper reasons exist it would be nice to have those two tags > set > > > >>> as default in PerMsgStatus.pm > > > >>> > > > >>> > > > >>> Cheers > > > >>> > > > >>> -- > > > >>> > > > >>> tobi > > > >> > > > >> -- > > > >> Kevin A. McGrail > > > >> kmcgr...@apache.org > > > >> > > > >> Member, Apache Software Foundation > > > >> Chair Emeritus Apache SpamAssassin Project > > > >> https://www.linkedin.com/in/kmcgrail - 703.798.0171 > > > > >
