Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syslog.syslog('msg') logs message with ident "python". #52698

jafo mannequin opened this issue Apr 19, 2010 · 12 comments

syslog.syslog('msg') logs message with ident "python". #52698

jafo mannequin opened this issue Apr 19, 2010 · 12 comments
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error


Copy link

jafo mannequin commented Apr 19, 2010

BPO 8451
Nosy @pitrou, @ericvsmith, @bitdancer
  • syslog-kwargs.patch: Initial version of this patch.
  • syslog-kwargs2.patch: This one includes a "NEWS" blurb.
  • syslog-kwargs3.patch: Fixes suggested by Antoine.
  • syslog-kwargs4.patch: Fixes suggested by Eric.
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2010-04-23.08:35:59.026>
    created_at = <Date 2010-04-19.05:26:38.039>
    labels = ['easy', 'type-bug', 'library']
    title = 'syslog.syslog(\'msg\') logs message with ident "python".'
    updated_at = <Date 2010-04-23.18:45:59.727>
    user = '' fields:

    activity = <Date 2010-04-23.18:45:59.727>
    actor = 'r.david.murray'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-04-23.08:35:59.026>
    closer = 'jafo'
    components = ['Library (Lib)']
    creation = <Date 2010-04-19.05:26:38.039>
    creator = 'jafo'
    dependencies = []
    files = ['16982', '16983', '17004', '17018']
    hgrepos = []
    issue_num = 8451
    keywords = ['patch', 'easy', 'needs review']
    message_count = 12.0
    messages = ['103555', '103573', '103668', '103706', '103724', '103798', '103822', '103856', '103994', '103995', '104036', '104037']
    nosy_count = 4.0
    nosy_names = ['jafo', 'pitrou', 'eric.smith', 'r.david.murray']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = ''
    versions = ['Python 2.7', 'Python 3.2']

    Copy link
    Mannequin Author

    jafo mannequin commented Apr 19, 2010

    As discussed in this thread:

    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.


    @jafo jafo mannequin added stdlib Python modules in the Lib dir easy type-bug An unexpected behavior, bug, or error labels Apr 19, 2010
    Copy link

    pitrou commented Apr 19, 2010

    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

    Copy link
    Mannequin Author

    jafo mannequin commented Apr 20, 2010

    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?

    Copy link

    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!

    Copy link

    The argument documentation style change was made for py3k. The old convention is still used in the 2.x docs.

    Copy link
    Mannequin Author

    jafo mannequin commented Apr 21, 2010

    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.

    Copy link

    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 :)

    Copy link

    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.

    Copy link
    Mannequin Author

    jafo mannequin commented Apr 23, 2010

    Committed as 80396. Included a change to let openlog(3) pick the ident instead of using the static string "python".

    @jafo jafo mannequin closed this as completed Apr 23, 2010
    Copy link
    Mannequin Author

    jafo mannequin commented Apr 23, 2010

    Ported to python3 and committed as 80401.

    Copy link

    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).

    Copy link

    Oh, and you forgot about your note to yourself to update the argument syntax for the 3.2 commit.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    None yet

    No branches or pull requests

    3 participants