From 158d51abde9501f1ecccf7b0102427a5754e6664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tilmann=20Z=C3=A4schke?= Date: Mon, 10 Jun 2024 17:35:32 +0200 Subject: [PATCH] Internal cleanup --- CHANGELOG.md | 1 + .../scion/jpan/AbstractDatagramChannel.java | 35 +++++++++---------- src/main/java/org/scion/jpan/Path.java | 26 ++++++++------ .../java/org/scion/jpan/ResponsePath.java | 7 ++-- .../org/scion/jpan/ScionDatagramChannel.java | 9 ++--- .../org/scion/jpan/ScionDatagramSocket.java | 2 +- .../java/org/scion/jpan/ScionService.java | 2 +- src/main/java/org/scion/jpan/ScmpChannel.java | 6 ++-- .../api/DatagramChannelApiServerTest.java | 3 +- .../jpan/api/DatagramChannelApiTest.java | 2 +- .../jpan/demo/PingPongChannelClient.java | 7 +--- 11 files changed, 51 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5abe7e65..b40459fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - SCMP API changes. [#71](https://github.com/scionproto-contrib/jpan/pull/71) - **BREAKING CHANGE**: `DatagramChannel.receive()` returns a subclass of `InetSocketAddress` [#86](https://github.com/scionproto-contrib/jpan/pull/86) +- Internal cleanup. [#88](https://github.com/scionproto-contrib/jpan/pull/88) ### Fixed - Fixed locking and resizing of buffers. [#68](https://github.com/scionproto-contrib/jpan/pull/68) diff --git a/src/main/java/org/scion/jpan/AbstractDatagramChannel.java b/src/main/java/org/scion/jpan/AbstractDatagramChannel.java index 3c3147bc..f6d08957 100644 --- a/src/main/java/org/scion/jpan/AbstractDatagramChannel.java +++ b/src/main/java/org/scion/jpan/AbstractDatagramChannel.java @@ -179,8 +179,10 @@ public InetSocketAddress getLocalAddress() throws IOException { /** * Returns the remote address. * + * @return The remote address or 'null' if this channel is not connected. * @see DatagramChannel#getRemoteAddress() - * @return The remote address. + * @see #connect(SocketAddress) + * @see #connect(RequestPath) * @throws IOException If an I/O error occurs */ public InetSocketAddress getRemoteAddress() throws IOException { @@ -193,7 +195,6 @@ public InetSocketAddress getRemoteAddress() throws IOException { public void disconnect() throws IOException { synchronized (stateLock) { - channel.disconnect(); // TODO Why ? We shouldn´t do that...? connectionPath = null; } } @@ -214,12 +215,15 @@ public void close() throws IOException { } /** - * Connect to a destination host. Note: - A SCION channel will internally connect to the next - * border router (first hop) instead of the remote host. + * Connect to a destination host. * - *

NB: This method does internally no call {@link java.nio.channels.DatagramChannel}.connect(), - * instead it calls bind(). That means this method does NOT perform any additional security checks - * associated with connect(), only those associated with bind(). + *

NB: A SCION channel will internally connect to the next border router (first hop) instead of + * the remote host. + * + *

NB: This method does internally not call {@link + * java.nio.channels.DatagramChannel}.connect(), instead it calls bind(). That means this method + * does NOT perform any additional security checks associated with connect(), only those + * associated with bind(). * * @param addr Address of remote host. * @return This channel. @@ -301,7 +305,7 @@ public C connect(RequestPath path) throws IOException { * * @return the current Path or `null` if not path is connected. */ - public Path getConnectionPath() { + public RequestPath getConnectionPath() { synchronized (stateLock) { return connectionPath; } @@ -398,17 +402,12 @@ public void setOverrideSourceAddress(InetSocketAddress address) { this.overrideExternalAddress = address; } - protected int sendRaw(ByteBuffer buffer, InetSocketAddress address, Path path) - throws IOException { - if (cfgRemoteDispatcher && path != null && path.getRawPath().length == 0) { - return channel.send( - buffer, new InetSocketAddress(address.getAddress(), Constants.DISPATCHER_PORT)); + protected int sendRaw(ByteBuffer buffer, Path path) throws IOException { + if (cfgRemoteDispatcher && path.getRawPath().length == 0) { + InetAddress remoteHostIP = path.getFirstHopAddress().getAddress(); + return channel.send(buffer, new InetSocketAddress(remoteHostIP, Constants.DISPATCHER_PORT)); } - return channel.send(buffer, address); - } - - protected int sendRaw(ByteBuffer buffer, InetSocketAddress address) throws IOException { - return sendRaw(buffer, address, null); + return channel.send(buffer, path.getFirstHopAddress()); } public Consumer setScmpErrorListener(Consumer listener) { diff --git a/src/main/java/org/scion/jpan/Path.java b/src/main/java/org/scion/jpan/Path.java index 898619ce..e2cd5a9b 100644 --- a/src/main/java/org/scion/jpan/Path.java +++ b/src/main/java/org/scion/jpan/Path.java @@ -55,15 +55,21 @@ public long getRemoteIsdAs() { @Override public String toString() { - return "Path{" - + "rmtIsdAs=" - + ScionUtil.toStringIA(dstIsdAs) - + ", rmtAddress=" - + dstAddress - + ", rmtPort=" - + dstPort - + ", pathRaw=" - + Arrays.toString(pathRaw) - + '}'; + try { + return "Path{" + + "rmtIsdAs=" + + ScionUtil.toStringIA(dstIsdAs) + + ", rmtAddress=" + + dstAddress + + ", rmtPort=" + + dstPort + + ", firstHop=" + + getFirstHopAddress() + + ", pathRaw=" + + Arrays.toString(getRawPath()) + + '}'; + } catch (UnknownHostException e) { + throw new ScionRuntimeException(e); + } } } diff --git a/src/main/java/org/scion/jpan/ResponsePath.java b/src/main/java/org/scion/jpan/ResponsePath.java index b53b4cc7..623a91e4 100644 --- a/src/main/java/org/scion/jpan/ResponsePath.java +++ b/src/main/java/org/scion/jpan/ResponsePath.java @@ -22,6 +22,8 @@ * ISD/AS, IP and port of the local host. This is mostly for convenience to avoid looking up this * information, but it also ensures that the return packet header contains the exact information * sent/expected by the client. + * + *

A ResponsePath is immutable and thus thread safe. */ public class ResponsePath extends Path { @@ -80,15 +82,14 @@ public int getLocalPort() { @Override public String toString() { return "ResponsePath{" - + super.toString() - + ", firstHopAddress=" - + firstHopAddress + ", localIsdAs=" + srcIsdAs + ", localAddress=" + srcAddress + ", localPort=" + srcPort + + ", " + + super.toString() + '}'; } } diff --git a/src/main/java/org/scion/jpan/ScionDatagramChannel.java b/src/main/java/org/scion/jpan/ScionDatagramChannel.java index 5107e122..ca8279c2 100644 --- a/src/main/java/org/scion/jpan/ScionDatagramChannel.java +++ b/src/main/java/org/scion/jpan/ScionDatagramChannel.java @@ -101,7 +101,7 @@ public void send(ByteBuffer srcBuffer, SocketAddress destination) throws IOExcep * Attempts to send the content of the buffer to the destinationAddress. * * @param srcBuffer Data to send - * @param path Path to destination. If this is a Request path and it is expired then it will + * @param path Path to destination. If this is a RequestPath, and it is expired, then it will * automatically be replaced with a new path. Expiration of ResponsePaths is not checked * @return either the path argument or a new path if the path was an expired RequestPath. Note * that ResponsePaths are not checked for expiration. @@ -123,7 +123,7 @@ public Path send(ByteBuffer srcBuffer, Path path) throws IOException { throw new IOException("Packet is larger than max send buffer size."); } buffer.flip(); - sendRaw(buffer, actualPath.getFirstHopAddress(), actualPath); + sendRaw(buffer, actualPath); return actualPath; } finally { writeLock().unlock(); @@ -169,15 +169,16 @@ public int write(ByteBuffer src) throws IOException { try { checkOpen(); checkConnected(true); + Path path = getConnectionPath(); ByteBuffer buffer = getBufferSend(src.remaining()); int len = src.remaining(); // + 8 for UDP overlay header length - checkPathAndBuildHeader(buffer, getConnectionPath(), len + 8, InternalConstants.HdrTypes.UDP); + checkPathAndBuildHeader(buffer, path, len + 8, InternalConstants.HdrTypes.UDP); buffer.put(src); buffer.flip(); - int sent = sendRaw(buffer, getConnectionPath().getFirstHopAddress(), getConnectionPath()); + int sent = sendRaw(buffer, path); if (sent < buffer.limit() || buffer.remaining() > 0) { throw new ScionException("Failed to send all data."); } diff --git a/src/main/java/org/scion/jpan/ScionDatagramSocket.java b/src/main/java/org/scion/jpan/ScionDatagramSocket.java index e052f877..6c1e96dc 100644 --- a/src/main/java/org/scion/jpan/ScionDatagramSocket.java +++ b/src/main/java/org/scion/jpan/ScionDatagramSocket.java @@ -535,7 +535,7 @@ public void leaveGroup(SocketAddress mcastaddr, NetworkInterface netIf) { * @see ScionDatagramChannel#getConnectionPath() */ public RequestPath getConnectionPath() { - return (RequestPath) channel.getConnectionPath(); + return channel.getConnectionPath(); } /** diff --git a/src/main/java/org/scion/jpan/ScionService.java b/src/main/java/org/scion/jpan/ScionService.java index f65e8329..b8107ecf 100644 --- a/src/main/java/org/scion/jpan/ScionService.java +++ b/src/main/java/org/scion/jpan/ScionService.java @@ -377,7 +377,7 @@ public long getLocalIsdAs() { /** * @param hostName hostName of the host to resolve - * @return A ScionAddress + * @return The ISD/AS code for a hostname * @throws ScionException if the DNS/TXT lookup did not return a (valid) SCION address. */ public long getIsdAs(String hostName) throws ScionException { diff --git a/src/main/java/org/scion/jpan/ScmpChannel.java b/src/main/java/org/scion/jpan/ScmpChannel.java index 458c63fa..5ba65d8a 100644 --- a/src/main/java/org/scion/jpan/ScmpChannel.java +++ b/src/main/java/org/scion/jpan/ScmpChannel.java @@ -229,7 +229,7 @@ Scmp.TimedMessage sendEchoRequest(Scmp.EchoMessage request) throws IOException { buffer, Scmp.Type.INFO_128, localPort, request.getSequenceNumber(), request.getData()); buffer.flip(); request.setSizeSent(buffer.remaining()); - sendRaw(buffer, path.getFirstHopAddress()); + sendRaw(buffer, path); int sizeReceived = receive(request); request.setSizeReceived(sizeReceived); @@ -261,7 +261,7 @@ Scmp.TimedMessage sendTracerouteRequest( int posPath = ScionHeaderParser.extractPathHeaderPosition(buffer); buffer.put(posPath + node.posHopFlags, node.hopFlags); - sendRaw(buffer, path.getFirstHopAddress()); + sendRaw(buffer, path); receive(request); return request; @@ -356,7 +356,7 @@ void sendEchoResponses() throws IOException { buffer, Scmp.Type.INFO_129, port, msg.getSequenceNumber(), msg.getData()); buffer.flip(); msg.setSizeSent(buffer.remaining()); - sendRaw(buffer, path.getFirstHopAddress()); + sendRaw(buffer, path); log.info("Responded to SCMP {} from {}", type, path.getRemoteAddress()); } else { log.info("Dropped SCMP message with type {} from {}", type, path.getRemoteAddress()); diff --git a/src/test/java/org/scion/jpan/api/DatagramChannelApiServerTest.java b/src/test/java/org/scion/jpan/api/DatagramChannelApiServerTest.java index abd09fa8..dafbf13e 100644 --- a/src/test/java/org/scion/jpan/api/DatagramChannelApiServerTest.java +++ b/src/test/java/org/scion/jpan/api/DatagramChannelApiServerTest.java @@ -85,9 +85,8 @@ void send_withoutService() throws IOException { new byte[4], 1, new InetSocketAddress("127.0.0.1", 1)); - Path p2 = channel.send(ByteBuffer.allocate(0), path); + channel.send(ByteBuffer.allocate(0), path); assertNull(channel.getService()); - assertEquals(path, p2); } } diff --git a/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java b/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java index e174854e..95ad747f 100644 --- a/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java +++ b/src/test/java/org/scion/jpan/api/DatagramChannelApiTest.java @@ -509,7 +509,7 @@ void write_expiredRequestPath() throws IOException { ByteBuffer sendBuf = ByteBuffer.wrap(PingPongChannelHelper.MSG.getBytes()); try { channel.write(sendBuf); - RequestPath newPath = (RequestPath) channel.getConnectionPath(); + RequestPath newPath = channel.getConnectionPath(); assertTrue(newPath.getExpiration() > expiredPath.getExpiration()); assertTrue(Instant.now().getEpochSecond() < newPath.getExpiration()); } catch (IOException e) { diff --git a/src/test/java/org/scion/jpan/demo/PingPongChannelClient.java b/src/test/java/org/scion/jpan/demo/PingPongChannelClient.java index 765986ef..8f14aec1 100644 --- a/src/test/java/org/scion/jpan/demo/PingPongChannelClient.java +++ b/src/test/java/org/scion/jpan/demo/PingPongChannelClient.java @@ -17,7 +17,6 @@ import java.io.IOException; import java.net.*; import java.nio.ByteBuffer; -import org.scion.jpan.RequestPath; import org.scion.jpan.ScionDatagramChannel; import org.scion.jpan.ScionUtil; import org.scion.jpan.testutil.MockDNS; @@ -51,11 +50,7 @@ private static void run() throws IOException { String msg = "Hello there!"; ByteBuffer sendBuf = ByteBuffer.wrap(msg.getBytes()); channel.write(sendBuf); - println( - "Sent via " - + ScionUtil.toStringPath((RequestPath) channel.getConnectionPath()) - + ": " - + msg); + println("Sent via " + ScionUtil.toStringPath(channel.getConnectionPath()) + ": " + msg); println("Receiving ... (" + channel.getLocalAddress() + ")"); ByteBuffer buffer = ByteBuffer.allocate(512);