bpo-29808: SyslogHandler: fix initial connect to syslog by socketpair · Pull Request #663 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation23 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).
Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:
- If you don't have an account on b.p.o, please create one
- Make sure your GitHub username is listed in "Your Details" at b.p.o
- If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
- If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
- Reply here saying you have completed the above steps
Thanks again to your contribution and we look forward to looking at it!
| if socktype == socket.SOCK_STREAM: |
|---|
| self.socket.connect(address) |
| self.socktype = socktype |
| self.socket.connect(address) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually works for UDP sockets too. This simplify send() later since we should not specify address every time.
| self._connect_unixsocket(address) |
|---|
| self._unix_socktype = socktype |
| try: |
| self._connect_unixsocket() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syslog server may not work at the time of the constructor call.
Thank you for your contribution but please first open an issue for the problem on https://bugs.python.org and then add the issue number (bpo-nnnnnn) to the title of this pull request.
SyslogHandler: fix initial connect to syslog + code simplification. SyslogHandler: fix initial connect to syslog + code simplification (bpo-29808)
SyslogHandler: fix initial connect to syslog + code simplification (bpo-29808) bpo-29808: SyslogHandler: fix initial connect to syslog + code simplification
@socketpair Did you add your GitHub username to the account in b.p.o?
| @@ -800,52 +800,54 @@ def __init__(self, address=('localhost', SYSLOG_UDP_PORT), |
|---|
| Initialize a handler. |
| If address is specified as a string, a UNIX socket is used. To log to a |
| local syslogd, "SysLogHandler(address="/dev/log")" can be used. |
| local syslogd, `SysLogHandler(address="/dev/log")` can be used. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't make random stylistic changes.
| If facility is not specified, LOG_USER is used. If socktype is |
|---|
| specified as socket.SOCK_DGRAM or socket.SOCK_STREAM, that specific |
| socket type will be used. For Unix sockets, you can also specify a |
| socktype of None, in which case socket.SOCK_DGRAM will be used, falling |
| back to socket.SOCK_STREAM. |
| """ |
| logging.Handler.__init__(self) |
| super().__init__() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't make changes to "tidy up the code base". Address only specific things which cause the issue.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, should I make another PR, that fixes such things?
@socketpair in your account at https://bugs.python.org, you'll need to put your GitHub username (socketpair) into your user details page.
Please read the message from the-knights-who-say-ni above, and click on "Your Details" at b.p.o in number 2, to see where the username should be entered.
Thanks :)
- I set socketpair as github username
- I changed bugs.python.org's username from
mmarkktosocketpair - Please note, I signed CLA as username
socketpair
bpo-29808: SyslogHandler: fix initial connect to syslog + code simplification bpo-29808: SyslogHandler: fix initial connect to syslog
| try: |
|---|
| self._connect_unixsocket(address) |
| except OSError: |
| pass |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty surprising to read—it makes more sense with the comment from the associated bug report.
Would you be averse to adding a comment in here to explain why it's safe to suppress this error?
| @@ -815,7 +815,10 @@ def __init__(self, address=('localhost', SYSLOG_UDP_PORT), |
|---|
| if isinstance(address, str): |
| self.unixsocket = True |
| self._connect_unixsocket(address) |
| try: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's contextlib.suppress which you may enjoy if you like context managers :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like. Is it required here in order patch to be accepted ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
Quite busy at the moment. Will get to it when I have time. Please be patient.
Does this need backport to 3.5 and 3.6? The ticket says this applies to those versions too.
Actually this bug happens with Python 3.5.2. So, yes, it will be nice if you apply this patch to these Python versions.
This was referenced
May 2, 2018
Tatsh added a commit to Tatsh/xirvik-tools that referenced this pull request
