report a bug |  advanced search |  statistics |  developer log in/out
Bug #3 mlmmj-process.c:724 log error
Submitted: 2010-08-26 03:23 UTC Modified: -
Votes:5
Avg. Score:4.0 ± 0.6
Reproduced:5 of 5 (100.0%)
Same Version:2 (40.0%)
Same OS:2 (40.0%)
From: and at gmx dot li Assigned:
Status: Open
Mlmmj Version: 1.2.17 OS: any
MTA: any MTA Version: any
View Add Comment Developer Edit
You must login as a developer to do anything here. You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
 [2010-08-26 03:23 UTC] and at gmx dot li
Description:
------------
see also
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=569709

I think function in mlmmj-process.c:724 and mlmmj-process.c:734
deals wrong with return codes.



Test script:
---------------
normal list example

Expected result:
----------------
clear logfile

Actual result:
--------------
Aug 25 11:21:36 mailserver /opt/mlmmj/bin/mlmmj-process[28770]: mlmmj-process.c:724: Found To: test@example.com: No such file or directory
Aug 25 11:21:36 mailserver /opt/mlmmj/bin/mlmmj-process[28770]: mlmmj-process.c:724: Found To: test@example.com: Bad file number
Aug 25 11:21:36 mailserver /opt/mlmmj/bin/mlmmj-process[28770]: mlmmj-process.c:734: Found Cc: testl@example.com: Bad file number


Patches

src-mlmmj-process.c-use-log_info-appropriately.patch (last revision 2011-05-30 22:09 UTC) by mlmmj at cryptocrack dot de)
introduce-log_info.patch (last revision 2011-05-30 22:08 UTC) by mlmmj at cryptocrack dot de)

Add a Patch

History

AllCommentsChanges
 [2011-03-15 04:19 UTC] mlmmj at cryptocrack dot de
Can reproduce that one here.

The problem is that "log_error(LOG_ARGS, [...])" always appends the errstring of the last error (errno) to the error message in the logs - check the "LOG_ARGS" #define in "include/log_error.h". To fix this, I'd suggest to create a log_custom() (or whatever) function similar to log_error() that doesn't have the errstr parameter and a "LOG_CUSTOM_ARGS" define similar to "LOG_ARGS" passing "__FILE__" and "__LINE__" only. This can be used whenever it's no syscall/glibc call that fails but a custom error. We probably should also grep the source tree for all log_error() function calls to be sure we don't miss anything.

Unfortunately I don't have enough time to write a patch right now.
 [2011-05-31 06:10 UTC] mlmmj at cryptocrack dot de
Patches attached. Changes were tested successfully in a production environment.
 [2011-06-28 03:15 UTC] coil93 at gmail dot com
This patches are invalid for mlmmj-1.2.17.1
 [2011-06-28 03:18 UTC] mlmmj at cryptocrack dot de
These patches were built against tip and apply fine for me.
 [2011-08-10 05:29 UTC] mlmmj at cryptocrack dot de
*bump*
 [2011-09-06 00:06 UTC] maintainer at mlmmj dot org
Thank you so much for this work. I haven't looked at the patch in detail, but at a first glance it looks like a GREAT start on fixing the current logging mess.

My only hesitation is that I'd prefer to do a 'big fix' that comprehensively fixes most/all of the logging at once than put out spot fires. So a few comments/requests/etc.:

- Did you take care to consider each log_error() case in your patch on its merits? I.e. did you ensure there was a system call that sets errno before each log_error call? I'd rather catch all the ones that should omit errno if possible, rather than just the ones we've come across 'by accident'.

- Is there any chance you could also make a patch to eliminate mlmmj-operation.log? The stuff in there should go to syslog like everything else.

- Also, could we set appropriate severities at the same time? It would be good if messages could be logged appropriately as notices, warnings or errors (or whatever).

- While we're refactoring, we probably should change to better function names. log_error() and log_info() don't accurately represent what they do, since the main distinction is whether or not they follow a system call that sets errno. I'd like to see something like log_errno(LOG_ARGS, severity, message, errno) and log_message(LOG_ARGS, severity, message) or something like that.

What do you think? Are you able to invest any more time in this? I'd be happy if you'd like to make patches that apply over the work you've already done, and/or update the existing patch. Whatever you think is best in terms of making code review straightforward and accurately representing the changes in sensible 'units' in version control.

Ben.
 [2011-09-06 00:30 UTC] mlmmj at cryptocrack dot de
> - Did you take care to consider each log_error() case in your patch on its merits? I.e. did you ensure there was a system call that sets errno before each log_error call? I'd rather catch all the ones that should omit errno if possible, rather than just the ones we've come across 'by accident'.

Iirc, I started to look at each log_error() occurrence but stopped checking them after a while as I didn't have so much time back then. I finally just used sed(1) to replace all invocations and ran make(1) to make sure I didn't miss any. Agreed that we should rather do this right and check for all calls in detail.

> - Is there any chance you could also make a patch to eliminate mlmmj-operation.log? The stuff in there should go to syslog like everything else.

Yeah, this should go into another patch, though.

> - Also, could we set appropriate severities at the same time? It would be good if messages could be logged appropriately as notices, warnings or errors (or whatever).

+1. Separate patch.

> - While we're refactoring, we probably should change to better function names. log_error() and log_info() don't accurately represent what they do, since the main distinction is whether or not they follow a system call that sets errno. I'd like to see something like log_errno(LOG_ARGS, severity, message, errno) and log_message(LOG_ARGS, severity, message) or something like that.

Agreed.

> What do you think? Are you able to invest any more time in this? I'd be happy if you'd like to make patches that apply over the work you've already done, and/or update the existing patch. Whatever you think is best in terms of making code review straightforward and accurately representing the changes in sensible 'units' in version control.

Yes, I will have a look at that and try to fix everything you mentioned. I'll have a look if there's a sane tool to import a Git repository into Mercurial (or convert git-formatted patches, as least). Mercurial sucks, Git just feels so much more comfortable. Just as replying on this bug tracker does :>
 
Based on the PHP bug tracker.
Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Wed Jan 30 01:27:44 2013 UTC