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). 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. Better solution here is to check
the return value of enable / disable calls and then set rv appropriately.
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.
Best regards,
Filip Varga
st 23. 11. 2022 o 9:47 Jon Loeliger via lists.fd.io <jdl=
netgate....@lists.fd.io> napísal(a):
> Hi VPPeople,
>
> I would like to open a discussion about some strange code and return values
> from a few of the NAT CLI and API interface functions.
>
> My quest for a reasonable handling when my management plane attempted to
> enable an already-enabled NAT 44 ED feature was handled poorly. It turns
> out,
> rather than a negative API return value with an error indicating that the
> feature
> was already enabled, nor did it return 0 indicating that all was well.
> Instead it
> returned a positive 1 value. As there is no indication of return value in
> the reply
> API call, a normal, negative value was expected.
>
> When I first read through
> the vl_api_nat44_ed_plugin_enable_disable_t_handler()
> and the underlying nat44_plugin_enable() code, I couldn't see where it even
> returned anything other than a straight open-coded 0 return value.
>
> A closer read showed two seriously weird coding issues, both related. At
> the beginning
> of nat44_plugin_enable() we find this code from
> src/plugin/nat/nat44_ed_api.c
> and nat44_ed.c:
>
> int
> nat44_plugin_enable (nat44_config_t c)
> {
> snat_main_t *sm = &snat_main;
>
> fail_if_enabled ();
>
> sm->forwarding_enabled = 0;
> sm->mss_clamping = 0;
> ...
>
> The apparent function fail_if_enabled() isn't a function at all:
>
> #define fail_if_enabled()
> \
> do
> \
> {
> \
> snat_main_t *sm = &snat_main;
> \
> if (PREDICT_FALSE (sm->enabled))
> \
> {
> \
> nat_log_err ("plugin enabled");
> \
> return 1;
> \
> }
> \
> }
> \
> while (0)
>
> It is a single compound statement with the hidden side effect
> of returning directly out of the containing function with, get
> this, a return value of 1. This return value is then used in
> turn directly as the error return value for an API call:
>
> static void
> vl_api_nat44_ed_plugin_enable_disable_t_handler (
> vl_api_nat44_ed_plugin_enable_disable_t *mp)
> {
> ...
> if (mp->enable)
> {
> if ((mp->flags & NAT44_API_IS_STATIC_MAPPING_ONLY) ||
> (mp->flags & NAT44_API_IS_CONNECTION_TRACKING))
> {
> rv = VNET_API_ERROR_UNSUPPORTED;
> }
> else
> {
> c.sessions = ntohl (mp->sessions);
> c.inside_vrf = ntohl (mp->inside_vrf);
> c.outside_vrf = ntohl (mp->outside_vrf);
>
> rv = nat44_plugin_enable (c);
> }
> }
> ...
>
> OK, I am proposing two fixes here.
>
> First, hiding unexpected control flow in a #define like this is bad for
> many reasons.
> Allowing for a nominal function to determine if the feature is already
> enabled
> or not is fine. It should look like an actual inline function with a
> legitimate return value.
> It should likely be a TRUE/FALSE "is enabled" testing function, It should
> probably
> leave the interpretation of that value to the caller. That is, the caller
> should issue error
> messages and generate proper return codes accordingly.
>
> Second, the actual return values for the API should be both listed in the
> API error values
> enumeration and be negative for errors. If there is a need, and I
> maintainer there is, for
> an error code that indicates "already enabled", it should be added if not
> already present.
>
> Anyone have any problem with me submitting patches to fix these issues?
>
> Thanks,
> jdl
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22234): https://lists.fd.io/g/vpp-dev/message/22234
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]
-=-=-=-=-=-=-=-=-=-=-=-
