Skip to content

Conversation

@PlagueCZ
Copy link
Contributor

For #697 I need to re-use SNAT create/delete functions.

This PR reorganizes the code slightly for that to be easier in a followup PR.

I have separated the work into small commits so that each is easier to be reviewed as a separate change.

@PlagueCZ PlagueCZ requested a review from a team as a code owner July 18, 2025 15:38
@github-actions github-actions bot added size/L enhancement New feature or request labels Jul 18, 2025
@PlagueCZ
Copy link
Contributor Author

Oh, the "edge case" is the fact that when previously doing a rollback (which basically never happens) portmap_data->flow_cnt++; was not reverted.

@PlagueCZ
Copy link
Contributor Author

I added a commit using a goto to a common return code because returning a positive result from the middle of the function is not common in our codebase and also any additional stuff needed to be done when returning the NAT port would need to be duplicate.

For example - changes from #700 would require duplicating DP_STATS_NAT_INC_USED_PORT_CNT(port).

Also for #697 there needs to be some dp_send_add_nat_to_other_dpservice() which would also then need to be done in two places.

That's why I opted for a simple goto to the return code. This can also be replaces by a wrapper function if preferred.

@PlagueCZ PlagueCZ force-pushed the feature/allocate_snat_refactor branch from 0997628 to 1cf9cdb Compare July 22, 2025 16:42
@PlagueCZ
Copy link
Contributor Author

I had to rebase onto #700 because otherwise the review is really misleading as you never saw the actual code that I am working on.

@guvenc guvenc moved this to In Progress in Networking Jul 23, 2025
@guvenc guvenc added this to the H2/2025 milestone Jul 23, 2025
@guvenc guvenc removed this from the H2/2025 milestone Jul 23, 2025
@guvenc guvenc removed this from Networking Jul 23, 2025
@PlagueCZ PlagueCZ force-pushed the feature/allocate_snat_refactor branch from f5b4bde to 881e5d1 Compare July 24, 2025 10:53
Copy link
Collaborator

@guvenc guvenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@guvenc guvenc merged commit 881e5d1 into main Jul 24, 2025
6 checks passed
@guvenc guvenc deleted the feature/allocate_snat_refactor branch July 24, 2025 17:12
@github-project-automation github-project-automation bot moved this to Done in Roadmap Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants