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 (#22233): https://lists.fd.io/g/vpp-dev/message/22233
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]
-=-=-=-=-=-=-=-=-=-=-=-
