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

Teach trio.socket.fromfd and friends to autodetect socket configuration on Linux #577

Closed
wants to merge 1 commit into from

Conversation

2easy
Copy link

@2easy 2easy commented Jul 29, 2018

Hi, I'd like to propose this solution to the issue #251 .

@2easy
Copy link
Author

2easy commented Jul 29, 2018

There is problem with SO_PROTOCOL on mac, will fix that in a moment.

@codecov
Copy link

codecov bot commented Jul 29, 2018

Codecov Report

Merging #577 into master will decrease coverage by 2.16%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #577      +/-   ##
=========================================
- Coverage   99.27%   97.1%   -2.17%     
=========================================
  Files          89      89              
  Lines       10628   10656      +28     
  Branches      747     750       +3     
=========================================
- Hits        10551   10348     -203     
- Misses         59     274     +215     
- Partials       18      34      +16
Impacted Files Coverage Δ
trio/tests/test_ssl.py 100% <ø> (ø) ⬆️
trio/tests/test_socket.py 97.83% <ø> (-2.17%) ⬇️
trio/_socket.py 94.94% <93.93%> (-5.06%) ⬇️
trio/_core/_windows_cffi.py 0% <0%> (-91.31%) ⬇️
trio/_core/_io_windows.py 0% <0%> (-77.1%) ⬇️
trio/_core/tests/test_windows.py 23.33% <0%> (-76.67%) ⬇️
trio/_highlevel_open_unix_stream.py 66.66% <0%> (-11.12%) ⬇️
trio/_util.py 92.68% <0%> (-7.32%) ⬇️
trio/tests/test_highlevel_open_unix_stream.py 93.54% <0%> (-6.46%) ⬇️
trio/_highlevel_open_tcp_listeners.py 94.73% <0%> (-5.27%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd0d877...0a86243. Read the comment docs.

@2easy 2easy force-pushed the 2easy/issue_251 branch from 0c7b0ac to 0a86243 Compare July 29, 2018 16:26
@njsmith
Copy link
Member

njsmith commented Jul 30, 2018

Hello!

I'm afraid I have misled you and made this much more complicated than it has to be :-(. The code you used for a reference jumps through all kinds of hoops using ctypes, because it's trying to call getsockopt on a socket that it only has a fileno for, not a socket object. But on all the versions of Python we care about, we can create a socket object (maybe with incorrect values), and then we can directly use Python's regular getsockopt method and skip all that ctypes stuff. Also, the versions of Python we care about have the relevant SO_WHATEVER magic constants on the socket module on all the systems where they work at all, so we don't need to have the magic numbers like "39" for SO_DOMAIN. (And besides, those magic numbers are only correct for Linux.)

So my suggestion would be to delete all the ctypes stuff, and instead have a function like this, that takes whatever the user gave us, fixes it up as best we can (which might be more or less depending on the OS), and returns the fixed up values.

def _fix_attrs_for_fileno(family, type, proto, fileno):
    # Wrap the raw fileno into a socket object
    sockobj = _stdlib_socket.socket(fileno=fileno)
    try:
        if hasattr(socket, "SO_DOMAIN"):
            family = sockobj.getsockopt(SOL_SOCKET, SO_DOMAIN)
        if hasattr(socket, "SO_TYPE"):
            type = sockobj.getsockopt(SOL_SOCKET, SO_TYPE)
        if hasattr(socket, "SO_PROTOCOL"):
            proto = sockobj.getsockopt(SOL_SOCKET, SO_PROTOCOL)
    finally:
        # Unwrap it again, so that sockobj.__del__ doesn't try to close our socket
        sockobj.detach()
    return family, type, proto

And then in fromfd and socket we can do something like:

family, type, proto = _fix_attrs_for_fileno(family, type, proto, fileno)

Does that make sense?

@njsmith njsmith changed the title 2easy/issue 251 Teach trio.socket.fromfd and friends to autodetect socket configuration on Linux Jul 31, 2018
@sorcio sorcio mentioned this pull request Jul 31, 2018
@njsmith
Copy link
Member

njsmith commented Aug 21, 2018

@2easy Hey, just wanted to check in to see whether you're still interested in working on this :-)

@2easy
Copy link
Author

2easy commented Aug 21, 2018

Sure thing, sorry for the delay. I'll fix it in the evening.

@njsmith
Copy link
Member

njsmith commented Aug 28, 2018

Haven't heard anything for a week, so I'm going to close this for now to take it off the list of active PRs. Feel free to reopen it, or open a new PR, if you do decide to come back to it later! We've all been there :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants