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

Add DNS forwarder to DNSServer #7237

Merged
merged 39 commits into from
May 8, 2022
Merged

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Apr 22, 2020

The key functions added are:

bool enableForwarder(const String &domainName=emptyString, const IPAddress &dns=uint32_t)0)

If specified, enableForwarder will update the domainName that is used to match DNS request to this AP's IP Address. A non-matching request will be forwarded to the DNS server specified by dns.

Returns true on success.

Returns false,

  • when forwarding dns is not set, or
  • unable to allocate resources for managing the DNS forward function.

void disableForwarder(const String &domainName=emptyString, bool freeResources=false)

disableForwarder will stop forwarding DNS requests. If specified, updates the domainName that is matched for returning this AP's IP Address.
Optionally, resources used for the DNS forward function can be freed.

@d-a-v d-a-v added the alpha included in alpha release label Jul 16, 2020
@d-a-v
Copy link
Collaborator

d-a-v commented Jul 17, 2020

Is it expensive in RAM when it is enabled ?
Is it always the same price, even when STA is not enabled ?
I guess it isn't useful when NAPT is not enabled.

Those questions because I wonder what is against enabling it by default, and its impact on sketch that are not using STA.

@mhightower83
Copy link
Contributor Author

The forwarder queue size is a power of 2. It is currently at 8 items, total size of 96 bytes.

I don't think an STA configured, enters directly into the DNSForwarder resource requirements.

As you indicated it isn't very useful outside of using NAPT. From time to time I see complaints about the slowly increasing of resources with each release, so I tried to make it resource flexible. Also as it is written, it is easy to increase or decrease the queue size. It is a simple constant in the .h. However, for me, 8 items seemed to work well with one host browsing. With a smaller value, I saw more dropped requests.

@mhightower83
Copy link
Contributor Author

I don't think an STA configured, enters directly into the DNSForwarder resource requirements.

Sorry, I wasn't thinking that all the way through. W/O STA there is no need for the forwarder.

libraries/DNSServer/src/DNSServer.h Outdated Show resolved Hide resolved
libraries/DNSServer/src/DNSServer.h Outdated Show resolved Hide resolved
@@ -0,0 +1,330 @@
#ifndef WIFIHTTP_H_
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not .ino? just to avoid confusion with static things in headers and conditional typos
configHead, configHeadMin, and etc.
(although, I am coming from the external language server experience, perhaps it is easier in the current arduino IDE environment to work with files like this)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and also, why not a single .ino entrypoint, with .cpp actually implementing things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and also, why not a single .ino entrypoint, with .cpp actually implementing things?

I don't think I follow this point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and also, why not a single .ino entrypoint, with .cpp actually implementing things?

I don't think I follow this point.

Only keeping NAPTCaptivePortal.ino for setup() & loop(), for the sole purpose of avoiding ide concatenation of .ino files. Just a consideration, as I noted I may be out-of-loop and this is actually easier to manage within IDE

removed typedef

Changed run_CI_locally.sh to run ci/style_check.sh.
Copied resulting style changes from /tmp to PR
Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Just tried with a couple of devices, all seems to work like intended

libraries/DNSServer/src/DNSServer.cpp Outdated Show resolved Hide resolved
libraries/DNSServer/src/DNSServer.h Outdated Show resolved Hide resolved
DNSReplyCode _errorReplyCode;
bool _forwarder;
unsigned char _resolvedIP[4];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw shouldn't this be IPAddress?

removed stale #define
add const
remove struct prefix
@mcspr mcspr merged commit bcb5464 into esp8266:master May 8, 2022
@mcspr mcspr added this to the 3.1 milestone May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha included in alpha release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants