search

p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
144 stars 88 forks source link

Should the egress_port field in the Replica message also be changed to a bytes string #329

Closed antoninbas closed 1 year ago

antoninbas commented 3 years ago

https://github.com/p4lang/p4runtime/pull/317 deprecated the watch field (int32) in favor of a watch_port field (bytes) for action profile group programming.

However, there is another field used to represent ports, that is still using an integral type (for some reason it is uint32, while watch was using int32...): https://github.com/p4lang/p4runtime/blob/bd19174b0f8a99dd0ea39a185dae311e5f8b3606/proto/p4/v1/p4runtime.proto#L406-L410

It seems that the fields play a similar role and that one may want to leverage translation for ports when programming packet replication / cloning, so it seems that we should make the same change as for watch. @stefanheule is there a reason why we didn't make the same change for the egress_port field at the time? I don't recall the topic even coming up but I may have forgotten...

stefanheule commented 3 years ago

Just an oversight, I think, I didn't realize we had another port field there. And yeah, we were inconsistent with int vs uint before on ports for some reason.

antoninbas commented 3 years ago

I have created a PR for this: https://github.com/p4lang/p4runtime/pull/331

But I feel like we have opened a can of worms: we also need to address the case of counters / meters, for which the index field should probably be changed to bytes as well. It makes the API less convenient to use IMO, but it makes sense in a way since PSA typically uses bit<W> values to index counters in the datapath.