-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add an LLMNR responder implementation #2880
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2880 +/- ##
=======================================
Coverage 27.82% 27.82%
=======================================
Files 20 20
Lines 3626 3626
Branches 656 656
=======================================
Hits 1009 1009
Misses 2441 2441
Partials 176 176 Continue to review full report at Codecov.
|
Thanks for contributing this library! |
I don't expect to make many more changes to the code; it seems to work as-is. Of course, there are some violations of the RFC (unimplemented features), but they shouldn't cause problems in practice. I don't know if anyone else will implement them. I'd guess that this library would see roughly the same rate of changes as the existing MDNS code, since it does roughly the same thing. About the only changes I might guess at are:
I see an advantage of keeping this library with all the other ESP8266 libraries. It's easier for people to find and install just one extra thing in the Arduino IDE, rather than having to track down each library separately. It also makes it easier to test all the libraries and core ESP8266/SDK code together since everything is part of the same release, so there's no mixing/matching versions. Honestly, I'm not really looking to pick up maintenance of anything much. As I'm sure you're aware, that rather takes time away from doing other stuff. I'm happy to answer questions or fix issues if they come up though, to keep the code working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of things I noticed when I was about to merge...
_hostname = hostname; | ||
_hostname.toLowerCase(); | ||
|
||
WiFi.onStationModeGotIP([this](const WiFiEventStationModeGotIP& event){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to save the return value as a member variable of the class. The callback lifetime is controlled by the returned EventHandler
value.
Same thing a few lines below.
_conn = new UdpContext; | ||
_conn->ref(); | ||
|
||
if (!_conn->listen(*IP_ADDR_ANY, LLMNR_PORT)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a minor thing, but in some other places you put braces around one-line if bodies. Suggest doing the same thing everywhere.
This is a simple implementation of an LLMNR responder for the ESP8266 Arduino package. Only support for advertizing a single hostname is currently implemented. LLMNR is a very similar protocol to MDNS. The primary practical difference is that Windows systems (at least Windows 7 and later; perhaps earlier) support the protocol out-of-the-box, whereas additional software is required to support MDNS. However, Linux support is currently more complex, and MacOS X support appears non-existent.
New version uploaded to address the review comments, thanks. |
Great, thanks! |
This is a simple implementation of an LLMNR responder for the ESP8266
Arduino package. Only support for advertizing a single hostname is
currently implemented.
LLMNR is a very similar protocol to MDNS. The primary practical
difference is that Windows systems (at least Windows 7 and later;
perhaps earlier) support the protocol out-of-the-box, whereas additional
software is required to support MDNS. However, Linux support is
currently more complex, and MacOS X support appears non-existent.