Add netlink support to if_gre
Details
Details
I also wrote tests for gre, since it doesn't have it before.
# kyua test -k /usr/tests/sys/netlink/Kyuafile test_rtnl_gre:test_rtnl_gre test_rtnl_gre:test_rtnl_gre -> passed [0.002s] Results file id is usr_tests_sys_netlink.20260101-180800-774463 Results saved to /root/.kyua/store/results.usr_tests_sys_netlink.20260101-180800-774463.db 1/1 passed (0 failed)
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 70395 Build 67278: arc lint + arc unit
Event Timeline
Comment Actions
There're three tasks in this change,
- Migrate to new if_clone KPI
- Refactor some ioctls, say GRESKEY
- Add netlink support
The first two are simple and easy to review, I expect them can be landed quickly. I'd support you to split the change.
Comment Actions
I understand your perspective, however, migrating directly to the new if_clone_addreq_v2 without netlink support is not possible.
I could migrate to if_clone_addreq without netlink support first, but then transition to v2 to support netlink would ultimately be redundant.
The same reasoning applies to the GRESKEY ioctl.
Refactoring that ioctl is intended to reuse those configuration functions through netlink.
Therefore, without netlink support, refactoring the ioctl interface is pointless.
Comment Actions
- Add types in if_gre.h to fix world build
- Add copyright to test_rtnl_gre.c
- Rebase to main and fix cleanup in test_rtnl_gre.
@glebius done.
Comment Actions
Approved, but please address comment in the module event switch statement.
| sys/net/if_gre.c | ||
|---|---|---|
| 1146 | See note about /* FALLTHROUGH */ in style(9). This actually is not only about style. Some linters may complain on this function unless this special comment is added. Adding break; instead will also work. So add either of them, depending on how would you expect gremodevent() change in the future. IMHO, break; is more likely to stay here in future. | |
This comment was removed by pouria.
Comment Actions
I see no problems with this patch alongside my changes.
We have a minor conflict, which I resolved at github.
Also, AFAICU, I don't need to use priv instead of softc for netlink because I didn't touch the data plane.
Your code will synchronize priv and softc together so there should be no problem to directly using the softc.
Should I do anything else?
Comment Actions
@zlei
Can I have your opinion on this review, too?
I'm ready to commit it, but I'd prefer to wait for your feedback as well.
Comment Actions
Except for the tests, do you have real usage of the netlink protocol ? The commit message does not mention it. So it is better to explain, otherwise would confuse people.
I'm still learning netlink. @melifaro introduced netlink to FreeBSD and I believe he has better insight over the design.
| sys/net/if_gre.c | ||
|---|---|---|
| 408 | Naming is hard. I'd personally prefer to use prefix, say gre_nl_set_addr() other than suffix gre_set_addr_nl(), so that it will be easier to grep all netlink related function. | |
| 408 | Probably you do not want to interleave netlink related functions with domain specific functions. I'd suggest you move all netlink related functions down to the end of if_gre.c, just before gremodevent(). | |
| sys/net/if_gre.h | ||
| 165 | Naming. I think nl_gre_parsed reads more fluently than nl_parsed_gre. Well I'm not a native speaker, just my personal opinion. Will this be public KPI ? Probably this should be in sys/net/if_gre.c ? | |
| sys/netlink/route/interface.h | ||
| 251 | It appears these will be public KPI. Will we want to sync with Linux about the naming ? ( I have not looked at Linux sources about them, just presume it has. ) If roll our own types, what's the policy to assign type to existing tunnels ( me, gif, vxlan, etc ) ? Probably @melifaro had thoughts on this ( ever ). | |
| tests/sys/netlink/test_rtnl_gre.c | ||
| 70 | Small nits. Probably test_gre_nl or test_nl_gre is better ? test_rtnl_gre reads "test route netlink gre" to me. Get a little confused by the naming rt. | |
| sys/net/if_gre.c | ||
|---|---|---|
| 408 |
This suffix is already widely used inside the kernel. | |
| 408 |
Good point, I will. Thanks! | |
| sys/net/if_gre.h | ||
| 165 |
The reason for this is trying to match naming of other netlink parsers.
Yes it's, I'll add the support for ifconfig netlink implementation of gre interface in another review. | |
| sys/netlink/route/interface.h | ||
| 251 |
I have tried to find a middle ground between melifaro implementation and linux naming!
Currently, there is no policy. | |
| tests/sys/netlink/test_rtnl_gre.c | ||
| 70 |
I tried to match the current naming, especially test_rtnl_iface since it already has another test interface (if_vlan) inside it. | |
Comment Actions
Make nl_parsed_gre private.
@zlei done.
After doing my research, If find out you were right.
Dictating netlink data structure to userland defeats
the purpose of using netlink in first place.