From 88f68dd50eb1799758000c9435c3a4ad84227fe2 Mon Sep 17 00:00:00 2001 From: Sergey Shatunov Date: Wed, 4 Oct 2023 18:21:21 +0300 Subject: [PATCH 1/4] FileDescriptor support improvements 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 --- dbus-java-core/src/main/java/module-info.java | 2 + .../org/freedesktop/dbus/FileDescriptor.java | 66 +++++++++----- .../dbus/connections/AbstractConnection.java | 4 + .../transports/AbstractTransport.java | 4 + .../IFileDescriptorHelper.java | 13 +++ .../ReflectionFileDescriptorHelper.java | 63 ++++++++++++++ .../AbstractOutputStreamMessageWriter.java | 8 +- .../dbus/test/FileDescriptorsTest.java | 86 +++++++++++++++++++ .../src/main/java/module-info.java | 3 + .../JUnixSocketFileDescriptorHelper.java | 34 ++++++++ .../junixsocket/JUnixSocketMessageReader.java | 3 +- .../junixsocket/JUnixSocketMessageWriter.java | 14 +-- ....spi.filedescriptors.IFileDescriptorHelper | 1 + 13 files changed, 267 insertions(+), 34 deletions(-) create mode 100644 dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java create mode 100644 dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java create mode 100644 dbus-java-tests/src/test/java/org/freedesktop/dbus/test/FileDescriptorsTest.java create mode 100644 dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java create mode 100644 dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper diff --git a/dbus-java-core/src/main/java/module-info.java b/dbus-java-core/src/main/java/module-info.java index 9145774f..26709a4c 100644 --- a/dbus-java-core/src/main/java/module-info.java +++ b/dbus-java-core/src/main/java/module-info.java @@ -14,6 +14,7 @@ exports org.freedesktop.dbus.messages; exports org.freedesktop.dbus.spi.transport; exports org.freedesktop.dbus.spi.message; + exports org.freedesktop.dbus.spi.filedescriptors; exports org.freedesktop.dbus.types; exports org.freedesktop.dbus.utils; @@ -25,4 +26,5 @@ uses org.freedesktop.dbus.spi.message.ISocketProvider; uses org.freedesktop.dbus.spi.transport.ITransportProvider; + uses org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; } \ No newline at end of file diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java index 7f329d09..ba15a443 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java @@ -1,10 +1,11 @@ package org.freedesktop.dbus; import org.freedesktop.dbus.exceptions.MarshallingException; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; +import org.freedesktop.dbus.spi.filedescriptors.ReflectionFileDescriptorHelper; -import java.lang.reflect.*; +import java.util.Optional; +import java.util.ServiceLoader; /** * Represents a FileDescriptor to be passed over the bus. Can be created from @@ -13,8 +14,7 @@ * */ public class FileDescriptor { - - private final Logger logger = LoggerFactory.getLogger(getClass()); + private static final ServiceLoader HELPERS = ServiceLoader.load(IFileDescriptorHelper.class, FileDescriptor.class.getClassLoader()); private final int fd; @@ -27,7 +27,16 @@ public FileDescriptor(java.io.FileDescriptor _data) throws MarshallingException } public java.io.FileDescriptor toJavaFileDescriptor() throws MarshallingException { - return createFileDescriptorByReflection(fd); + for (IFileDescriptorHelper helper : HELPERS) { + Optional result = helper.createFileDescriptor(fd); + if (result.isPresent()) { + return result.get(); + } + } + + return ReflectionFileDescriptorHelper.getInstance() + .flatMap(helper -> helper.createFileDescriptor(fd)) + .orElseThrow(() -> new MarshallingException("Could not create new FileDescriptor instance")); } public int getIntFileDescriptor() { @@ -35,26 +44,37 @@ public int getIntFileDescriptor() { } private int getFileDescriptor(java.io.FileDescriptor _data) throws MarshallingException { - Field declaredField; - try { - declaredField = _data.getClass().getDeclaredField("fd"); - declaredField.setAccessible(true); - return declaredField.getInt(_data); - } catch (NoSuchFieldException | SecurityException | IllegalArgumentException | IllegalAccessException _ex) { - logger.error("Could not get filedescriptor by reflection.", _ex); - throw new MarshallingException("Could not get member 'fd' of FileDescriptor by reflection!", _ex); + for (IFileDescriptorHelper helper : HELPERS) { + Optional result = helper.getFileDescriptorValue(_data); + if (result.isPresent()) { + return result.get(); + } } + + return ReflectionFileDescriptorHelper.getInstance() + .flatMap(helper -> helper.getFileDescriptorValue(_data)) + .orElseThrow(() -> new MarshallingException("Could not get FileDescriptor value")); } - private java.io.FileDescriptor createFileDescriptorByReflection(long _demarshallint) throws MarshallingException { - try { - Constructor constructor = java.io.FileDescriptor.class.getDeclaredConstructor(int.class); - constructor.setAccessible(true); - return constructor.newInstance((int) _demarshallint); - } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException - | IllegalArgumentException | InvocationTargetException _ex) { - logger.error("Could not create new FileDescriptor instance by reflection.", _ex); - throw new MarshallingException("Could not create new FileDescriptor instance by reflection", _ex); + @Override + public boolean equals(Object _o) { + if (this == _o) { + return true; + } + if (_o == null || getClass() != _o.getClass()) { + return false; } + FileDescriptor that = (FileDescriptor) _o; + return fd == that.fd; + } + + @Override + public int hashCode() { + return fd; + } + + @Override + public String toString() { + return FileDescriptor.class.getSimpleName() + "[fd=" + fd + "]"; } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/AbstractConnection.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/AbstractConnection.java index 847c57c8..6e1d7a8c 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/AbstractConnection.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/AbstractConnection.java @@ -1275,6 +1275,10 @@ public TransportConfig getTransportConfig() { return transport.getTransportConfig(); } + public boolean isFileDescriptorSupported() { + return transport.isFileDescriptorSupported(); + } + @Override public String toString() { return getClass().getSimpleName() + "[address=" + busAddress + "]"; diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java index 7afaec69..ad52cbda 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java @@ -313,6 +313,10 @@ public TransportConfig getTransportConfig() { return config; } + public boolean isFileDescriptorSupported() { + return fileDescriptorSupported; + } + @Override public String toString() { StringBuilder sb = new StringBuilder(getClass().getSimpleName()); diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java new file mode 100644 index 00000000..81bc9c84 --- /dev/null +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java @@ -0,0 +1,13 @@ +package org.freedesktop.dbus.spi.filedescriptors; + +import java.io.FileDescriptor; +import java.util.Optional; + +/** + * Interface used by {@link java.util.ServiceLoader ServiceLoader} to provide a helper to work with file descriptors. + */ +public interface IFileDescriptorHelper { + Optional getFileDescriptorValue(FileDescriptor _fd); + + Optional createFileDescriptor(int _fd); +} diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java new file mode 100644 index 00000000..e097109b --- /dev/null +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java @@ -0,0 +1,63 @@ +package org.freedesktop.dbus.spi.filedescriptors; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.FileDescriptor; +import java.lang.reflect.Constructor; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.util.Optional; + +public class ReflectionFileDescriptorHelper implements IFileDescriptorHelper { + private static final Logger LOGGER = LoggerFactory.getLogger(ReflectionFileDescriptorHelper.class); + private static volatile IFileDescriptorHelper instance; + + private final Field fdField; + private final Constructor constructor; + + public ReflectionFileDescriptorHelper() throws ReflectiveOperationException { + fdField = FileDescriptor.class.getDeclaredField("fd"); + fdField.setAccessible(true); + constructor = FileDescriptor.class.getDeclaredConstructor(int.class); + constructor.setAccessible(true); + } + + public static Optional getInstance() { + if (instance == null) { + synchronized (ReflectionFileDescriptorHelper.class) { + if (instance == null) { + try { + instance = new ReflectionFileDescriptorHelper(); + } catch (ReflectiveOperationException _ex) { + LOGGER.error("Unable to hook up java.io.FileDescriptor by using reflection."); + return Optional.empty(); + } + } + } + } + + return Optional.ofNullable(instance); + } + + @Override + public Optional getFileDescriptorValue(FileDescriptor _fd) { + try { + return Optional.of(fdField.getInt(_fd)); + } catch (SecurityException | IllegalArgumentException | IllegalAccessException _ex) { + LOGGER.error("Could not get file descriptor by reflection.", _ex); + return Optional.empty(); + } + } + + @Override + public Optional createFileDescriptor(int _fd) { + try { + return Optional.of(constructor.newInstance(_fd)); + } catch (SecurityException | InstantiationException | IllegalAccessException + | IllegalArgumentException | InvocationTargetException _ex) { + LOGGER.error("Could not create new FileDescriptor instance by reflection.", _ex); + return Optional.empty(); + } + } +} diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java index 8e6424a3..c83162b1 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java @@ -40,6 +40,10 @@ public final void writeMessage(Message _msg) throws IOException { return; } + if (hasFileDescriptorSupport) { + writeFileDescriptors(outputChannel, _msg.getFiledescriptors()); + } + for (byte[] buf : _msg.getWireData()) { if (logger.isTraceEnabled()) { logger.trace("{}", null == buf ? "(buffer was null)" : Hexdump.format(buf)); @@ -51,10 +55,6 @@ public final void writeMessage(Message _msg) throws IOException { outputChannel.write(ByteBuffer.wrap(buf)); } - if (hasFileDescriptorSupport) { - writeFileDescriptors(outputChannel, _msg.getFiledescriptors()); - } - logger.trace("Message sent: {}", _msg); } diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/FileDescriptorsTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/FileDescriptorsTest.java new file mode 100644 index 00000000..3389c1c8 --- /dev/null +++ b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/FileDescriptorsTest.java @@ -0,0 +1,86 @@ +package org.freedesktop.dbus.test; + +import org.freedesktop.dbus.FileDescriptor; +import org.freedesktop.dbus.connections.impl.DBusConnection; +import org.freedesktop.dbus.connections.impl.DBusConnectionBuilder; +import org.freedesktop.dbus.connections.transports.TransportBuilder; +import org.freedesktop.dbus.exceptions.DBusException; +import org.freedesktop.dbus.exceptions.DBusExecutionException; +import org.freedesktop.dbus.interfaces.DBusInterface; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIf; + +import java.io.IOException; +import java.util.stream.Stream; + +@EnabledIf(value = "isFileDescriptorSupported", disabledReason = "file descriptors not supported with the current transport") +public class FileDescriptorsTest extends AbstractDBusBaseTest { + public static final String TEST_OBJECT_PATH = "/FileDescriptorsTest"; + + private DBusConnection serverconn = null, clientconn = null; + + @BeforeEach + public void setUp() throws DBusException { + serverconn = DBusConnectionBuilder.forSessionBus().withShared(false).build(); + clientconn = DBusConnectionBuilder.forSessionBus().withShared(false).build(); + serverconn.requestBusName("foo.bar.Test"); + serverconn.exportObject(TEST_OBJECT_PATH, new FDPassingImpl()); + } + + @AfterEach + public void tearDown() throws Exception { + logger.debug("Checking for outstanding errors"); + DBusExecutionException dbee = serverconn.getError(); + if (null != dbee) { + throw dbee; + } + dbee = clientconn.getError(); + if (null != dbee) { + throw dbee; + } + + logger.debug("Disconnecting"); + /* Disconnect from the bus. */ + clientconn.disconnect(); + serverconn.releaseBusName("foo.bar.Test"); + serverconn.disconnect(); + } + + public static boolean isFileDescriptorSupported() throws DBusException, IOException { + if (!TransportBuilder.getRegisteredBusTypes().contains("UNIX")) { + return false; + } + try (DBusConnection conn = DBusConnectionBuilder.forSessionBus().build()) { + return conn.isFileDescriptorSupported(); + } + } + + @Test + public void fileDescriptorPassing() throws DBusException { + FDPassing remoteObject = clientconn.getRemoteObject("foo.bar.Test", TEST_OBJECT_PATH, FDPassing.class); + Stream.of(0, 1, 2).map(FileDescriptor::new).forEach(fd -> { + // that's not a mistake of using NotEquals here, as fd passing make a new copy with a new value + Assertions.assertNotEquals(fd.getIntFileDescriptor(), remoteObject.doNothing(fd).getIntFileDescriptor()); + }); + } + + public interface FDPassing extends DBusInterface { + FileDescriptor doNothing(FileDescriptor _fd); + } + + private static final class FDPassingImpl implements FDPassing { + + @Override + public String getObjectPath() { + return TEST_OBJECT_PATH; + } + + @Override + public FileDescriptor doNothing(FileDescriptor _fd) { + return _fd; + } + } +} diff --git a/dbus-java-transport-junixsocket/src/main/java/module-info.java b/dbus-java-transport-junixsocket/src/main/java/module-info.java index d9061708..8aa3d5c9 100644 --- a/dbus-java-transport-junixsocket/src/main/java/module-info.java +++ b/dbus-java-transport-junixsocket/src/main/java/module-info.java @@ -10,4 +10,7 @@ provides org.freedesktop.dbus.spi.message.ISocketProvider with org.freedesktop.dbus.transport.junixsocket.JUnixSocketSocketProvider; + + provides org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper + with org.freedesktop.dbus.transport.junixsocket.JUnixSocketFileDescriptorHelper; } diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java new file mode 100644 index 00000000..82c44e5b --- /dev/null +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java @@ -0,0 +1,34 @@ +package org.freedesktop.dbus.transport.junixsocket; + +import org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; +import org.newsclub.net.unix.FileDescriptorCast; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.FileDescriptor; +import java.io.IOException; +import java.util.Optional; + +public class JUnixSocketFileDescriptorHelper implements IFileDescriptorHelper { + private static final Logger LOGGER = LoggerFactory.getLogger(JUnixSocketFileDescriptorHelper.class); + + @Override + public Optional getFileDescriptorValue(FileDescriptor _fd) { + try { + return Optional.of(FileDescriptorCast.using(_fd).as(Integer.class)); + } catch (IOException _ex) { + LOGGER.error("Could not get file descriptor by using junixsocket library", _ex); + return Optional.empty(); + } + } + + @Override + public Optional createFileDescriptor(int _fd) { + try { + return Optional.of(FileDescriptorCast.unsafeUsing(_fd).getFileDescriptor()); + } catch (IOException _ex) { + LOGGER.error("Could not create new FileDescriptor instance by using junixsocket library.", _ex); + return Optional.empty(); + } + } +} diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java index ed992f41..b9f01b19 100644 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java @@ -3,7 +3,6 @@ import org.freedesktop.dbus.exceptions.DBusException; import org.freedesktop.dbus.spi.message.AbstractInputStreamMessageReader; import org.newsclub.net.unix.AFUNIXSocketChannel; -import org.newsclub.net.unix.FileDescriptorCast; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -30,7 +29,7 @@ protected List readFileDescriptors(SocketCh } else { List fds = new ArrayList<>(); for (FileDescriptor fd : receivedFileDescriptors) { - fds.add(new org.freedesktop.dbus.FileDescriptor(FileDescriptorCast.using(fd).as(Integer.class))); + fds.add(new org.freedesktop.dbus.FileDescriptor(fd)); } logger.debug("=> {}", fds); diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java index dd2d54f0..a9079086 100644 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java @@ -1,8 +1,8 @@ package org.freedesktop.dbus.transport.junixsocket; +import org.freedesktop.dbus.exceptions.MarshallingException; import org.freedesktop.dbus.spi.message.AbstractOutputStreamMessageWriter; import org.newsclub.net.unix.AFUNIXSocketChannel; -import org.newsclub.net.unix.FileDescriptorCast; import java.io.FileDescriptor; import java.io.IOException; @@ -19,11 +19,15 @@ public JUnixSocketMessageWriter(AFUNIXSocketChannel _socket, boolean _hasFileDes protected void writeFileDescriptors(SocketChannel _outputChannel, List _filedescriptors) throws IOException { if (_outputChannel instanceof AFUNIXSocketChannel) { if (_filedescriptors != null && !_filedescriptors.isEmpty()) { - FileDescriptor[] fds = new FileDescriptor[_filedescriptors.size()]; - for (int i = 0; i < _filedescriptors.size(); i++) { - fds[i] = FileDescriptorCast.unsafeUsing(_filedescriptors.get(i).getIntFileDescriptor()).getFileDescriptor(); + try { + FileDescriptor[] fds = new FileDescriptor[_filedescriptors.size()]; + for (int i = 0; i < _filedescriptors.size(); i++) { + fds[i] = _filedescriptors.get(i).toJavaFileDescriptor(); + } + ((AFUNIXSocketChannel) _outputChannel).setOutboundFileDescriptors(fds); + } catch (MarshallingException _ex) { + throw new IOException("unable to marshall file descriptors", _ex); } - ((AFUNIXSocketChannel) _outputChannel).setOutboundFileDescriptors(fds); } else { ((AFUNIXSocketChannel) _outputChannel).setOutboundFileDescriptors((FileDescriptor[]) null); } diff --git a/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper b/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper new file mode 100644 index 00000000..c538e5ed --- /dev/null +++ b/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper @@ -0,0 +1 @@ +org.freedesktop.dbus.transport.junixsocket.JUnixSocketFileDescriptorHelper \ No newline at end of file From 1b248b032e35e1aae81413ef7c9d41a972f6c1c8 Mon Sep 17 00:00:00 2001 From: Sergey Shatunov Date: Sat, 7 Oct 2023 18:10:11 +0300 Subject: [PATCH 2/4] Refactor file descriptors as stated in the code review --- dbus-java-core/src/main/java/module-info.java | 2 -- .../org/freedesktop/dbus/FileDescriptor.java | 17 +++++----- .../IFileDescriptorHelper.java | 13 ------- .../dbus/spi/message/ISocketProvider.java | 28 +++++++++++++++ .../ReflectionFileDescriptorHelper.java | 31 ++++++++++++----- .../src/main/java/module-info.java | 3 -- .../JUnixSocketFileDescriptorHelper.java | 34 ------------------- .../JUnixSocketSocketProvider.java | 32 +++++++++++++++++ ....spi.filedescriptors.IFileDescriptorHelper | 1 - 9 files changed, 91 insertions(+), 70 deletions(-) delete mode 100644 dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java rename dbus-java-core/src/main/java/org/freedesktop/dbus/{spi/filedescriptors => utils}/ReflectionFileDescriptorHelper.java (69%) delete mode 100644 dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java delete mode 100644 dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper diff --git a/dbus-java-core/src/main/java/module-info.java b/dbus-java-core/src/main/java/module-info.java index 26709a4c..9145774f 100644 --- a/dbus-java-core/src/main/java/module-info.java +++ b/dbus-java-core/src/main/java/module-info.java @@ -14,7 +14,6 @@ exports org.freedesktop.dbus.messages; exports org.freedesktop.dbus.spi.transport; exports org.freedesktop.dbus.spi.message; - exports org.freedesktop.dbus.spi.filedescriptors; exports org.freedesktop.dbus.types; exports org.freedesktop.dbus.utils; @@ -26,5 +25,4 @@ uses org.freedesktop.dbus.spi.message.ISocketProvider; uses org.freedesktop.dbus.spi.transport.ITransportProvider; - uses org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; } \ No newline at end of file diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java index ba15a443..bdf867b5 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java @@ -1,8 +1,8 @@ package org.freedesktop.dbus; import org.freedesktop.dbus.exceptions.MarshallingException; -import org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; -import org.freedesktop.dbus.spi.filedescriptors.ReflectionFileDescriptorHelper; +import org.freedesktop.dbus.spi.message.ISocketProvider; +import org.freedesktop.dbus.utils.ReflectionFileDescriptorHelper; import java.util.Optional; import java.util.ServiceLoader; @@ -11,10 +11,9 @@ * Represents a FileDescriptor to be passed over the bus. Can be created from * either an integer(gotten through some JNI/JNA/JNR call) or from a * java.io.FileDescriptor. - * */ -public class FileDescriptor { - private static final ServiceLoader HELPERS = ServiceLoader.load(IFileDescriptorHelper.class, FileDescriptor.class.getClassLoader()); +public final class FileDescriptor { + private static final ServiceLoader SPI_LOADER = ServiceLoader.load(ISocketProvider.class, FileDescriptor.class.getClassLoader()); private final int fd; @@ -27,8 +26,8 @@ public FileDescriptor(java.io.FileDescriptor _data) throws MarshallingException } public java.io.FileDescriptor toJavaFileDescriptor() throws MarshallingException { - for (IFileDescriptorHelper helper : HELPERS) { - Optional result = helper.createFileDescriptor(fd); + for (ISocketProvider provider : SPI_LOADER) { + Optional result = provider.createFileDescriptor(fd); if (result.isPresent()) { return result.get(); } @@ -44,8 +43,8 @@ public int getIntFileDescriptor() { } private int getFileDescriptor(java.io.FileDescriptor _data) throws MarshallingException { - for (IFileDescriptorHelper helper : HELPERS) { - Optional result = helper.getFileDescriptorValue(_data); + for (ISocketProvider provider : SPI_LOADER) { + Optional result = provider.getFileDescriptorValue(_data); if (result.isPresent()) { return result.get(); } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java deleted file mode 100644 index 81bc9c84..00000000 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/IFileDescriptorHelper.java +++ /dev/null @@ -1,13 +0,0 @@ -package org.freedesktop.dbus.spi.filedescriptors; - -import java.io.FileDescriptor; -import java.util.Optional; - -/** - * Interface used by {@link java.util.ServiceLoader ServiceLoader} to provide a helper to work with file descriptors. - */ -public interface IFileDescriptorHelper { - Optional getFileDescriptorValue(FileDescriptor _fd); - - Optional createFileDescriptor(int _fd); -} diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/ISocketProvider.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/ISocketProvider.java index 0215bf21..5359a011 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/ISocketProvider.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/ISocketProvider.java @@ -2,8 +2,10 @@ import org.freedesktop.dbus.connections.transports.AbstractTransport; +import java.io.FileDescriptor; import java.io.IOException; import java.nio.channels.SocketChannel; +import java.util.Optional; public interface ISocketProvider { /** @@ -39,4 +41,30 @@ public interface ISocketProvider { * @return true if file descriptors are supported by this provider, false otherwise */ boolean isFileDescriptorPassingSupported(); + + /** + * Attempts to extract raw FileDescriptor value from {@link FileDescriptor} instance. + * Note that not any {@link FileDescriptor} can be represented as int, for example Windows uses HANDLE as descriptor, + * which excess range of int values, thus cannot be safely cast to int. + * + * @param _fd FileDescriptor to extract value from + * @return int representation, packed to {@link Optional} if operation succeeded, or {@link Optional#empty()} otherwise + * @see #createFileDescriptor(int) + * @since 5.0.0 - 2023-10-07 + */ + default Optional getFileDescriptorValue(FileDescriptor _fd) { + return Optional.empty(); + } + + /** + * Attempts to create native {@link FileDescriptor} from raw int value. + * + * @param _fd FileDescriptor to extract value from + * @return {@link FileDescriptor}, instantiated with provided value, packed to {@link Optional} if operation succeeded, or {@link Optional#empty()} otherwise + * @see #getFileDescriptorValue(FileDescriptor) + * @since 5.0.0 - 2023-10-07 + */ + default Optional createFileDescriptor(int _fd) { + return Optional.empty(); + } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/utils/ReflectionFileDescriptorHelper.java similarity index 69% rename from dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java rename to dbus-java-core/src/main/java/org/freedesktop/dbus/utils/ReflectionFileDescriptorHelper.java index e097109b..3f90cb32 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/filedescriptors/ReflectionFileDescriptorHelper.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/utils/ReflectionFileDescriptorHelper.java @@ -1,5 +1,6 @@ -package org.freedesktop.dbus.spi.filedescriptors; +package org.freedesktop.dbus.utils; +import org.freedesktop.dbus.spi.message.ISocketProvider; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -9,28 +10,38 @@ import java.lang.reflect.InvocationTargetException; import java.util.Optional; -public class ReflectionFileDescriptorHelper implements IFileDescriptorHelper { +/** + * Helper to work with {@link FileDescriptor} instances by using reflection + * + * @author Sergey Shatunov + * @since 5.0.0 - 2023-10-07 + */ +public final class ReflectionFileDescriptorHelper { private static final Logger LOGGER = LoggerFactory.getLogger(ReflectionFileDescriptorHelper.class); - private static volatile IFileDescriptorHelper instance; + private static volatile ReflectionFileDescriptorHelper instance; private final Field fdField; private final Constructor constructor; - public ReflectionFileDescriptorHelper() throws ReflectiveOperationException { + private ReflectionFileDescriptorHelper() throws ReflectiveOperationException { fdField = FileDescriptor.class.getDeclaredField("fd"); fdField.setAccessible(true); constructor = FileDescriptor.class.getDeclaredConstructor(int.class); constructor.setAccessible(true); } - public static Optional getInstance() { + /** + * @return {@link ReflectionFileDescriptorHelper} instance, or {@link Optional#empty()} if it cannot be initialized + * (mainly due to missing reflection access) + */ + public static Optional getInstance() { if (instance == null) { synchronized (ReflectionFileDescriptorHelper.class) { if (instance == null) { try { instance = new ReflectionFileDescriptorHelper(); } catch (ReflectiveOperationException _ex) { - LOGGER.error("Unable to hook up java.io.FileDescriptor by using reflection."); + LOGGER.error("Unable to hook up java.io.FileDescriptor by using reflection.", _ex); return Optional.empty(); } } @@ -40,7 +51,9 @@ public static Optional getInstance() { return Optional.ofNullable(instance); } - @Override + /** + * @see ISocketProvider#getFileDescriptorValue(FileDescriptor) + */ public Optional getFileDescriptorValue(FileDescriptor _fd) { try { return Optional.of(fdField.getInt(_fd)); @@ -50,7 +63,9 @@ public Optional getFileDescriptorValue(FileDescriptor _fd) { } } - @Override + /** + * @see ISocketProvider#createFileDescriptor(int) + */ public Optional createFileDescriptor(int _fd) { try { return Optional.of(constructor.newInstance(_fd)); diff --git a/dbus-java-transport-junixsocket/src/main/java/module-info.java b/dbus-java-transport-junixsocket/src/main/java/module-info.java index 8aa3d5c9..d9061708 100644 --- a/dbus-java-transport-junixsocket/src/main/java/module-info.java +++ b/dbus-java-transport-junixsocket/src/main/java/module-info.java @@ -10,7 +10,4 @@ provides org.freedesktop.dbus.spi.message.ISocketProvider with org.freedesktop.dbus.transport.junixsocket.JUnixSocketSocketProvider; - - provides org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper - with org.freedesktop.dbus.transport.junixsocket.JUnixSocketFileDescriptorHelper; } diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java deleted file mode 100644 index 82c44e5b..00000000 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketFileDescriptorHelper.java +++ /dev/null @@ -1,34 +0,0 @@ -package org.freedesktop.dbus.transport.junixsocket; - -import org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper; -import org.newsclub.net.unix.FileDescriptorCast; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import java.io.FileDescriptor; -import java.io.IOException; -import java.util.Optional; - -public class JUnixSocketFileDescriptorHelper implements IFileDescriptorHelper { - private static final Logger LOGGER = LoggerFactory.getLogger(JUnixSocketFileDescriptorHelper.class); - - @Override - public Optional getFileDescriptorValue(FileDescriptor _fd) { - try { - return Optional.of(FileDescriptorCast.using(_fd).as(Integer.class)); - } catch (IOException _ex) { - LOGGER.error("Could not get file descriptor by using junixsocket library", _ex); - return Optional.empty(); - } - } - - @Override - public Optional createFileDescriptor(int _fd) { - try { - return Optional.of(FileDescriptorCast.unsafeUsing(_fd).getFileDescriptor()); - } catch (IOException _ex) { - LOGGER.error("Could not create new FileDescriptor instance by using junixsocket library.", _ex); - return Optional.empty(); - } - } -} diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketSocketProvider.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketSocketProvider.java index 56230706..2b5b57ed 100644 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketSocketProvider.java +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketSocketProvider.java @@ -6,10 +6,18 @@ import org.newsclub.net.unix.AFSocket; import org.newsclub.net.unix.AFSocketCapability; import org.newsclub.net.unix.AFUNIXSocketChannel; +import org.newsclub.net.unix.FileDescriptorCast; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.FileDescriptor; +import java.io.IOException; import java.nio.channels.SocketChannel; +import java.util.Optional; public class JUnixSocketSocketProvider implements ISocketProvider { + private static final Logger LOGGER = LoggerFactory.getLogger(JUnixSocketSocketProvider.class); + private boolean hasFileDescriptorSupport = false; @Override @@ -43,4 +51,28 @@ public void setFileDescriptorSupport(boolean _support) { public boolean isFileDescriptorPassingSupported() { return AFSocket.supports(AFSocketCapability.CAPABILITY_FILE_DESCRIPTORS) && AFSocket.supports(AFSocketCapability.CAPABILITY_UNSAFE); } + + @Override + public Optional getFileDescriptorValue(FileDescriptor _fd) { + try { + return Optional.of(FileDescriptorCast.using(_fd).as(Integer.class)); + } catch (IOException | ClassCastException _ex) { + LOGGER.error("Could not get file descriptor by using junixsocket library", _ex); + return Optional.empty(); + } + } + + @Override + public Optional createFileDescriptor(int _fd) { + if (!AFSocket.supports(AFSocketCapability.CAPABILITY_UNSAFE)) { + LOGGER.debug("Could not create new FileDescriptor instance by using junixsocket library, as unsafe capabilities of that library is disabled."); + return Optional.empty(); + } + try { + return Optional.of(FileDescriptorCast.unsafeUsing(_fd).getFileDescriptor()); + } catch (IOException _ex) { + LOGGER.error("Could not create new FileDescriptor instance by using junixsocket library.", _ex); + return Optional.empty(); + } + } } diff --git a/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper b/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper deleted file mode 100644 index c538e5ed..00000000 --- a/dbus-java-transport-junixsocket/src/main/resources/META-INF/services/org.freedesktop.dbus.spi.filedescriptors.IFileDescriptorHelper +++ /dev/null @@ -1 +0,0 @@ -org.freedesktop.dbus.transport.junixsocket.JUnixSocketFileDescriptorHelper \ No newline at end of file From b9f09b42ec83cc5da5508bbac4dee86b70b34bd0 Mon Sep 17 00:00:00 2001 From: David M Date: Mon, 9 Oct 2023 17:01:21 +0200 Subject: [PATCH 3/4] Refactoring to avoid using second service loader for same SPI --- .../org/freedesktop/dbus/FileDescriptor.java | 65 ++++++++++++------- .../transports/AbstractTransport.java | 4 +- .../transports/TransportConnection.java | 11 +++- .../AbstractInputStreamMessageReader.java | 19 ++++-- .../AbstractOutputStreamMessageWriter.java | 19 ++++-- .../spi/message/DefaultSocketProvider.java | 40 ++++++++++++ .../spi/message/InputStreamMessageReader.java | 2 +- .../message/OutputStreamMessageWriter.java | 2 +- .../junixsocket/JUnixSocketMessageReader.java | 14 ++-- .../junixsocket/JUnixSocketMessageWriter.java | 7 +- .../JUnixSocketSocketProvider.java | 17 ++--- 11 files changed, 138 insertions(+), 62 deletions(-) create mode 100644 dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/DefaultSocketProvider.java diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java index bdf867b5..e8aef9d0 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/FileDescriptor.java @@ -5,15 +5,13 @@ import org.freedesktop.dbus.utils.ReflectionFileDescriptorHelper; import java.util.Optional; -import java.util.ServiceLoader; /** - * Represents a FileDescriptor to be passed over the bus. Can be created from - * either an integer(gotten through some JNI/JNA/JNR call) or from a - * java.io.FileDescriptor. + * Represents a FileDescriptor to be passed over the bus.
+ * Can be created from either an integer (gotten through some JNI/JNA/JNR call) or from a + * {@link java.io.FileDescriptor}. */ public final class FileDescriptor { - private static final ServiceLoader SPI_LOADER = ServiceLoader.load(ISocketProvider.class, FileDescriptor.class.getClassLoader()); private final int fd; @@ -21,13 +19,19 @@ public FileDescriptor(int _fd) { fd = _fd; } - public FileDescriptor(java.io.FileDescriptor _data) throws MarshallingException { - fd = getFileDescriptor(_data); - } - - public java.io.FileDescriptor toJavaFileDescriptor() throws MarshallingException { - for (ISocketProvider provider : SPI_LOADER) { - Optional result = provider.createFileDescriptor(fd); + /** + * Converts this DBus {@link FileDescriptor} to a {@link java.io.FileDescriptor}.
+ * Tries to use the provided ISocketProvider if present first.
+ * If not present or conversion failed, tries to convert using reflection. + * + * @param _provider provider or null + * + * @return java file descriptor + * @throws MarshallingException when converting fails + */ + public java.io.FileDescriptor toJavaFileDescriptor(ISocketProvider _provider) throws MarshallingException { + if (_provider != null) { + Optional result = _provider.createFileDescriptor(fd); if (result.isPresent()) { return result.get(); } @@ -42,19 +46,6 @@ public int getIntFileDescriptor() { return fd; } - private int getFileDescriptor(java.io.FileDescriptor _data) throws MarshallingException { - for (ISocketProvider provider : SPI_LOADER) { - Optional result = provider.getFileDescriptorValue(_data); - if (result.isPresent()) { - return result.get(); - } - } - - return ReflectionFileDescriptorHelper.getInstance() - .flatMap(helper -> helper.getFileDescriptorValue(_data)) - .orElseThrow(() -> new MarshallingException("Could not get FileDescriptor value")); - } - @Override public boolean equals(Object _o) { if (this == _o) { @@ -76,4 +67,28 @@ public int hashCode() { public String toString() { return FileDescriptor.class.getSimpleName() + "[fd=" + fd + "]"; } + + /** + * Utility method to create a DBus {@link FileDescriptor} from a {@link java.io.FileDescriptor}.
+ * Tries to use the provided ISocketProvider if present first.
+ * If not present or conversion failed, tries to convert using reflection. + * + * @param _data file descriptor + * @param _provider socket provider or null + * + * @return DBus FileDescriptor + * @throws MarshallingException when conversion fails + */ + public static FileDescriptor fromJavaFileDescriptor(java.io.FileDescriptor _data, ISocketProvider _provider) throws MarshallingException { + if (_provider != null) { + Optional result = _provider.getFileDescriptorValue(_data); + if (result.isPresent()) { + return new FileDescriptor(result.get()); + } + } + + return new FileDescriptor(ReflectionFileDescriptorHelper.getInstance() + .flatMap(helper -> helper.getFileDescriptorValue(_data)) + .orElseThrow(() -> new MarshallingException("Could not get FileDescriptor value"))); + } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java index ad52cbda..ca441e39 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/AbstractTransport.java @@ -235,6 +235,7 @@ private void authenticate(SocketChannel _sock) throws IOException { private TransportConnection createInputOutput(SocketChannel _socket) { IMessageReader reader = null; IMessageWriter writer = null; + ISocketProvider providerImpl = null; try { for (ISocketProvider provider : spiLoader) { logger.debug("Found ISocketProvider {}", provider); @@ -244,6 +245,7 @@ private TransportConnection createInputOutput(SocketChannel _socket) { writer = provider.createWriter(_socket); if (reader != null && writer != null) { logger.debug("Using ISocketProvider {}", provider); + providerImpl = provider; break; } } @@ -261,7 +263,7 @@ private TransportConnection createInputOutput(SocketChannel _socket) { // allows it } - return new TransportConnection(messageFactory, _socket, writer, reader); + return new TransportConnection(messageFactory, _socket, providerImpl, writer, reader); } /** diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/TransportConnection.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/TransportConnection.java index 6b0ed378..88945b6a 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/TransportConnection.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/transports/TransportConnection.java @@ -1,8 +1,7 @@ package org.freedesktop.dbus.connections.transports; import org.freedesktop.dbus.messages.MessageFactory; -import org.freedesktop.dbus.spi.message.IMessageReader; -import org.freedesktop.dbus.spi.message.IMessageWriter; +import org.freedesktop.dbus.spi.message.*; import java.io.Closeable; import java.io.IOException; @@ -27,12 +26,14 @@ public class TransportConnection implements Closeable { private final SocketChannel channel; private final IMessageWriter writer; private final IMessageReader reader; + private final ISocketProvider socketProviderImpl; private final MessageFactory messageFactory; - public TransportConnection(MessageFactory _factory, SocketChannel _channel, IMessageWriter _writer, IMessageReader _reader) { + public TransportConnection(MessageFactory _factory, SocketChannel _channel, ISocketProvider _socketProviderImpl, IMessageWriter _writer, IMessageReader _reader) { messageFactory = _factory; channel = _channel; + socketProviderImpl = _socketProviderImpl; writer = _writer; reader = _reader; } @@ -49,6 +50,10 @@ public IMessageReader getReader() { return reader; } + public ISocketProvider getSocketProviderImpl() { + return socketProviderImpl; + } + public long getId() { return id; } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractInputStreamMessageReader.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractInputStreamMessageReader.java index 9632ab68..91779c6c 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractInputStreamMessageReader.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractInputStreamMessageReader.java @@ -27,13 +27,14 @@ public abstract class AbstractInputStreamMessageReader implements IMessageReader private final byte[] buf; private final byte[] tbuf; private final SocketChannel inputChannel; - private final boolean hasFileDescriptorSupport; private byte[] header; private byte[] body; - public AbstractInputStreamMessageReader(final SocketChannel _in, boolean _hasFileDescriptorSupport) { - hasFileDescriptorSupport = _hasFileDescriptorSupport; + private final ISocketProvider socketProviderImpl; + + public AbstractInputStreamMessageReader(final SocketChannel _in, ISocketProvider _socketProviderImpl) { + socketProviderImpl = Objects.requireNonNull(_socketProviderImpl, "ISocketProvider implementation required"); inputChannel = Objects.requireNonNull(_in, "SocketChannel required"); len = new int[4]; tbuf = new byte[4]; @@ -168,7 +169,7 @@ public final Message readMessage() throws IOException, DBusException { try { List fds = null; - if (hasFileDescriptorSupport) { + if (socketProviderImpl.isFileDescriptorPassingSupported()) { fds = readFileDescriptors(inputChannel); } @@ -203,6 +204,14 @@ public final Message readMessage() throws IOException, DBusException { */ protected abstract List readFileDescriptors(SocketChannel _inputChannel) throws DBusException; + protected Logger getLogger() { + return logger; + } + + protected ISocketProvider getSocketProviderImpl() { + return socketProviderImpl; + } + @Override public void close() throws IOException { if (inputChannel.isOpen()) { @@ -218,7 +227,7 @@ public boolean isClosed() { @Override public String toString() { - return getClass().getSimpleName() + " [inputChannel=" + inputChannel + ", hasFileDescriptorSupport=" + hasFileDescriptorSupport + "]"; + return getClass().getSimpleName() + " [inputChannel=" + inputChannel + ", socketProviderImpl=" + socketProviderImpl + "]"; } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java index c83162b1..f8aae728 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/AbstractOutputStreamMessageWriter.java @@ -22,11 +22,12 @@ public abstract class AbstractOutputStreamMessageWriter implements IMessageWrite private final Logger logger = LoggerFactory.getLogger(getClass()); private final SocketChannel outputChannel; - private final boolean hasFileDescriptorSupport; - public AbstractOutputStreamMessageWriter(final SocketChannel _out, boolean _fileDescriptorSupport) { + private final ISocketProvider socketProviderImpl; + + public AbstractOutputStreamMessageWriter(final SocketChannel _out, ISocketProvider _socketProviderImpl) { outputChannel = Objects.requireNonNull(_out, "SocketChannel required"); - hasFileDescriptorSupport = _fileDescriptorSupport; + socketProviderImpl = Objects.requireNonNull(_socketProviderImpl, "ISocketProvider implementation required"); } @Override @@ -40,7 +41,7 @@ public final void writeMessage(Message _msg) throws IOException { return; } - if (hasFileDescriptorSupport) { + if (socketProviderImpl.isFileDescriptorPassingSupported()) { writeFileDescriptors(outputChannel, _msg.getFiledescriptors()); } @@ -69,6 +70,14 @@ public final void writeMessage(Message _msg) throws IOException { */ protected abstract void writeFileDescriptors(SocketChannel _outputChannel, List _filedescriptors) throws IOException; + protected Logger getLogger() { + return logger; + } + + protected ISocketProvider getSocketProviderImpl() { + return socketProviderImpl; + } + @Override public void close() throws IOException { logger.debug("Closing Message Writer"); @@ -85,7 +94,7 @@ public boolean isClosed() { @Override public String toString() { - return getClass().getSimpleName() + " [outputChannel=" + outputChannel + ", hasFileDescriptorSupport=" + hasFileDescriptorSupport + "]"; + return getClass().getSimpleName() + " [outputChannel=" + outputChannel + ", socketProviderImpl=" + socketProviderImpl + "]"; } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/DefaultSocketProvider.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/DefaultSocketProvider.java new file mode 100644 index 00000000..5072c1da --- /dev/null +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/DefaultSocketProvider.java @@ -0,0 +1,40 @@ +package org.freedesktop.dbus.spi.message; + +import java.io.IOException; +import java.nio.channels.SocketChannel; + +/** + * Default internally used socket provider implementation. + * + * @author hypfvieh + * @since 5.0.0 - 2023-10-09 + */ +final class DefaultSocketProvider implements ISocketProvider { + + static final ISocketProvider INSTANCE = new DefaultSocketProvider(); + + private DefaultSocketProvider() { + + } + + @Override + public IMessageReader createReader(SocketChannel _socket) throws IOException { + return new InputStreamMessageReader(_socket); + } + + @Override + public IMessageWriter createWriter(SocketChannel _socket) throws IOException { + return new OutputStreamMessageWriter(_socket); + } + + @Override + public void setFileDescriptorSupport(boolean _support) { + // not supported + } + + @Override + public boolean isFileDescriptorPassingSupported() { + return false; + } + +} diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/InputStreamMessageReader.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/InputStreamMessageReader.java index 9ab356df..3446c269 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/InputStreamMessageReader.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/InputStreamMessageReader.java @@ -8,7 +8,7 @@ public class InputStreamMessageReader extends AbstractInputStreamMessageReader { public InputStreamMessageReader(final SocketChannel _in) { - super(_in, false); + super(_in, DefaultSocketProvider.INSTANCE); } @Override diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/OutputStreamMessageWriter.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/OutputStreamMessageWriter.java index 9fa768b2..be0440a1 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/OutputStreamMessageWriter.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/spi/message/OutputStreamMessageWriter.java @@ -8,7 +8,7 @@ public class OutputStreamMessageWriter extends AbstractOutputStreamMessageWriter { public OutputStreamMessageWriter(SocketChannel _out) { - super(_out, false); + super(_out, DefaultSocketProvider.INSTANCE); } @Override diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java index b9f01b19..2b0326c2 100644 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageReader.java @@ -2,21 +2,20 @@ import org.freedesktop.dbus.exceptions.DBusException; import org.freedesktop.dbus.spi.message.AbstractInputStreamMessageReader; +import org.freedesktop.dbus.spi.message.ISocketProvider; import org.newsclub.net.unix.AFUNIXSocketChannel; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import java.io.FileDescriptor; import java.io.IOException; import java.nio.channels.SocketChannel; import java.util.ArrayList; import java.util.List; +import java.util.Optional; public class JUnixSocketMessageReader extends AbstractInputStreamMessageReader { - private final Logger logger = LoggerFactory.getLogger(getClass()); - public JUnixSocketMessageReader(AFUNIXSocketChannel _socket, boolean _hasFileDescriptorSupport) { - super(_socket, _hasFileDescriptorSupport); + public JUnixSocketMessageReader(AFUNIXSocketChannel _socket, ISocketProvider _socketProviderImpl) { + super(_socket, _socketProviderImpl); } @Override @@ -29,10 +28,11 @@ protected List readFileDescriptors(SocketCh } else { List fds = new ArrayList<>(); for (FileDescriptor fd : receivedFileDescriptors) { - fds.add(new org.freedesktop.dbus.FileDescriptor(fd)); + Optional fileDescriptorValue = getSocketProviderImpl().getFileDescriptorValue(fd); + fileDescriptorValue.ifPresent(f -> fds.add(new org.freedesktop.dbus.FileDescriptor(f))); } - logger.debug("=> {}", fds); + getLogger().debug("=> {}", fds); return fds; } } catch (IOException _ex) { diff --git a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java index a9079086..a5604f4c 100644 --- a/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java +++ b/dbus-java-transport-junixsocket/src/main/java/org/freedesktop/dbus/transport/junixsocket/JUnixSocketMessageWriter.java @@ -2,6 +2,7 @@ import org.freedesktop.dbus.exceptions.MarshallingException; import org.freedesktop.dbus.spi.message.AbstractOutputStreamMessageWriter; +import org.freedesktop.dbus.spi.message.ISocketProvider; import org.newsclub.net.unix.AFUNIXSocketChannel; import java.io.FileDescriptor; @@ -11,8 +12,8 @@ public class JUnixSocketMessageWriter extends AbstractOutputStreamMessageWriter { - public JUnixSocketMessageWriter(AFUNIXSocketChannel _socket, boolean _hasFileDescriptorSupport) { - super(_socket, _hasFileDescriptorSupport); + public JUnixSocketMessageWriter(AFUNIXSocketChannel _socket, ISocketProvider _socketProviderImpl) { + super(_socket, _socketProviderImpl); } @Override @@ -22,7 +23,7 @@ protected void writeFileDescriptors(SocketChannel _outputChannel, List Date: Wed, 11 Oct 2023 07:57:28 +0200 Subject: [PATCH 4/4] Issue #238: Fixed issues with autoConnect option; Added methed 'register' to DBusConnection to allow registering session manually when it was created with 'registerSelf' = 'false' --- README.md | 1 + .../connections/config/TransportConfig.java | 9 +++++ .../config/TransportConfigBuilder.java | 13 ++++++ .../dbus/connections/impl/DBusConnection.java | 40 ++++++++++++++----- .../impl/DBusConnectionBuilder.java | 13 +----- .../examples/pulseaudio/PulseAudioDbus.java | 4 +- 6 files changed, 57 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index f7b470cb..77cf53f2 100644 --- a/README.md +++ b/README.md @@ -99,6 +99,7 @@ The library will remain open source and MIT licensed and can still be used, fork - Updated minimum required Java version to 17 - Improved handling of listening connections to allow proper bootstrapping the connection before actually starting accepting new connections (thanks to [brett-smith](https://github.com/brett-smith) ([#213](https://github.com/hypfvieh/dbus-java/issues/213))) - Updated export-object documentation ([#236](https://github.com/hypfvieh/dbus-java/issues/236)) + - Fixed issues with autoConnect option, added method to register to bus by 'Hello' message manually, thanks to [brett-smith](https://github.com/brett-smith) ([#238](https://github.com/hypfvieh/dbus-java/issues/238)) ##### Changes in 4.3.1 (2023-10-03): - Provide classloader to ServiceLoader in TransportBuilder (for loading actual transports) and AbstractTransport (for loading IMessageReader/Writer implementations), thanks to [cthbleachbit](https://github.com/cthbleachbit) ([#210](https://github.com/hypfvieh/dbus-java/issues/210), [PR#211](https://github.com/hypfvieh/dbus-java/issues/211)) diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfig.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfig.java index 60762831..ed03cc80 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfig.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfig.java @@ -32,6 +32,7 @@ public final class TransportConfig { private String fileGroup; private byte endianess = BaseConnectionBuilder.getSystemEndianness(); + private boolean registerSelf = true; /** * Unix file permissions to set on socket file if this is a server transport (ignored on Windows, does nothing if @@ -151,6 +152,14 @@ public void setEndianess(byte _endianess) { endianess = _endianess; } + public boolean isRegisterSelf() { + return registerSelf; + } + + public void setRegisterSelf(boolean _registerSelf) { + registerSelf = _registerSelf; + } + /** * Toggles the busaddress to be a listening (server) or non listening (client) connection. * @param _listening true to be a server connection diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfigBuilder.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfigBuilder.java index c202867b..974215d5 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfigBuilder.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/config/TransportConfigBuilder.java @@ -102,6 +102,19 @@ public X withAutoConnect(boolean _connect) { return self(); } + /** + * Register the new connection on DBus using 'hello' message. Default is true. + * + * @param _register boolean + * @return this + * + * @since 5.0.0 - 2023-10-11 + */ + public X withRegisterSelf(boolean _register) { + config.setRegisterSelf(_register); + return self(); + } + /** * Switch to the {@link SaslConfigBuilder} to configure the SASL authentication mechanism.
* Use {@link SaslConfigBuilder#back()} to return to this builder when finished. diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnection.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnection.java index c39c8817..f297ec29 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnection.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnection.java @@ -47,6 +47,9 @@ public final class DBusConnection extends AbstractConnection { private final String machineId; private DBus dbus; + /** Whether the connection was registered using 'Hello' message. */ + private boolean registered; + /** Count how many 'connections' we manage internally. * This is required because a {@link DBusConnection} to the same address will always return the same object and * the 'real' disconnection should only occur when there is no second/third/whatever connection is left. */ @@ -73,10 +76,9 @@ private AtomicInteger getConcurrentConnections() { /** * Connect to bus and register if asked. Should only be called by Builder. * - * @param _registerSelf true to register * @throws DBusException if registering or connection fails */ - void connect(boolean _registerSelf) throws DBusException { + void connectImpl() throws DBusException { // start listening for calls try { listen(); @@ -89,14 +91,32 @@ void connect(boolean _registerSelf) throws DBusException { addSigHandlerWithoutMatch(DBus.NameAcquired.class, h); // register ourselves if not disabled - if (_registerSelf) { - dbus = getRemoteObject("org.freedesktop.DBus", "/org/freedesktop/DBus", DBus.class); - try { - busnames.add(dbus.Hello()); - } catch (DBusExecutionException _ex) { - logger.debug("Error while doing 'Hello' handshake", _ex); - throw new DBusException(_ex.getMessage()); - } + if (getTransportConfig().isRegisterSelf() && getTransport().isConnected()) { + register(); + } + } + + /** + * Register this connection on the bus using 'Hello' message.
+ * Will do nothing if session was already registered. + * + * @throws DBusException when sending message fails + * + * @since 5.0.0 - 2023-10-11 + */ + public void register() throws DBusException { + if (registered) { + return; + } + + dbus = getRemoteObject("org.freedesktop.DBus", "/org/freedesktop/DBus", DBus.class); + + try { + busnames.add(dbus.Hello()); + registered = true; + } catch (DBusExecutionException _ex) { + logger.debug("Error while doing 'Hello' handshake", _ex); + throw new DBusException(_ex.getMessage(), _ex); } } diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java index 72905e56..3d30e946 100644 --- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java +++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java @@ -20,7 +20,6 @@ public final class DBusConnectionBuilder extends BaseConnectionBuilder { private final String machineId; - private boolean registerSelf = true; private boolean shared = true; private DBusConnectionBuilder(BusAddress _address, String _machineId) { @@ -146,16 +145,6 @@ private static BusAddress validateTransportAddress(BusAddress _address) { return address; } - /** - * Register the new connection on DBus using 'hello' message. Default is true. - * - * @param _register boolean - * @return this - */ - public DBusConnectionBuilder withRegisterSelf(boolean _register) { - registerSelf = _register; - return this; - } /** * Use this connection as shared connection. Shared connection means that the same connection is used multiple times @@ -199,7 +188,7 @@ public DBusConnection build() throws DBusException { c.setDisconnectCallback(getDisconnectCallback()); c.setWeakReferences(isWeakReference()); - c.connect(registerSelf); + c.connectImpl(); return c; } diff --git a/dbus-java-examples/src/main/java/com/github/hypfvieh/dbus/examples/pulseaudio/PulseAudioDbus.java b/dbus-java-examples/src/main/java/com/github/hypfvieh/dbus/examples/pulseaudio/PulseAudioDbus.java index fb401a78..fbaadba3 100755 --- a/dbus-java-examples/src/main/java/com/github/hypfvieh/dbus/examples/pulseaudio/PulseAudioDbus.java +++ b/dbus-java-examples/src/main/java/com/github/hypfvieh/dbus/examples/pulseaudio/PulseAudioDbus.java @@ -35,8 +35,10 @@ public static void main(String[] _args) throws DBusException { } else { DBusConnection connection = DBusConnectionBuilder .forAddress(address) - .withRegisterSelf(false) .withShared(false) + .transportConfig() + .withRegisterSelf(false) + .back() .build(); Properties core1Props = connection.getRemoteObject("org.PulseAudio.Core1", "/org/pulseaudio/core1", Properties.class);