pcn-iptables: fix horus module by mbertrone · Pull Request #151 · polycube-network/polycube

mbertrone · 2019-06-11T07:50:23Z

avoid incompatibility with conntrack rules
increase horus max size
use attribute packed to avoid memory alignment issues

mauriciovasquezbernal · 2019-06-13T11:51:46Z

  for (auto const &rule : rules) {
    i++;
-    fromRuleToHorusKeyValue(rule, key, value);
+    if (i > HorusConst::MAX_RULE_SIZE_FOR_HORUS)


Shouldn't this be >=?

It should be >, but since I reworked the cycle to avoid confusion >= works now.

mauriciovasquezbernal · 2019-06-13T11:52:44Z

-    fromRuleToHorusKeyValue(rule, key, value);
+    if (i > HorusConst::MAX_RULE_SIZE_FOR_HORUS)
+      break;
+    if(fromRuleToHorusKeyValue(rule, key, value)) {


What about refactoring a little bit this?

if (!fromRuleToHorusKeyValue(rule, key,value) break;

That else and return at the end is quite difficult to read.

Thanks for the suggestion. I agree it is more clear.

mauriciovasquezbernal · 2019-06-13T11:53:51Z

 // matching the pattern, at ruleset begin
 const uint8_t MIN_RULE_SIZE_FOR_HORUS = 1;
-const uint8_t MAX_RULE_SIZE_FOR_HORUS = -1;  // not used
+const uint8_t MAX_RULE_SIZE_FOR_HORUS = 2048;  // not used


Is this still not used?

Forgot to remove this comment.

I just got a compilation warning here, 2048 is too big for a uint8_t, I really don't know how this is working.

@sebymiano Isn't something you already fixed on some of your branch?

I just changed it to uint32_t to remove the compilation warning but I haven't tested it.
I guess it should be ok to change.

@mauriciovasquezbernal @frisso @sebymiano It was my mistake during code alignment (the code is correct in the firewall, not in iptables).
I've opened a PR for that #163

mauriciovasquezbernal · 2019-06-13T11:59:43Z

    // Independently from the final action (ACCEPT or DROP)
    // I have to update the counters
-    if (value->ruleID <= 1024) {
+    if (value->ruleID <= _MAX_RULE_SIZE_FOR_HORUS) {


I think here the test has to be <=.
The range of ruleID is [0, _MAX_RULE_SIZE_FOR_HORUS - 1]

The test has to be <

I meant <, this is very easy to get confused :D

Signed-off-by: Matteo Bertrone <m.bertrone@gmail.com>

mbertrone · 2019-06-13T13:58:50Z

@mauriciovasquezbernal I force-pushed an updated version according to your comments.
Thanks for reviewing it.

mauriciovasquezbernal

LGTM, thanks Matteo.

mbertrone requested a review from a team as a code owner June 11, 2019 07:50

frisso approved these changes Jun 11, 2019

View reviewed changes

mbertrone force-pushed the pr/fix_iptables_horus branch from ea7aca6 to 4e86738 Compare June 11, 2019 13:54

mauriciovasquezbernal reviewed Jun 13, 2019

View reviewed changes

pcn-iptables: fix horus module

cad34be

Signed-off-by: Matteo Bertrone <m.bertrone@gmail.com>

mbertrone force-pushed the pr/fix_iptables_horus branch from 4e86738 to cad34be Compare June 13, 2019 13:55

mauriciovasquezbernal approved these changes Jun 13, 2019

View reviewed changes

frisso merged commit 0c26a38 into master Jun 13, 2019

frisso deleted the pr/fix_iptables_horus branch June 14, 2019 06:17

Conversation

mbertrone commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbertrone commented Jun 13, 2019

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants