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

dnsdist: Change dnsdist stats functions to always return lowercase names #5383

Merged
merged 1 commit into from Jun 21, 2017

Conversation

@RobinGeuze
Copy link
Contributor

@RobinGeuze RobinGeuze commented Jun 6, 2017

Short description

Changes getTopQueries and getGenResponses to return lowercase versions for all names. Closes #5287

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
@@ -158,7 +158,7 @@ std::unordered_map<int, vector<boost::variant<string,double>>> getGenResponses(u
vector<pair<int, DNSName>> rcounts;
rcounts.reserve(counts.size());
for(const auto& c : counts)
rcounts.push_back(make_pair(c.second, c.first));
rcounts.push_back(make_pair(c.second, c.first.makeLowerCase()));

This comment has been minimized.

@rgacogne

rgacogne Jun 7, 2017
Member

I think you need to lowercase earlier otherwise the counts are going to be off.

This comment has been minimized.

@RobinGeuze

RobinGeuze Jun 7, 2017
Author Contributor

Nope since it uses maps, which uses the default compare and less operators, which in turn lowercase everything themselves.

This comment has been minimized.

@rgacogne

rgacogne Jun 7, 2017
Member

Oh, you are right, my bad!

This comment has been minimized.

@rgacogne

rgacogne Jun 7, 2017
Member

I wonder if it wouldn't be better to lowercase a bit later then, replacing rc.second.toString() by something like rc.second.makeLowerCase().toString(), so we only do the lowercasing for the names we are going to print, not all of them.

@@ -1351,7 +1351,7 @@ vector<std::function<void(void)>> setupLua(bool client, const std::string& confi
vector<pair<int, DNSName>> rcounts;
rcounts.reserve(counts.size());
for(const auto& c : counts)
rcounts.push_back(make_pair(c.second, c.first));
rcounts.push_back(make_pair(c.second, c.first.makeLowerCase()));

This comment has been minimized.

@rgacogne

rgacogne Jun 7, 2017
Member

Same thing here.

This comment has been minimized.

@RobinGeuze

RobinGeuze Jun 7, 2017
Author Contributor

As above.

@rgacogne rgacogne merged commit f770bbf into PowerDNS:master Jun 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants