Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#4388 enhancement closed fixed (fixed)

bindings for sendmsg and recvmsg

Reported by: glyph Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: marcandre.lureau@…, thijs Branch: branches/sendmsg-4388-2
(diff, github, buildbot, log)
Author: glyph, exarkun Launchpad Bug:

Description

These functions are useful for various things, including sending file descriptors along to other processes.

Change History (30)

comment:1 Changed 5 years ago by glyph

It's too bad that this hasn't been done yet in Python itself, but we've been waiting for quite a while, so maybe we should just bite the bullet.

comment:2 Changed 4 years ago by mlureau

  • Cc marcandre.lureau@… added

comment:4 in reply to: ↑ 3 Changed 4 years ago by glyph

Replying to exarkun:

Both are GPL (not even LGPL!), and therefore unusable in Twisted :(.

I've written a version in Calendar Server that is currently Apache licensed, but I do have the go-ahead to re-license this as a contribution to Twisted. It's on my to-do list.

Or, if someone wants to talk to the authors of these packages to change one of their licenses, we might be able to add a dependency if someone wants to look over their code in more detail. (I'd love to avoid long-term maintenance of C code, but a quick scan of these packages' respective web sites does not fill me with confidence.)

comment:5 Changed 4 years ago by <automation>

  • Owner glyph deleted

comment:6 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/sendmsg-4388

(In [34069]) Branching to 'sendmsg-4388'

comment:7 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:8 Changed 2 years ago by exarkun

  • Branch changed from branches/sendmsg-4388 to branches/sendmsg-4388-2

(In [34097]) Branching to 'sendmsg-4388-2'

comment:9 Changed 2 years ago by exarkun

(In [34166]) Contributed to Twisted under Twisted's license; copyright notices changed with permission. See http://twistedmatrix.com/trac/ticket/4388#comment:4.

refs #4388

comment:10 Changed 2 years ago by exarkun

  • Author changed from exarkun to glyph, exarkun
  • Keywords review added
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new

I think this is ready. Build results will be available at some point.

comment:11 Changed 2 years ago by thijs

  • Cc thijs added

Some minor stuff:

  1. twisted/python/test/pullpipe.py
    1. still uses old twext.python.test.test_sendmsg testcase name
  2. doc/core/howto/sendmsg.xhtml
    1. Adding links to recvmsg and sendmsg wouldn't hurt if the document is intended for Twisted maintainers
  3. twisted/python/test/test_sendmsg.py
    1. Needs a module docstring
  4. News file is missing

comment:12 Changed 2 years ago by exarkun

(In [34174]) Address review points

refs #4388

  1. Fixed pullpipe test-case-name
  2. Linked to the opengroup documentation for sendmsg and recvmsg in the howto
  3. Added a module docstring to the test module
  4. Added a news fragment

comment:13 Changed 2 years ago by exarkun

(In [34175]) Change the tests to assert that socket.error is raised, not IOError

socket.error is an IOError subclass from Python 2.6 and onward, but not in Python 2.5.
The tests are better for being more specific, anyway.

refs #4388

comment:14 Changed 2 years ago by exarkun

(In [34176]) Improve the skip logic for Windows

refs #4388

comment:15 Changed 2 years ago by exarkun

(In [34177]) sigh, no socketpair on Windows either

refs #4388

comment:16 Changed 2 years ago by itamar

  • sendmsg.c
    1. sendmsg_getsockfam_doc says "SOCKET.AF_UNIX", that should be lowercase.
    2. sendmsg_sendmsg never frees message_header.message_control in the success case where sendmsg actually returned a result.

I will continue later.

comment:17 Changed 2 years ago by exarkun

(In [34182]) Fix the casing of the socket.AF_UNIX link

refs #4388

comment:18 Changed 2 years ago by exarkun

(In [34183]) Change the error handling and cleanup logic in sendmsg_sendmsg to use the goto idiom, reducing the duplication in decref/free logic.

refs #4388

comment:19 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to exarkun

Thanks for getting this done! I'm looking forward to doing a twistd metaplugin hack so I can do "twistd multicore web --wsgi=foo.bar".

  1. sendmsg.c
    1. Docstring for sendmsg_recvmsg should note the default sizes that will be used if none are provided.
    2. The way you calculate length of ancillary data in sendmsg_recvmsg (control_message->cmsg_len - sizeof(struct cmsghdr)) bothers me: UNP suggests there might be padding in between the struct and the data bytes, and maybe sizeof takes that into account but I'm wary. It seems more correct to do control_message->cmsg_len - (CMSG_DATA(control_message) - control_message), I think? Since you know pointer address of data you can directly calculate how many bytes are between it and start of struct.
  2. sendmsg.xhtml
    1. The howto talks about scatter/gather and datagrams even though they're not supported. Instead might want to mention those as things that are *not* supported. (I guess technically conncected datagrams are supported, but without msg_name there's no unconnected support.)
  3. test_sendmsg.py
    1. You import AF_INET6 unconditionally; elsewhere I've seen the IPv6 be done conditionally, IIRC.
    2. StartStopProtocol doesn't have two line breaks between methods.
    3. The fact BadList can be iterated a first time but not the second time is not made explicit in the docstring.
    4. No tests that flags are passed to underlying sendmsg()/recvmsg() calls.
    5. Might be better to manipulate PYTHONPATH in spawn(), rather than manipulating sys.path inside pullpipe.py? OTOH this is probably more succinct.
    6. You don't test return results of sendmsg_sendmsg(), or sufficiently test return results of sendmsg_recvmsg().
    7. There's no test for sending so much data the total length wrapped around, but that's probably OK :)
  4. Given the fd copying code is repeated twice (sample code, unit test) and I assume will be showing up in library code, might be nice to have only one copy eventually, if there's not already a ticket for that.

