Mailing List Archive: SpamAssassin: devel

[Bug 5965] header field with a value of "0" (zero) invisible

Index | Next | Previous | View Threaded

bugzilla-daemon at bugzilla

Aug 26, 2008, 11:35 AM

Post #1 of 11 (407 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

Mark Martinec <Mark.Martinec[at]ijs.si> changed:

What |Removed |Added
----------------------------------------------------------------------------
Severity|trivial |normal
Summary|custom rule with pattern |header field with a value of
|test of 0 (zero) not |"0" (zero) invisible
|processed |
Target Milestone|Undefined |3.3.0

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 26, 2008, 11:36 AM

Post #2 of 11 (394 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

Mark Martinec <Mark.Martinec[at]ijs.si> changed:

What |Removed |Added
----------------------------------------------------------------------------
Component|Rules (Eval Tests) |Libraries

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 27, 2008, 6:09 PM

Post #3 of 11 (382 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #3 from Mark Martinec <Mark.Martinec[at]ijs.si> 2008-08-27 18:09:36 PST ---
Now in trunk
(directly or indirectly related to this bug):

r689129 | mmartinec | 2008-08-26 18:28:52 +0200 (Tue, 26 Aug 2008)
bug 5965: do not treat user data as perl booleans (a string "0" is a false);
differentiate between missing and empty header fields; tweak header parsing

r689130 | mmartinec | 2008-08-26 18:53:14 +0200 (Tue, 26 Aug 2008)
fixed previous patch (hb separator test failing)

