Fix implicit conversion warning in DSR with GENEVE #25299
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
|
/test |
|
/test-runtime |
|
I suppose the alternative would be to have the agent emit |
I thought it needed to be uint32 for the ELF templating, but it isn't the case anymore? |
It's all a bit magic to me 😄. Looks like #25195 just improved this area. But don't ask me why it required the Either way - it would be cool if we could handle this sort of value clamping in one location (ie. the agent), instead of potentially duplicating it across multiple locations in the datapath. |
|
Hmm, this is, lets call it interesting. So for devices with L2, There are also plenty of location where it is used as in |
|
Tested diff --git a/bpf/bpf_host.c b/bpf/bpf_host.c
index 06c1614807..375d233e5d 100644
--- a/bpf/bpf_host.c
+++ b/bpf/bpf_host.c
@@ -1234,6 +1234,9 @@ int cil_from_netdev(struct __ctx_buff *ctx)
#endif
#endif
+#ifdef DEBUG
+ printk("ETH_HLEN = %u\n", ETH_HLEN);
+#endif
/* Filter allowed vlan id's and pass them back to kernel.
* We will see the packet again in from-netdev@eth0.vlanXXX.
*/
diff --git a/pkg/datapath/linux/config/config.go b/pkg/datapath/linux/config/config.go
index 8aed3d379b..c199cf9eaf 100644
--- a/pkg/datapath/linux/config/config.go
+++ b/pkg/datapath/linux/config/config.go
@@ -816,7 +816,7 @@ func (h *HeaderfileWriter) writeStaticData(fw io.Writer, e datapath.EndpointConf
// Use templating for ETH_HLEN only if there is any L2-less device
if !mac.HaveMACAddrs(option.Config.GetDevices()) {
// L2 hdr len (for L2-less devices it will be replaced with "0")
- fmt.Fprint(fw, defineUint32("ETH_HLEN", mac.EthHdrLen))
+ fmt.Fprint(fw, defineUint16("ETH_HLEN", mac.EthHdrLen))
}
} else {
// We want to ensure that the template BPF program always has "LXC_IP"bpftool prog tracelog |
And this did not cause type comparison errors/warnings in bpf_lxc.c and bpf_host.c? |
ETH_HLEN using defineUint16() didn't cause the type comparison errors/warnings and it's working normally, as far as I checked it in my env. |
@julianwiedmann so I guess in that case I had a quick look, but always having ETH_HLEN in global data would require way more changes, so I will drop that suggestion. @julianwiedmann if you agree, I think that should be the path forward for this PR |
Yep feels like the smoothest approach. It's not like we would actually support |
ysksuzuki
force-pushed
the
fix-geneve-with-l2-less
branch
from
2f6137e
to
f1075c4
Compare
|
/test |
1 similar comment
|
/test |
|
The Jenkins job status isn't reported |
ysksuzuki
force-pushed
the
fix-geneve-with-l2-less
branch
from
f1075c4
to
f05a085
Compare
Currently, ETH_HLEN is explicitly defined as __u32 if there's any L2-less device, and it causes implicit conversion loses integer precision warning when we calculate encap_len using ETH_HLEN. This commit fixes the warning by defining ETH_HELN as __u16. Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>
|
/test |
dylandreimerink
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
Currently, ETH_HLEN is explicitly defined as __u32 if there's any L2-less device, and it causes implicit conversion loses integer precision warning when we calculate encap_len using ETH_HLEN. This commit fixes the warning by defining ETH_HELN as __u16.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #25238