Fair enough. Ok to move forward with another rc do.you think? On Thu, Nov 28, 2019, 05:40 Henrik K <h...@hege.li> wrote:
> > It's not that the issue itself needs a test. AskDNS and/or tag handling > currently have no tests at all. In addition there's some open bugs looking > at the ways to use AskDNS and it's shortcomings (empty tags, meta tests > breaking on askdns etc). TLDR: It actually requires a lot of work, don't > have time or stamina right now. > > On Thu, Nov 28, 2019 at 05:33:03AM -0500, Kevin A. McGrail wrote: > > 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 <[1]h...@hege.li> wrote: > > > > > > Fixed: > > [2]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 <[3]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 <[4]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 > > > > >> [5]https://www.linkedin.com/in/kmcgrail - 703.798.0171 > > > > > > > > > > > References: > > > > [1] mailto:h...@hege.li > > [2] http://svn.apache.org/viewvc?view=revision&revision=1870552 > > [3] mailto:jahli...@gmx.ch > > [4] mailto:jahli...@gmx.ch > > [5] https://www.linkedin.com/in/kmcgrail >
