Retrieve vlan tag id from skb, if present #417
Conversation
lneto
left a comment
lneto
left a comment
There was a problem hiding this comment.
I'm still in doubt if we should use mark instead, associating a luanetfilter object to a specific VLAN ID.. another approach would be to have more SKB related methods on luadata or even evolve to inheritance, having a luaskb binding.. (@sneaky-potato I think that's a good reflection for you ;-).
That said, I would try to run some tests on this and comparing this approach and the one I mentioned using marks. I'm making some inline comments anyway, so it will help at least as a exercise for integration on our code base =).
lib/luanetfilter.c
Outdated
| goto out; | ||
|
|
||
| if (skb_mac_header_was_set(skb)) | ||
| bool mac_header_was_set = skb_mac_header_was_set(skb); |
There was a problem hiding this comment.
we try to not have very long names..
| bool mac_header_was_set = skb_mac_header_was_set(skb); | |
| bool has_mac_header = skb_mac_header_was_set(skb); |
couldn't figure anything better =)
There was a problem hiding this comment.
the struggle :D
again, won't be needed (see below)
lib/luanetfilter.c
Outdated
| lua_pushnil(L); /* dev may be NULL if hook is LOCAL_OUT */ | ||
|
|
||
| if (lua_pcall(L, 2, 2, 0) != LUA_OK) { | ||
| /* work out vlan TAG id, if present */ |
There was a problem hiding this comment.
I would move the block below to an aux.. e.g., luanetfilter_pushvlanid
There was a problem hiding this comment.
If I ignore if vlan is not present, like you suggested below, then the change is very small, probably won't need a helper
lib/luanetfilter.c
Outdated
| /* work out vlan TAG id, if present */ | ||
| if (skb_vlan_tag_present(skb)) { | ||
| lua_pushinteger(L, skb_vlan_tag_get_id(skb)); | ||
| } else if (mac_header_was_set && (skb->protocol == htons(ETH_P_8021Q) || skb->protocol == htons(ETH_P_8021AD))) { |
There was a problem hiding this comment.
we use \n after }, on if-then-else
There was a problem hiding this comment.
btw, if tag is not present, wouldn't be safe to give up? why should we extract it by ourselves? if needed, we can do this logic directly in lua, right?
There was a problem hiding this comment.
That makes it more simple, I think it should be possible to retrieve from lua, similar to what we had in lldp example, for mac address.
I will push a change
lib/luanetfilter.c
Outdated
| if (veh) | ||
| lua_pushinteger(L, ntohs(veh->h_vlan_TCI) & VLAN_VID_MASK); | ||
| else | ||
| lua_pushnil(L); |
There was a problem hiding this comment.
we don't need to pass nil as the third arg.. we can pass none.. it's both more luaish and clearer, I think.. for this, we might use a narg var and increment it in the cases we push it..
| if (skb_vlan_tag_present(skb)) { | ||
| lua_pushinteger(L, skb_vlan_tag_get_id(skb)); | ||
| narg++; | ||
| } |
There was a problem hiding this comment.
We should use the same approach for dev, for consistency ;-)
There was a problem hiding this comment.
You mean, dropping narg and pushing nil if vlan not available?
There was a problem hiding this comment.
I meant on if (de) do narg++ as well and suppress pushnil otherwise
There was a problem hiding this comment.
We could even have an inline for that..
There was a problem hiding this comment.
But if we also make dev an optional arg, I think we'll have to push something so don't mess with expected arguments, on lua side?
There was a problem hiding this comment.
I think you're right, we can have VLAN tag present, but not dev, right? If so, I agree, it might be confusing. I'd leave at this indeed. However, I think we should plan to implement a luaskb binding, so we can move most of this logic to there.. I put some thoughts on this, we can just have a method there to create a luadata, something like skb:todata(). I think it should be straight forward to implement and a good way to get more familiarized with lunatik modules.. =)
|
@sheharyaar it would be good to have your review before merging it.. |
Added a way of retrieving the vlan ID from
skbTried to make it support non-linear skb and it is idiomatic for kernel 6.x ATM
However I wasn't able to fully test in a vlan network (working on that) but I think it is safe to PR this and get some reviews in the meantime.