Date: Tue, 4 Jul 2000 01:41:34 -0400 (EDT) From: Brian Fundakowski Feldman <green@FreeBSD.org> 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.0007040112540.45364-100000@green.dyndns.org> In-Reply-To: <Pine.BSF.4.20.0007030934050.58061-100000@mx.webgiro.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
>
> // <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/ ----
>
>
>
--
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0007040112540.45364-100000>