[SDFAB-1147] Encode P4Runtime messages with canonical byte string
In ONOS, we still use non-canonical byte strings when sending P4Runtime
write requests.
Ref: https://p4.org/p4-spec/p4runtime/v1.3.0/P4Runtime-Spec.html#sec-bytestrings
Change-Id: I27d7977660bea462de82ebe2a4cb5d14500d3b69
diff --git a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
index b7e806b..1ee881b 100644
--- a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
+++ b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/P4RuntimeGroupTest.java
@@ -185,7 +185,8 @@
assertEquals(false, response.isSuccess());
assertEquals(1, response.all().size());
- assertEquals("Wrong size for param 'port' of action 'set_egress_port', expected 2 bytes, but found 13",
+ assertEquals("Wrong size for param 'port' of action 'set_egress_port', " +
+ "expected no more than 2 bytes, but found 13",
response.all().iterator().next().explanation());
}
@@ -244,7 +245,7 @@
Action.Param param = action.getParamsList().get(0);
assertEquals(1, param.getParamId());
byte outPort = (byte) (member.getMemberId() - BASE_MEM_ID);
- ByteString bs = ByteString.copyFrom(new byte[]{0, outPort});
+ ByteString bs = ByteString.copyFrom(new byte[]{outPort});
assertEquals(bs, param.getValue());
}
}
diff --git a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/codec/TableEntryEncoderTest.java b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/codec/TableEntryEncoderTest.java
index 29f2936..a5ccecf 100644
--- a/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/codec/TableEntryEncoderTest.java
+++ b/protocols/p4runtime/ctl/src/test/java/org/onosproject/p4runtime/ctl/codec/TableEntryEncoderTest.java
@@ -256,7 +256,7 @@
// Ternary match.
byte[] encodedTernaryMatchValue = tableEntryMsg.getMatch(0).getTernary().getValue().toByteArray();
- assertThat(encodedTernaryMatchValue, is(ethAddr.asArray()));
+ assertThat(encodedTernaryMatchValue, is(ethAddr.canonical().asArray()));
Action actionMsg = tableEntryMsg.getAction().getAction();
@@ -386,7 +386,7 @@
// Ternary match.
byte[] encodedTernaryMatchValue = tableEntryMsg.getMatch(0).getTernary().getValue().toByteArray();
- assertThat(encodedTernaryMatchValue, is(ethAddr.asArray()));
+ assertThat(encodedTernaryMatchValue, is(ethAddr.canonical().asArray()));
// no action
assertThat(tableEntryMsg.hasAction(), is(false));
diff --git a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/ActionCodec.java b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/ActionCodec.java
index daf91f5..cbf112c 100644
--- a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/ActionCodec.java
+++ b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/ActionCodec.java
@@ -49,11 +49,14 @@
for (PiActionParam p : piAction.parameters()) {
final P4InfoOuterClass.Action.Param paramInfo = browser.actionParams(actionId)
.getByName(p.id().toString());
- final ByteString paramValue = ByteString.copyFrom(p.value().asReadOnlyBuffer());
- if (!browser.isTypeString(paramInfo.getTypeName())) {
+ final ByteString paramValue;
+ if (browser.isTypeString(paramInfo.getTypeName())) {
+ paramValue = ByteString.copyFrom(p.value().asReadOnlyBuffer());
+ } else {
+ paramValue = ByteString.copyFrom(p.value().canonical().asReadOnlyBuffer());
// Check size only if the param type is not a sdn_string
assertSize(format("param '%s' of action '%s'", p.id(), piAction.id()),
- paramValue, paramInfo.getBitwidth());
+ paramValue, paramInfo.getBitwidth());
}
actionMsgBuilder.addParams(P4RuntimeOuterClass.Action.Param.newBuilder()
.setParamId(paramInfo.getId())
diff --git a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/FieldMatchCodec.java b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/FieldMatchCodec.java
index 48baa17..4b8fe50 100644
--- a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/FieldMatchCodec.java
+++ b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/FieldMatchCodec.java
@@ -74,8 +74,11 @@
switch (piFieldMatch.type()) {
case EXACT:
PiExactFieldMatch fieldMatch = (PiExactFieldMatch) piFieldMatch;
- ByteString exactValue = ByteString.copyFrom(fieldMatch.value().asReadOnlyBuffer());
- if (!isSdnString) {
+ ByteString exactValue;
+ if (isSdnString) {
+ exactValue = ByteString.copyFrom(fieldMatch.value().asReadOnlyBuffer());
+ } else {
+ exactValue = ByteString.copyFrom(fieldMatch.value().canonical().asReadOnlyBuffer());
assertSize(VALUE_OF_PREFIX + entityName, exactValue, fieldBitwidth);
}
return messageBuilder.setExact(
@@ -86,8 +89,8 @@
.build();
case TERNARY:
PiTernaryFieldMatch ternaryMatch = (PiTernaryFieldMatch) piFieldMatch;
- ByteString ternaryValue = ByteString.copyFrom(ternaryMatch.value().asReadOnlyBuffer());
- ByteString ternaryMask = ByteString.copyFrom(ternaryMatch.mask().asReadOnlyBuffer());
+ ByteString ternaryValue = ByteString.copyFrom(ternaryMatch.value().canonical().asReadOnlyBuffer());
+ ByteString ternaryMask = ByteString.copyFrom(ternaryMatch.mask().canonical().asReadOnlyBuffer());
if (isSdnString) {
sdnStringUnsupported(entityName, piFieldMatch.type());
}
@@ -102,7 +105,7 @@
.build();
case LPM:
PiLpmFieldMatch lpmMatch = (PiLpmFieldMatch) piFieldMatch;
- ByteString lpmValue = ByteString.copyFrom(lpmMatch.value().asReadOnlyBuffer());
+ ByteString lpmValue = ByteString.copyFrom(lpmMatch.value().canonical().asReadOnlyBuffer());
int lpmPrefixLen = lpmMatch.prefixLength();
if (isSdnString) {
sdnStringUnsupported(entityName, piFieldMatch.type());
@@ -117,8 +120,8 @@
.build();
case RANGE:
PiRangeFieldMatch rangeMatch = (PiRangeFieldMatch) piFieldMatch;
- ByteString rangeHighValue = ByteString.copyFrom(rangeMatch.highValue().asReadOnlyBuffer());
- ByteString rangeLowValue = ByteString.copyFrom(rangeMatch.lowValue().asReadOnlyBuffer());
+ ByteString rangeHighValue = ByteString.copyFrom(rangeMatch.highValue().canonical().asReadOnlyBuffer());
+ ByteString rangeLowValue = ByteString.copyFrom(rangeMatch.lowValue().canonical().asReadOnlyBuffer());
if (isSdnString) {
sdnStringUnsupported(entityName, piFieldMatch.type());
}
@@ -132,8 +135,11 @@
.build();
case OPTIONAL:
PiOptionalFieldMatch optionalMatch = (PiOptionalFieldMatch) piFieldMatch;
- ByteString optionalValue = ByteString.copyFrom(optionalMatch.value().asReadOnlyBuffer());
- if (!isSdnString) {
+ ByteString optionalValue;
+ if (isSdnString) {
+ optionalValue = ByteString.copyFrom(optionalMatch.value().asReadOnlyBuffer());
+ } else {
+ optionalValue = ByteString.copyFrom(optionalMatch.value().canonical().asReadOnlyBuffer());
assertSize(VALUE_OF_PREFIX + entityName, optionalValue, fieldBitwidth);
}
return messageBuilder.setOptional(
diff --git a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/PacketMetadataCodec.java b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/PacketMetadataCodec.java
index ab29e62..24aa00e 100644
--- a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/PacketMetadataCodec.java
+++ b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/PacketMetadataCodec.java
@@ -25,6 +25,8 @@
import p4.config.v1.P4InfoOuterClass;
import p4.v1.P4RuntimeOuterClass;
+import java.nio.ByteBuffer;
+
import static org.onlab.util.ImmutableByteSequence.copyAndFit;
import static org.onlab.util.ImmutableByteSequence.copyFrom;
@@ -41,12 +43,18 @@
PiPacketMetadata piEntity, P4InfoOuterClass.Preamble ctrlPktMetaPreamble,
PiPipeconf pipeconf, P4InfoBrowser browser)
throws P4InfoBrowser.NotFoundException {
- final int metadataId = browser
+ P4InfoOuterClass.ControllerPacketMetadata.Metadata packetMetadata = browser
.packetMetadatas(ctrlPktMetaPreamble.getId())
- .getByName(piEntity.id().id()).getId();
+ .getByName(piEntity.id().id());
+ final ByteBuffer value;
+ if (browser.isTypeString(packetMetadata.getTypeName())) {
+ value = piEntity.value().asReadOnlyBuffer();
+ } else {
+ value = piEntity.value().canonical().asReadOnlyBuffer();
+ }
return P4RuntimeOuterClass.PacketMetadata.newBuilder()
- .setMetadataId(metadataId)
- .setValue(ByteString.copyFrom(piEntity.value().asReadOnlyBuffer()))
+ .setMetadataId(packetMetadata.getId())
+ .setValue(ByteString.copyFrom(value))
.build();
}
diff --git a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/Utils.java b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/Utils.java
index 2895b83..af1c159 100644
--- a/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/Utils.java
+++ b/protocols/p4runtime/utils/src/main/java/org/onosproject/p4runtime/ctl/codec/Utils.java
@@ -34,9 +34,9 @@
throws CodecException {
int byteWidth = (int) Math.ceil((float) bitWidth / 8);
- if (value.size() != byteWidth) {
+ if (value.size() > byteWidth) {
throw new CodecException(format(
- "Wrong size for %s, expected %d bytes, but found %d",
+ "Wrong size for %s, expected no more than %d bytes, but found %d",
entityDescr, byteWidth, value.size()));
}
}
diff --git a/utils/misc/src/main/java/org/onlab/util/ImmutableByteSequence.java b/utils/misc/src/main/java/org/onlab/util/ImmutableByteSequence.java
index 32e9d0b..41c6ba5 100644
--- a/utils/misc/src/main/java/org/onlab/util/ImmutableByteSequence.java
+++ b/utils/misc/src/main/java/org/onlab/util/ImmutableByteSequence.java
@@ -326,7 +326,7 @@
* @return an integer value
*/
public int size() {
- return this.value.capacity();
+ return this.value.limit() - this.value.position();
}
/**
@@ -547,6 +547,22 @@
}
/**
+ * Returns a new ImmutableByteSequence with same content as this one, but with leading zero bytes stripped.
+ *
+ * @return new ImmutableByteSequence
+ */
+ public ImmutableByteSequence canonical() {
+ ByteBuffer newByteBuffer = this.value.duplicate();
+ ImmutableByteSequence canonicalBs = new ImmutableByteSequence(newByteBuffer);
+ canonicalBs.value.rewind();
+ while (canonicalBs.value.hasRemaining() && canonicalBs.value.get() == 0) {
+ // Make style check happy
+ }
+ canonicalBs.value.position(canonicalBs.value.position() - 1);
+ return canonicalBs;
+ }
+
+ /**
* Signals a trim exception during byte sequence creation.
*/
public static class ByteSequenceTrimException extends Exception {
diff --git a/utils/misc/src/test/java/org/onlab/util/ImmutableByteSequenceTest.java b/utils/misc/src/test/java/org/onlab/util/ImmutableByteSequenceTest.java
index c83c6b2..f9c2a0b 100644
--- a/utils/misc/src/test/java/org/onlab/util/ImmutableByteSequenceTest.java
+++ b/utils/misc/src/test/java/org/onlab/util/ImmutableByteSequenceTest.java
@@ -376,4 +376,52 @@
assertThat("Invalid bitwise XOR result",
xorBs.asReadOnlyBuffer().getLong(), is(long1 ^ long2));
}
+
+ @Test
+ public void testCanonical() {
+ ImmutableByteSequence bs = ImmutableByteSequence.copyFrom(0x000000ff);
+ ImmutableByteSequence canonicalBs = bs.canonical();
+ assertThat("Incorrect size", canonicalBs.size(), is(1));
+ ByteBuffer bb = canonicalBs.asReadOnlyBuffer();
+ assertThat("Incorrect byte buffer position", bb.position(), is(3));
+
+ bs = ImmutableByteSequence.copyFrom(0x100000ff);
+ canonicalBs = bs.canonical();
+ assertThat("Incorrect size", canonicalBs.size(), is(4));
+ bb = canonicalBs.asReadOnlyBuffer();
+ assertThat("Incorrect byte buffer position", bb.position(), is(0));
+
+ bs = ImmutableByteSequence.copyFrom(0x00000000ff0000ffL);
+ canonicalBs = bs.canonical();
+ assertThat("Incorrect size", canonicalBs.size(), is(4));
+ bb = canonicalBs.asReadOnlyBuffer();
+ assertThat("Incorrect byte buffer position", bb.position(), is(4));
+
+ bs = ImmutableByteSequence.copyFrom(0);
+ canonicalBs = bs.canonical();
+ assertThat("Incorrect size", canonicalBs.size(), is(1));
+ bb = canonicalBs.asReadOnlyBuffer();
+ assertThat("Incorrect byte buffer position", bb.position(), is(3));
+
+ bs = ImmutableByteSequence.copyFrom(0L);
+ canonicalBs = bs.canonical();
+ assertThat("Incorrect size", canonicalBs.size(), is(1));
+ bb = canonicalBs.asReadOnlyBuffer();
+ assertThat("Incorrect byte buffer position", bb.position(), is(7));
+
+ new EqualsTester()
+ .addEqualityGroup(
+ ImmutableByteSequence.copyFrom(0x000000ff).canonical(),
+ ImmutableByteSequence.copyFrom((short) 0x00ff).canonical())
+ .addEqualityGroup(
+ ImmutableByteSequence.copyFrom(0x000001ff).canonical(),
+ ImmutableByteSequence.copyFrom(0x00000000000001ffL).canonical())
+ .addEqualityGroup(
+ ImmutableByteSequence.copyFrom(0xc00001ff).canonical(),
+ ImmutableByteSequence.copyFrom(0x00000000c00001ffL).canonical())
+ .addEqualityGroup(
+ ImmutableByteSequence.copyFrom(0).canonical(),
+ ImmutableByteSequence.copyFrom(0L).canonical())
+ .testEquals();
+ }
}