|
SA Bugzilla – Full Text Bug Listing |
Summary: | provide info about missing/disabled meta dependencies | ||
---|---|---|---|
Product: | Spamassassin | Reporter: | Loren Wilton <lwilton> |
Component: | spamassassin | Assignee: | SpamAssassin Developer Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | levkoy, mail, wtogami |
Priority: | P4 | ||
Version: | SVN Trunk (Latest Devel Version) | ||
Target Milestone: | 3.2.0 | ||
Hardware: | Other | ||
OS: | other | ||
Whiteboard: | (skip to comment #49) | ||
Bug Depends on: | |||
Bug Blocks: | 5085 | ||
Attachments: |
meta lint patch
info only patch |
since in 3.1 we don't even report an error on that line... moving to the 3.1 queue. btw, there are better error messages in 3.1, if the code actually validates the input. the standard ones do, the options that have its own code may or may not. as an example: [25654] warn: config: SpamAssassin failed to parse line, "" is not valid for "required_score", skipping: required_score bumping up priority... Subject: Re: report_safe errors give crummy error messages > btw, there are better error messages in 3.1, if the code actually validates the input. the standard ones do, > the options that have its own code may or may not. as an example: Yup, knew that. Didn't recall which release that had been cleaned up in, but figured this one might have been missed. the issue is larger than report_safe. perhaps I didn't make that as clear as I could have. we need to go through all the default config options and make sure they appropriately check values and return errors on failure. it's not a blocking issue really, but in some places we're more lax than I think we should be. a quick run through shows: allow_user_rules, score, possible the whitelist/blacklist stuff, rewrite_header, trusted_networks, internal_networks, bayes_ignore_header, report_safe_copy_headers, redirector_pattern, version_tag, loadplugin, replace_rules, spamcop_from_address, spamcop_to_address, uridnsbl, urirhsbl, urirhssub, uridnsbl_skip_domain. they all, in some form, avoid returning errors. either we ignore lines with blank parameters, or we don't check values before pushing onto arrays, etc. revision 171195: check for blank values for: bayes_ignore_header, report_safe_copy_headers, allow_user_rules, redirector_pattern, version_tag, loadplugin validate: allow_user_rules, redirector_pattern, loadplugin ok, r178134 is the first blast at a test for this. please feel free to hack away, esp in adding support for the *many* other params we're not checking :( not ok 4 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "score", skipping:)) not ok 5 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_from", skipping:)) not ok 6 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_from", skipping:)) not ok 7 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_from_rcvd", skipping:)) not ok 8 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "def_whitelist_from_rcvd", skipping:)) not ok 9 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_allows_relays", skipping:)) not ok 10 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "unwhitelist_from_rcvd", skipping:)) not ok 11 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "blacklist_from", skipping:)) not ok 12 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "blacklist_from", skipping:)) not ok 13 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_to", skipping:)) not ok 14 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "more_spam_to", skipping:)) not ok 15 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "all_spam_to", skipping:)) not ok 16 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "blacklist_to", skipping:)) not ok 17 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "rewrite_header", skipping:)) ok 18 ok 19 not ok 20 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "clear_headers", skipping:)) ok 21 not ok 22 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "ok_locales", skipping:)) not ok 23 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "trusted_networks", skipping:)) not ok 24 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "clear_trusted_networks", skipping:)) not ok 25 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "internal_networks", skipping:)) not ok 26 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "clear_internal_networks", skipping:)) not ok 27 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "always_trust_envelope_sender", skipping:)) not ok 28 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "skip_rbl_checks", skipping:)) ok 29 ok 30 not ok 31 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "use_bayes", skipping:)) not ok 32 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "use_bayes_rules", skipping:)) not ok 33 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_auto_learn", skipping:)) ok 34 not ok 35 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_ignore_from", skipping:)) not ok 36 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_ignore_to", skipping:)) ok 37 ok 38 not ok 39 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_learn_during_report", skipping:)) not ok 40 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_sql_override_username", skipping:)) not ok 41 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_use_hapaxes", skipping:)) ok 42 ok 43 not ok 44 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_auto_expire", skipping:)) not ok 45 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_learn_to_journal", skipping:)) ok 46 not ok 47 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "fold_headers", skipping:)) ok 48 not ok 49 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "envelope_sender_header", skipping:)) not ok 50 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "descriptions", skipping:)) not ok 51 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "report_charset", skipping:)) not ok 52 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "report_contact", skipping:)) not ok 53 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "report_hostname", skipping:)) ok 54 ok 55 ok 56 ok 57 ok 58 ok 59 ok 60 ok 61 ok 62 ok 63 ok 64 not ok 65 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "test", skipping:)) not ok 66 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_path", skipping:)) ok 67 ok 68 not ok 69 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_sql_dsn", skipping:)) not ok 70 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_sql_username", skipping:)) not ok 71 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_sql_password", skipping:)) not ok 72 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "bayes_sql_username_authorized", skipping:)) not ok 73 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_dsn", skipping:)) not ok 74 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_sql_username", skipping:)) not ok 75 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_sql_password", skipping:)) not ok 76 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_sql_custom_query", skipping:)) not ok 77 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_ldap_username", skipping:)) not ok 78 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_scores_ldap_password", skipping:)) ok 79 ok 80 ok 81 not ok 82 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "uridnsbl", skipping:)) not ok 83 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "urirhsbl", skipping:)) not ok 84 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "urirhssub", skipping:)) not ok 85 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "uridnsbl_skip_domain", skipping:)) ok 86 not ok 87 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "hashcash_doublespend_path", skipping:)) ok 88 not ok 89 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "hashcash_accept", skipping:)) not ok 90 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "whitelist_from_spf", skipping:)) not ok 91 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "def_whitelist_from_spf", skipping:)) not ok 92 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "use_pyzor", skipping:)) ok 93 ok 94 ok 95 not ok 96 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "pyzor_path", skipping:)) not ok 97 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "spamcop_from_address", skipping:)) not ok 98 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "spamcop_to_address", skipping:)) ok 99 not ok 100 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "use_auto_whitelist", skipping:)) ok 101 not ok 102 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_awl_sql_override_username", skipping:)) not ok 103 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "auto_whitelist_factory", skipping:)) not ok 104 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "auto_whitelist_path", skipping:)) not ok 105 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "auto_whitelist_db_modules", skipping:)) ok 106 not ok 107 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_awl_dsn", skipping:)) not ok 108 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_awl_sql_username", skipping:)) not ok 109 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_awl_sql_password", skipping:)) not ok 110 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "user_awl_sql_table", skipping:)) ok 111 ok 112 ok 113 ok 114 ok 115 ok 116 ok 117 not ok 118 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "replace_rules", skipping:)) not ok 119 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "replace_start", skipping:)) not ok 120 (wanted: (?-xism:failed to parse line, (?:no value provided|"" is not valid) for "replace_end", skipping:)) grabbing bug r178359 validates the majority of the config options. There may be a few that could be a little stricter and there are probably some missing for optional modules like Razor2 (I'll test those out tomorrow), so the test is still disabled. The last commit was missing the updated test script -- updated in r178361. After taking a quick look at all of the plugins set_config subs, I believe all of their config options are validated -- at least minimally. TextCat's string config options could use stricter validation though (right now it only checks that the string isn't blank). I'm also unsure of what is intended with the test t/whitelist_to.t. Subject: Re: many config options are not validated -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > > I'm also unsure of what is intended with the test t/whitelist_to.t. > I was just looking at this file. The test is obviously wrong and is passing due to using an email that contains the test string. Changing the whitelist-to to what I thought should be the right whitelist_to still issues an error. Maybe I'm missing something. There is for sure a problem here. Michael -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.2 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFClUFUG4km+uS4gOIRAoL6AJ9ZlhsUR41jIjdZJHkZYQSE6TQTtACfcbG5 ceTaqPZJUXImx9jfMfuughs= =h1M0 -----END PGP SIGNATURE----- Subject: Re: many config options are not validated Currently the the validation, based on what the Conf.pm POD says/infers, requires an @ in the address. So "procmail*" isn't valid according to the validation. I don't know if the test should be changed or if the validation code should be changed. Subject: Re: many config options are not validated -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > > Currently the the validation, based on what the Conf.pm POD says/infers, > requires an @ in the address. So "procmail*" isn't valid according to > the validation. > > I don't know if the test should be changed or if the validation code > should be changed. > I disagree on your interpretation of the docs. It even has *.domain.net as an example. The validation code needs to be changed. Michael -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.2 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFClUQbG4km+uS4gOIRAs0HAJ48nBfZ5n+/lZEb1jCx7D8dWwry1gCfSybv PWPZuX8RqVH9Eyuf0lhapPY= =GpDe -----END PGP SIGNATURE----- Subject: Re: many config options are not validated
> I disagree on your interpretation of the docs. It even has *.domain.net
> as an example.
Ah, I missed that in the docs and neither t/whitelist_from.t nor
t/whitelist_addrs.t actually test for that.
r178573 only requires that CONF_TYPE_ADDRLIST values not be blank. this is no longer a P2 issue booting to 3.1.1.... I think validation could be stricter in a few circumstances, but it's now a lot better than it was before. r191268 removes the requirement for whitelist_from_rcvd entries to have @ signs Need to validate "score" values... score RULE_NAME n.nn # literally n.nn, if you can believe that [10272] warn: Argument "n.nn" isn't numeric in addition (+) at /usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251. [10272] warn: Argument "n.nn" isn't numeric in addition (+) at /usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251. [10272] warn: Argument "n.nn" isn't numeric in addition (+) at /usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251. [10272] warn: Argument "n.nn" isn't numeric in addition (+) at /usr/lib/perl5/site_perl/5.8.0/Mail/SpamAssassin/Conf.pm line 251. Also some more meta chacking couldn't hurt: body __LW_TEST1 /foo/i body __LW_TEST2 /bag/i meta LW_TEST __LW_TEST1 && __LW+TEST2 No warning, no workee. Comment #20 is fixed in bug 4795. (In reply to comment #21) > Also some more meta chacking couldn't hurt: > > body __LW_TEST1 /foo/i > body __LW_TEST2 /bag/i > meta LW_TEST __LW_TEST1 && __LW+TEST2 Hrm. I don't know if we want to flag that or not. I know the above error is supposed to be the "+" instead of a "_" but assume it's not a syntax error and __LW or TEST2 is simply another rule which requires a plugin to be enabled. Should lint fail because of this? For a more concrete example: meta DIGEST_MULTIPLE RAZOR2_CHECK + DCC_CHECK + PYZOR_CHECK > 1 all of those dependencies are via plugin. So if any of those are disabled, should this rule get flagged as having an error? I think we should probably at least throw an info() for a missing dependency or a dependency with a score of 0. I'll attach a short patch that causes a lint error to occur, but it can easily be modified to only throw an info(). Created attachment 3448 [details]
meta lint patch
I'd suggest throwing a lint warning, same as score for nonexistant rule, rather than a lint error. I agree that it *could* be legit. But if you are doing a lint and the plugin isn't enabled then it would probably still be worth mentioning that things may not be working as expected. (Like maybe the guy thought the plugin WAS enabled.) not it! Created attachment 3574 [details]
info only patch
I'd prefer just an info() about missing/zeroed dependencies.
We certaintly can't ship anything that causes lint errors by default. That
just doesn't make sense.
I don't think we should even warn without an error -- it'll turn into a support
nightmare for us and third party rule developlers. There's been no noise about
this on the users' list so I think the debug info is more than enough.
Please vote on what to do and it's corrosponding patch. * 3448: lint error when meta dependencies are missing/disabled * 3574: provide debug info for missing/disabled meta dependencies * do nothing I'm +1 for 3574. > Please vote on what to do and it's corrosponding patch.
> * 3448: lint error when meta dependencies are missing/disabled
> * 3574: provide debug info for missing/disabled meta dependencies
> * do nothing
I'm still in favor of a warning on a meta for the following cases:
a. Referenced rule name does not exist (ie: mis-spelled in meta, or not
available in the current rules.
b. Meta depends on a disabled plugin.
I am NOT in favor of a warning in the following case:
c. Meta depends on a rule with zero score. (But a debug message would be nice.)
Logic behind these suggestions:
Item A is a frequent problem in SARE masschecks. I've been burned myself any
number of times. While griping on the user's list doesn't show up, it is
frequent and loud on the SARE list where we frequently build metas.
There are really two problems here: mis-spelling a term in the meta, and
forgetting to include all of the necessary required rules to run a masscheck
(which runs with ONLY the submitted rules, not the standard rules.) Both of
those reasons end up causing more than their fair share to pulled hair and
wasted masscheck runs. A warning would give a strong hint of the problem.
In item B, if the meta depends on a disabled plugin, the meta should be in an
ifplugin group itself. After all, it ain't never gonna work right without the
plugin, so there is no point in having it sitting there doing nothing but
wasting space and time. (Yea, one COULD make a meta that depended in OR
condition on 5 different plugins, with the hope of making a score if any of
them happen to be enabled. I have a simple solution for that case: don't.)
For item C, we don't warn now for zero-score rules, because a zero score is how
you disable a rule. It follows that this will disable a meta based on that
rule (again, unless in an OR condition with something still alive). Since we
don't warn for zeroed rules, we shouldn't warn for effectively zeroed metas
that are dependent, either. However, a debug message would be worthwhile IF
the meta score is not itself also zeroed.
General comment on the concept of warning on a non-existant dependency: you do
this right now for scores and descriptions (which are dependencies of the
actual rule) if the actual rule is missing. I don't see a meta as being any
different than a description in this case. Either there should be a warning
for a missing dependency on a meta, or the warning for missing rule in
descriptions and scores should be removed. Obviously I don't vote for removing
the existing warnings. But it is worth considering, if it is decided that a
meta shouldn't warn for missing dependency.
+1 on 3574. The DIGEST_MULTIPLE case: meta DIGEST_MULTIPLE RAZOR2_CHECK + DCC_CHECK + PYZOR_CHECK > 1 where RAZOR2_CHECK, DCC_CHECK, PYZOR_CHECK could all be defined by unloaded plugins means that we should not make this an error or visible by default, I agree. A possible way around that would be if we could add a new config setting that indicated "RAZOR2_CHECK is a valid rule name, although it's not currently defined, so do not issue missing-meta-dependency warnings about it". That would allow 3448. But I don't think there's any hurry to do that... unless Loren fancies coding it up ;) Well, the obvious solution would seem to be to add 'tflags plugin' for plugin rules, allow this with the rule name (obviously) undefined; that is, you DO NOT put it inside the 'ifplugin', and check it. Like: + if (!exists $self->{conf}->{scores}->{$token}) { if (exists $self->(conf)->(tflags)->($token) && $self->(conf)->(tflags)->($token) ~= /\bplugin/ { # this is a plugin rule, it is ok if it doesn't exist if ($self->{main}->{lint_rules}) { my $msg = "rules: meta test $rulename depends on undefined plugin rule '$token'"; warn $msg."\n"; } } else { + $self->{rule_errors}++; # flag to --lint that there was an error ... + my $msg = + "rules: meta test $rulename has undefined dependency '$token'"; + if ($self->{main}->{lint_rules}) { + warn $msg."\n"; + } + else { + info($msg); + } } + } + elsif ($self->{conf}->{scores}->{$token} == 0) { After some more thought, I'm -1 on changing the behaviour of --lint on a stable branch since it'd cause way too many people unnecessary work/problems, so anything like 3448 (including something that would use additional config options to silence warnings) would have to wait for 3.2. For now, "spamassassin --lint -Dinfo" gives the output Loren wants, it's just lacking a non-zero return code. One more vote for 3574 for 3.1 please. (In reply to comment #32) > For now, "spamassassin --lint -Dinfo" gives the output Loren wants, it's just > lacking a non-zero return code. > > One more vote for 3574 for 3.1 please. +1 for 3574 > After some more thought, I'm -1 on changing the behaviour of --lint
> on a stable branch since it'd cause way too many people unnecessary
> work/problems, so
Well, I'm not sure I'd agree with the conclusion; I doubt anyone would notice,
much less it would cause problems.
That said, I do agree that this probably shouldn't go in the stable branch.
I'd be in favor of retargeting the original request at 3.2x after putting 3574
into 3.1.
(In reply to comment #34) > > After some more thought, I'm -1 on changing the behaviour of --lint > > on a stable branch since it'd cause way too many people unnecessary > > work/problems, so > > Well, I'm not sure I'd agree with the conclusion; I doubt anyone would notice, > much less it would cause problems. Nobody would notice? Anyone with their own meta rules that they publish across all their systems that depend on plugins that may or may not be enabled would be affected. When things stopped linting I'm sure they'd notice. My conclusion wasn't based on speculation. I *know* that 3 of the 4 major ISPs in my area would be affected by such a change. I really don't imagine that my friends are *that much* weirder than anyone else's. It'd certainly affect a significant amount of people, especially in the context of a maintenance branch. > That said, I do agree that this probably shouldn't go in the stable branch. > I'd be in favor of retargeting the original request at 3.2x after putting 3574 > into 3.1. Yeah, retargeting at 3.2. I still think this'd be better off as a "strict lint" that was discussed elsewhere before. [dos@FC5-VPC 3.1]$ svn ci -m "provide debug info about missing/disabled meta dependencies" Sending lib/Mail/SpamAssassin/PerMsgStatus.pm Transmitting file data . Committed revision 422038. [dos@FC5-VPC trunk]$ svn ci -m "provide debug info about missing/disabled meta dependencies" Sending lib/Mail/SpamAssassin/PerMsgStatus.pm Transmitting file data . Committed revision 422039. totally agreed with Daryl. --lint is run implicitly by sa-update, for example; even if the admins don't run lint checks regularly themselves, if they use updates, they'd run into changes pretty quickly. (In reply to comment #36) > totally agreed with Daryl. --lint is run implicitly by sa-update, for example; > even if the admins don't run lint checks regularly themselves, if they use > updates, they'd run into changes pretty quickly. Just an FYI about this... sa-update runs --lint w/out any .pre files, so things that will normally be on (like MIMEHeader) aren't, leading to things like: [2533] info: rules: meta test TVD_FW_MESG1 has undefined dependency '__GIF_ATTACH' this definitely shouldn't be considered an error. (In reply to comment #37) > Just an FYI about this... sa-update runs --lint w/out any .pre files, so things > that will normally be on (like MIMEHeader) aren't, leading to things like: It's also worth noting that if someone runs "spamassassin --lint -L", we don't want to throw an error because the score for a net rule is 0 in a non-network scoreset, ie: [19950] info: rules: meta test DIGEST_MULTIPLE has dependency 'RAZOR2_CHECK' with a zero score [19950] info: rules: meta test DIGEST_MULTIPLE has dependency 'DCC_CHECK' with a zero score [19950] info: rules: meta test DIGEST_MULTIPLE has undefined dependency 'PYZOR_CHECK' > It's also worth noting that if someone runs "spamassassin --lint -L", we
> don't want to throw an error because the score for a net rule is 0 in a
> non-network scoreset, ie:
But then, its perhaps worth noting that I never requested that this be an
*error*, although everyone seems to have interpreted the original request that
way. The original request was that it throw a warning, just exactly like
the 'info' messages it is now cranking out. With one exception that is - I
wanted them to come out on normal lint without -D to bury them in noise.
Aug 2 16:44:46 master spamd[7290]: rules: meta test DIGEST_MULTIPLE has undefined dependency 'DCC_CHECK' I understand the reasoning for this change, but it is not good that messages like this appear by default in a user's logs. Users will think this is an error, and it will forever be a cause for concern and a waste of time for both users and support persons. Is it really appropriate to have this logged by default? Why not keep it only in lint? If logging by default is appropriate, can anything be done to suppress these particular messages that would ALWAYS happen by default? Perhaps this belongs in a separate bug? 'Aug 2 16:44:46 master spamd[7290]: rules: meta test DIGEST_MULTIPLE has undefined dependency 'DCC_CHECK' I understand the reasoning for this change, but it is not good that messages like this appear by default in a user's logs. Users will think this is an error, and it will forever be a cause for concern and a waste of time for both users and support persons.' totally agreed; even if it might seem logical, the amount of *work* it'd generate in spurious errors and warnings would be massive. I think the best fix (as I said above) is to provide a new setting that indicates "DCC_CHECK is a valid rule name, even though it may be undefined for various reasons"; then if DCC_CHECK appears in a meta declaration, it won't generate this warning. something like "tflags DCC_CHECK exists" maybe? (In reply to comment #41) > I think the best fix (as I said above) is to provide a new setting that > indicates "DCC_CHECK is a valid rule name, even though it may be undefined for > various reasons"; then if DCC_CHECK appears in a meta declaration, it won't > generate this warning. something like "tflags DCC_CHECK exists" maybe? Let's please stop overcomplicating things and overloading tflags with stuff it doesn't need. The problem is that people are complaining about the code doing what it's supposed to be doing ... DCC_CHECK, for example, *DOES NOT EXIST* if the plugin isn't loaded -- I really don't see the necessity to put a "tflag RULE exists" for every rule inside of an ifplugin statement, that's ridiculous imo. It seems as if the easiest course of action is: if --lint, do info possibly a more complex version: if --lint, do info else if debug channel rules is specified, do dbg then most people will never notice it, unless they run "--lint -D" or "-D rules", wherein they really should be told about this stuff because they're looking for things like this. FWIW, something that came up in IRC while chatting w/ wtogami was that the "rule is undefined" message could be made nicer if it were "rule is undefined because plugin X is not loaded". Which is doable, except we'd have to completely change how ifplugin and such is handled (like, don't completely skip everything in the block, and parse it enough to get rule names out but don't actually include the rule in the conf object, and ...) I think users would be happiest and least confused with any solution that hides this warning by default in logs, although I suppose a more informative message would be a distant second in acceptability. 'we'd have to completely change how ifplugin and such is handled (like, don't completely skip everything in the block, and parse it enough to get rule names out but don't actually include the rule in the conf object, and ...)' yeah -- that'd be pretty tricky. consider a "urirhssub" rule definition; that setting is just invalid gibberish if the plugin isn't loaded. let's just keep it as it currently is. Is there a lint output message level that isn't logged by default when you simply start SA? For instance, how does the lint 'score for nonexistant rule' warning appear? Is that something that would be logged on every SA start? Or am I misunderstanding and somehow SA --lint is being run and any output at all from it is being logged? If the 'score for nonexistant rule' message isn't logged under whatever these conditions are, I'd just change the meta rule missing dependencies message to the same form. (In reply to comment #45) > Is there a lint output message level that isn't logged by default when you > simply start SA? For instance, how does the lint 'score for nonexistant rule' > warning appear? Is that something that would be logged on every SA start? No, there's no special lint level. However, when lint is running, typically messages get bumped up in priority ala: if ($self->{main}->{lint_rules}) { warn $msg."\n"; } else { info($msg); } there's usually also a counter increase of config errors which will cause lint to throw the error and set the exit code. > If the 'score for nonexistant rule' message isn't logged under whatever these > conditions are, I'd just change the meta rule missing dependencies message to > the same form. that throws a warn() and sets the lint error code. see bug 5040: ''meta test has dependency with zero score' should ignore rules in other scoresets' *** Bug 5085 has been marked as a duplicate of this bug. *** fyi, I just marked bug 5085 as a dupe of this one. the issue there is that via spamd, debug (with -D), info(), and warn() are logged in syslog, and generally speaking mail.* or mail.info is used, which means these "dependency not defined" info messages are going into syslog. this is actually fixed nowadays, isn't it? if rules are referred to, but are nonexistent (or in non-loaded ifplugin blocks), nothing is output by default. if you use -D, however, you'll see them: [30895] info: rules: meta test STOCK_IMG_HTML has undefined dependency '__HTML_IMG_ONLY' [30895] info: rules: meta test STOCK_IMG_CTYPE has undefined dependency '__HTML_IMG_ONLY' [30895] info: rules: meta test STOCK_IMG_HDR_FROM has undefined dependency '__HTML_IMG_ONLY' looks good to me... (In reply to comment #50) > this is actually fixed nowadays, isn't it? No, it's not. These particular info messages still get logged to syslog, even without asking for -Dinfo. They need the would_log(whatever) == 2, or however that works. ;) r486500 will stop the standard ruleset from nagging about undefined meta dependencies in the syslog when some plugins (like DCC) are disabled. Anyone looking to debug meta rules should run a message through SA with -Drules. *** Bug 5248 has been marked as a duplicate of this bug. *** |
From a recent set of transactions on the users list: On Thu, May 19, 2005 at 09:44:34AM -0700, John Schneider wrote: > Secondly, I installed this version of SpamAssassin from the FreeBSD port of > 3.03. I built the port again and checked the Conf.pm file inside the port > folder against the installed Conf.pm file and there are no differences. > >> -----Original Message----- > >> From: Theo Van Dinter [mailto:felicity@kluge.net] > >> Bad config line. Looking at the code, a bad "report_safe" line. As I stated, you have a bad config line. The code isn't the problem, check your config. ---------------- It seems that this user is confused about where the error lies, since the error message for a misconfigured report_safe line seems to be "argument not numeric somewhere in the middle of SA code". Suggest enhancing the relevent code to detect a parameter that isn't 0/1 and output a human-comprehensible error message such as "report_safe parameter isn't 0 or 1".