bindings for sendmsg and recvmsg

[glyph]

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

•  recvmsg
•  sendmsg

[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.

[exarkun]

•  http://pypi.python.org/pypi/PyXAPI/0.1
•  http://pypi.python.org/pypi/python-passfd/0.1

[glyph]

Replying to exarkun:

*  http://pypi.python.org/pypi/PyXAPI/0.1 *  http://pypi.python.org/pypi/python-passfd/0.1

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.)

[exarkun]

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

[exarkun]

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

[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

[exarkun]

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

[thijs]

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

[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

[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

[exarkun]

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

refs #4388

[exarkun]

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

refs #4388

[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.

[exarkun]

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

refs #4388

[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

[itamar]

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.

[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.

[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.

[exarkun]

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.

[exarkun]

(In [34195]) Fix a pyflakes warning

refs #4388

[exarkun]

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

Filed #5643 for releasing the GIL.

[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

[itamar]

Looks good, please merge.

[exarkun]

(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).

[exarkun]

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

[benjamin]

I see you haven't check PyIter_Next for error.

[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.