Skip to content

auth lmdb: full support for comments#16522

Draft
Habbie wants to merge 35 commits intoPowerDNS:masterfrom
Habbie:lmdb-full-comments
Draft

auth lmdb: full support for comments#16522
Habbie wants to merge 35 commits intoPowerDNS:masterfrom
Habbie:lmdb-full-comments

Conversation

@Habbie
Copy link
Member

@Habbie Habbie commented Nov 17, 2025

Short description

WIP. TODO:

  • comment search
  • comment sorting? (by timestamp) - but we already group by RRset, so there's not much sorting to be done really
  • schema upgrade
  • better serialisation (need something stable/canonical; looks like protozero might be it. Alternatively, have my -hash- depend on something simple and stable while the serialisation is at least robust)
  • CAP flag?
  • squashing
  • fix pdnsutil comment add example.com foobar A 'hello protobuf' petre -> Error: std::string keyConv(const T&) [with T = DNSName; typename std::enable_if<std::is_same<T, DNSName>::value, T>::type* <anonymous> = 0; std::string = std::__cxx11::basic_string<char>] Attempt to serialize an unset DNSName

Checklist

I have:

  • read the CONTRIBUTING.md document
  • read and accepted the Developer Certificate of Origin document, including the AI Policy, and added a "Signed-off-by" to my commits
  • 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


bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key)
{
// FIXME: bit of duplication from below here, but much simpler

Check notice

Code scanning / CodeQL

FIXME comment

FIXME comment: bit of duplication from below here, but much simpler
// whether to include disabled records in the results
bool includedisabled;
// whether we are doing comments (false=records, true=comments)
bool comments;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe prefer putting a pointer to the *dbi to use here rather than a bool? Might not be as simple as it looks.

Copy link
Member Author

Choose a reason for hiding this comment

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

then we'd have to compare dbi pointers in getInternal, because the logic is different for comments vs. records (which I am unhappy about but I haven't found a clean refactor for it yet)

"\tUnpublish the zone key with key id KEY_ID in ZONE"}}}
};

static const groupCommandDispatcher commentCommands{
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this would be part of the rrset commands. As in rrset add-comment, rrset list-comment, rrset forget-comment, rrset why-the-hell-did-I-forget-to-put-a-comment, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes perfect sense

@coveralls
Copy link

coveralls commented Nov 19, 2025

Pull Request Test Coverage Report for Build 19674913787

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

  • 136 of 150 (90.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on lmdb-full-comments at 73.078%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modules/lmdbbackend/lmdbbackend.cc 117 122 95.9%
pdns/dnsbackend.cc 19 28 67.86%
Totals Coverage Status
Change from base Build 19426528573: 73.1%
Covered Lines: 128116
Relevant Lines: 164582

💛 - Coveralls

@Habbie Habbie force-pushed the lmdb-full-comments branch from c6904be to ba2d37b Compare March 6, 2026 10:17
@Habbie
Copy link
Member Author

Habbie commented Mar 6, 2026

rebased

Co-authored-by: Miod Vallat <miod.vallat@powerdns.com>
Signed-off-by: Peter van Dijk <peter.van.dijk@powerdns.com>
@Habbie Habbie force-pushed the lmdb-full-comments branch from 08619a3 to d976bd3 Compare March 6, 2026 10:26
Co-authored-by: Miod Vallat <miod.vallat@powerdns.com>
Signed-off-by: Peter van Dijk <peter.van.dijk@powerdns.com>
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