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

DNSName: correct len and offset types #13723

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

chbruyand
Copy link
Member

@chbruyand chbruyand commented Jan 18, 2024

Short description

This PR format dnsname.{hh,cc} files and corrects the type of len and offset arguments from int to size_t.

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)
  • checked that this code was merged to master

pdns/dnsname.hh Fixed Show fixed Hide fixed
pdns/dnsname.hh Fixed Show fixed Hide fixed
@coveralls
Copy link

coveralls commented Jan 18, 2024

Pull Request Test Coverage Report for Build 7623113506

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 56 of 61 (91.8%) changed or added relevant lines in 1 file are covered.
  • 947 unchanged lines in 22 files lost coverage.
  • Overall coverage decreased (-0.04%) to 57.674%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/dnsname.cc 56 61 91.8%
Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/dnsdist-console.hh 1 0.0%
pdns/pollmplexer.cc 1 82.39%
ext/luawrapper/include/LuaContext.hpp 2 13.77%
pdns/dnsname.cc 4 92.03%
pdns/misc.cc 4 62.51%
pdns/dnssecsigner.cc 5 96.49%
pdns/signingpipe.cc 5 86.02%
pdns/dnsdist-carbon.cc 6 59.34%
pdns/dnsdist-tcp.cc 8 75.9%
Totals Coverage Status
Change from base Build 7580951414: -0.04%
Covered Lines: 107823
Relevant Lines: 155564

💛 - Coveralls

@chbruyand chbruyand force-pushed the dnsname-position-types branch 2 times, most recently from d83edfb to bdc207c Compare January 18, 2024 16:55
@rgacogne
Copy link
Member

I'm wondering if it would be better to move the formatting of the files to a different pull request, so that we can focus on the (hopefully) non-impact of the type changes in this PR? In theory it should not matter but we get clang-tidy warnings for the whole PR, not per commit, so it feels like this PR is changing a lot of things when it isn't.

@chbruyand
Copy link
Member Author

I'm wondering if it would be better to move the formatting of the files to a different pull request, so that we can focus on the (hopefully) non-impact of the type changes in this PR? In theory it should not matter but we get clang-tidy warnings for the whole PR, not per commit, so it feels like this PR is changing a lot of things when it isn't.

Agreed. I'll also stick to not format/clean anything not required.

@chbruyand chbruyand force-pushed the dnsname-position-types branch from bdc207c to 7b3df79 Compare January 19, 2024 08:29
@chbruyand chbruyand force-pushed the dnsname-position-types branch from 7b3df79 to cce0983 Compare January 19, 2024 08:32
@chbruyand chbruyand force-pushed the dnsname-position-types branch from 1ebcf92 to 05fe745 Compare January 19, 2024 09:57
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

The change looks good to me, there is no valid reason for the length or the offset to be negative. Of course there is a small risk than an existing piece of code relies on the existing behaviour, but then I think it's a bug that needs to be fixed.
I just pushed a commit to a throw-away branch (https://github.com/rgacogne/pdns/tree/dnsname-test-negative) that aborts if the length or the offset is negative, let's see if it passes our CI.

@chbruyand chbruyand changed the title DNSName: format files and correct len and offset types DNSName: correct len and offset types Jan 22, 2024
@rgacogne
Copy link
Member

Of course there is a small risk than an existing piece of code relies on the existing behaviour, but then I think it's a bug that needs to be fixed. I just pushed a commit to a throw-away branch (https://github.com/rgacogne/pdns/tree/dnsname-test-negative) that aborts if the length or the offset is negative, let's see if it passes our CI.

The CI run was successful!

I would not mind at least one other review before merging this PR, though :-)

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

One nit

pdns/dnsname.cc Outdated Show resolved Hide resolved
@rgacogne rgacogne merged commit 9b4a83a into PowerDNS:master Jan 23, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants