SCMP cleanup (#19)

---------

Co-authored-by: Tilmann Zäschke <tilmann.zaeschke@inf.ethz.ch>

CHANGELOG.md

@@ -7,15 +7,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 ### Added
 - Code coverage. [#11](https://github.com/tzaeschke/phtree-cpp/pull/11)
-- SCMP error handling
-
+- SCMP error handling: change paths in case of broken links or routers. 
+  
 ### Changed
 - BREAKING CHANGE: Changed maven artifactId to "client"
   [#9](https://github.com/tzaeschke/phtree-cpp/pull/9)
 - BREAKING CHANGE: SCMP refactoring, renamed several SCMP related classes.
   [#14](https://github.com/tzaeschke/phtree-cpp/pull/14), 
   [#15](https://github.com/tzaeschke/phtree-cpp/pull/15),
-  [#17](https://github.com/tzaeschke/phtree-cpp/pull/17)
+  [#17](https://github.com/tzaeschke/phtree-cpp/pull/17),
+  [#19](https://github.com/tzaeschke/phtree-cpp/pull/19)
 - BREAKING CHANGE:`ScionService` instances created via `Scion.newXYZ`
   will not be considered by `Scion.defaultService()`. Also, `DatagramChannel.getService()`
   does not create a service if none exists.

TODO.md

@@ -72,16 +72,22 @@
 ## Plan
 
 ### 0.1.0
-- SCMP error handling (only error, not info)
-  - BUG: Ping to iaOVGU causes 3 CS requests that have type UP,CORE,CORE....?
+- Review + clean up
+
+### 0.2.0
+- SCMP errors handling (above)
+  - Especially for type ¨5: External Interface Down" and "6: Internal Connectivity Down"
+    Problem: we need to receive() or read() to actually receive SCMP errors.
+    We could do this concurrently (actually, this would probably block writes),
+    or we do this onyl if the user calls read/receive. We can then store the failure info
+    (path + AS/IP/IF of failure location). During next send/write, we compare the 
+    path against this failure and try to find a better one. If no better path is found
+    we can just drop the packet (default; consistent with UDP behavior) or throw an error. 
+    Also: The list of broken paths should be cleaned up once the path is expired (or earlier?). 
+- SCION-Proto questions:
   - FIX: Ask why requesting an UP segment effectively returns a DOWN segment
     (it needs to be reversed + the SegID needs to be XORed)
   - Why are Java pings 8 bytes shorter than scionproto pings? -> local AS
-Discuss required for 0.1.0:
-- SCMP errors handling (above)
-  - Especially for expired paths / revoked paths / broken paths?  
-
-### 0.2.0
 - Segments:
   - Sorting by weight (see graph.go:195)
   - Consider peering

src/main/java/org/scion/AbstractDatagramChannel.java

@@ -231,7 +231,7 @@ protected InternalConstants.HdrTypes receiveExtensionHeader(
   }
 
   protected void receiveScmp(ByteBuffer buffer, Path path) {
-    Scmp.ScmpType type = ScmpParser.extractType(buffer);
+    Scmp.Type type = ScmpParser.extractType(buffer);
     Scmp.Message msg = Scmp.createMessage(type, path);
     ScmpParser.consume(buffer, msg);
     checkListeners(msg);

src/main/java/org/scion/Scmp.java

@@ -35,7 +35,7 @@ static <E extends ParseEnum> E parse(Class<E> e, int code) {
     }
   }
 
-  public enum ScmpType implements ParseEnum {
+  public enum Type implements ParseEnum {
     // SCMP error messages:
     ERROR_1(1, "Destination Unreachable"),
     ERROR_2(2, "Packet Too Big"),
@@ -59,13 +59,13 @@ public enum ScmpType implements ParseEnum {
     final int id;
     final String text;
 
-    ScmpType(int id, String text) {
+    Type(int id, String text) {
       this.id = id;
       this.text = text;
     }
 
-    public static ScmpType parse(int id) {
-      return ParseEnum.parse(ScmpType.class, id);
+    public static Type parse(int id) {
+      return ParseEnum.parse(Type.class, id);
     }
 
     @Override
@@ -83,7 +83,7 @@ public String toString() {
     }
   }
 
-  public enum ScmpTypeCode implements ParseEnum {
+  public enum TypeCode implements ParseEnum {
     // SCMP type 1 messages:
     TYPE_1_CODE_0(1, 0, "No route to destination"),
     TYPE_1_CODE_1(1, 1, "Communication administratively denied"),
@@ -132,16 +132,16 @@ public enum ScmpTypeCode implements ParseEnum {
     final int id;
     final String text;
 
-    ScmpTypeCode(int type, int code, String text) {
+    TypeCode(int type, int code, String text) {
       this.type = type;
       this.id = code;
       this.text = text;
     }
 
-    public static ScmpTypeCode parse(int type, int code) {
-      ScmpTypeCode[] values = ScmpTypeCode.class.getEnumConstants();
+    public static TypeCode parse(int type, int code) {
+      TypeCode[] values = TypeCode.class.getEnumConstants();
       for (int i = 0; i < values.length; i++) {
-        ScmpTypeCode pe = values[i];
+        TypeCode pe = values[i];
         if (pe.id() == code && pe.type == type) {
           return pe;
         }
@@ -167,7 +167,7 @@ public String getText() {
     }
 
     public boolean isError() {
-      return type >= ScmpType.ERROR_1.id && type <= ScmpType.ERROR_127.id;
+      return type >= Type.ERROR_1.id && type <= Type.ERROR_127.id;
     }
 
     @Override
@@ -177,13 +177,13 @@ public String toString() {
   }
 
   public static class Message {
-    private ScmpTypeCode typeCode;
+    private TypeCode typeCode;
     private int identifier;
     private int sequenceNumber;
     private Path path;
 
     /** DO NOT USE! */
-    public Message(ScmpTypeCode typeCode, int identifier, int sequenceNumber, Path path) {
+    public Message(TypeCode typeCode, int identifier, int sequenceNumber, Path path) {
       this.typeCode = typeCode;
       this.identifier = identifier;
       this.sequenceNumber = sequenceNumber;
@@ -198,7 +198,7 @@ public int getSequenceNumber() {
       return sequenceNumber;
     }
 
-    public ScmpTypeCode getTypeCode() {
+    public TypeCode getTypeCode() {
       return typeCode;
     }
 
@@ -210,7 +210,7 @@ public void setPath(Path path) {
       this.path = path;
     }
 
-    public void setMessageArgs(ScmpTypeCode sc, int identifier, int sequenceNumber) {
+    public void setMessageArgs(TypeCode sc, int identifier, int sequenceNumber) {
       this.typeCode = sc;
       this.identifier = identifier;
       this.sequenceNumber = sequenceNumber;
@@ -221,7 +221,7 @@ public abstract static class TimedMessage extends Message {
     private long nanoSeconds;
     private boolean timedOut = false;
 
-    private TimedMessage(ScmpTypeCode typeCode, int identifier, int sequenceNumber, Path path) {
+    private TimedMessage(TypeCode typeCode, int identifier, int sequenceNumber, Path path) {
       super(typeCode, identifier, sequenceNumber, path);
     }
 
@@ -248,19 +248,19 @@ public static class EchoMessage extends TimedMessage {
     private int sizeReceived;
 
     private EchoMessage(
-        ScmpTypeCode typeCode, int identifier, int sequenceNumber, Path path, byte[] data) {
+        TypeCode typeCode, int identifier, int sequenceNumber, Path path, byte[] data) {
       super(typeCode, identifier, sequenceNumber, path);
       this.data = data;
     }
 
     public static EchoMessage createEmpty(Path path) {
-      return new EchoMessage(ScmpTypeCode.TYPE_128, -1, -1, path, null);
+      return new EchoMessage(TypeCode.TYPE_128, -1, -1, path, null);
     }
 
     public static EchoMessage createRequest(int sequenceNumber, Path path, ByteBuffer payload) {
       byte[] data = new byte[payload.remaining()];
       payload.get(data);
-      return new EchoMessage(ScmpTypeCode.TYPE_128, -1, sequenceNumber, path, data);
+      return new EchoMessage(TypeCode.TYPE_128, -1, sequenceNumber, path, data);
     }
 
     public byte[] getData() {
@@ -293,26 +293,20 @@ public static class TracerouteMessage extends TimedMessage {
     private long isdAs;
     private long ifID;
 
-    private TracerouteMessage(
-        ScmpTypeCode typeCode, int identifier, int sequenceNumber, Path path) {
+    private TracerouteMessage(TypeCode typeCode, int identifier, int sequenceNumber, Path path) {
       this(typeCode, identifier, sequenceNumber, 0, 0, path);
     }
 
     public static TracerouteMessage createEmpty(Path path) {
-      return new TracerouteMessage(ScmpTypeCode.TYPE_130, -1, -1, path);
+      return new TracerouteMessage(TypeCode.TYPE_130, -1, -1, path);
     }
 
     public static TracerouteMessage createRequest(int sequenceNumber, Path path) {
-      return new TracerouteMessage(ScmpTypeCode.TYPE_130, -1, sequenceNumber, path);
+      return new TracerouteMessage(TypeCode.TYPE_130, -1, sequenceNumber, path);
     }
 
     public TracerouteMessage(
-        ScmpTypeCode typeCode,
-        int identifier,
-        int sequenceNumber,
-        long isdAs,
-        long ifID,
-        Path path) {
+        TypeCode typeCode, int identifier, int sequenceNumber, long isdAs, long ifID, Path path) {
       super(typeCode, identifier, sequenceNumber, path);
       this.isdAs = isdAs;
       this.ifID = ifID;
@@ -340,7 +334,7 @@ public void setTracerouteArgs(long isdAs, long ifID) {
     }
   }
 
-  static Scmp.Message createMessage(Scmp.ScmpType type, Path path) {
+  static Scmp.Message createMessage(Type type, Path path) {
     switch (type) {
       case INFO_128:
       case INFO_129:
@@ -371,6 +365,19 @@ public static ScmpChannel createChannel(RequestPath path) throws IOException {
    * @return New SCMP channel
    */
   public static ScmpChannel createChannel(RequestPath path, int listeningPort) throws IOException {
-    return new ScmpChannel(path, listeningPort);
+    return new ScmpChannel(Scion.defaultService(), path, listeningPort);
+  }
+
+  /**
+   * Create a channel for sending SCMP requests.
+   *
+   * @param service the ScionService instance
+   * @param path Path to destination
+   * @param listeningPort Local port to listen for SCMP requests.
+   * @return New SCMP channel
+   */
+  public static ScmpChannel createChannel(ScionService service, RequestPath path, int listeningPort)
+      throws IOException {
+    return new ScmpChannel(service, path, listeningPort);
   }
 }

src/main/java/org/scion/ScmpChannel.java

@@ -37,11 +37,11 @@ public class ScmpChannel implements AutoCloseable {
   private final InternalChannel channel;
 
   ScmpChannel(RequestPath path) throws IOException {
-    this(path, 12345);
+    this(Scion.defaultService(), path, 12345);
   }
 
-  ScmpChannel(RequestPath path, int port) throws IOException {
-    channel = new InternalChannel(Scion.defaultService(), path, port);
+  ScmpChannel(ScionService service, RequestPath path, int port) throws IOException {
+    channel = new InternalChannel(service, path, port);
   }
 
   /**
@@ -57,7 +57,7 @@ public class ScmpChannel implements AutoCloseable {
   public Scmp.EchoMessage sendEchoRequest(int sequenceNumber, ByteBuffer data) throws IOException {
     RequestPath path = (RequestPath) channel.getCurrentPath();
     Scmp.EchoMessage request = Scmp.EchoMessage.createRequest(sequenceNumber, path, data);
-    sendScmpRequest(() -> channel.sendEchoRequest(request), Scmp.ScmpTypeCode.TYPE_129);
+    sendScmpRequest(() -> channel.sendEchoRequest(request), Scmp.TypeCode.TYPE_129);
     return request;
   }
 
@@ -78,17 +78,16 @@ public Collection<Scmp.TracerouteMessage> sendTracerouteRequest() throws IOExcep
       Scmp.TracerouteMessage request = Scmp.TracerouteMessage.createRequest(i, path);
       requests.add(request);
       PathHeaderParser.Node node = nodes.get(i);
-      sendScmpRequest(
-          () -> channel.sendTracerouteRequest(request, node), Scmp.ScmpTypeCode.TYPE_131);
+      sendScmpRequest(() -> channel.sendTracerouteRequest(request, node), Scmp.TypeCode.TYPE_131);
       if (request.isTimedOut()) {
         break;
       }
     }
     return requests;
   }
 
-  private void sendScmpRequest(
-      IOCallable<Scmp.TimedMessage> sender, Scmp.ScmpTypeCode expectedTypeCode) throws IOException {
+  private void sendScmpRequest(IOCallable<Scmp.TimedMessage> sender, Scmp.TypeCode expectedTypeCode)
+      throws IOException {
     long sendNanos = System.nanoTime();
     Scmp.TimedMessage result = sender.call();
     long nanos = System.nanoTime() - sendNanos;

src/main/java/org/scion/internal/ScmpParser.java

@@ -16,9 +16,9 @@
 
 import static org.scion.Scmp.EchoMessage;
 import static org.scion.Scmp.Message;
-import static org.scion.Scmp.ScmpType;
-import static org.scion.Scmp.ScmpTypeCode;
 import static org.scion.Scmp.TracerouteMessage;
+import static org.scion.Scmp.Type;
+import static org.scion.Scmp.TypeCode;
 
 import java.nio.ByteBuffer;
 
@@ -36,7 +36,7 @@ public static void buildExtensionHeader(ByteBuffer buffer, InternalConstants.Hdr
 
   public static void buildScmpPing(
       ByteBuffer buffer, int identifier, int sequenceNumber, byte[] data) {
-    buffer.put(ByteUtil.toByte(ScmpType.INFO_128.id()));
+    buffer.put(ByteUtil.toByte(Type.INFO_128.id()));
     buffer.put(ByteUtil.toByte(0));
     buffer.putShort((short) 0); // TODO checksum
     buffer.putShort((short) identifier); // unsigned
@@ -46,7 +46,7 @@ public static void buildScmpPing(
 
   public static void buildScmpPing(
       ByteBuffer buffer, int identifier, int sequenceNumber, ByteBuffer data) {
-    buffer.put(ByteUtil.toByte(ScmpType.INFO_128.id()));
+    buffer.put(ByteUtil.toByte(Type.INFO_128.id()));
     buffer.put(ByteUtil.toByte(0));
     buffer.putShort((short) 0); // TODO checksum
     buffer.putShort((short) identifier); // unsigned
@@ -55,7 +55,7 @@ public static void buildScmpPing(
   }
 
   public static void buildScmpTraceroute(ByteBuffer buffer, int identifier, int sequenceNumber) {
-    buffer.put(ByteUtil.toByte(ScmpType.INFO_130.id()));
+    buffer.put(ByteUtil.toByte(Type.INFO_130.id()));
     buffer.put(ByteUtil.toByte(0));
     buffer.putShort((short) 0); // TODO checksum
     buffer.putShort((short) identifier); // unsigned
@@ -75,9 +75,9 @@ public static void buildScmpTraceroute(ByteBuffer buffer, int identifier, int se
     buffer.putLong(0);
   }
 
-  public static ScmpType extractType(ByteBuffer data) {
+  public static Type extractType(ByteBuffer data) {
     // Avoid changing the position!
-    return ScmpType.parse(ByteUtil.toUnsigned(data.get(data.position())));
+    return Type.parse(ByteUtil.toUnsigned(data.get(data.position())));
   }
 
   /**
@@ -93,8 +93,8 @@ public static void consume(ByteBuffer data, Message holder) {
     data.getShort(); // checksum
     // TODO validate checksum
 
-    ScmpType st = ScmpType.parse(type);
-    ScmpTypeCode sc = ScmpTypeCode.parse(type, code);
+    Type st = Type.parse(type);
+    TypeCode sc = TypeCode.parse(type, code);
     int short1 = ByteUtil.toUnsigned(data.getShort());
     int short2 = ByteUtil.toUnsigned(data.getShort());
     holder.setMessageArgs(sc, short1, short2);