From owner-freebsd-arch Mon Jul 3 22:42:31 2000 Delivered-To: freebsd-arch@freebsd.org Received: from localhost (localhost [127.0.0.1]) by hub.freebsd.org (Postfix) with ESMTP id 2112A37C24E; Mon, 3 Jul 2000 22:41:51 -0700 (PDT) (envelope-from green@FreeBSD.org) Date: Tue, 4 Jul 2000 01:41:34 -0400 (EDT) From: Brian Fundakowski Feldman X-Sender: green@green.dyndns.org To: Andrzej Bialecki Cc: dfr@freebsd.org, jlemon@freebsd.org, freebsd-arch@freebsd.org Subject: Re: UPDATE: Re: Dynamic sysctls, next round (please review) In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Mon, 3 Jul 2000, Andrzej Bialecki wrote: > 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. Actually, I think you're definitely right :) It's a good thing to have this as a management tool; I was only considering it from the standpoint of it protecting people from depending on things in a wrong way. > > > 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. Well, I dunno what else I'd call it. I don't really see the point of keeping it as a separate definition from M_SYSCTL except for initial debugging. If you think it's worth keeping for later debugging help (detecting leaks and such, of course) as opposed to just M_SYSCTL, I won't argue there =) > > 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. It's just my anti-bloat feelings that say they shouldn't be there. They don't hurt much, but they really don't help like checking for NULL function pointers does. NULL data pointers are easy to debug, but NULL function pointers... trashing your IP is not fun :( > > 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 *. Silence those few warnings yourself with casts where it is necessary. The const will prevent mistakes by others later, since they do _not_ have a reason to be modifying oid_name. > > You still don't want anything modifying the > > contents of *oid_name. > > I'd qualify free(oidp->oid_name, M_SYSCTL1) as modification... Well, you can cast it there. It's really an important thing to give the compiler a hint that you don't want other people messing with it. It doesn't change the code here, but only serves as the advisory not to touch the value. It makes it easy to check that noone else is doing evil things to the oid_name later on by grepping for (const char *), as you're the only person who should be doing it, and only in the sysctl implementation proper :) > > 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? Here's an example: if (SYSCTL_CTX_CREATE(ctx) == NULL) return (NULL); expands as such: if (ctx = sysctl_ctx_create() == NULL) return (NULL); The order of operations dictate that this is necessarily equal to: ctx = (sysctl_ctx_create() == NULL); if (ctx != NULL) return (NULL); If sysctl_ctx_create() succeeds here, ctx will be equal to 1 (or some other truth value), and you will panic soon. > > 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? The implementation of SYSCTL_CTX_CREATE() defines it as an rvalue, which encourages people to treat it as such, like above. If people do that, the behavior will be incorrect. Now, simply adding parentheses around the macro's definition will fix this specifically. If you don't want people treating it as an rvalue, you should do as is done for all other macros like that, which is to enclose the implementation in a do { } while(0); The idiom is bad because C does not have references (automatic pointers) like C++ (and other languages, of course). SYSCTL_CTX_INIT(&ctx) is okay because you are treating ctx as a normal parameter, where to be modified it must be a pointer. SYSCTL_CTX_CREATE(ctx) is bad because C would not allow passing a struct pointer by reference to do void sysctl_ctx_create(struct sysctl_ctx *&); In C, you would do void sysctl_ctx_create(struct sysctl_ctx **); and pass a pointer to the sysctl_ctx pointer you want filled in. The idiom you're suggesting (references as in C++) suggests that the language would allow modifying of data you did not pass a pointer to. The macros like this are Certified-by-Brian-Evil =] As you have it now, code I write will look like: struct sysctl_ctx *ctxp; SYSCTL_CTX_CREATE(ctxp); /* modification of an argument itself */ That is no better than: struct sysctl_ctx *ctxp; ctxp = sysctl_ctx_create(); Either way, if the implementation changes, you will have the same behavior defined as requisite in the API (keeping a struct sysctl_ctx *). There's no way that the macro SYSCTL_CTX_CREATE() could prevent future API incompatibilities that wouldn't be provided by just using sysctl_ctx_create(). What could possibly change in the future? If you added an argument to sysctl_ctx_create(), then that would be Bad Magic to add that to SYSCTL_CTX_CREATE()'s implementation and blithely accept that the user will have the argument type you want in the name you want by coincidence. You'd have to add another argument to SYSCTL_CTX_CREATE(), changing the API, and SYSCTL_CTX_CREATE() didn't prevent the API from changing. I hope that made sense ;-) > > 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? Doug Rabson himself would be a good choice. He did most of dynamicism for sysctl, the SLIST tree structure which allows for the runtime expansion. I think dfr is the right guy for reviewing this :) > > 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 > > // WebGiro AB, Sweden (http://www.webgiro.com) > // ------------------------------------------------------------------- > // ------ FreeBSD: The Power to Serve. http://www.freebsd.org -------- > // --- Small & Embedded FreeBSD: http://www.freebsd.org/~picobsd/ ---- > > > -- Brian Fundakowski Feldman \ FreeBSD: The Power to Serve! / green@FreeBSD.org `------------------------------' To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message