Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-app-usteer: added nslookup of AP name #6829

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Ramon00
Copy link
Contributor

@Ramon00 Ramon00 commented Jan 12, 2024

added nslookup of AP name

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 13, 2024

if anybody can test, that would be great

@McGiverGim
Copy link
Contributor

Tested but it seems not to work if I enable to use IPv6 in the config.
I have observed that the Hosts variable contains both, the IPv4 and the IPv6 of the hosts, for example, this are my two APs:
image

Maybe you can search for the IP at the Hosts variable?

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 15, 2024

I actually did not test IPv6. Seems the issue is that it uses a link local address, which does not appear to resolve with an nslookup. I can manually search the hosthints but well that is kinda inefficients if there is a large list... So hmmm

@McGiverGim
Copy link
Contributor

It's not similar to the search for mac, ip, name, etc. of each device?

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 15, 2024

All the other lookups are just indexed by mac address. Another issue is that all the link local addresses of the other APs are only available on the router (at least not on my setup), while the IPv4 can be made availble elsewhere via dnsmasq. So im not sure how to proceed. I could do the one for IPv4 and the other for IPv6 i suppose

@McGiverGim
Copy link
Contributor

If performance is a problem, maybe you can create a new global variable starting from Hosts and indexing from IP?

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 15, 2024

To be honest I dont know if that is a problem, not in my setup, but not sure how it scales... Anyway I added a search in the hosthints. Give it a try

@McGiverGim
Copy link
Contributor

Give it a try

Working perfectly!! In my case only two APs, and about 15 devices in hearing map, so I can't speak about performance. It works fast in my case.

Maybe some people want to have the MAC too, not only the name. To me is enough like it is now, but only a suggestion.

@McGiverGim
Copy link
Contributor

I suppose in the worst case, if someone complains for performance, exists the possibility to add a check in the hearing tab. When checked, do the translation, if not let the mac alone.

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 16, 2024

about 15 devices in hearing map
Its about the amount of APs times the amount of IP address in the host hints. Anyway, the way it is implemented now is that it only does it one time the page is loaded, not every refresh. So I think it should be fine.

@Ramon00 Ramon00 closed this Jan 16, 2024
@Ramon00 Ramon00 reopened this Jan 16, 2024
@Ramon00 Ramon00 marked this pull request as ready for review January 16, 2024 17:45
@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 19, 2024

@systemcrash Could you do a review of the code and merge if ok?

@systemcrash
Copy link
Contributor

I don't use usteer, but the code largely looks acceptable. @McGiverGim seems to verify its stability.

Performance we can iterate on.

@systemcrash
Copy link
Contributor

So while I'm not a big fan of global variables like var dns_cache = [];, the only other way around it is variable passing. Not a deal breaker. Everything works for you as intended, yes?

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 27, 2024

yes everything works as intended

@systemcrash systemcrash merged commit 7a9ae68 into openwrt:master Jan 29, 2024
2 checks passed
@systemcrash
Copy link
Contributor

OK - thank you :)

@Ramon00
Copy link
Contributor Author

Ramon00 commented Jan 29, 2024

@systemcrash And thank you :)

@Ramon00 Ramon00 deleted the luci-app-usteer branch March 31, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants