bpo-30378: Fix the problem that SysLogHandler can't handle IPv6 addresses #1676
Conversation
|
@zhangyangyu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vsajip, @tiran and @benjaminp to be potential reviewers. |
Lib/logging/handlers.py
Outdated
| @@ -812,6 +812,7 @@ def __init__(self, address=('localhost', SYSLOG_UDP_PORT), | |||
| self.address = address | |||
| self.facility = facility | |||
| self.socktype = socktype | |||
| self.formatter = None | |||
There was a problem hiding this comment.
This should not be needed, as it is done in the base class __init__(), which is invoked on line 810.
Lib/logging/handlers.py
Outdated
| host, port = address | ||
| err = None | ||
| for res in socket.getaddrinfo(host, port, 0, socktype): | ||
| af, socktype, proto, cannonname, sa = res |
There was a problem hiding this comment.
The canonical name isn't used, so could use _ to bind it. Otherwise, use canonical as the name to bind to - cannonname is wrong.
Lib/logging/handlers.py
Outdated
| sock.connect(sa) | ||
| self.socket = sock | ||
| self.socktype = socktype | ||
| return |
Lib/logging/handlers.py
Outdated
| self.socket = sock | ||
| self.socktype = socktype | ||
| return | ||
| except OSError as _: |
There was a problem hiding this comment.
Why not just use a finally clause to close the socket, and let the error propagate naturally?
There was a problem hiding this comment.
Honestly speaking, I copied most logic from socket.create_connection(). But I don't understand how to use finally here. We only need to close the socket when socket.connect() fails.
There was a problem hiding this comment.
Ah, sorry, you're right. My mistake.
Lib/logging/handlers.py
Outdated
| sock.close() | ||
| if err is not None: | ||
| raise err | ||
| else: |
There was a problem hiding this comment.
Doesn't this else really belong to the for loop rather than the if?
There was a problem hiding this comment.
If we use return, there's no difference.
There was a problem hiding this comment.
Are you referring to my other comment? If so, I agree that return is equivalent to break functionally, but I prefer to use break because then in future code can be added after the for loop if needed. I generally prefer to avoid return in the middle of functions when it's easy to do.
There was a problem hiding this comment.
Fair enough. But we can't make else belong to the for loop here. If all the results are tried and fail, we could also go into the else branch and give wrong message. I reformat the code a bit, please see it's okay or not. :-)
| @@ -327,6 +327,9 @@ Extension Modules | |||
| Library | |||
| ------- | |||
|
|
|||
| - bpo-30378: Fix the problem that logging.handlers.SysLogHandler cannot | |||
There was a problem hiding this comment.
Not sure having this in the patch is a good idea - may cause unnecessary merge conflicts.
There was a problem hiding this comment.
Yes, NEWS now is the main source of merge backport conflicts, but we need to log what happened. :-(
|
Notice the merge conflict with Misc/NEWS. Suggest you remove this from the patch - it can always be added in a separate patch which will not require careful review, since it won't have any code. |
|
@vsajip , I'll take care of it. I have solved such conflicts many times. So does the code change LGTY now and I can merge it? |
zhangyangyu
added
type-bug
|
Is it possible to have it favour the IPv4 addresses that are resolved? That will still allow us to use IPv6 but keep the same behaviour as before. |
|
@coldeasy Please open an issue on https://bugs.python.org to discuss it. |
No description provided.