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

auth: encapsulate lookup()-cleanup #14597

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

zeha
Copy link
Collaborator

@zeha zeha commented Aug 27, 2024

Short description

Encapsulate this pattern into a new function:

while (B.get(rr)) ;      // don't leave DB handle in bad state

I've tentatively called the function lookupEnd.

This is a tiny improvement and probably opens a can of clang-tidy worms. I'll preemptively say that a solution which would instead remove the need to explicitly call this would be better, but that can still come.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@coveralls
Copy link

coveralls commented Aug 27, 2024

Pull Request Test Coverage Report for Build 10593278659

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 9 of 13 (69.23%) changed or added relevant lines in 2 files are covered.
  • 59 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.02%) to 64.69%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/ueberbackend.cc 8 9 88.89%
pdns/packethandler.cc 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
ext/json11/json11.cpp 1 64.49%
pdns/pollmplexer.cc 1 83.66%
pdns/recursordist/sortlist.cc 2 72.94%
pdns/recursordist/aggressive_nsec.cc 2 66.17%
pdns/sstuff.hh 2 56.83%
pdns/recursordist/syncres.cc 2 79.35%
pdns/stubresolver.cc 3 77.58%
pdns/dnsdistdist/dnsdist-tcp.cc 4 76.18%
pdns/signingpipe.cc 5 85.75%
pdns/misc.cc 5 63.26%
Totals Coverage Status
Change from base Build 10593239165: 0.02%
Covered Lines: 124686
Relevant Lines: 162063

💛 - Coveralls

@zeha zeha marked this pull request as ready for review August 27, 2024 18:53
@zeha
Copy link
Collaborator Author

zeha commented Aug 27, 2024

opens a can of clang-tidy worms

No worms showed up. 😮

@Habbie
Copy link
Member

Habbie commented Aug 28, 2024

I'll preemptively say that a solution which would instead remove the need to explicitly call this would be better, but that can still come.

It has seemed obvious for quite some time now that having lookup() just fill a vector likely makes more sense. Some backends already do this internally.

@Habbie Habbie closed this Aug 28, 2024
@Habbie Habbie reopened this Aug 28, 2024
@zeha
Copy link
Collaborator Author

zeha commented Aug 28, 2024

I'll preemptively say that a solution which would instead remove the need to explicitly call this would be better, but that can still come.

It has seemed obvious for quite some time now that having lookup() just fill a vector likely makes more sense. Some backends already do this internally.

Ah yes, I was pondering this. Can take a look sometime; might still be nice to merge this in the meantime.

@Habbie
Copy link
Member

Habbie commented Aug 28, 2024

might still be nice to merge this in the meantime.

will do when green!

@Habbie Habbie merged commit aa4e367 into PowerDNS:master Aug 28, 2024
156 of 157 checks passed
@zeha zeha deleted the zeha-auth-backend-state branch August 28, 2024 13:11
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