r689682 | mmartinec | 2008-08-28 02:44:56 +0200 (Thu, 28 Aug 2008)
- continue work on avoiding user data to be tested as perl booleans,
instead test for defined or for an empty string as appropriate;
- pms->get can now distinguish between empty and nonexistent header
fields, undef is returned for nonexistent header field unless a
default value argument is explicitly set to some defined value
like an empty string;
- modified calls to pms->get to deal with undef as appropriate;
- Conf.pm, Conf/Parser.pm and Plugin/Check.pm now work together and turn
a rule 'exists:name_of_header' into a defined(name_of_header) instead
of a name_of_header =~ /./ to match the documentation ("Define a
header existence test") and make it possible to distinguish empty
from nonexistent header fields; in principle the new code could allow
operators like 'eq' and 'ne' or function calls in header tests
in addition to regexp matching operators '=~' and '!~' (but this
is currently not allowed by the parser);
(plus some collateral small fixes)

The whole change is too extensive for a 3.2 branch, and as the original
bug report had a label of minor severity, I suggest this ticket be
closed now, changes will only be in 3.3.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 2:28 AM

Post #4 of 11 (378 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #4 from Justin Mason <jm[at]jmason.org> 2008-08-28 02:28:37 PST ---
broadly good, but -- I'm not fond of the side-effect change to the PMS::get()
API. This is a public API for third party plugins and eval rules, so this will
create a lot of noise when they call ->get() in the previous manner, then match
against the returned value without checking for undef first.

Is there some other way we can support our own internal code getting undef
returns, without breaking backwards compat on that API? I'm thinking a new,
wrapper method around ->get(), which passes a new "magic" value for the
optional $defval argument to the ->_get() method, indicating that returning
undef is acceptable, and if that's not present then the old style of returning
'' on undef is used.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 5:41 AM

Post #5 of 11 (379 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #5 from Mark Martinec <Mark.Martinec[at]ijs.si> 2008-08-28 05:41:13 PST ---
(In reply to comment #4)
> broadly good, but -- I'm not fond of the side-effect change to the PMS::get()
> API. This is a public API for third party plugins and eval rules, so this will
> create a lot of noise when they call ->get() in the previous manner, then match
> against the returned value without checking for undef first.
>
> Is there some other way we can support our own internal code getting undef
> returns, without breaking backwards compat on that API? [...]

There is a better way, I had it implemented for a while but scrapped it
because calls to get() could look weird. I'll make the following change
to the tail section of a get() and appropriate changes to calls
to retain compatibility:

currently:
return (defined $found ? $found : $_[2]);

new:
# if the requested header wasn't found, we should return a default value
# as specified by the caller: if defval argument is present it represents
# a default value even if undef; if defval argument is absent a default
# value is an empty string for upwards compatibility
return (defined $found ? $found : @_ > 2 ? $_[2] : '');

so now the:
$pms->get('Subject')
becomes equivalent to:
$pms->get('Subject','')

and if one wants to receive undef for nonexistent header, the
undef needs to be specified explicitly as a second argument:
$pms->get('Subject',undef)

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 5:50 AM

Post #6 of 11 (379 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #6 from Justin Mason <jm[at]jmason.org> 2008-08-28 05:50:21 PST ---
(In reply to comment #5)
> so now the:
> $pms->get('Subject')
> becomes equivalent to:
> $pms->get('Subject','')
>
> and if one wants to receive undef for nonexistent header, the
> undef needs to be specified explicitly as a second argument:
> $pms->get('Subject',undef)

perfect, thanks! I was trying to figure out a way to do that, but didn't see
it.

+1 from me ;)

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 6:55 AM

Post #7 of 11 (379 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #7 from Michael Parker <parkerm[at]pobox.com> 2008-08-28 06:55:46 PST ---
Just to chime in a bit, the get code is a real hotspot in our profile, so
please be very very very careful what what you are doing there. Run some
before and after profiles to make sure you didn't slow it down.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 7:45 AM

Post #8 of 11 (378 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #8 from Justin Mason <jm[at]jmason.org> 2008-08-28 07:45:14 PST ---
agreed, it's a hotspot. I did a quick check of performance of this morning's
SVN code, based on 10000 accesses to a cached header; the new code seemed
slightly slower, about 8% (for the get() method alone). imo that's OK, and can
be optimized later.

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 8:16 AM

Post #9 of 11 (379 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #9 from Mark Martinec <Mark.Martinec[at]ijs.si> 2008-08-28 08:16:04 PST ---
Done (compatibility):

r689835 | mmartinec | 2008-08-28 16:28:02 +0200 (Thu, 28 Aug 2008)
Changed PMS::get and its calls for compatibility regarding a
default value: if the requested header field wasn't found,
return a default value as specified by the caller: if defval
argument is present it represents a default value even if undef;
if defval argument is absent a default value is an empty string
for compatibility

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 8:58 AM

Post #10 of 11 (377 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #10 from Mark Martinec <Mark.Martinec[at]ijs.si> 2008-08-28 08:58:07 PST ---
> agreed, it's a hotspot. I did a quick check of performance of this morning's
> SVN code, based on 10000 accesses to a cached header; the new code seemed
> slightly slower, about 8% (for the get() method alone). imo that's OK, and can
> be optimized later.

One must take into account the actual hit/miss cache ratio.
Instrumenting the pms->get with two counters and letting it run for
a while on our production mail server gives 45% hits/attempts ratio.

While the hit code path is a bit slower, the miss path is much faster
(original code evaluates $_[0]->{c}->{$_[1]} multiple times),
which in my benchmark yields an overall improvement. With a
simulated and quick _get the improvement is 30%, yet in practice
the speedup is negligible because the actual _get is very slow
compared to the get().

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

bugzilla-daemon at bugzilla

Aug 28, 2008, 10:14 AM

Post #11 of 11 (378 views)
Permalink

[Bug 5965] header field with a value of "0" (zero) invisible [In reply to]

https://issues.apache.org/SpamAssassin/show_bug.cgi?id=5965

--- Comment #11 from Mark Martinec <Mark.Martinec[at]ijs.si> 2008-08-28 10:14:07 PST ---
...not to mention that a meager 300 calls of get() per message check
are negligible anyway...

--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com