Repository navigation
Conversation
|
All commit authors signed the Contributor License Agreement. |
Documentation build overview
|
| :func:`socket.getfqdn`. If the :meth:`connect` call returns anything other | ||
| :func:`socket.getfqdn`. If it is not fully qualified, the connected socket's | ||
| local IP address is used as an address literal instead. IPv6 addresses use | ||
| the ``[IPv6:address]`` form. If the :meth:`connect` call returns anything other |
There was a problem hiding this comment.
I'm not sure it is useful to call out IPv6 separately here, since what we are doing is (now) following the standard, but I don't have a strong objection to including it.
| ) | ||
| for family, address, expected in cases: | ||
| for connect_in_init in (True, False): | ||
| with self.subTest(family=family, connect_in_init=connect_in_init): |
There was a problem hiding this comment.
I think it would be good to include address in the subtest description.
| except socket.gaierror: | ||
| pass | ||
| self.local_hostname = '[%s]' % addr | ||
|
|
There was a problem hiding this comment.
As far as I can see, the second part of this inner else is now code that doesn't have any effect, since local_hostname then gets reset unconditionally in connect. I suggest the following:
self.local_hostname = local_hostname
if local_hostname is None:
# RFC 2821 says we should use the fqdn in the EHLO/HELO verb, and
# if that can't be calculated, that we should use a domain literal
# instead (essentially an encoded IP address like [A.B.C.D]).
fqdn = socket.getfqdn()
if '.' in fqdn:
self.local_hostname = fqdn
else:
# We can't find an fqdn hostname, so we'll compute the
# correct domain literal in connect.
self._find_local_hostname = True
And then of course changing _use_socket_hostname to _find_local_hostname in connect. What do you think? The tests, including the new ones, still pass.
| if code == 220 and self._use_socket_hostname: | ||
| addr = self.sock.getsockname()[0] | ||
| if self.sock.family == socket.AF_INET6: | ||
| # An SMTP address literal does not include a scope identifier. |
There was a problem hiding this comment.
For clarity, let's add: but does require an IPv6 "tag" per RFC 5321 Section 4.1.3
| if self.sock.family == socket.AF_INET6: | ||
| # An SMTP address literal does not include a scope identifier. | ||
| addr = 'IPv6:' + addr.split('%', 1)[0] | ||
| self.local_hostname = '[%s]' % addr |
There was a problem hiding this comment.
I think it would be clearer to use an f-string here.
| @@ -0,0 +1,3 @@ | |||
| When no fully qualified hostname is available, :mod:`smtplib` now uses the | |||
| connected socket's local address for the HELO/EHLO address literal, including | |||
| properly formatted IPv6 literals. | |||
There was a problem hiding this comment.
I'd add: "Previously IPv6 was not handled correctly."
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
2 participants
Uh oh!
There was an error while loading. Please reload this page.