rpcd-mod-rrdns: add neighbors cache implementation#8189
rpcd-mod-rrdns: add neighbors cache implementation#8189Konstantin-Glukhov wants to merge 1 commit intoopenwrt:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
a0cfadf to
3cb6323
Compare
This comment has been minimized.
This comment has been minimized.
|
So this is changing the behavior of an existing function? If so, did you check that all the existing clients of this function want this behavior? Any downsides? |
|
Not touching this until all tests pass. Otherwise it looks useful. |
|
I don't know what's wrong with the signoff test. First it did not like my github email, I fixed it, then it does not like it that it does not match the original github email. What's up with that? Signed-off-by: Konstantin Glukhov KGlukhov@Hotmail.com [fail] |
It does not change the behavior of the existing function, it only enhances it by extending the domain of names to be returned. |
Please help me to pass the sign off test, I don't understand how to fix it. |
That is a change in behavior... what if the original client did not want it enhanced? Normal way to do it would be either verify that this is wanted behavior for all clients or define a new function/add a parameter to make sure you do not break something. So are there any downsides? |
I don't see any downside. The purpose of this module is to provide a reverse name resolution. I don't even understand your point of how not providing a name resolution in some cases is a change of behavior. If a module provides name resolutions for the cases it could not provide before, how is it a change in behavior? |
So if the calling program e.g. only expects dns results but now gets results from other (unverified?) sources then that clearly is a change in behavior. Downsides could be more memory usage which on certain devices may not be a good thing, could be speed of providing the answer, it could be that the answer it provides is now outdated etc. But I don't know, I did look into it, hence I was asking... |
If /etc/ethers is empty (be default) there is no impact at all. If a user has an entry in /etc/ethers there will be an attempt to resolve the name by looking into the neighbors table. Memory wise, in storage the module requires 4K more (12K vs 8K). |
Once you've updated your github config - you'll need to amend your commit, so the author and committer match. Then force push. |
|
I just did a quick search seem 3 apps/mods use network.rrdns via a ubus call (as present in the json file). Two of them (including one I made), use the hosthints rpcd to lookup already. So would that then not end up with partially double functionality? |
|
Maybe to add: if your goal is to makes the items in /etc/ethers show up in luc-mod-status then maybe its "cheaper" (in terms of resources) to adjust that? |
Are you thinking about something like this? In my opinion it is way more complicated than a single c-program approach:
If there is a concern about rrdns returning names not from dns lookups, how about making it optional via |
0ebb9f6 to
279d2db
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f20b43c to
a93b41d
Compare
|
@Alphix - thoughts on this? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
its already implemented in a c program: rpcd-mod-luci https://github.com/openwrt/luci/blob/master/libs/rpcd-mod-luci/src/luci.c |
a93b41d to
fe7355d
Compare
This comment has been minimized.
This comment has been minimized.
You need to fix your github email config to match. |
fe7355d to
18fa456
Compare
Yes, this is getting funny :-) I already changed my email in GitHub Settings and in .git/config, did a couple of amends after this, and it still has no effect. I have a theory that my anonymous email was somehow saved in the commit with my original --signof. I am going to do one more time: and if this fails then I am at a total loss. |
This comment has been minimized.
This comment has been minimized.
I think I finally figured out. It was the author of the commit that had be updated, not the email config. |
18fa456 to
aba16c4
Compare
This comment has been minimized.
This comment has been minimized.
Add a neighbors cache and integration for the rrdns RPC module
so IP lookups can fall back to local neighbor/ethers names when
no DNS PTR is returned.
Files changed:
- .gitignore: Added .vscode
- libs/rpcd-mod-rrdns/Makefile: Updated the package version
- libs/rpcd-mod-rrdns/src/CMakeLists.txt: Linked new neighbor.c into build.
- libs/rpcd-mod-rrdns/src/neighbor.h: New header for neighbor cache.
- Declares rrdns_neighbors_cache_init, rrdns_neighbors_cache_clear, and
neighbor_name, plus AVL-backed structs for IP/LLADDR caches.
- libs/rpcd-mod-rrdns/src/neighbor.c: New code for neighbor cache.
- Maintains two AVL caches (ipaddr and lladdr) using libubox/avl.
- Loads static MAC→name mappings from /etc/ethers.
- Queries kernel neighbors via NETLINK (RTM_GETNEIGH) to map IP→LLADDR
and LLADDR→name.
- Exposes neighbor_name(int family, const void *addr) returning a
cached/display name (caller-owned string).
- Provides cache init/clear functions and logs low-memory conditions.
- libs/rpcd-mod-rrdns/src/rrdns.c: Integration with rrdns.
- #include "neighbor.h".
- If no PTR records are found, call neighbor_name() as a fallback.
- Initialize neighbor cache in rpc_rrdns_lookup() and clear it in
rdns_shutdown().
- Minor reordering: avl_find() dedupe check moved earlier in
rrdns_next_query().
Behavioral impact:
- If no DNS PTR records are returned, rrdns will attempt to resolve
a human-friendly name via local neighbor information (kernel
neighbor table + /etc/ethers) and return that as a fallback.
- Adds runtime caching to avoid repeated lookups; cache cleared on
shutdown or when /etc/ethers mtime changes.
- Emits syslog warnings on memory allocation failures.
Signed-off-by: Konstantin Glukhov <KGlukhov@Hotmail.com>
aba16c4 to
53121f6
Compare
I’m not sure what is meant by “it’s already implemented.” After reviewing Specifically,
The resulting data structure is effectively: At this stage, the data is still organized primarily by MAC address, not by IP address. Resolving a hostname for a given IP therefore requires iterating over the MAC AVL tree and then over the associated IPv4/IPv6 subtrees. This traversal logic does not appear to be implemented in Note: There may be a minor precedence issue in step 5. Reverse DNS lookups are performed even when a hostname is already defined via |
|
The thing is the ethers file is used to connect a Mac to an ip (or hostname) . It is not meant to resolve host names to ip or the other way around. Dns is for doing names to ip and the other way around. If you want to add static items you do that in the hosts file or dhcp lease file, not the ethers file. So i really do not think that you want to start mixing this. But if you really want to put it in the ethers file then there is already code to scan the ether file. Please don't put similar code in multiple locations. And even worse, don't make it such that those duplicated codes run, butt alas that is exactly what will happen for 2 out of 3 use cases. Doing a lookup is a few lines of code and if you are worried about performance then note that it is running client side, and you could even invert the gathered table and cache it. So all of this will not impact the AP or routers performance or memory usage. You then only need to modify 1 component, maintenance is easy, using existing interfaces, performance is excellent as it runs client side etc |
|
(got an error and then posted the comment twice) |
Comments can be edited. |
There are no formal restrictions or written rules that limit how /etc/ethers can be used. Given that SLAAC IPv6 addresses are not registered in DNS , it is a logical option to use /etc/ethers to map node names via MAC address. I managed to implement this mapping in connections.js - it was not that complicated. I will submit a separate pull request for connections.js and will close this one. Thank you for your feedback and guidance. |
|
Extending rrdns with |
Add a neighbors cache and integration for the rrdns RPC module so IP lookups can fall back to local neighbor/ethers names when no DNS PTR is returned.
Files changed:
Behavioral impact:
If no DNS PTR records are returned, rrdns will attempt to resolve a human-friendly name via local neighbor information (kernel neighbor table + /etc/ethers) and return that as a fallback.
Adds runtime caching to avoid repeated lookups; cache cleared on shutdown or when /etc/ethers mtime changes.
Emits syslog warnings on memory allocation failures.
This PR is not from my main or master branch 💩, but a separate branch ✅
Each commit has a valid ✒️
Signed-off-by: <my@email.address>row (viagit commit --signoff)Each commit and PR title has a valid 📝
<package name>: titlefirst line subject for packagesIncremented 🆙 any
PKG_VERSIONin the MakefileTested on: (architecture: ARMv7, OpenWrt 25.12.0-rc1, browser: Chrome) ✅