Steve wrote:
-------- Original-Nachricht --------
Datum: Sun, 03 Feb 2008 23:21:29 -0800
Von: Jason Axley <[EMAIL PROTECTED]>
An: Mick Johnson <[EMAIL PROTECTED]>
CC: [email protected], [EMAIL PROTECTED], Julien Valroff
<[EMAIL PROTECTED]>
Betreff: Re: [dspam-users] CVS updates
Hey Mick,
Hallo Jason
I have the attached patch to the mysql driver that should go in. It's
part of the 3.8.1cvs build on debian/ubuntu and fixes some problematic
bugs that break global groups that would be good to include. I don't
believe it has been included in the kirya.net build yet, but I've been
running it on my busy server and it is like a dream...
I don't see the benefit of this patch. The only benefit I see is that the
LOGDEBUG message will print the proper name in the error message. That's all.
That's a bit harsh Steve. It does a bit more than that. And
admittedly, it may not fix the root cause of the problem but it fixes
the problematic result that I was seeing because of the broken logic:
the functions in the driver were returning EINVAL when problems looking
up the group with getpwnam. The driver treats that as a fatal error
instead of continuing on and processing
- if (CTX->flags & DSF_MERGED) {
+ if (CTX->group != NULL && CTX->flags & DSF_MERGED) {
Note the null check that I inserted so it will skip the body of the branch.
I changed all occurrences of the code that were _not_ checking for a
null group (even though the code below does this) to avoid returning
EINVAL if the group can't be looked up. Why the group ID is sometimes
NULL is something that still deserves looking into. But with this
patch, it seems to work like a charm with my config.
This bug caused dspam to deliver scads of spam without tagging it or
doing anything, which was flooding people's inboxes with spam that
couldn't be processed. At least now it can use the user's data to tag
the messages...
For example (original code):
int uid = -1, gid = -1;
[..]
if (!CTX->group || CTX->flags & DSF_MERGED)
p = _mysql_drv_getpwnam (CTX, CTX->username);
else
p = _mysql_drv_getpwnam (CTX, CTX->group);
Here you would add a variable name having either the username or the group name.
if (p == NULL)
{
LOGDEBUG ("_mysql_drv_get_spamtotals: unable to _mysql_drv_getpwnam(%s)",
CTX->username);
Here you would print out the CORRECT name used. The original code would always
print out the user name but NEVER the group name. Your patch fixes that.
I was getting _mysql_drv_get_spamtotals: unable to
_mysql_drv_getpwnam(NULL) messages because of that which was not helping
debugging. Fixing this error helps with debugging, as many of the other
debug statements I added do as well.
if (!(CTX->flags & DSF_MERGED))
return EINVAL;
} else {
uid = p->pw_uid;
}
if (CTX->flags & DSF_MERGED) {
p = _mysql_drv_getpwnam (CTX, CTX->group);
if (p == NULL)
{
LOGDEBUG ("_mysql_drv_getspamtotals: unable to _mysql_drv_getpwnam(%s)",
CTX->group);
return EINVAL;
}
}
gid = p->pw_uid;
Your patch would not jump into the above code block if group is NULL and you
run merged groups. It would as well NOT get the gid. In your case gid would be
-1 if some one is not running merged group.
Right, not desirable but at least the error conditions are handled as
transient and not fatal so the system doesn't cease to function just
because of this. There seem to still be problems with merged groups
somewhere.
snprintf (query, sizeof (query),
"select uid, spam_learned, innocent_learned, "
"spam_misclassified, innocent_misclassified, "
"spam_corpusfed, innocent_corpusfed, "
"spam_classified, innocent_classified "
" from dspam_stats where (uid = %d or uid = %d)",
uid, gid);
Even if group would be empty (having -1 as value) the above SQL query would
only return user data since negative gid's are not possible/normal.
Could you explain me what the benefits are of the patch set? What case does the
patch handle which is not handled in the original code (beside the proper
LOGDEBUG message)?
See above ;-)
And, it adds additional debug messages to the get_pwnam function to aid
in determining where the calls were returning NULL. Turns out they were
returning null...because NULL was being passed in sometimes for the
group name... I didn't have time to find out what code was goofing that
up, and this fixed the logic in the driver so that the bleeding stopped
enough to avoid me ripping dspam out entirely and replacing it with
something else...
-Jason.
Steve
Mick Johnson wrote:
All
A few pending updates have been pushed up to CVS :
* Allow users to select multiple rows by clicking
on the initial row, holding shift, and clicking on the final row.
* Adds a "select 200" button to the quarantine page.
* Removed some junk from a previous merge.
The Feature Request page has also been updated - the donations button
and
dollar value components have been removed as we're no longer accepting
donations for this project.
A few of the patches came in via this interface, this actually makes it
harder to patch as a) I don't know who submitted them and can't bring up
any
suggestions or corrections directly, and b) they have less visibility on
this list. In the future, if you wish to submit patches (and I'm always
happy when people do) please do so here to ensure the community can
review.
The latest CVS version seems to have been running stably for some time
now
and I'm looking to push this out as a stable 3.8.1 this month unless I
hear
otherwise.
Finally, looking forward to a great 2008!
Cheers
Mick Johnson
Sensory Networks
