Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(129423)

#21795: smtpd.SMTPServer should announce 8BITMIME when supported and accept SMTPUTF8 without it

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by milan.py
Modified:
5 years, 3 months ago
Reviewers:
rdmurray
CC:
barry, AntoinePitrou, r.david.murray, jesstess, devnull_psf.upfronthosting.co.za, zvyn
Visibility:
Public.

Patch Set 1 #

Total comments: 11

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/smtpd.rst View 1 3 chunks +20 lines, -15 lines 0 comments Download
Lib/smtpd.py View 1 10 chunks +53 lines, -39 lines 0 comments Download
Lib/test/test_smtpd.py View 1 4 chunks +72 lines, -21 lines 0 comments Download

Messages

Total messages: 1
r.david.murray
5 years, 3 months ago #1
I haven't reviewed the tests yet, but I wanted to publish this before the
checkin.

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst
File Doc/library/smtpd.rst (right):

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst#newcode57
Doc/library/smtpd.rst:57: When *decode_data* is set to ``True`` the server
accepts ``BODY=8BITMIME``
When *decode_data* is set to ``False`` and the client has executed the EHLO
command...

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst#newcode61
Doc/library/smtpd.rst:61: .. method:: process_message(peer, mailfrom, rcpttos,
data, \*\*kwargs)
I don't think the escapes are required here, did you try it?

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst#newcode76
Doc/library/smtpd.rst:76: unless ``decode_data=False`` or
``enable_SMTPUTF8=True`` was given as
unless at least one of

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst#newcode77
Doc/library/smtpd.rst:77: init parameter. Otherwise it contains the following
keys:
an init parameter.

http://bugs.python.org/review/21795/diff/12699/Doc/library/smtpd.rst#newcode81
Doc/library/smtpd.rst:81: *rcpt_options*: same as *mail_options* but for the
``RCPT`` command.
This should note that currently no RCPT TO options are supported.

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py
File Lib/smtpd.py (right):

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode389
Lib/smtpd.py:389: } if not self._decode_data else {}
Again, I would prefer an if here rather than a multiline expression:

kwargs = {}
if not self._decode_data:
   kwargs = dict(mail_options=self.mail_options, rcpt_options=self.rcpt_options)

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode539
Lib/smtpd.py:539: params_dict = self._getparams(self.mail_options)
I don't see the point in renaming this params_dict.  In python we generally
don't use any sort of hungarian notation.

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode541
Lib/smtpd.py:541: self.push(syntaxerr)
This was bad logic.  It depended on the fact that we only recognized *one*
parameter, SIZE, so if we had params and we got none back, that meant what we
did get was an invalid parameter and we pushed it.  Now we can have multiple
parameters, so if we have one good one and one bad one, we will this test will
pass and we will fail to generate a syntax error for the bad parameter.  So
_getparams needs to be changed to fix this.  The simplest fix is probably to
have it return None if there is an invalid parameter (something that doesn't
pass the isalnum test).

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode545
Lib/smtpd.py:545: params_dict.update({'BODY': body})
Shouldn't you instead be checking that BODY is one of 7BIT or 8BITMIME and
generating a syntax error if it is not?  And generating a syntax error in any
case if self.enable_8BITMIME is False?

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode550
Lib/smtpd.py:550: params_dict.update({'SMTPUTF8': True})
Please add a comment here that says "push the parameter back to produce an
unknown parameter error below".

http://bugs.python.org/review/21795/diff/12699/Lib/smtpd.py#newcode711
Lib/smtpd.py:711: - 'rcpt_options': same for the rcpt command.
To be consistent with the code and the docs, this should say "will contain".  I
think that's better for backward compatibility (if/when we do support rctp to
options, an application can depend on the empty list existing if run using an
older python that does not support them).
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+