Date: Sun, 2 Jul 2000 23:02:41 -0700 (PDT) From: Kelly Yancey <kbyanc@posi.net> To: Andrzej Bialecki <abial@webgiro.com> Cc: dfr@FreeBSD.ORG, jlemon@FreeBSD.ORG, freebsd-arch@FreeBSD.ORG Subject: Re: UPDATE: Re: Dynamic sysctls, next round (please review) Message-ID: <Pine.BSF.4.21.0007022223510.7980-100000@gateway.posi.net> In-Reply-To: <Pine.BSF.4.20.0007021654410.53282-100000@mx.webgiro.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 2 Jul 2000, Andrzej Bialecki wrote: > Hi, > > Ok, I think I solved all bad scenarios (described in the message at the > end). Instead of implementing some complex referencing mechanism, I just > added a possibility to do "safe delete" of context, so that it's possible > to rollback the changes if it fails in the middle. > > I tested that creating partially overlapping subtrees, and deleting them > independently works properly. > > The new patches are available at the same URL: > > http://www.freebsd.org/~abial/dyn_sysctl.tgz > > Please review and send me your comments. Thanks! > I think these look very good and I believe that most of my nits have already been mentioned by Brian. I do have a couple more issues though: It strikes me that the patch actually includes 2 semi-related changes: 1. The ability to create/delete oids at run-time, "dynamic sysctls". 2. Assistance to modules for managing these dynamic sysctls, "contexts". I should think that the former is not dependent on the latter. As such, I think that sysctl_add_oid should be able to accept a NULL clist pointer. Or, better yet, the context code should be completely independent to the extent that if the module wants to use contexts as a convenience for managing their dynamic sysctls, they should call sysctl_add_oid (which knows nothing of contexts now), return the syctl_oid * back, and pass that to the sysctl_ctx_add routine itself. The statements might look something like: oid = sysctl_add_oid(...) sysctl_ctx_add(myctx, oid); Of course, if the module didn't care about the oid handle for anything else, the two calls could be combined into one statement. In any event, I think this is cleaner and makes the purpose of contexts more clear. The other issue is with sysctl_remove_oid: it should never actually remove a node for which at least one child has a reference count > 0. As it stands, sysctl_remove_oid will recursively try to remove child nodes (which may only decrement their reference count to a value still > 0). But it will then proceed to delete the still-referenced children (presumably this was the justification for the warning comment). But this can be solved simply by checking if there are any nodes left after recursion, and if so, do not delete (but return success). Of course, none of this should matter if references counts are maintained correctly (in which case, I simply don't understand why the warning is there at all). But overall, I am looking forward to seeing this get into -current. This is very neat, and I'm already thinking of how it integrates with some sysctl work I've been up to. :) Kelly -- Kelly Yancey - kbyanc@posi.net - Belmont, CA System Administrator, eGroups.com http://www.egroups.com/ Maintainer, BSD Driver Database http://www.posi.net/freebsd/drivers/ Coordinator, Team FreeBSD http://www.posi.net/freebsd/Team-FreeBSD/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0007022223510.7980-100000>