bpo-29808: SyslogHandler: fix initial connect to syslog #663

socketpair · 2017-03-14T06:29:05Z

No description provided.

the-knights-who-say-ni · 2017-03-14T06:29:06Z

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!

mention-bot · 2017-03-14T06:29:07Z

@socketpair, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vsajip, @benjaminp, @nnorwitz, @Yhg1s and @birkenfeld to be potential reviewers.

socketpair · 2017-03-14T06:37:07Z

@the-knights-who-say-ni I have signed what you wanted me to sign.

socketpair · 2017-03-14T06:38:38Z

Lib/logging/handlers.py

-            if socktype == socket.SOCK_STREAM:
-                self.socket.connect(address)
-            self.socktype = socktype
+            self.socket.connect(address)


This actually works for UDP sockets too. This simplify send() later since we should not specify address every time.

socketpair · 2017-03-14T06:44:47Z

Lib/logging/handlers.py

-            self._connect_unixsocket(address)
+            self._unix_socktype = socktype
+            try:
+                self._connect_unixsocket()


Syslog server may not work at the time of the constructor call.

ned-deily · 2017-03-14T07:56:28Z

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.

Mariatta · 2017-03-14T10:14:05Z

@socketpair Did you add your GitHub username to the account in b.p.o?

socketpair · 2017-03-14T11:18:58Z

@Mariatta I don't know what is b.p.o. I have an account at bugs.python.org. And also have just signed something @the-knights-who-say-ni said to sign.

vsajip · 2017-03-14T17:05:05Z

Lib/logging/handlers.py

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


Please don't make random stylistic changes.

vsajip · 2017-03-14T17:05:45Z

Lib/logging/handlers.py

        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__()


Please don't make changes to "tidy up the code base". Address only specific things which cause the issue.

Well, should I make another PR, that fixes such things?

Mariatta · 2017-03-14T18:38:40Z

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

socketpair · 2017-03-14T18:49:17Z

@Mariatta

I set socketpair as github username
I changed bugs.python.org's username from mmarkk to socketpair
Please note, I signed CLA as username socketpair

socketpair · 2017-03-14T18:50:35Z

ahh.. b.p.o is "bugs.python.org". google gets https://en.wikipedia.org/wiki/Business_process_outsourcing

socketpair · 2017-03-14T18:51:42Z

@vsajip
@prophile
@Mariatta
What happens with code coverage check? I did not touch files that this tool mentions.

prophile · 2017-03-14T22:48:45Z

Lib/logging/handlers.py

+            try:
+                self._connect_unixsocket(address)
+            except OSError:
+                pass


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?

prophile · 2017-03-14T22:49:44Z

Lib/logging/handlers.py

@@ -815,7 +815,10 @@ def __init__(self, address=('localhost', SYSLOG_UDP_PORT),

        if isinstance(address, str):
            self.unixsocket = True
-            self._connect_unixsocket(address)
+            try:


There's contextlib.suppress which you may enjoy if you like context managers :)

I don't like. Is it required here in order patch to be accepted ?

…iled (#663)

socketpair · 2017-03-17T06:12:41Z

@vsajip
@prophile
@Mariatta

I have fixed everything you asked to fix. Ping.

vsajip · 2017-03-17T08:34:57Z

Quite busy at the moment. Will get to it when I have time. Please be patient.

Mariatta · 2017-03-17T15:36:04Z

Does this need backport to 3.5 and 3.6? The ticket says this applies to those versions too.

socketpair · 2017-03-17T15:49:38Z

Actually this bug happens with Python 3.5.2. So, yes, it will be nice if you apply this patch to these Python versions.

the-knights-who-say-ni added the CLA not signed label Mar 14, 2017

socketpair commented Mar 14, 2017

View reviewed changes

ned-deily assigned vsajip Mar 14, 2017

methane removed the CLA not signed label Mar 14, 2017

the-knights-who-say-ni added the CLA not signed label Mar 14, 2017

socketpair changed the title ~~SyslogHandler: fix initial connect to syslog + code simplification.~~ SyslogHandler: fix initial connect to syslog + code simplification (bpo-29808) Mar 14, 2017

Mariatta changed the title ~~SyslogHandler: fix initial connect to syslog + code simplification (bpo-29808)~~ bpo-29808: SyslogHandler: fix initial connect to syslog + code simplification Mar 14, 2017

vsajip reviewed Mar 14, 2017

View reviewed changes

Mariatta removed the CLA not signed label Mar 14, 2017

the-knights-who-say-ni added the CLA signed label Mar 14, 2017

socketpair changed the title ~~bpo-29808: SyslogHandler: fix initial connect to syslog + code simplification~~ bpo-29808: SyslogHandler: fix initial connect to syslog Mar 14, 2017

prophile reviewed Mar 14, 2017

View reviewed changes

bpo-29808: SysLogHandler: Do not fail if initial connect to syslog fa…

a7e4453

…iled (#663)

vsajip merged commit 1b038e0 into python:master Mar 17, 2017

Mariatta added needs backport to 3.5 labels Mar 17, 2017

vsajip removed needs backport to 3.5 labels Mar 17, 2017

This was referenced May 2, 2018

SysLogHandler stack traces when using incorrect log_file location in python3 saltstack/salt#47432

Closed

Catch Sysloghandler errors when log file does not exist saltstack/salt#47548

Merged

Tatsh added a commit to Tatsh/xirvik-tools that referenced this pull request Sep 2, 2018

Syslog fix based on python/cpython#663

681d8a3

Uh oh!

bpo-29808: SyslogHandler: fix initial connect to syslog #663

bpo-29808: SyslogHandler: fix initial connect to syslog #663

Uh oh!

Conversation

socketpair commented Mar 14, 2017

Uh oh!

the-knights-who-say-ni commented Mar 14, 2017

Uh oh!

mention-bot commented Mar 14, 2017

Uh oh!

socketpair commented Mar 14, 2017

Uh oh!

socketpair Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

socketpair Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ned-deily commented Mar 14, 2017

Uh oh!

Mariatta commented Mar 14, 2017

Uh oh!

socketpair commented Mar 14, 2017

Uh oh!

vsajip Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsajip Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socketpair Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Mar 14, 2017

Uh oh!

socketpair commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socketpair commented Mar 14, 2017

Uh oh!

socketpair commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prophile Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

prophile Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

socketpair Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

prophile Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

socketpair commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsajip commented Mar 17, 2017

Uh oh!

Mariatta commented Mar 17, 2017

Uh oh!

socketpair commented Mar 17, 2017

Uh oh!

Uh oh!

socketpair Mar 14, 2017 •

edited

Loading

vsajip Mar 14, 2017 •

edited

Loading

vsajip Mar 14, 2017 •

edited

Loading

socketpair commented Mar 14, 2017 •

edited

Loading

socketpair commented Mar 14, 2017 •

edited

Loading

socketpair commented Mar 17, 2017 •

edited

Loading