Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smtplib: add ability to bind to specific source IP address/port #55490

Closed
paulos mannequin opened this issue Feb 22, 2011 · 21 comments
Closed

smtplib: add ability to bind to specific source IP address/port #55490

paulos mannequin opened this issue Feb 22, 2011 · 21 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@paulos
Copy link
Mannequin

paulos mannequin commented Feb 22, 2011

BPO 11281
Nosy @giampaolo, @bitdancer
Files
  • smtp_lib_source_address.patch: add source_address arg to smtplib.xSMTP
  • smtp_lib_source_address.patch: Cleaned up patch
  • smtp_lib_source_address.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2011-07-30.02:58:43.538>
    created_at = <Date 2011-02-22.04:10:54.767>
    labels = ['type-feature', 'library']
    title = 'smtplib: add ability to bind to specific source IP address/port'
    updated_at = <Date 2011-07-30.02:58:43.536>
    user = 'https://bugs.python.org/paulos'

    bugs.python.org fields:

    activity = <Date 2011-07-30.02:58:43.536>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-07-30.02:58:43.538>
    closer = 'python-dev'
    components = ['Library (Lib)']
    creation = <Date 2011-02-22.04:10:54.767>
    creator = 'paulos'
    dependencies = []
    files = ['20854', '20944', '20945']
    hgrepos = []
    issue_num = 11281
    keywords = ['patch']
    message_count = 21.0
    messages = ['129035', '129059', '129085', '129092', '129098', '129100', '129101', '129103', '129106', '129108', '129112', '129150', '129152', '129153', '129447', '129496', '129699', '129701', '129703', '139343', '141417']
    nosy_count = 4.0
    nosy_names = ['giampaolo.rodola', 'r.david.murray', 'paulos', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue11281'
    versions = ['Python 3.3']

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 22, 2011

    In smtplib there is now way to bind to an specific source address on a machine with multiple interfaces; also, there no way to control the source port for the connection.

    Since 2.7, socket.create_connection accepts a source_address parameter, a (host, port) tuple for the socket to bind to as its source address before connecting. If host or port are '' or 0 respectively the OS default behavior will be used.

    I would like to add source_ip and source_port parameters to smtplib.SMTP, default to '' and 0 respectively.

    It is a small change with no impact over existing code. Should I submit a patch?

    @paulos paulos mannequin added the stdlib Python modules in the Lib dir label Feb 22, 2011
    @bitdancer
    Copy link
    Member

    This sounds like a reasonable feature request. If you would like to propose a patch against trunk (py3k, what will become 3.3), I will take a look at it.

    @bitdancer bitdancer added the type-feature A feature request or enhancement label Feb 22, 2011
    @giampaolo
    Copy link
    Contributor

    +1, this has been done for other modules such as ftplib as well and probably could be done for others such as httplib and poplib.

    @giampaolo
    Copy link
    Contributor

    I would like to add source_ip and source_port parameters to
    smtplib.SMTP, default to '' and 0 respectively.

    It would be better to provide a unique source_address parameter defaulting to None, for consistency with socket.create_connection() expecting a (addr, port) tuple.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 22, 2011

    Seems like the signature of Lib.test.mock_socket.create_connection does not match socket.create_connection since 2.7.

    I need help with the docs; the last argument is positional and optional, using the `[ ]' notation. What is the notation for the new keyword arguments?

    @giampaolo
    Copy link
    Contributor

    Seems like the signature of Lib.test.mock_socket.create_connection does
    not match socket.create_connection since 2.7.

    You can add a fake parameter which does nothing.

    What is the notation for the new keyword arguments?

    fun(a, b, c=None, d=None)

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 22, 2011

    Also, it is my first patch and I have no clue about what I'm doing, I don't expect to get it right in the first try - please point any mistakes.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 22, 2011

    @giampaolo: should I change the text in Doc/library/smtplib.rst or it is infered from the docstrings?

    @giampaolo
    Copy link
    Contributor

    Yes, you must update Doc/library/smtplib.rst and write tests.
    If you're not the one who's gonna commit the patch you can ignore Doc/whatsnew/3.3.srt and Misc/NEWS changes.

    Updating the docstring is usually optional but I see smtplib module is already well documented so I'd update docstrings as well.

    As for how to write a patch see:
    http://www.python.org/dev/faq/#patches
    ...or search in the bug tracker for committed patches, such as:
    http://bugs.python.org/issue8807
    http://svn.python.org/view?view=revision&revision=84144

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 22, 2011

    My first idea was to make the argument a tuple to match socket.create_connection(), but SMTP uses host and port arguments, for consistency it's better havin separate source_ip and source_port arguments. As a side effect, it makes easier to specify only source_port.

    The submited patch includes tests and updated docstrings;

    At the smtplib.rst, for example:

    SMTP(host='', port=0, local_hostname=None[, timeout])

    Would be ok to change it like:

    SMTP(host='', port=0, local_hostname=None[, timeout], source_ip='', source_port=0)

    @giampaolo
    Copy link
    Contributor

    We already have 3 places where a tuple is used:

    socket.socket.bind
    socket.create_connection
    http.client.HTTPConnection

    Changing this notation in smtplib for such a rarely used feature is not worth the effort, imo.
    Also, I would not add two new SMTP attributes. Passing a tuple to socket.create_connection() is just fine. If you want to know the source address afterwards you can use socket.getsockname().

    PS - I modified smtplib.py in meantime. Please update your local copy.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 23, 2011

    Ok, I changed to a single parameter matching socket.create_connection().

    I made my best to update tests and docs, but I don't have a clue if it is right.

    @giampaolo
    Copy link
    Contributor

    Follow my comments:

    •             source_address=('', 0)):
      

    Make that default to None instead and pass it as-is to socket.create_connection().

    •    self.source_address = source_address
      

    There's no need to store the source address as an instance attribute.
    Just pass it as-is to socket.create_connection() in __init__ and connect methods and then get rid of it.

    I think source_address no longer makes sense in UNIX sockets / LMTP class.
    Or at least, this is what I get if I try to bind() a UNIX socket:

    >>> import socket
    >>> sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
    >>> sock.bind(("", 0))
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "<string>", line 1, in bind
    TypeError: argument must be string or read-only character buffer, not tuple
    >>>

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 23, 2011

    There's no need to store the source address as an instance attribute.
    Just pass it as-is to socket.create_connection() in __init__ and
    connect methods and then get rid of it.

    Ok, what if user initialize with something like smtplib.SMTP(source_address=('200.165.12.1', 25)) and calls connect without source_address?

    @giampaolo
    Copy link
    Contributor

    You are right, I haven't thought about that.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 26, 2011

    Giampaolo,

    Thanks for your kind review, I will send a patch with suggested changes this weekend.

    Grazie,

    Paulo

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 28, 2011

    Patch attached.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 28, 2011

    oops...

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Feb 28, 2011

    Sorry for the last post, I had the impression that I forgot to attach the patch. I'm slow this Monday.

    @paulos
    Copy link
    Mannequin Author

    paulos mannequin commented Jun 28, 2011

    Is there anything I should improve in order to get this patch commited?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 30, 2011

    New changeset 26839edf3cc1 by Senthil Kumaran in branch 'default':
    Fix closes bpo-11281 - smtplib.STMP gets source_address parameter, which adds the ability to bind to specific source address on a machine with multiple interfaces. Patch by Paulo Scardine.
    http://hg.python.org/cpython/rev/26839edf3cc1

    @python-dev python-dev mannequin closed this as completed Jul 30, 2011
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants