CC: Received: from bikeshed.isc.org (bikeshed.isc.org [IPv6:2001:4f8:3:d::19]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mail.isc.org", Issuer "RapidSSL CA" (not verified)) by bugs.isc.org (Postfix) with ESMTPS id CA593D78B04 for ; Wed, 6 Sep 2017 20:51:02 +0000 (UTC) Received: by bikeshed.isc.org (Postfix, from userid 10292) id 7898B216C1E; Wed, 6 Sep 2017 20:51:01 +0000 (UTC) Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [ISC-Bugs #45186] name server library From each@isc.org Wed Sep 6 20:51:02 2017 X-RT-Incoming-Encryption: Not encrypted References: X-RT-Interface: Email MIME-Version: 1.0 X-Original-To: bind9-public@bugs.isc.org X-RT-Original-Encoding: utf-8 content-type: text/plain; charset="utf-8" From: "Evan Hunt" Message-ID: <20170906205101.GA82383@isc.org> In-Reply-To: Delivered-To: bind9-public@bugs.isc.org Return-Path: Date: Wed, 6 Sep 2017 20:51:01 +0000 To: "Michał Kępień via RT" RT-Message-ID: Content-Length: 1544 Thanks for the long review. I think I've addressed all your comments except the ones noted below: > o What benefit do the non-NULL checks in commit 3eb5aad6e9 bring? I > was unable to figure out a code path which would cause these checks > to evaluate to false. Also, there are other instances of > ns_server_detach() calls without a similar non-NULL check (e.g. in > clientmgr_destroy() in lib/ns/client.c). I think I prefer to keep them, at least for now, as it just seems like a good pattern to check before detaching. I know of no reason to expect them to be NULL, but it helps to future-proof the code. > o Commit c336ed47ba moves the isself() callback back into > bin/named/zoneconf.c. However, I was unable to quickly figure out > why that is needed or desired. Sorry, I can't recall now. It may just have seemed aesthetically righteous. Perhaps I realized my earlier move of the code was unnecessary and so I figured it shouldn't be done. > o Commit cfb4fc12f8 "incidentally fixes a potential problem with > dns_aclenv_destroy()". Could you please explain what that potential > problem is? If you discovered a code path which causes erroneuous > behavior, can it be reproduced in a test? I don't remember the reason for this now either, sorry. > o A lot of function prototypes in lib/ns/include/ns/ are not > commented: You're right, that needs doing. I'll get to it later, though. Can you go over commit ad416a5f90 and confirm that it addresses the concerns you raised?