Date: Fri, 04 Jan 2008 08:04:59 -0800 From: Julian Elischer <julian@elischer.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-arch@freebsd.org Subject: Re: RFC: sysctl additional functions/macros Message-ID: <477E592B.9040106@elischer.org> In-Reply-To: <200801041042.09573.jhb@freebsd.org> References: <477D931D.4000303@elischer.org> <200801041042.09573.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
>
> <sys/sysctl.h>:
>
> 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:
>
> <sys/sysctl.h>:
>
> 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:
>
> <sys/sysctl.h>:
>
> 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.. :-)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?477E592B.9040106>