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

Add socket.fdtype() #71564

Closed
nascheme opened this issue Jun 23, 2016 · 17 comments
Closed

Add socket.fdtype() #71564

nascheme opened this issue Jun 23, 2016 · 17 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@nascheme
Copy link
Member

BPO 27377
Nosy @nascheme, @tiran, @vadmium
PRs
  • bpo-27377: Add socket.fromfd2() and socket.fdtype(). #1333
  • bpo-27377: Add socket.fdtype() #1348
  • Files
  • fromfd2.txt
  • fromfd2_v2.txt
  • fromfd2_v3.txt: revised patch
  • fromfd2_v4.txt
  • fromfd2_v5.txt
  • 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 2020-10-31.20:04:07.375>
    created_at = <Date 2016-06-23.18:33:36.554>
    labels = ['3.7', 'type-feature', 'library']
    title = 'Add socket.fdtype()'
    updated_at = <Date 2020-10-31.20:04:07.375>
    user = 'https://github.com/nascheme'

    bugs.python.org fields:

    activity = <Date 2020-10-31.20:04:07.375>
    actor = 'nascheme'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-10-31.20:04:07.375>
    closer = 'nascheme'
    components = ['Library (Lib)']
    creation = <Date 2016-06-23.18:33:36.554>
    creator = 'nascheme'
    dependencies = []
    files = ['43524', '43530', '43572', '43656', '43722']
    hgrepos = []
    issue_num = 27377
    keywords = []
    message_count = 17.0
    messages = ['269131', '269140', '269143', '269149', '269196', '269219', '269451', '269456', '269953', '270155', '270426', '270429', '270432', '270669', '276631', '292549', '380093']
    nosy_count = 4.0
    nosy_names = ['nascheme', 'christian.heimes', 'SilentGhost', 'martin.panter']
    pr_nums = ['1333', '1348']
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27377'
    versions = ['Python 3.6', 'Python 3.7']

    @nascheme
    Copy link
    Member Author

    When implementing server software that inherits a socket via a file descriptor, it is useful to be able to query the descriptor and find out what kind of socket has been passed. This can be done with getsockopt() and getsockname(). Python does not expose a clean way to do this.

    One example use case is receiving an open socket from systemd. For example, systemd will pass the open socket using file descriptor 3. It would be handy if the Python server did not have to hard-code the type of the socket but instead could determine it. This patch provides a function that, when given an integer file descriptor, returns a Python socket object with the correct family and kind/type attributes. This seems like a useful, high-level interface.

    This patch adds two new functions to the socket module. I'm not so happy about the names. Suggestions welcome. The fromfd2() function does not duplicate the file descriptor. I think this was a design mistake in fromfd(). If it wasn't for the duplication, I would change fromfd() to have None as default arguments and then use fdtype() to get the family, kind and protocol. The fdtype() function returns (family, type, proto) for an integer file descriptor.

    There does not appear to be any way to query the protocol of the passed socket. I just assume it is zero.

    @nascheme nascheme added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 23, 2016
    @nascheme
    Copy link
    Member Author

    Add documentation for new functions.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Jun 23, 2016

    The patch applies cleanly, not sure why the rietveld link doesn't appear. Perhaps because of the extension?

    In test, I noticed, that you're not testing that OSError is raised, perhaps something to add?

    @vadmium
    Copy link
    Member

    vadmium commented Jun 24, 2016

    Maybe if the patch was regenerated with Mercurial off a public revision, it would work with Rietveld. I have seen non-Mercurial patches work too though. Perhaps this one doesn’t because the line numbers are off compared to the default branch?

    See also bpo-18391 discussing extending the existing fromfd() function to automatically determine the socket parameters.

    I agree that the name fromfd2() is a bit ugly, although there is precedence at least in Python 2: urllib2, os.popen2(), popen2 module, etc. Maybe other bike shed colours might be from_existing_fd(), using_fd(), fdopen(). Something that suggests creating an object from an FD without duplication.

    I wonder if fdtype() is really needed as a public API.

    The new fromfd2() API is missing when you do “from socket import * ”. There is a new function <https://docs.python.org/3.6/library/test.html#test.support.check__all__\> that may be useful to automate checking for this error.

    What prevents this from working on Windows? I understand there is less of a need, but I imagine it would be relatively easy to support.

    It looks like some platforms support SO_PROTOCOL and/or SO_PROTOTYPE to get a socket’s protocol: <http://austingroupbugs.net/view.php?id=840\>. Also, there might be a Windows-specific SO_PROTOCOL_INFO option.

    >>> s
    <socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 0)>
    >>> SO_PROTOCOL = 38  # On Linux x86-64; see Issue 18391
    >>> protocol_struct = Struct("I")
    >>> protocol = s.getsockopt(SOL_SOCKET, SO_PROTOCOL, protocol_struct.size)
    >>> protocol_struct.unpack(protocol)
    (6,)
    >>> IPPROTO_TCP
    6

    IMO if Python grows the ability to determine a socket’s protocol, it might be better for socket.proto to use it automatically, at least if proto=0 was specified to the socket() function.

    diff --git a/Doc/library/socket.rst b/Doc/library/socket.rst
    +.. function:: fromfd2(fd)
    +
    + Create a socket object from a file descriptor. The file descriptor is
    + not duplicated. The family, type and protocol for the socket are
    This needs fixing about how and if the protocol is determined.

    + determined from the file descriptor. If the file descriptor is not a
    + socket, OSError is raised. The socket is assumed to be in blocking mode.
    :exc:`OSError`

    + The newly created socket is :ref:`non-inheritable <fd_inheritance>`.
    This does not seem to be true, nor desirable:

    >>> s = socket()
    >>> s.get_inheritable()
    False
    >>> s.set_inheritable(True)
    >>> ss = fromfd2(s.fileno())
    >>> ss.get_inheritable()
    True
    >>> s.get_inheritable()
    True

    + .. versionadded:: 3.6
    + The returned socket is now non-inheritable.
    The whole function is new, not just process inheritance.

    diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
    + with s:
    + s2 = socket.fromfd2(s.fileno())
    + with s2:
    Isn’t this incorrect due to closing the same file descriptor twice? I think you should use socket.detach(). See bpo-26685, whose changes should raise an exception for this bug in 3.6.

    diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
    + if (!S_ISSOCK(st_fd.st_mode)) {
    + PyErr_SetString(PyExc_ValueError, "fdtype: "
    + "file descriptor is not a socket");
    This seems to be contrary to your documentation. Also, perhaps it is good enough to let getsockopt() later raising ENOTSOCK?

    + union sockaddr_union sockaddr = {};
    Why not just use a struct sockaddr? All we need is the sa.sa_family field right?

    @nascheme
    Copy link
    Member Author

    Thank you for the excellent review Martin. I'm attaching a new patch which I think addresses your points. I created it from hg, maybe that works nicer.

    I've added constants for SO_DOMAIN, SO_PROTOCOL, SO_PASSCRED, SO_PEERSEC, and SO_PASSEC.

    Using SO_PROTOCOL to get the protocol value seems to work fine on Linux.

    I've dropped the union as you suggest. I think the docs have been fixed.

    I would be okay with making fdtype() a non-public function. It seems possibly useful though.

    Regarding the name, currently I think fromfd2 is best. It really does what fromfd() should have done if SO_TYPE/SO_PROTOCOL was available and portable.

    Extending fromfd() seems a bad idea. I don't want dup() and it seems better that application code can use hasattr() to test for new functionality. You can work around not having fromfd2() by using getsockname() and then using some ugly text matching to work out what kind of socket you have.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 25, 2016

    Okay, I guess the fdtype() API could be useful if we don’t alter fromfd(), i.e. as a workaround for bpo-18391.

    IMO the specific SO_* constants should at least be listed in the documentation, so users know which versions of Python have which constants. Although exposing them is not necessary for the fromfd() implementation.

    See also more comments on the code review.

    @nascheme
    Copy link
    Member Author

    I've revised the patch based on the second round of comments from Martin. I've removed the ifdef test for CO_TYPE and assumed it is always available. That means fdtype() should be available on all platforms.

    I did not change the test as suggested to call s.detach(), I think my test code should work okay. The compare of 'protocol' has been removed from the test.

    The duplicate definition of SO_PASSCRED has been removed. I didn't add the constants to the documentation as other constants are only documented on a wildcard basis (e.g. SO_*).

    @vadmium
    Copy link
    Member

    vadmium commented Jun 29, 2016

    The s.detach() thing isn’t a big deal, but I thought it would simplify the code (eliminate the need for the “finally” clause).

    I will try to propose a minor documentation update for SO_PASSCRED as a starting point for adding the others.

    @nascheme
    Copy link
    Member Author

    nascheme commented Jul 7, 2016

    Adding yet another revised patch. I think this is ready to commit, if someone would like to do it. The documentation for constants can be added as a separate commit, if Martin wants. I think the generic SO_* style documentation is okay.

    Changes in this version:

    • fix documentation wording
    • add tests for different protocol types

    It would be really good(TM) if someone could test this on other platforms. I only tested on vanilla Debian Linux 3.16.0.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 11, 2016

    Left some minor code review nitpicks.

    I opened bpo-27409 with an attempt at documenting exactly which SO_* etc symbols may be available. But so far I haven’t got any positive or negative feedback. If it were up to me, I would either commit everything except the new SO_* constants, or briefly list them in the documentation.

    One more thing that occurred to me is maybe we should check for EINVAL from SO_PROTOCOL. That option was apparently added in Linux 2.6.32, and Free BSD 8.4, 9.1, 10.0. Even if you think these version numbers are too old to worry about, what happens if a Python package is compiled with a new (e.g.) OS X or Windows version that supports SO_PROTOCOL, and then run on an old (existing) OS version?

    @nascheme
    Copy link
    Member Author

    I just tested on Windows. fdtype() fails with:
    OSError: [WinError 10022] An invalid argument was supplied

    The getsockname() call fails with WSAGetLastError() == 10022. getsockname() is used to find the address family. Perhaps there is some other way to get it on Windows.

    The easiest fix would be to #ifdef MS_WINDOWS out the whole socket_fdtype() function. Passing file descriptors around is not a thing on Windows anyhow.

    There is still the issue of unexpected errors getting returned inside fdtype() (e.g. as suggested by Martin). One idea is to commit the code as is and fix errors as they appear.

    @nascheme
    Copy link
    Member Author

    Updated patch, v5. Disable fdtype() function on Windows. Fix documentation nits as suggested by review of v4.

    @nascheme
    Copy link
    Member Author

    Tested on FreeBSD 10.3. getsockname() on a IPPROTO_SCTP protocol socket returns errno = FileNoFoundError. We could just comment out that test I guess.

    My theory is that on FreeBSD, getsockname() on an SCTP socket fails if it is not bound. Indeed, adding a bind(('127.0.0.1', 11234)) call before the fdtype() call makes the test pass.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 17, 2016

    The Windows problem, error 10022 = WSAEINVAL from getsockname(), seems to be documented at <https://msdn.microsoft.com/en-us/library/windows/desktop/ms738543%28v=vs.85%29.aspx\>: “The socket has not been bound”.

    Regarding the SCTP problem, raising an error seems like an OS bug to me. However, according to POSIX <http://pubs.opengroup.org/onlinepubs/9699919799/functions/getsockname.html\>, we cannot rely on getsockname() indicating any particular address family if the socket is unbound.

    So on some platforms, it seems like it will only work with bound sockets, or you have to use less-standard options like SO_PROTOCOL_INFO or SO_DOMAIN. Although SO_DOMAIN won’t help on Free BSD. The simplest solution may be to document and test that it only works with bound sockets.

    For the problem with SO_PROTOCOL being unsupported at run-time, I think it would be better to anticipate EINVAL now, rather than wait for someone to complain. But it is not a big deal.

    @tiran
    Copy link
    Member

    tiran commented Sep 15, 2016

    I opened bpo-28134 because I was running into a similar issue with socket.socket(fileno). Since this new feature didn't make it into 3.6, can we at least change the constructor of socket.socket() to set a correct family, type and proto?

    Any fix is better than no fix. It doesn't have to work for all socket types on all operating systems. The feature is mostly used for socket inheritance, systemd socket activation or fd transfer over Unix sockets of AF_INET, AF_INET6 and AF_UNIX file descriptors.

    @tiran tiran added the 3.7 (EOL) end of life label Sep 15, 2016
    @nascheme
    Copy link
    Member Author

    Changing title, I think bpo-28134 (socket.socket(fileno=fd) does not work as documented) is a cleaner solution. However, exposing the ability to query a socket file descriptor seems useful so I'm still proposing to add socket.fdtype().

    The github PR 1348 has been modified to remove fromfd2() and only add the function fdtype().

    @nascheme nascheme changed the title Add smarter socket.fromfd() Add socket.fdtype() Apr 28, 2017
    @nascheme
    Copy link
    Member Author

    I believe this is not needed anymore. Closing.

    @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
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants