Patches item #1711603, was opened at 2007-05-02 19:12
Message generated for change (Comment added) made by luke-jr
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1711603&group_id=5470
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Library (Lib)
Group: None
>Status: Open
Resolution: Rejected
Priority: 5
Private: No
Submitted By: Luke-Jr (luke-jr)
Assigned to: Nobody/Anonymous (nobody)
Summary: syslog syscall support for SysLogLogger
Initial Comment:
This allows SysLogLogger to be used via the normal syslog syscall.
----------------------------------------------------------------------
>Comment By: Luke-Jr (luke-jr)
Date: 2007-05-25 10:10
Message:
Logged In: YES
user_id=25634
Originator: YES
This would be the equivalent to having a EmailLogger that supports XMPP
and IMAP, but not SMTP. I don't see how you can argue it as not being a
bug.
I have a patch that works, yes, but to make use of it, I need to patch
Python every time I upgrade, as would all my users. Seeing as we are all
users of the Python language, not developers of it, manual patching should
*not* be necessary. The SyslogLogger could easily contain a documented note
that syscall support may not be thread-safe. Note that in practice, I
haven't had any problems-- the application I am writing is highly threaded.
----------------------------------------------------------------------
Comment By: Vinay Sajip (vsajip)
Date: 2007-05-25 08:45
Message:
Logged In: YES
user_id=308438
Originator: NO
It's not a bugfix - it's an enhancement, which no one else has asked for.
Given that there's a possible thread safety issue, I see no strong reason
to commit a patch to call syslog. After all, you have a patch which works
for you; and there is no strong demand in the community for a patch to use
syslog. Keep using your patch, where's the problem with that?
I look at bug reports and patches and fix/apply after assessing whether or
not the change is an improvement. I don't accept all patches or bug reports
as valid - but I don't reject things out of hand, either. This particular
patch might make things worse if thread safefy issues aren't thought
through properly - then it would be me, not you, who would have to take on
any additional support burden. While I can't speak for the Python library
as a whole, I have certainly taken a fair amount of trouble to make the
logging package thread-safe.
Take a look at the patches list for Python - there are quite a lot of
patches which aren't accepted and haven't been accepted for a long time
because that's the committer's judgement. Some may get accepted in the
future, others will not.
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-25 05:35
Message:
Logged In: YES
user_id=25634
Originator: YES
What logic allows you to reject/postpone this bugfix, but not
reject/postpone other patchless bug reports to fix more specifics? Better
to have this work as non-thread-safe than not at all, even if the eventual
ideal is thread-safe. I find it hard to believe that thread-safety is
consistently present throughout the rest of the Python library.
----------------------------------------------------------------------
Comment By: Vinay Sajip (vsajip)
Date: 2007-05-25 02:44
Message:
Logged In: YES
user_id=308438
Originator: NO
Unfortunately, while thread safety remains a concern, I won't be able to
put in a patch - I don't have time to handle any additional support
requests or bug reports. Since you have a patch which works for you, I
suggest you stick with it. Any further updates to the module should be
fairly easy for you to merge with e.g. Meld.
A conditional import for Windows compatibility would be something like
this:
try:
import syslog # Will fail where unavailable, e.g. Windows
except ImportError:
syslog = None
and then, later, in the relevant code:
if syslog is None:
# Code avoiding use of syslog module
else:
# Code using syslog module
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-24 13:10
Message:
Logged In: YES
user_id=25634
Originator: YES
Not all system loggers support sockets. In my real-world situation, this
is Metalog (on Gentoo). Thread safety is a concern, but single-thread
support is better than none for now.
How would I go about doing a conditional import for Windows compatibility?
----------------------------------------------------------------------
Comment By: Vinay Sajip (vsajip)
Date: 2007-05-24 12:55
Message:
Logged In: YES
user_id=308438
Originator: NO
I didn't close the patch, I just marked it as "Pending" - not the same
thing as closing, but giving an opportunity for an improved patch to be
uploaded, and putting the ball in your court. And opinions differ about the
"standard method" - in fact the UDP/domain socket SysLogHandler
implementation was contributed by someone who was using it in a real-world
scenario. The syslog call method doesn't appear to provide the ability to
log to remote daemons - so the UDP/domain socket route would appear to
cover all bases. Are you saying that you are not able to use UDP or Unix
sockets in your environment? Can you please give more details about this
environment - operating system, version etc.? What about my comment about
thread safety of syslog, which you have not addressed?
Marking this item's status as Pending (i.e. ball in your court) and
resolution as Rejected (indicating that the patch cannot be accepted as is
due to breakage on Windows).
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-24 07:49
Message:
Logged In: YES
user_id=25634
Originator: YES
The purpose of the SysLogHandler is to log to the system logger. The
standard method for doing this is via the syscall. UDP and UNIX sockets are
*additional* methods that are not necessarily supported by all system
logging daemons.
If the patch needs work, that's no reason to close it altogether.
----------------------------------------------------------------------
Comment By: Vinay Sajip (vsajip)
Date: 2007-05-24 07:36
Message:
Logged In: YES
user_id=308438
Originator: NO
The whole purpose of the module is not to log to syslog - it logs to a
variety of destinations. If you meant the SysLogHandler class - why exactly
do you need the syslogging to be done via a syscall? You have two options
already - log using a hostname/port for local or remote daemons, or use
e.g. address="/dev/log" to use a Unix domain socket for access to the local
syslog daemon. So I'm not sure how the purpose is defeated.
The patch as it stands will not import cleanly in Windows (since there is
no "syslog" module there), so it cannot be accepted as it stands.
Ordinarily I would make any small amendments myself, but at the moment I
don't have the time.
You should also be aware that the syslog module may not be thread-safe on
all platforms - for instance, see the first Google hit I got for the query
"syslog openlog":
http://www.unet.univie.ac.at/aix/libs/basetrf2/syslog.htm
Now logging does provide an I/O lock, but that's only around the emit()
call. Your syslog calls have a broader scope (handler open/close) and I'm
not sure whether thread-safety could be guaranteed in all cases.
If you want to work up a patch which works across platforms and can allay
my worries about multi-threading, I'll certainly look at it again.
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-16 15:40
Message:
Logged In: YES
user_id=25634
Originator: YES
There is no syslog on Windows period. I guess you'd better remove chmod
support from Python as well-- Windows doesn't use UNIX permissions! The
whole purpose of this module is to log to syslog; the standard way for this
is via a syscall. Not supporting the syscall kindof defeats the purpose.
----------------------------------------------------------------------
Comment By: Vinay Sajip (vsajip)
Date: 2007-05-16 14:13
Message:
Logged In: YES
user_id=308438
Originator: NO
The use of syslog is system-specific - there is no syslog syscall on
Windows.
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-10 17:16
Message:
Logged In: YES
user_id=25634
Originator: YES
Yes, that might be a sane thing to add :)
That exact problem bit me a few days ago, actually (I'm using this code
already).
----------------------------------------------------------------------
Comment By: jos (josm)
Date: 2007-05-10 16:52
Message:
Logged In: YES
user_id=1776568
Originator: NO
Now I understand. Thank you for your patience :)
BTW, wouldn't it be nice to do os.path.basename() on ident?
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-10 10:09
Message:
Logged In: YES
user_id=25634
Originator: YES
No, 'syslog' wouldn't work because the running program's name is not
'syslog'. It should be the same name as appears in the process list, which
is argv[0]
----------------------------------------------------------------------
Comment By: jos (josm)
Date: 2007-05-10 07:31
Message:
Logged In: YES
user_id=1776568
Originator: NO
Sorry, I quoted wrong PEP... PEP 7 is for C code, not Python's one.
PEP 8 is right guide.
I'm still wondering whether using sys.argv[0] for default value is valid
or not.
How about changing this as ident = 'syslog'?
doesn't that work?
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-06 11:14
Message:
Logged In: YES
user_id=25634
Originator: YES
File Added: SysLogHandler-syslog-0.04.patch
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-06 11:05
Message:
Logged In: YES
user_id=25634
Originator: YES
The 'ident' value is normally the name of the program, which should be
argv[0]. I prefer and normally use single-tab indents. However, when
patching code, I adjust to the existing standard within the source. In this
case, my handlers.py was already indented with spaces so I conformed to
that. I glanced at svn before posting this, and it doesn't appear to be
much different for this file.
----------------------------------------------------------------------
Comment By: jos (josm)
Date: 2007-05-06 04:58
Message:
Logged In: YES
user_id=1776568
Originator: NO
Thank you for your reply.
A few more comments and requests.
+ def __init__(self, address=('localhost', SYSLOG_UDP_PORT),
facility=LOG_USER, ident=sys.argv[0]):
Why sys.argv[0]? Assuming command line arguments doesn't look reasonable
to me.
+ self.unixsocket = 0
+ self.syscall = 0
Please use single-tab indents. (PEP7)
Can I have the patch patch against the svn repository?
(against trunk should be preferable)
I had some problem applying your patch.
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-05 12:28
Message:
Logged In: YES
user_id=25634
Originator: YES
Not all syslog daemons accept socket connections. For example, metalog.
----------------------------------------------------------------------
Comment By: jos (josm)
Date: 2007-05-05 08:25
Message:
Logged In: YES
user_id=1776568
Originator: NO
What advantage does OS's syslog have over the logging module's one?
I've never been familiar with syslog,
So could you please tell me the difference?
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-02 19:22
Message:
Logged In: YES
user_id=25634
Originator: YES
File Added: SysLogHandler-syslog-0.03.patch
----------------------------------------------------------------------
Comment By: Luke-Jr (luke-jr)
Date: 2007-05-02 19:14
Message:
Logged In: YES
user_id=25634
Originator: YES
File Added: SysLogHandler-syslog-0.02.patch
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1711603&group_id=5470
_______________________________________________
Patches mailing list
Patches@python.org
http://mail.python.org/mailman/listinfo/patches
