Date: Mon, 3 Jul 2000 09:49:54 +0200 (CEST) From: Andrzej Bialecki <abial@webgiro.com> To: Brian Fundakowski Feldman <green@FreeBSD.org> 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.20.0007030934050.58061-100000@mx.webgiro.com> In-Reply-To: <Pine.BSF.4.21.0007022000500.38342-100000@green.dyndns.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 2 Jul 2000, Brian Fundakowski Feldman wrote: > On Mon, 3 Jul 2000, Andrzej Bialecki wrote: > > > On Sun, 2 Jul 2000, Brian Fundakowski Feldman wrote: > > > > > On Sun, 2 Jul 2000, Andrzej Bialecki wrote: > > > > * module B is left with subtree hanging in void: > > > > -static_root > > > > NOTHING! > > > > -node2(1) > > > > -leaf2(1) > > > > > > > > I don't have any good solution for it right now, except to warn users that > > > > they should always hang their subtrees off of static nodes. :-( > > > > > > There's already a good solution for this: you must always MODULE_DEPEND() > > > upon the owner of a node which you hang things from :) Now, we just need > > > to make sure each node is created by a module which you can MODULE_DEPEND() > > > on, and that's not too hard! > > > > I don't think this is relevant here. See the example (which is BTW a > > module, but doesn't need to be) provided with the patches. The granularity > > of dependancy is *per context*, not per module - in fact, the code that > > creates dynamic sysctls doesn't have to be a module, and it can create > > multiple contexts. > > I'm not understanding something here. Why would you want to add nodes to > a dynamically created node when you don't know who created the node? If > you don't know who created the node, you don't know whether it's appropriate > to add your oids to it; if you do know who created it, you should be doing > a MODULE_DEPEND() on them anyway. If you're the one creating that node, > then you should be depended on, so why would you want two modules which > both create the same node? I really can't see why you'd realistically > want to do that and therefore need the context. Hehe.. I don't claim it makes sense to add oids this way - I just wanted to protect the system from mistakes of module programmers. Before, it was easy to panic the system, now it's more difficult. As for the context - I think it's a useful concept even without this inter-dependency issue. If we allow to create sysctl oids on the fly (and deleting them), the context concept helps programmers to easily manage them. > > > The latest patches provide a working solution to this - perhaps not > > beautiful, but still something... > > Overall, it's quite good :) but I still have some issues with it. Other > than indentation style, which I'm quite willing to fix for you if you'd > like, there are some strange things. Why M_SYSCTL1? If anything, I'd > name it M_SYSCTLOID. Indentation is my weak point, true.. :-) M_SYSCTLOID sounds a bit like mongoloid, but I agree it makes more sense. > What's the point of all of the KASSERT()s against > NULL? The system will crash in either case. I planned to remove them later on. Now they provide easy means of identifying the cause. > Why was const removed > from const char *oid_name? To silence the compiler warning: it didn't like assigning and freeing pointers that are const char *. > You still don't want anything modifying the > contents of *oid_name. I'd qualify free(oidp->oid_name, M_SYSCTL1) as modification... > int ref should really be unsigned int oid_refcnt. Yes, that's a good idea. > Why do you want SYSCTL_CTX_* macros exactly? To hide the possible changes of implementation. > The SYSCTL_CTX_CREATE() one > is broken, Why? > but I don't see the need for it anyway, and it encourages the > MALLOC()-like lvalue-in-parameter list idiom which only leads to confusion. Could you elaborate? > As far as the algorithms themselves go and the code, it looks good to me :) > I would not be the one to give the final review for sysctl-related things, Who would? > it does look alright to me. I'd like to have the functionality before 5.0. > > > BTW. I could limit support for dyn_sysctls to disjoint subtrees > > exclusively - but I think the ability to have partially overlapping trees > > is attractive enough to justify small overhead of traversing the tailq's > > several times... > > I wouldn't worry about performance issues that are only applicable to > the actual addition and deletion of nodes, and not the lookup :) Thank you very much for your comments! They are very useful. Andrzej Bialecki // <abial@webgiro.com> WebGiro AB, Sweden (http://www.webgiro.com) // ------------------------------------------------------------------- // ------ FreeBSD: The Power to Serve. http://www.freebsd.org -------- // --- Small & Embedded FreeBSD: http://www.freebsd.org/~picobsd/ ---- 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.20.0007030934050.58061-100000>