search

hypfvieh / dbus-java

Improved version of java DBus library provided by freedesktop.org (https://dbus.freedesktop.org/doc/dbus-java/)
https://hypfvieh.github.io/dbus-java/
MIT License
180 stars 72 forks source link

FileDescriptor support improvements #237

Closed Prototik closed 11 months ago

Prototik commented 11 months ago

  1. Fixed writing of file descriptors (it was broken by 5e036a9ed18a796e46ba1ca246713369c2ac8c55) 1.1. Added a test case for it

  2. Introducing new SPI called IFileDescriptorHelper to help convert java.io.FileDescriptor and raw int values. 2.1. Adapt org.freedesktop.dbus.FileDescriptor to use that helper. 2.2. Adapt junixsocket transport to provide a IFileDescriptorHelper with using operations from junixsocket itself (it has native impl for this) 2.3. Fallback to the old one, reflective based mode (not sure if anyone uses it, as it doesn't work on java 9+ out of box without --add-opens). 2.4. Implement hashCode/equals/toString for org.freedesktop.dbus.FileDescriptor

  3. Added AbstractTransport.isFileDescriptorSupported() and AbstractConnection.isFileDescriptorSupported() to check if current transport/connection supports fd passing.

hypfvieh commented 11 months ago

Thanks for the PR. I looked at it and I would change some parts.

Another ServiceLoader is not necessary in my opinion. Creating FileDescriptor is something the SocketProvider (ISocketProvider) should do, and not something completely different. There are some methods for FileDescriptor handling in the interface, so adding more is just fine. Also adding a IFileDescriptorHelper to dbus-java-core can introduce problems, because your code takes the first IFileDescriptorHelper it finds, and there is no guarantee that the first find in class path is the one you are looking for and not the default.

I therefore would prefer to add the two methods of your new ServiceLoader to ISocketProvider and implement the default behavior in that interface. That would allow the new code to use proper FileDescriptor creation code without additional ServiceLoader (and the possible problems explained before) and also be compatible to previous code because the of the default implementation.

It would also be nice if you could add javadoc to your class files including date, version and author.

Prototik commented 11 months ago

Ok, I'll remove separate SPI interface.

What version should I mark new classes? I suppose 4.3.2?

hypfvieh commented 11 months ago

Please set it to 5.0.0 (master branch is already updated to that version). I may backport the changes later, if so I'll update the version information.

Prototik commented 11 months ago

Ok, done, please review again