comment:20 follow-up: Changed 2 years ago by itamar

Oh, and: maybe we should release GIL around the underlying calls? That is much less useful when dealing with non-blocking sockets (and quite possibly will slow things down) so maybe not worth doing.

comment:21 in reply to: ↑ 20 Changed 2 years ago by glyph

Replying to itamar:

Oh, and: maybe we should release GIL around the underlying calls? That is much less useful when dealing with non-blocking sockets (and quite possibly will slow things down) so maybe not worth doing.

Separate ticket, please. The current implementation should be sufficient for our purposes.

comment:22 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to itamar

Docstring for sendmsg_recvmsg should note the default sizes that will be used if none are provided.

Done in r34187

The way you calculate length of ancillary data in sendmsg_recvmsg (control_message->cmsg_len - sizeof(struct cmsghdr)) bothers me: UNP suggests there might be padding in between the struct and the data bytes, and maybe sizeof takes that into account but I'm wary. It seems more correct to do control_message->cmsg_len - (CMSG_DATA(control_message) - control_message), I think? Since you know pointer address of data you can directly calculate how many bytes are between it and start of struct.

Done in r34188

The howto talks about scatter/gather and datagrams even though they're not supported. Instead might want to mention those as things that are *not* supported. (I guess technically conncected datagrams are supported, but without msg_name there's no unconnected support.)

These missing features are unfortunate shortcomings. :/ I still want to rename these functions, to leave the good names available for the fully featured versions. I clarified the documentation in r34192 and renamed the functions to reflect their shortcomings in r34193 and r34194.

You import AF_INET6 unconditionally; elsewhere I've seen the IPv6 be done conditionally, IIRC.

The imports all look unconditional to me, so I left it alone.

StartStopProtocol doesn't have two line breaks between methods.
No tests that flags are passed to underlying sendmsg()/recvmsg() calls.

Fixed in r34189

The fact BadList can be iterated a first time but not the second time is not made explicit in the docstring.

Fixed in r34190

Might be better to manipulate PYTHONPATH in spawn(), rather than manipulating sys.path inside pullpipe.py? OTOH this is probably more succinct.

All solutions seem terrible. I certainly don't like what's there now. Succinct seems like the only differentiating feature, so I'll stick with that.

You don't test return results of sendmsg_sendmsg(), or sufficiently test return results of sendmsg_recvmsg().

Fixed this for sendmsg_sendmsg in r34191. I'm not sure which cases are missing for sendmsg_recvmsg(). Oh, maybe an assertion about the flags? I don't know if it's possible to get that to be non-zero.

There's no test for sending so much data the total length wrapped around, but that's probably OK :)

Yea, I thought about that for a minute. I think we need to skip it.

Given the fd copying code is repeated twice (sample code, unit test) and I assume will be showing up in library code, might be nice to have only one copy eventually, if there's not already a ticket for that.

There's #5615, which I plan to work on next, and ask you to review. :)

Forced a new build with all of the changes, in progress.

comment:23 Changed 2 years ago by exarkun

(In [34195]) Fix a pyflakes warning

refs #4388

comment:24 Changed 2 years ago by exarkun

Separate ticket, please. The current implementation should be sufficient for our purposes.

Filed #5643 for releasing the GIL.

comment:25 Changed 2 years ago by exarkun

(In [34203]) Only run the MSG_DONTWAIT flags tests on Linux, since MSG_DONTWAIT appears to not operate properly on OS X or FreeBSD, despite those platforms defining the flag.

refs #4388

comment:26 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to exarkun

Looks good, please merge.

comment:27 Changed 2 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [34215]) Merge sendmsg-4388-2

Author: glyph, exarkun
Reviewer: exarkun
Fixes: #4388

Add low-level wrappers for sendmsg(2) and recvmsg(2), originally developed
by Apple, to support later high-level functionality that requires them (such
as passing file descriptors between processes).

comment:28 Changed 2 years ago by exarkun

I messed up the commit message. Clearly itamar was the indefatigable reviewer.

comment:29 Changed 2 years ago by benjamin

I see you haven't check PyIter_Next for error.

comment:30 Changed 2 years ago by exarkun

Argh. Actually, the code does check for PyIter_Next errors - but it only does it for the second PyIter_Next call, not the first.

The whole thing with double iteration bothers me a bit anyway. I think it can probably be changed to single iteration (of course that just shifts complexity over to the error-case memory cleanup code - but maybe that's still better).

Thanks for pointing out this oversight. I filed #5646 to address it.

Note: See TracTickets for help on using tickets.