Skip to content

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Adds missing check for runtime ctl:ruleRemoveByTag (Issue #2099) #2102

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0a7906
Fix 2099 / ruleRemoveByTag
airween May 23, 2019
ebdb36b
Added some test cases
airween May 23, 2019
5497b86
Added plus condition to prevent the unnecessary loops
airween May 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/rules.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ int Rules::evaluate(int phase, Transaction *t) {
break;
}
}
if (remove_rule) {
Copy link
Contributor

@zimmerle zimmerle May 27, 2019

Choose a reason for hiding this comment

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

Hi @airween,

I am assuming that you have copy && paste the remove_rule check from the block just above. Indeed, that was missing. However you may have pasted in the wrong place, that may lead to unnecessary loops. Can you make it exactly in the same fashion as the others checks?

Copy link
Member Author

@airween airween May 27, 2019

Choose a reason for hiding this comment

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

Hi @zimmerle, sorry, I don't know what do you think about - why is this a wrong place, and which loops could occures?

Do you mean that it needs a plus condition before the for loop, as the previous variables?

Copy link
Member Author

@airween airween May 27, 2019

Choose a reason for hiding this comment

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

Do you think about like this?
5497b86

Copy link
Contributor

@zimmerle zimmerle May 28, 2019

Choose a reason for hiding this comment

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

Yes, exactly. There are three loops that aiming to do the very same thing. There is no reason to have one different for the others.

Copy link
Member Author

@airween airween May 28, 2019

Choose a reason for hiding this comment

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

Right, thanks for the clarification.

continue;
}

rule->evaluate(t, NULL);
if (t->m_it.disruptive == true) {
Expand Down