WIP: rule: add String() method #108
Conversation
Before this commit: the printing of a rule results in a pointer address. After this commit: the printing of a rules results in a human-readable text. Resolves: google#104 Signed-off-by: Paul Greenberg <[email protected]>
| NFT_STOLEN = 2 | ||
| NFT_QUEUE = 3 | ||
| NFT_REPEAT = 4 | ||
| NFT_STOP = 5 |
There was a problem hiding this comment.
Why did we not have these declared before? Where are they canonically defined in the upstream sources? Can you add a link please?
There was a problem hiding this comment.
Why did we not have these declared before? Where are they canonically defined in the upstream sources? Can you add a link please?
There is no definition. They are defined with the following:
const (
VerdictReturn VerdictKind = iota - 5
VerdictGoto
VerdictJump
VerdictBreak
VerdictContinue
VerdictDrop
VerdictAccept
VerdictStolen
VerdictQueue
VerdictRepeat
VerdictStop
)
I am looking in https://github.com/torvalds/linux/blob/47ec5303d73ea344e84f46660fff693c57641386/include/uapi/linux/netfilter/nf_tables.h#L64-L70 ... yet to discover.
There was a problem hiding this comment.
There was a problem hiding this comment.
@stapelberg , in retrospect, after finding https://git.netfilter.org/libnftnl/tree/src/utils.c#n182, I say was mimicking this function 😄
const char *nftnl_verdict2str(uint32_t verdict)
{
switch (verdict) {
case NF_ACCEPT:
return "accept";
case NF_DROP:
return "drop";
case NF_STOLEN:
return "stolen";
case NF_QUEUE:
return "queue";
case NF_REPEAT:
return "repeat";
case NF_STOP:
return "stop";
case NFT_RETURN:
return "return";
case NFT_JUMP:
return "jump";
case NFT_GOTO:
return "goto";
case NFT_CONTINUE:
return "continue";
case NFT_BREAK:
return "break";
default:
return "unknown";
}
}There was a problem hiding this comment.
@stapelberg , also the NF_STOLEN comes from https://git.netfilter.org/libnftnl/tree/include/linux/netfilter.h#n9
/* Responses from hook functions. */
#define NF_DROP 0
#define NF_ACCEPT 1
#define NF_STOLEN 2
#define NF_QUEUE 3
#define NF_REPEAT 4
#define NF_STOP 5
#define NF_MAX_VERDICT NF_STOPThere was a problem hiding this comment.
After having another look, I think this duplicates the verdict codes we already have defined here, no?
Lines 39 to 54 in c25e4f6
Why not just use e.g. VerdictReturn in the switch statement below in String()?
| var v string | ||
| switch e.Kind { | ||
| case unix.NFT_RETURN: | ||
| v = "return" // -0x5 |
There was a problem hiding this comment.
What did you want to express with these comments?
There was a problem hiding this comment.
What did you want to express with these comments?
@stapelberg , just a note as to what the id is for the return.
There was a problem hiding this comment.
If you follow my other suggestion above regarding using the Verdict* names, I think it’d be cleaner to not duplicate these values (we only want a single place to update, not multiple). If people are inclined to find out the value, they should jump to the definition using their editor.
| t.Fatalf("bad verdict string at %d: %s (received) vs. %s (expected)", i, rr.String(), wantStrings[i]) | ||
| } | ||
|
|
||
| t.Logf("%s", rr) |
There was a problem hiding this comment.
remove debugging left-over?
There was a problem hiding this comment.
remove debugging left-over?
@stapelberg , 👍 will do prior to removing WIP.
| @@ -0,0 +1,3 @@ | |||
| go test ./... | |||
There was a problem hiding this comment.
Can you remove this file from the commit?
Making testing a little easier is a good idea in general, but I’d like to avoid establishing precedent of shell scripts :)
Maybe a “make test” target in a Makefile in a separate PR would be the best way forward?
There was a problem hiding this comment.
Can you remove this file from the commit?
@stapelberg , 👍 will do prior to removing WIP.
Maybe a “make test” target in a Makefile in a separate PR would be the best way forward?
@stapelberg , that would be nice! I agree.
| NFT_STOLEN = 2 | ||
| NFT_QUEUE = 3 | ||
| NFT_REPEAT = 4 | ||
| NFT_STOP = 5 |
There was a problem hiding this comment.
After having another look, I think this duplicates the verdict codes we already have defined here, no?
Lines 39 to 54 in c25e4f6
Why not just use e.g. VerdictReturn in the switch statement below in String()?
| var v string | ||
| switch e.Kind { | ||
| case unix.NFT_RETURN: | ||
| v = "return" // -0x5 |
There was a problem hiding this comment.
If you follow my other suggestion above regarding using the Verdict* names, I think it’d be cleaner to not duplicate these values (we only want a single place to update, not multiple). If people are inclined to find out the value, they should jump to the definition using their editor.
|
@greenpau are you still planning to get this PR merged? Just started playing with this library and your branch was super helpful for debugging 🙏 |
Before this commit: the printing of a rule results in
a pointer address.
After this commit: the printing of a rules results in
a human-readable text.
Resolves: #104
Signed-off-by: Paul Greenberg [email protected]