Fix SNAT rollback

[PlagueCZ]

Addressed rollback problems as per issue.

Hopefully I did not misunderstand the code flow, please verify.

Fixes #699

[PlagueCZ]

The second commit is an addition as it occurred to me that having the INC macro used in snat_node.c while the DEC macro used in dp_nat.c is asking for trouble, so I moved them to be in one place (inside NAT functions themselves).

[byteocean]

Thanks for spotting this potential leak. I was able to understand the first two commits, but couldn't relate the third commit "Fix edge-case in SNAT deletion" with the wording in the issue description, as it seems to be a code reorg, which looks better for sure. Could you help to elaborate more or point to the relevant description in the issue?

[PlagueCZ]

Thanks for spotting this potential leak. I was able to understand the first two commits, but couldn't relate the third commit "Fix edge-case in SNAT deletion" with the wording in the issue description, as it seems to be a code reorg, which looks better for sure. Could you help to elaborate more or point to the relevant description in the issue?

I edited the issue. The problem was that there was a quick-return but that jumped over the code at the end with DP_STATS_NAT_DEC_USED_PORT_CNT().

This would become a bigger issue in the followup PRs as I will be adding more to the "finishing" part of this function

[byteocean]

Approved as communicated

[guvenc]

I understand that this control flow fall through is intentional now so that it can hit the INC macro at the end but this would also cause the new code to hit the lines below for ret == ENOENT case.

created_port = dp_get_port_by_id(cntrack->created_port_id);
if (!created_port)
return DP_ERROR;

Was this intentional ?

[PlagueCZ]

Yes. This is the commit that "fixes SNAT edge case" because previously it could have found a portmap entry (and immediately deleted it), but not found portoverload entry and then return without actually decreasing the counter.