classification
Title: syslog.syslog('msg') logs message with ident "python".
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: eric.smith, jafo, pitrou, r.david.murray
Priority: normal Keywords: easy, needs review, patch

Created on 2010-04-19 05:26 by jafo, last changed 2010-04-23 18:45 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
syslog-kwargs.patch jafo, 2010-04-19 05:27 Initial version of this patch.
syslog-kwargs2.patch jafo, 2010-04-19 06:59 This one includes a "NEWS" blurb.
syslog-kwargs3.patch jafo, 2010-04-20 04:25 Fixes suggested by Antoine.
syslog-kwargs4.patch jafo, 2010-04-21 02:14 Fixes suggested by Eric.
Messages (12)
msg103555 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-04-19 05:26
As discussed in this thread:

http://mail.python.org/pipermail/python-dev/2010-March/098500.html

The syslog module is using the C argv[0] as the program name, not the python sys.argv[0].  So, in most cases this means that unless you explicitly set a ident, you get "python" as the ident.  Not entirely helpful.

This patch:

 - Makes openlog arguments keyword args.
 - Makes openlog ident argument optional.
 - If ident is not passed to ident, basename(sys.argv[0]) is used.
 - The first call to syslog.syslog() calls ident() with no options
   (if it hasn't previously been called).
 - Variously related documentation changes.

"make test" with this succeeds.

I think this is ready to go into the trunk, but would like a review.  I'll check with the release maintainer about if this is appropriate for 2.7b.

Sean
msg103573 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-19 10:34
Some notes about the patch:

- NEWS blurbs generally appear antichronologically, that is newest first :-)
- you don't have to mention that "The 3.2 changes mentioned above are included in 2.7"
- the part of the PyArg_ParseTupleAndKeywords string after the semi-colon is supposed to be a custom error message, which "[ident string [, logoption [, facility]]]" doesn't look like; how about simply using a colon and putting the function name?
- you should check PyList_Size()'s return value for error (sys.argv could have been overriden to something else than a list)
- similarly, scriptobj is not guaranteed to be a string object, so you have to check for that too
- to check for string length > 0, it looks more natural to check either that script[0] != 0, or that PyString_GET_SIZE(...) > 0 (rather than calling PyObject_IsTrue())
- why do you Py_INCREF() the "openargs" tuple? It looks like a reference leak
- if PyTuple_New(0) fails (which I admit is highly unlikely), you should free all other resources and bail out
- when manually calling syslog_openlog(), you should check the return value and either Py_DECREF it (if non-NULL) or bail out
msg103668 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-04-20 04:25
Antoine: I believe I have everything you mentioned addressed with the new patch.  That was an awesome review, thank you so much.

The only things I didn't do were parts of the last two items you bring up:

If PyTuple_New(0) fails, bail out.
If syslog_openlog fails, bail out.

syslog(3) can continue even if the openlog() fails.  It won't have the expected "ident" string, but it *WILL* log.

I believe this is the desired behavior.

NOTE: I puled the code out that does all the sys.argv handling, which I think made that whole section of code much easier to read, particularly with the new changes.  The down side is that the code to be reviewed is quite different now.

I also found a leak in the call to syslog_openlog() where I wasn't DECREFing the return.

Can you please review these changes?
msg103706 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-04-20 13:27
A couple of points:

Didn't we decide that instead of using:
  openlog(ident[, logopt[, facility]])
we'd use:
  openlog(ident, logopt=None, facility=None)
(or whatever the defaults are)? I can't find a reference, but the argument was that it's how Python signatures are written, so it's clearer if they're all written this way.

You should add some comments to syslog_get_argv explaining why you're handling errors the way you are. That is, why you're swallowing exceptions and continuing. Similarly with the call to PyTuple_New(0).

I also think it would be clearer if using the string "python" were inside syslog_get_argv, but that's a style thing.

Should the fallback be "python", or derived from C's argv[0]?

Is it possible that sys.argv[0] would be unicode?

Is SEP correct, or should it really be using os.path.sep and/or os.path.altsep? This is probably a nit, but I could see it being a problem under cygwin (which I haven't tested yet).

Your "if" statements shouldn't all be on one line. The single-line style with braces isn't used anywhere else in this module, and it's not in the Python code base that I could see (except for the occasional macro).

The example code has some extra spaces around the equal signs. It should be:
   syslog.openlog(logopt=syslog.LOG_PID, facility=syslog.LOG_MAIL)

Thanks for doing this!
msg103724 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-20 15:27
The argument documentation style change was made for py3k.  The old convention is still used in the 2.x docs.
msg103798 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-04-21 02:14
Ok, so I left the argument style in the docs as it was.

*** NOTE ***: Sean: Change this for the 3 trunk commit.

Setting the "python" string is outside of the argv function because we want to use "python" on any of the many places where the return is done.

As far as using the C argv[0], I decided to specifically use "python" as a fallback.  We could easily just let openlog pick it, which is what the old behavior was, but in that case it's dependent on the implementation.

I can't answer about sys.argv being unicode.

From my looking at the code, SEP will vary depending on the platform, and is just what os.path.sep is -- I was going to use os.path.sep, but found it just returns SEP.

Thanks for the great review, I've attached a new version.
msg103822 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-21 11:36
One argument in favor of letting openlog pick it (assuming it uses argv[0]) is that for a while at least we will have many systems running both python2 and python3, and it might be useful to have that distinction show up in the log if the fallback is used.  I'm not sure this is a strong argument :)
msg103856 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-04-21 15:29
I have the same reasoning as David, although I was thinking about python vs. pythonw. But it's not a big deal.

I think you should check it in as-is, and we can worry about modifying it later, if need be.
msg103994 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-04-23 08:35
Committed as 80396.  Included a change to let openlog(3) pick the ident instead of using the static string "python".
msg103995 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2010-04-23 09:30
Ported to python3 and committed as 80401.
msg104036 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-23 18:44
Assuming that it is OK for this to be in 2.7 beta2 (it smells almost like a feature, but I'm certainly willing to let it pass as a bugfix myself), the 2.7 doc change in the commit contains a typo (it says versionchanged 3.2 instead of versionchanged 2.7).
msg104037 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-23 18:45
Oh, and you forgot about your note to yourself to update the argument syntax for the 3.2 commit.
History
Date User Action Args
2010-04-23 18:45:59r.david.murraysetmessages: + msg104037
2010-04-23 18:44:49r.david.murraysetmessages: + msg104036
2010-04-23 09:30:37jafosetmessages: + msg103995
2010-04-23 08:35:59jafosetstatus: open -> closed
resolution: accepted
messages: + msg103994
2010-04-21 15:29:59eric.smithsetmessages: + msg103856
2010-04-21 11:36:00r.david.murraysetmessages: + msg103822
2010-04-21 02:14:52jafosetfiles: + syslog-kwargs4.patch

messages: + msg103798
2010-04-20 15:27:12r.david.murraysetnosy: + r.david.murray
messages: + msg103724
2010-04-20 13:27:40eric.smithsetmessages: + msg103706
2010-04-20 04:25:55jafosetfiles: + syslog-kwargs3.patch

messages: + msg103668
2010-04-19 10:34:24pitrousetnosy: + pitrou
messages: + msg103573
2010-04-19 06:59:49jafosetfiles: + syslog-kwargs2.patch
2010-04-19 05:30:36jafosetnosy: + eric.smith
2010-04-19 05:27:43jafosetfiles: + syslog-kwargs.patch
2010-04-19 05:26:38jafocreate