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

FileDescriptor support improvements #237

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

Prototik
Copy link
Contributor

@Prototik Prototik commented Oct 4, 2023

  1. Fixed writing of file descriptors (it was broken by 5e036a9)
    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.

1. Fixed writing of file descriptors
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 it)
2.3. Fallback to the old one, reflective based mode.
2.4. Implement hashCode/equals/toString for org.freedesktop.dbus.FileDescriptor
@hypfvieh
Copy link
Owner

hypfvieh commented Oct 7, 2023

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
Copy link
Contributor Author

Prototik commented Oct 7, 2023

Ok, I'll remove separate SPI interface.

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

@hypfvieh
Copy link
Owner

hypfvieh commented Oct 7, 2023

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
Copy link
Contributor Author

Prototik commented Oct 7, 2023

Ok, done, please review again

@hypfvieh hypfvieh merged commit 1b248b0 into hypfvieh:master Oct 10, 2023
2 of 4 checks passed
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