Add GetRuleByHandle, ResetRule & ResetRules methods #326
Conversation
This commit introduces the following methods: - GetRuleByHandle - ResetRule - ResetRules It also refactors GetRules and the deprecated GetRule methods to share a common getRules implementation.
nickgarlis
force-pushed
the
add-rule-methods
branch
from
490ed77 to
8f120d4
Compare
| // rule reflects its state prior to the reset. The provided rule must have a | ||
| // valid Handle. | ||
| // https://docs.kernel.org/networking/netlink_spec/nftables.html#getrule-reset | ||
| func (cc *Conn) ResetRule(t *Table, c *Chain, handle uint64) (*Rule, error) { |
There was a problem hiding this comment.
Same assumptions as above but this could perhaps be (r *Rule) instead.
|
|
||
| // ResetRules resets the stateful expressions (e.g., counters) of all rules | ||
| // in the given table and chain. The reset is applied immediately (no Flush | ||
| // is required). The returned rules reflect their state prior to the reset. |
There was a problem hiding this comment.
Huh, it’s a little surprising to read “no Flush is required”. Is this documented upstream somewhere?
There was a problem hiding this comment.
@stapelberg I was also surprised to discover this, but I think it’s correct. I couldn’t find explicit documentation, though the nftables docs 1 seem to hint at it in the “Non batched handlers” section:
These are getters for:
- table
- chain
- rule
- set
- set element
- generation ID
- stateful objects
- flowtable
Since this message is of type NFT_MSG_GETRULE_RESET, I would classify it as a getter operation. Further evidence is that this message type is classified as NFNL_CB_RCU 2 rather than NFNL_CB_BATCH.
I also tried restructuring my code to append this operation to the message slice sent when Flush() is called, but the kernel returned an EINVAL. I can share those changes if you want in case I made a mistake.
It seems that ResetObj is also handled in a similar way 3. You can see that Flush is not called before asserting that the object has been reset. Perhaps these operations could be renamed/aliased to match the naming of the NFT_MSG type ? That might make it clearer to users that they don’t need to call Flush().
There was a problem hiding this comment.
You can find the experiment mentioned above in a test branch I pushed along with the failing test
There was a problem hiding this comment.
Thanks for the explanation and pointers!
Perhaps these operations could be renamed/aliased to match the naming of the
NFT_MSGtype ? That might make it clearer to users that they don’t need to callFlush().
Hm, can you spell out what the rename would mean? Are you suggesting renaming ResetRules to something that matches NFT_MSG_GETRULE_RESET, like GetRuleReset? I don’t think that would be much clearer per se.
Maybe we should extend the package comment to explain that some methods manipulate a batch that needs flushing whereas others don’t (and ideally, which ones are which)? Probably better to do that in a separate PR, though.
There was a problem hiding this comment.
Yes, I was suggesting matching the naming as you described but I agree it doesn't necessarily make things much clearer. Thanks for the review !
2 participants
This commit introduces the following methods:
It also refactors GetRules and the deprecated GetRule methods to share a common getRules implementation.