Report information
The Basics
Id:
46718
Status:
resolved
Priority:
Low/Low
Queue:
bind9-public
People
BugTracker
Version Fixed:
9.11.3, 9.12.0
Version Found:
(no value)
Versions Affected:
(no value)
Versions Planned:
(no value)
Priority:
P3 Low
Severity:
S2 Normal
CVSS Score:
(no value)
CVE ID:
(no value)
Component:
BIND Server
Area:
bug
Dates
Created:Tue, 28 Nov 2017 08:50:37 -0500
Updated:Thu, 30 Nov 2017 14:24:32 -0500
Closed:Thu, 30 Nov 2017 14:24:32 -0500

This bug tracker is no longer active.

Please go to our Gitlab to submit issues (both feature requests and bug reports) for active projects maintained by Internet Systems Consortium (ISC).

Due to security and confidentiality requirements, full access is limited to the primary maintainers.

History
  Tue, 28 Nov 2017 08:50:37 -0500 Michał Kępień <michal@isc.org> - Ticket created
From: michal@isc.org
Subject: Clean up LMDB-related code
Date: Tue, 28 Nov 2017 14:50:37 +0100
To: bind9-public@isc.org

LMDB support is currently non-functional on OpenBSD. There are also some cosmetic discrepancies between LMDB docs/code and LMDB-related code in BIND.
  Tue, 28 Nov 2017 08:54:12 -0500 Michał Kępień <michal@isc.org> - Correspondence added

Please review rt46718 which applies a number of fixes, tweaks and cleanups to LMDB-related code. Suggestions regarding naming and placement of DNS_LMDB_* constants (see commit 1970f078cf) are welcome.
Tue, 28 Nov 2017 08:54:13 -0500 Michał Kępień <michal@isc.org> - Status changed from 'open' to 'review'
Tue, 28 Nov 2017 08:54:13 -0500 Michał Kępień <michal@isc.org> - Untaken
  Tue, 28 Nov 2017 17:26:09 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Tue Nov 28 13:54:12 2017, michal wrote: > Please review rt46718 which applies a number of fixes, tweaks and > cleanups to LMDB-related code. Suggestions regarding naming and > placement of DNS_LMDB_* constants (see commit 1970f078cf) are welcome. While we are touching the code: a) Please refactor the configure_newzones function to eliminate duplicate code (the for loops looks identical to me) and probably result2 error code might need to be handled. The double cleanup gotos also doesn't look very convincing. b) A comment why failed data_to_cfg in the first loop results into running the same loop again would be useful too. The rest looks sane. Thanks. Please reassign back when finished.
Tue, 28 Nov 2017 17:26:10 -0500 Ondrej Sury <ondrej@isc.org> - Given to Michał Kępień <michal@isc.org>
  Wed, 29 Nov 2017 09:41:21 -0500 Michał Kępień <michal@isc.org> - Correspondence added

The two for loops you mentioned do different things. However, I agree the purpose of this code is hard to grasp in its current form. I am intentionally not telling you what the code does; please look at the few refactoring commits I just pushed to rt46718 and let me know if it is clear now. If it is not, even more comments are likely due.
Wed, 29 Nov 2017 09:41:22 -0500 Michał Kępień <michal@isc.org> - Given to Ondrej Sury <ondrej@isc.org>
  Wed, 29 Nov 2017 10:07:43 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Wed Nov 29 14:41:21 2017, michal wrote: > The two for loops you mentioned do different things. However, I agree > the purpose of this code is hard to grasp in its current form. > > I am intentionally not telling you what the code does; please look at > the few refactoring commits I just pushed to rt46718 and let me know if > it is clear now. If it is not, even more comments are likely due. Ah, it's much better now. Just one nit - why struct and not va_list (or just pass all four arguments) for the callback? The callback_data makes sense when you need to store the callback_data in an async structure. (Actually it's not a 'callback' but a function that does things directly, right?)
  Wed, 29 Nov 2017 13:11:53 -0500 Michał Kępień <michal@isc.org> - Correspondence added

