Patches item #1711603, was opened at 2007-05-03 00:12 Message generated for change (Comment added) made by vsajip 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: Pending >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: Vinay Sajip (vsajip) Date: 2007-05-24 17: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 12: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 12: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 20: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 19: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 22: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 21: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 15: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 12: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 16: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 16: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 09: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 17: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 13: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-03 00: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-03 00: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
