R. David Murray added the comment: For some reason the diff shown by the review link is very different from the one show by the patch file itself. I'm not sure what is causing that, since the diff appears to be in the correct hg format. I don't even know where reitveld is getting the stuff on the left hand side.
So, comments: Instead of issuing a warning for decode_data=True when smtputf8 is specified, having decode_data explicitly set to True when smtputf8 is specified should be a ValueError. Your modifications to the handling of max_command_size are technically buggy (and clearly there's no test for it): when we aren't in extended smtp mode, the limit should be the hardcoded value, as in the original code. My reading of the RFC (specifically 4.1.1.5) indicates that it is legal to send a new HELO or EHLO command without issuing a QUIT, meaning a single SMTPChannel instance might have to handle both. This isn't a likely scenario except in testing...but testing is probably one of the main uses of smtpd, so I think we ought to support it. Also, the code you use to update the max length value for MAIL takes a bit of thought to understand. I think it would be clearer to use an unconditional set of the value based on the command_size_limit constant, which you'll have to restore to address my previous comment, instead of the pop (though the pop is a nice trick :). The answer to your 'XXX should we check this?' is no. Remember that just because the client *says* SMTPUTF8, that doesn't mean the message actually has to be SMTPUTF8. So whatever bytes it sends need to be passed through. When SMTPUTF8 is not turned on, the server should act like it doesn't know anything about it, which means that if SMTPUTF8 is specified on the BODY command it should be treated just like an unknown parameter would be. We shouldn't leak the fact that we "know about" SMTPUTF8 if the server hasn't been enabled for SMTPUTF8. There is one piece missing here: providing a way for process_message to know that the client claimed the message was using SMTPUTF8 (we know it might lie, but we need to transmit what the client said). Since process_message is an 'override in subclass' API, we can't just extend it's call signature. So I think we need to provide a new process_smtputf8_message method that will be called for such messages. Note that you also need to reset the 'this message is SMTPUTF8 flag' on RSET, HELO/EHLO, and at the completion of the call to the new process_message API. This notes should also give you some clues about what additional test need to be written. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue21725> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
