search

antrea-io / libOpenflow

OpenFlow Go library
Apache License 2.0
9 stars 18 forks source link

Replace NXRange with mask in NewRegMatchField #16

Closed commandgjj closed 2 years ago

commandgjj commented 2 years ago

For NXReg, OvS will accept value/mask on arbitary bitwise. If using range[start:end], it is hard to combine discontinuous value/mask, such as, to merge reg0[0:0] with reg0[2:2], with range method, we will pass NXRange[0:2] and it will generate mask 0x7, but actually, the mask is 0x5. Here, we can use mask directly.

After this change, we also need to change caller's parameter in ofnet.

Signed-off-by: Jinjun Gao gjinjun@gmail.com

commandgjj commented 2 years ago

@hangyan @tnqn @srikartati @Jexf @wenyingd Can you please help review this patch? I also open a PR in ofnet repo to use this change. Thanks!

wenyingd commented 2 years ago

Function NewRegMatchField is actually used in other repos, if you want to support using mask value directly, I think it is better to add new function but not replace the existing one directly.

commandgjj commented 2 years ago

Function NewRegMatchField is actually used in other repos, if you want to support using mask value directly, I think it is better to add new function but not replace the existing one directly.

Yeah, we can add a new API to use mask directly. But as PR summary said, the current NewRegMatchField cannot compute mask from range [start:end] correctly. It may mislead caller or other repo still to call this API if we keep it there.

wenyingd commented 2 years ago

Yeah, we can add a new API to use mask directly. But as PR summary said, the current NewRegMatchField cannot compute mask from range [start:end] correctly. It may mislead caller or other repo still to call this API if we keep it there.

I got your concern, but actually the existing API (function) is designed for a field value with continuous mask bits. I think supporting discontinuous bit masks is a new requirement but not to resolve an issue, so I suggest adding new signature for it. Besides, as there is exising code in other repo using the original API for continuous bit mask, if we change the signature to a mask value from NXRange, the code in other repos calling that function needs changes, and these changes are actually not wanted.

commandgjj commented 2 years ago

Yeah, we can add a new API to use mask directly. But as PR summary said, the current NewRegMatchField cannot compute mask from range [start:end] correctly. It may mislead caller or other repo still to call this API if we keep it there.

I got your concern, but actually the existing API (function) is designed for a field value with continuous mask bits. I think supporting discontinuous bit masks is a new requirement but not to resolve an issue, so I suggest adding new signature for it. Besides, as there is exising code in other repo using the original API for continuous bit mask, if we change the signature to a mask value from NXRange, the code in other repos calling that function needs changes, and these changes are actually not wanted.

Thanks for detail explanation, Wenying. I got your point here and I will provide a new API to support discontinuous mask.

commandgjj commented 2 years ago

Hello, Wenying, can you please help take a look again? Thanks!

commandgjj commented 2 years ago

I also filed a PR in ofnet to use this new API, please help to review it too. https://github.com/antrea-io/ofnet/pull/20

commandgjj commented 2 years ago

code LGTM, but there is a golang-ci issue, please fix it first.

Done