Opened 3 years ago

Closed 2 years ago

#5615 enhancement closed fixed (fixed)

Allow UNIX transports to send and receive file descriptors

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/sendfiledescriptor-5615-3
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Using sendmsg and recvmsg (#4388), it is possible to pass a file descriptor to another process. Our UNIX transports should support this.

Change History (18)

comment:1 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/sendfiledescriptor-5615

(In [34096]) Branching to 'sendfiledescriptor-5615'

comment:2 Changed 3 years ago by exarkun

  • Branch changed from branches/sendfiledescriptor-5615 to branches/sendfiledescriptor-5615-2

(In [34216]) Branching to 'sendfiledescriptor-5615-2'

comment:3 Changed 3 years ago by exarkun

(In [34223]) Add some interfaces related to sending file descriptors; make endpoints support the new optional protocol interface.

refs #5615

comment:4 Changed 3 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

I think this is in fairly reasonable shape at this point.

Build results

comment:5 Changed 2 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • In test_unix, it may make sense to define SendFileDescriptor and ReceiveFileDescriptor before the tests using them.
  • Talking about those classes, one uses "reason" and the other users "_reason", it'd be good to unify it.
  • Severals tests are failing on my linux box if the sendmsg extension is not present. Those should probably be skipped instead.
  • _SendmsgMixin is missing documentation of _writeSomeDataBase.
  • It seems that the calls to recv1msg and send1msg should pass MSG_DONTWAIT. If not I'd be interested to know why.
  • The file descriptor is dropped on the floor if IFileDescriptorReceiver is not provided on the receiving side. A log message in the case would be nice.
  • Please update doc/core/examples/index.xhtml with a section about the 2 new example files.
  • The branch is missing a NEWS fragment.
  • +        # If there are file descriptors to send, try sending them first, using a
    +        # little bit of data from the stream-oriented write buffer too.  It is
    +        # not possible to send a file descriptor without sending some regular
    +        # data.
    
    I don't understand this comment, as we don't touch the data underneath.

The branch looks nice, good documentation and API. Please merge when the trivial points are fixed.

comment:6 Changed 2 years ago by exarkun

(In [34261]) Address review feedback regarding the unit tests

  • Skip the tests if twisted.python.sendmsg is not available. Also skip providing the functionality in that case, to make the rest of twisted.internet.unix keep working even without the extension module.
  • Re-order the classes in the test module so that helpers are defined before they are used.
  • Remove some unused, undocumented helpers in the tests. Document some other parts of them.

refs #5615

comment:7 Changed 2 years ago by exarkun

(In [34262]) Address some review feedback about _SendmsgMixin docs

refs #5615

comment:8 Changed 2 years ago by exarkun

(In [34263]) Address review feedback regarding logging

refs #5615

comment:9 Changed 2 years ago by exarkun

(In [34264]) Update comment to reflect the fact that data is no longer taken from the normal stream

refs #5615

comment:10 Changed 2 years ago by exarkun

(In [34265]) news fragment

refs #5615

comment:11 Changed 2 years ago by exarkun

(In [34266]) Add the new examples to the example index

Also clean up the whitespace

refs #5615

comment:12 Changed 2 years ago by exarkun

There's a problem on BSD:

[FAIL]
Traceback (most recent call last):
  File "/usr/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/internet/test/test_unix.py", line 300, in test_descriptorDeliveredBeforeBytes
    self.assertEqual([(int, server.message), normalBytes], client.events)
  File "/usr/home/buildbot/work/freebsd-8.2-i386/Twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = [(<type 'int'>, 'cheese'), 'junk']
b = [(<type 'int'>, 'cheesejunk')]


twisted.internet.test.test_unix.UNIXTestsBuilder_KQueueReactor.test_descriptorDeliveredBeforeBytes
twisted.internet.test.test_unix.UNIXTestsBuilder_PollReactor.test_descriptorDeliveredBeforeBytes
twisted.internet.test.test_unix.UNIXTestsBuilder_SelectReactor.test_descriptorDeliveredBeforeBytes

comment:13 Changed 2 years ago by exarkun

(In [34267]) Change sendFileDescriptor back to only accepting a single argument

refs #5615

comment:14 Changed 2 years ago by exarkun

  • Branch changed from branches/sendfiledescriptor-5615-2 to branches/sendfiledescriptor-5615-3

(In [34278]) Branching to 'sendfiledescriptor-5615-3'

comment:15 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Changed back to the streamier version of the interface.

Build results

comment:16 Changed 2 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • If I understand correctly, sendFileDescriptor needs to be accompanied by a write call to work properly. I don't think this is made very clear in the docstring of IUNIXTransport. That said, what would you think of adding the data parameter explicitly to sendFileDescriptor method (like the previous API)? We could then make the write call for the user, as it needs to happen anyway.
  • Some pyflakes:
    twisted/internet/test/test_unix.py:32: 'Factory' imported but unused
    twisted/internet/test/test_unix.py:32: 'Protocol' imported but unused
    twisted/internet/test/test_unix.py:42: 'sendmsg' imported but unused
    twisted/internet/test/test_unix.py:188: undefined name 'Failure'
    doc/core/examples/sendfd.py:82: local variable 'port' is assigned to but never used
    

Please merge, fixing the first point with either documentation or the API change.

comment:17 Changed 2 years ago by exarkun

Fixed the pyflakes issues in r34287 and added more sendFileDescription docs in r34286. Opted for the doc fix because of ordering issues with sendFileDescriptor, and the fact that the data you write is not really logically associated with the file descriptor (at least, there is no guarantee of that). iow, the bytes you send might arrive a long time after the file descriptor.

comment:18 Changed 2 years ago by exarkun

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

(In [34288]) Merge sendfiledescriptor-5615-3

Author: exarkun
Reviewer: therve
Fixes: #5615

Add an API for sending file descriptors over UNIX connections.

Note: See TracTickets for help on using tickets.