From owner-freebsd-arch@FreeBSD.ORG Fri Jan 4 16:15:14 2008 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 107B616A420 for ; Fri, 4 Jan 2008 16:15:14 +0000 (UTC) (envelope-from julian@elischer.org) Received: from outI.internet-mail-service.net (outI.internet-mail-service.net [216.240.47.232]) by mx1.freebsd.org (Postfix) with ESMTP id E84F813C447 for ; Fri, 4 Jan 2008 16:15:13 +0000 (UTC) (envelope-from julian@elischer.org) Received: from mx0.idiom.com (HELO idiom.com) (216.240.32.160) by out.internet-mail-service.net (qpsmtpd/0.40) with ESMTP; Fri, 04 Jan 2008 08:04:45 -0800 Received: from julian-mac.elischer.org (localhost [127.0.0.1]) by idiom.com (Postfix) with ESMTP id 46839126E0A; Fri, 4 Jan 2008 08:04:45 -0800 (PST) Message-ID: <477E592B.9040106@elischer.org> Date: Fri, 04 Jan 2008 08:04:59 -0800 From: Julian Elischer User-Agent: Thunderbird 2.0.0.9 (Macintosh/20071031) MIME-Version: 1.0 To: John Baldwin References: <477D931D.4000303@elischer.org> <200801041042.09573.jhb@freebsd.org> In-Reply-To: <200801041042.09573.jhb@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: freebsd-arch@freebsd.org Subject: Re: RFC: sysctl additional functions/macros X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Jan 2008 16:15:14 -0000 John Baldwin wrote: > On Thursday 03 January 2008 08:59:57 pm Julian Elischer wrote: >> I would like to extend the current SYSCTL_INT() with >> SYSCTL_INT_CLAMPED() or similar, where you also supply a >> maximum acceptable value. (and maybe a clue as to what to say if it is >> a bad value). >> >> so many users of SYSCTL_INT don't check for bad values because it's so >> much harder (you need to supply your own handler), and so many >> simple handlers exist fo rthe people that DO check that it seems to >> me that we should provide a pre-canned way to do this.... >> >> we are limited to using the existing structure, >> but as we have no existing callers we can redefine >> one element.... > > In HEAD you can change the ABI of struct sysctl_oid. Or even better, you > could do something like this: > > : > > struct sysctl_int_validator { > int (*siv_validator)(void *arg, int newvalue); > void *siv_arg; > int *siv_data; > } > > #define SYSCTL_INT_VALIDATED(parent, nbr, name, access, ptr, validator, arg, descr) \ > struct sysctl_int_validator siv__##parent##_##name = { \ > (validator), (arg), (ptr) }; \ > SYSCTL_OID(parent, nbr, name, CTLTYPE_INT|(access), \ > &siv__##parent##_##name, 0, sysctl_handle_validated_int, \ > "I", descr) > > int sysctl_handle_validated_int(SYSCTL_HANDLER_ARGS); > > kern/kern_sysctl.c: > > int > sysctl_handle_validated_int(SYSCTL_HANDLER_ARGS) > { > struct sysctl_int_validator *siv; > int error, tmp; > > siv = arg1; > if (!siv) > return (EINVAL); > tmp = *siv->siv_data; > error = sysctl_handle_int(oidp, &tmp, 0, req); > if (error || !req->newptr) > return (error); > error = siv->siv_validator(siv->siv_arg, tmp); > if (error == 0) > *siv->siv_data = tmp; > return (error); > } > > You can repeat this for the various sysctl types adding a validator handler > for each type. This gives you a framework to add simple validation. You > could then add wrappers to that for specific cases if they are common. For > example, you could implement the _CLAMPED variant like so: > > : > > SYSCTL_INT_CLAMPED(parent, nbr, name, access, ptr, max, descr) \ > SYSCTL_INT_VALIDATED(parent, nbr, name, access, ptr, \ > sysctl_validate_clamped_int, (void *)max, descr) > > int sysctl_validate_clamped_int(void *arg, int newvalue); > > kern/kern_sysctl.c: > > int > sysctl_validate_clamped_int(void *arg, int newvalue) > { > int max = (intptr_t)arg; > > if (newvalue > max) > return (ERANGE); > return (0); > } > > > You could even add min/max boundaries if desired: > > : > > struct sysctl_int_bounds = { > int sib_min; > int sib_max; > }; > > SYSCTL_INT_BOUNDED(parent, nbr, name, access, ptr, min, max, descr) \ > struct sysctl_int_bounds sib__##parent##_##name = { \ > (min), (max) }; \ > SYSCTL_INT_VALIDATED(parent, nbr, name, access, ptr, \ > sysctl_validate_bounded_int, &sib__##parent##_##name, \ > descr) > > int sysctl_validate_bounded_int(void *arg, int newvalue); > > kern/kern_sysctl.c: > > int > sysctl_validate_bounded_int(void *arg, int newwvalue) > { > struct sysctl_bounded_int *sib; > > sib = arg; > if (newvalue < sib->sib_min || newvalue > sib->sib_max) > return (ERANGE); > return (0); > } > > It would be nice to allow the validator function to be an anonymous routine > like phk suggests. Perhaps something like: > > #define SYSCTL_INT_VALIDATOR(parent, nbr, name, access, ptr, descr, validator) \ > int \ > validate__##parent##_##name(int newvalue) \ > validator \ > SYSCTL_INT_VALIDATED(parent, nbr, name, access, ptr, \ > &validate__##parent##_##name, 0, descr) > > would work, though I'm not sure how cpp(1) would really handle that. > > You could also inline sysctl_handle_int() into sysctl_handle_validated_int() > if you wanted. > right, so you've solved that one.. now for global warming.. You might as well go ahead and commit it I think.. :-)