> Just one nit - why struct and not va_list (or just pass all four arguments) for the callback? The callback_data makes sense when you need to store the callback_data in an async structure. Neat idea, thanks, I have not thought about that. Just pushed a commit which uses a va_list in the callback prototype instead of a pointer to a structure. I chose this way because I really do not like passing a lot of arguments around. I think this solution also better conveys the intent. Could you please take a look? > (Actually it's not a 'callback' but a function that does things directly, right?) Correct, but I failed to find a better term and "callback" should give the reader the right idea. Plain "function" sounds too generic to me. If you have any suggestions, I am all ears (or you can even push a relevant tweak to the branch if you feel like it, as far as I am concerned).
  Wed, 29 Nov 2017 14:45:02 -0500 Michał Kępień <michal@isc.org> - Correspondence added

On second thought, by using a va_list as a callback argument we lose type safety. The code currently only uses a single call site for each callback, so keeping callers and callees in sync is not an issue, but if anyone has strong feelings about this, I am okay with modifying the code to pass all the arguments explicitly instead of using a va_list.
  Wed, 29 Nov 2017 15:58:38 -0500 Evan Hunt <Evan_Hunt@isc.org> - Correspondence added

On Wed Nov 29 19:45:02 2017, michal wrote: > On second thought, by using a va_list as a callback argument we lose > type safety. The code currently only uses a single call site for each > callback, so keeping callers and callees in sync is not an issue, but if > anyone has strong feelings about this, I am okay with modifying the code > to pass all the arguments explicitly instead of using a va_list. I agree. I don't even really think it's necessary to use a context struct as you did in 40562e37, but it's okay if you still want to. (I'd probably have chosen a more specific and less cumbersome name than "struct configure_zone_data", but that's just a cosmetic nit. BTW you want a different term than "callback" you could use "cfg_action" but I think it's clear enough as it is. I've reviewed all the commits; Michel asked me to be a third pair of eyes on this because I was the original author of this. I pushed a few style corrections. I'd suggest reverting the va_list change and the corresponding comment change in 8b9e2015. Other than that, this looks fine to me.
  Wed, 29 Nov 2017 16:51:48 -0500 Ray Bellis <ray@isc.org> - Correspondence added
To: bind9-public@isc.org
From: "Ray Bellis" <ray@isc.org>
Date: Wed, 29 Nov 2017 21:51:41 +0000
Subject: Re: [ISC-Bugs #46718] Clean up LMDB-related code

On 29/11/2017 18:11, Michał Kępień via RT wrote: > Correct, but I failed to find a better term and "callback" should give > the reader the right idea. Plain "function" sounds too generic to me. > If you have any suggestions, I am all ears (or you can even push a > relevant tweak to the branch if you feel like it, as far as I am > concerned). "Callback" is already in common parlance in many cases when a function pointer is being passed even for immediate use, rather than asynchronously. Ray
  Thu, 30 Nov 2017 03:01:12 -0500 Michał Kępień <michal@isc.org> - Correspondence added

Pushed a commit which causes callback parameters to be passed explicitly instead of using a va_list. As this is one of the options Ondřej suggested in his last message, I think rt46718 will be good to go once someone takes one final look at it.
  Thu, 30 Nov 2017 06:54:18 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Thu Nov 30 08:01:12 2017, michal wrote: > Pushed a commit which causes callback parameters to be passed explicitly > instead of using a va_list. As this is one of the options Ondřej > suggested in his last message, I think rt46718 will be good to go once > someone takes one final look at it. A really minor nit, I would personally prefer to replace: if (!commit || result != ISC_R_SUCCESS) { with if ((commit == NULL)[1] || (result != ISC_R_SUCCESS)[2]) { with this explanation: [1] to be consistent with rest of code where we use 'text != NULL' (as an example from this commit [2] And the parentheses while not necessary just contributes to readability and prevents future mistakes. ~~~ But no strong feelings, just an observation. It could be merged as is even without this change. So, go ahead and merge at your discretion.
Thu, 30 Nov 2017 06:54:20 -0500 Ondrej Sury <ondrej@isc.org> - Given to Michał Kępień <michal@isc.org>
  Thu, 30 Nov 2017 06:58:03 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Wed Nov 29 21:51:48 2017, ray wrote: > On 29/11/2017 18:11, Michał Kępień via RT wrote: > > > Correct, but I failed to find a better term and "callback" should give > > the reader the right idea. Plain "function" sounds too generic to me. > > If you have any suggestions, I am all ears (or you can even push a > > relevant tweak to the branch if you feel like it, as far as I am > > concerned). > > "Callback" is already in common parlance in many cases when a function > pointer is being passed even for immediate use, rather than asynchronously. It's no biggie, but overloading terms will usually hit you sooner or later (like passing the callback data as one "pointer", because it's what you usually do). But let's not spend more time than needed here - I can add this to some list for all hands when/if we discuss the coding style. So we are done with this for now.
  Thu, 30 Nov 2017 07:28:28 -0500 Michał Kępień <michal@isc.org> - Correspondence added

> A really minor nit, I would personally prefer to replace: > > if (!commit || result != ISC_R_SUCCESS) { > > with > > if ((commit == NULL)[1] || (result != ISC_R_SUCCESS)[2]) { > > with this explanation: > > [1] to be consistent with rest of code where we use 'text != NULL' (as an example from this commit But "commit" is a boolean here. > [2] And the parentheses while not necessary just contributes to readability and prevents future mistakes. This is not documented in our current coding style guidelines, so I decided against doing this. Naturally, we can further discuss this suggestion elsewhere.
  Thu, 30 Nov 2017 07:46:21 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Thu Nov 30 12:28:28 2017, michal wrote: > > A really minor nit, I would personally prefer to replace: > > > > if (!commit || result != ISC_R_SUCCESS) { > > > > with > > > > if ((commit == NULL)[1] || (result != ISC_R_SUCCESS)[2]) { > > > > with this explanation: > > > > [1] to be consistent with rest of code where we use 'text != NULL' > > (as an example from this commit > > But "commit" is a boolean here. Ah, ok, I haven't checked. > > [2] And the parentheses while not necessary just contributes to > > readability and prevents future mistakes. > > This is not documented in our current coding style guidelines, so I > decided against doing this. Naturally, we can further discuss this > suggestion elsewhere. Ok, I'll add this to my All-Hands coding style updates.
  Thu, 30 Nov 2017 07:47:57 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

On Thu Nov 30 12:46:21 2017, ondrej wrote: > On Thu Nov 30 12:28:28 2017, michal wrote: > > But "commit" is a boolean here. > > Ah, ok, I haven't checked. That's why I dislike '!' (typeless) conditions.
  Thu, 30 Nov 2017 08:49:34 -0500 Michał Kępień <michal@isc.org> - Correspondence added

4835. [cleanup] Clean up and refactor LMDB-related code. [RT #46718] 4834. [port] Fix LMDB support on OpenBSD. [RT #46718] 9.11.3, 9.12.0
Thu, 30 Nov 2017 08:49:35 -0500 Michał Kępień <michal@isc.org> - Status changed from 'review' to 'resolved'
Thu, 30 Nov 2017 08:49:35 -0500 Michał Kępień <michal@isc.org> - Version Fixed 9.11.3, 9.12.0 added
  Thu, 30 Nov 2017 09:25:05 -0500 Ondrej Sury <ondrej@isc.org> - Correspondence added

> BTW C has no boolean type (oops) ISO C99 has native boolean type (_Bool typed to bool if <stdbool.h> is included)
Thu, 30 Nov 2017 09:25:06 -0500 The RT System itself - Status changed from 'resolved' to 'open'
Thu, 30 Nov 2017 14:24:32 -0500 Michał Kępień <michal@isc.org> - Status changed from 'open' to 'resolved'