-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Move resolution of client and upstream host names to a dedicated thread #405
Conversation
…ad. Fixes #404 Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
resolve.c
Outdated
// to account for new clients (and forward destinations) | ||
void resolveNewClients(void) | ||
// Resolve upstream destination host names | ||
static void resolveForwardDestinations(bool onlynew) |
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.
We have the capability to resolve all forward destinations instead of just new ones, but we don't use it currently. When all clients are re-resolved, we should also re-resolve forward destination names.
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.
I wonder if this is really meaningful, as we don't expect the forward destination host names to ever change, do we? In contrast, users may decide to rename their local devices, e.g., from android-a479b97ef09880b890d233a
to paul-iphone
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.
Perhaps if the upstream is localhost, and the hostname changes? It may not be worth implementing, but on the other hand, it is a very small cost (usually only 1 to 3 upstreams).
resolve.c
Outdated
void *resolver_thread(void *val) | ||
{ | ||
// Set thread name | ||
prctl(PR_SET_NAME,"resolver",0,0,0); |
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.
This may get confused with DNS resolver.
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.
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.
To me, I see in that list the jobs of FTL listed out. It resolves domains on the resolver thread, does database stuff on the database thread, listens to sockets and telnet on their respective threads, and does some housekeeping on the housekeeper thread. I don't see any other thread that looks like "DNS" or "dnsmasq", so the resolver thread seems to be doing that job. A better name might be "internal resolver".
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.
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.
another possibility might be DNS client
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.
DNS client sounds fine.
… destinations Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
resolve.c
Outdated
for(i = 0; i < counters.forwarded; i++) | ||
} | ||
|
||
pthread_t resolverthread; |
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.
Can this be moved to where the other threads are defined (main)?
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.
They are all defined in dnsmasq_interface.c
(close to where the threads are launched). I'll move the definition there.
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.
Ah ya, sorry, thought that was main for a second (we used to launch the threads from there).
dnsmasq_interface.c
Outdated
// Start thread that will stay in the background until host names needs to be resolved | ||
if(pthread_create( &resolverthread, &attr, resolver_thread, NULL ) != 0) | ||
{ | ||
logg("Unable to open resolver thread. Exiting..."); |
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.
As the thread was renamed to DNS client, rename this reference.
FTL.h
Outdated
@@ -279,3 +279,4 @@ extern pthread_t telnet_listenthreadv6; | |||
extern pthread_t socket_listenthread; | |||
extern pthread_t DBthread; | |||
extern pthread_t GCthread; | |||
extern pthread_t resolverthread; |
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.
As the thread was renamed to DNS client, rename this variable.
resolve.c
Outdated
} | ||
|
||
pthread_t resolverthread; | ||
void *resolver_thread(void *val) |
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.
As the thread was renamed to DNS client, rename this function.
Signed-off-by: DL6ER <dl6er@dl6er.de>
resolve.c
Outdated
if(strlen(hostname) > 0) | ||
// If onlynew flag is set, we will only resolve new clients | ||
// If not, we will try to re-resolve all known clients | ||
if(onlynew && clients[i].new) |
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.
If onlynew
is false, no resolving will occur at all.
resolve.c
Outdated
if(clients[i].new) | ||
// If onlynew flag is set, we will only resolve new upstream destinations | ||
// If not, we will try to re-resolve all known upstream destinations | ||
if(onlynew && forwarded[i].new) |
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.
See above comment.
Signed-off-by: DL6ER <dl6er@dl6er.de>
…T new, we skip this one. In all other cases, we try to resolve. Signed-off-by: DL6ER <dl6er@dl6er.de>
By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:
Please submit all pull requests against the
development
branch. Failure to do so will delay or deny your requestHow familiar are you with the codebase?:
10
Move resolution of client and upstream host names to a dedicated thread. This PR fixes #404
This template was created based on the work of
udemy-dl
.