Hi, I understand your perspective. But you have to think in a longer perspective of NAT changes. The concept of enabling / disabling plugins on the go is not super common among VPP. This was just an addition for SDN deployments so configuration through startup config can be avoided and reallocation - change in number of sessions etc. can be done on the fly. Because of that we haven't yet committed to a "proper" way to handle events. Why did I choose those return values ? Because UNSUPPORTED is widely used if some action is not supported at the moment. This also applies to calling different control plane logic through API on nat44-ed and among other plugins. So trying to enable already enabled plugin returns UNSUPPORTED and trying to disable plugin that is not configured will result in VNET_API_ERROR_UNSUPPORTED. Maybe addition to add VNET_API_ERROR_FEATURE_ENABLED could work for you. What I would advise is to wait for this patch to get merged and then add additional VNET_API_ERROR_FEATURE_ENABLED into vnet/error.h and changing the return value (just so we comply with how patches should be submitted).
I will do the same for EI. Thank you for pointing that out. Also consider that the part of code returning VNET_API_ERROR_UNSUPPORTED in nat44-ed enable/disable call for unsupported flags is just a backward API compatibility logic. That means it should never happen so relying on VNET_API_ERROR_UNSUPPORTED if plugin is already enabled should be sufficient. As I mentioned before NAT44-ED code architecture isn't ideal but that is because of its history and both ED and EI and other flavors of NAT where one big plugin previously (SNAT) now we are moving forward and cleaning it up (or I am). Thank you for your interest. Best regards, Filip Varga st 23. 11. 2022 o 12:59 Jon Loeliger via lists.fd.io <jdl= netgate....@lists.fd.io> napísal(a): > On Wed, Nov 23, 2022 at 12:25 PM filvarga <filipvarg...@gmail.com> wrote: > >> Hi, >> >> Thank you for your notes and pointing out the issue. Feel free to submit >> any changes in the future and add appropriate people to your review (based >> on maintainers list). >> > > Thanks, will do! > > For now I am posting a quick fix. As the nat44-ed control plane is >> undergoing a continuous huge refactor I wouldn't advise too much diverting >> from its current state.\ >> > > I might point out that this applies to both ED and EI. They both display > the same issues. > > >> Better solution here is to check the return value of enable / disable >> calls and then set rv appropriately. >> > > Exactly. But in this case I couldn't because it was buried in a macro > that didn't return. > > >> For now (based on return codes from vnet/error.h) I will use these >> VNET_API_ERROR_FEATURE_DISABLED >> VNET_API_ERROR_UNSUPPORTED >> for enable/disable api errors rather than introducing something new that >> can get removed in near future. >> > > Those error values miss the important case of identifying the case where > one is > trying to enable the already-enabled feature. We need to know and > identify that > case, just as many of the other Features do. > > Thanks, > jdl > > > > >
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#22237): https://lists.fd.io/g/vpp-dev/message/22237 Mute This Topic: https://lists.fd.io/mt/95222842/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
