dp_04_module_param_ranged.diff
This is the 4th patch of 16 patches for devparam.
This patch implements module_param_flag() and module_param_invflag().
They appear as boolean parameter to the outside, and bitwise OR the
specified flag to flags when the specified boolean value is 1 and 0
respectively.
Signed-off-by: Tejun Heo <[email protected]>
Index: linux-devparam-export/include/linux/moduleparam.h
===================================================================
--- linux-devparam-export.orig/include/linux/moduleparam.h 2004-10-23 11:09:28.000000000 +0900
+++ linux-devparam-export/include/linux/moduleparam.h 2004-10-23 11:09:28.000000000 +0900
@@ -26,12 +26,7 @@ struct kernel_param {
param_set_fn set;
param_get_fn get;
void *arg;
-};
-
-/* Special one for strings we want to copy into */
-struct kparam_string {
- unsigned int maxlen;
- char *string;
+ long min, max;
};
/* Special one for arrays */
@@ -55,32 +50,41 @@ struct kparam_flag
parameters. perm sets the visibility in driverfs: 000 means it's
not there, read bits mean it's readable, write bits mean it's
writable. */
-#define __module_param_call(prefix, name, set, get, arg, perm) \
+#define __module_param_call(prefix, name, set, get, arg, min, max, perm) \
static char __param_str_##name[] = prefix #name; \
static struct kernel_param const __param_##name \
__attribute_used__ \
__attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *)))) \
- = { __param_str_##name, perm, set, get, arg }
+ = { __param_str_##name, perm, set, get, arg, min, max }
+
+#define module_param_call_ranged(name, set, get, arg, min, max, perm) \
+ __module_param_call(MODULE_PARAM_PREFIX, name, set, get, arg, \
+ min, max, perm)
-#define module_param_call(name, set, get, arg, perm) \
- __module_param_call(MODULE_PARAM_PREFIX, name, set, get, arg, perm)
+#define module_param_call(name, set, get, arg, perm) \
+ module_param_call_ranged(name, set, get, arg, 1, 0, perm)
/* Helper functions: type is byte, short, ushort, int, uint, long,
ulong, charp, bool or invbool, or XXX if you define param_get_XXX,
param_set_XXX and param_check_XXX. */
-#define module_param_named(name, value, type, perm) \
- param_check_##type(name, &(value)); \
- module_param_call(name, param_set_##type, param_get_##type, &value, perm)
+#define module_param_named_ranged(name, value, type, min, max, perm) \
+ param_check_##type(name, &(value)); \
+ module_param_call_ranged(name, param_set_##type, param_get_##type, \
+ &value, min, max, perm)
+
+#define module_param_ranged(name, type, min, max, perm) \
+ module_param_named_ranged(name, name, type, min, max, perm) \
+
+#define module_param_named(name, value, type, perm) \
+ module_param_named_ranged(name, value, type, 1, 0, perm)
-#define module_param(name, type, perm) \
+#define module_param(name, type, perm) \
module_param_named(name, name, type, perm)
/* Actually copy string: maxlen param is usually sizeof(string). */
#define module_param_string(name, string, len, perm) \
- static struct kparam_string __param_string_##name \
- = { len, string }; \
- module_param_call(name, param_set_copystring, param_get_string, \
- &__param_string_##name, perm)
+ module_param_call_ranged(name, param_set_copystring, \
+ param_get_string, string, 0, len, perm)
/* Bit flag */
#define __module_param_flag(name, flags, flag, inv, perm) \
@@ -150,14 +154,20 @@ extern int param_get_invbool(char *buffe
#define param_check_invbool(name, p) __param_check(name, p, int)
/* Comma-separated array: *nump is set to number they actually specified. */
-#define module_param_array_named(name, array, type, nump, perm) \
+#define module_param_array_named_ranged(name, array, type, nump, min, max, perm)\
static struct kparam_array __param_arr_##name \
= { ARRAY_SIZE(array), nump, param_set_##type, param_get_##type,\
sizeof(array[0]), array }; \
- module_param_call(name, param_array_set, param_array_get, \
- &__param_arr_##name, perm)
+ module_param_call_ranged(name, param_array_set, param_array_get,\
+ &__param_arr_##name, min, max, perm)
+
+#define module_param_array_ranged(name, type, nump, min, max, perm) \
+ module_param_array_named_ranged(name, name, type, nump, min, max, perm)
+
+#define module_param_array_named(name, array, type, nump, perm) \
+ module_param_array_named_ranged(name, array, type, nump, 1, 0, perm)
-#define module_param_array(name, type, nump, perm) \
+#define module_param_array(name, type, nump, perm) \
module_param_array_named(name, name, type, nump, perm)
extern int param_array_set(const char *val, struct kernel_param *kp);
@@ -169,9 +179,8 @@ extern int param_get_string(char *buffer
extern int param_set_flag(const char *val, struct kernel_param *kp);
extern int param_get_flag(char *buffer, struct kernel_param *kp);
-int param_array(const char *name,
- const char *val,
- unsigned int min, unsigned int max,
+int param_array(const char *name, const char *val,
+ long min_val, long max_val, unsigned int min, unsigned int max,
void *elem, int elemsize,
int (*set)(const char *, struct kernel_param *kp),
int *num);
Index: linux-devparam-export/kernel/module.c
===================================================================
--- linux-devparam-export.orig/kernel/module.c 2004-10-22 17:13:33.000000000 +0900
+++ linux-devparam-export/kernel/module.c 2004-10-23 11:09:28.000000000 +0900
@@ -748,19 +748,19 @@ int set_obsolete(const char *val, struct
max = min;
switch (*endp) {
case 'b':
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
1, param_set_byte, &dummy);
case 'h':
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
sizeof(short), param_set_short, &dummy);
case 'i':
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
sizeof(int), param_set_int, &dummy);
case 'l':
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
sizeof(long), param_set_long, &dummy);
case 's':
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
sizeof(char *), param_set_charp, &dummy);
case 'c':
@@ -777,7 +777,7 @@ int set_obsolete(const char *val, struct
}
if (size >= maxsize)
goto oversize;
- return param_array(kp->name, val, min, max, obsparm->addr,
+ return param_array(kp->name, val, 1, 0, min, max, obsparm->addr,
maxsize, obsparm_copy_string, &dummy);
}
printk(KERN_ERR "Unknown obsolete parameter type %s\n", obsparm->type);
Index: linux-devparam-export/kernel/params.c
===================================================================
--- linux-devparam-export.orig/kernel/params.c 2004-10-23 11:09:28.000000000 +0900
+++ linux-devparam-export/kernel/params.c 2004-10-23 11:09:28.000000000 +0900
@@ -138,6 +138,11 @@ int parse_args(const char *name,
"%s: `%s' too large for parameter `%s'\n",
name, val ?: "", param);
return ret;
+ case -ERANGE:
+ printk(KERN_ERR
+ "%s: `%s' out of range for parameter `%s'\n",
+ name, val ?: "", param);
+ return ret;
case 0:
break;
default:
@@ -163,6 +168,9 @@ int parse_args(const char *name,
l = strtolfn(val, &endp, 0); \
if (endp == val || ((type)l != l)) \
return -EINVAL; \
+ if (!(kp->min == 1 && kp->max == 0) && \
+ (l < (type)kp->min || l > (type)kp->max)) \
+ return -ERANGE; \
*((type *)kp->arg) = l; \
return 0; \
} \
@@ -181,15 +189,26 @@ STANDARD_PARAM_DEF(ulong, unsigned long,
int param_set_charp(const char *val, struct kernel_param *kp)
{
+ size_t min, max, len;
+
if (!val) {
printk(KERN_ERR "%s: string parameter expected\n",
kp->name);
return -EINVAL;
}
- if (strlen(val) > 1024) {
- printk(KERN_ERR "%s: string parameter too long\n",
- kp->name);
+ if (kp->min == 1 && kp->max == 0) {
+ min = 0;
+ max = 1024;
+ } else {
+ min = kp->min;
+ max = kp->max;
+ }
+
+ len = strlen(val);
+ if (len < min || len > max) {
+ printk(KERN_ERR "%s: string parameter too %s\n",
+ kp->name, len < min ? "short" : "long");
return -ENOSPC;
}
@@ -246,9 +265,8 @@ int param_get_invbool(char *buffer, stru
}
/* We cheat here and temporarily mangle the string. */
-int param_array(const char *name,
- const char *val,
- unsigned int min, unsigned int max,
+int param_array(const char *name, const char *val,
+ long min_val, long max_val, unsigned int min, unsigned int max,
void *elem, int elemsize,
int (*set)(const char *, struct kernel_param *kp),
int *num)
@@ -260,6 +278,8 @@ int param_array(const char *name,
/* Get the name right for errors. */
kp.name = name;
kp.arg = elem;
+ kp.min = min_val;
+ kp.max = max_val;
/* No equals sign? */
if (!val) {
@@ -304,8 +324,9 @@ int param_array_set(const char *val, str
struct kparam_array *arr = kp->arg;
unsigned int t;
- return param_array(kp->name, val, 1, arr->max, arr->elem,
- arr->elemsize, arr->set, arr->num ?: &t);
+ return param_array(kp->name, val, kp->min, kp->max,
+ 1, arr->max, arr->elem, arr->elemsize,
+ arr->set, arr->num ?: &t);
}
int param_array_get(char *buffer, struct kernel_param *kp)
@@ -330,21 +351,18 @@ int param_array_get(char *buffer, struct
int param_set_copystring(const char *val, struct kernel_param *kp)
{
- struct kparam_string *kps = kp->arg;
-
- if (strlen(val)+1 > kps->maxlen) {
- printk(KERN_ERR "%s: string doesn't fit in %u chars.\n",
- kp->name, kps->maxlen-1);
+ if (strlen(val)+1 > kp->max) {
+ printk(KERN_ERR "%s: string doesn't fit in %lu chars.\n",
+ kp->name, kp->max - 1);
return -ENOSPC;
}
- strcpy(kps->string, val);
+ strcpy(kp->arg, val);
return 0;
}
int param_get_string(char *buffer, struct kernel_param *kp)
{
- struct kparam_string *kps = kp->arg;
- return strlcpy(buffer, kps->string, kps->maxlen);
+ return strlcpy(buffer, kp->arg, kp->max);
}
int param_set_flag(const char *val, struct kernel_param *kp)
Oops, I've used explanation for dp_3 again for dp_4. Sorry.
On Sat, Oct 23, 2004 at 01:25:51PM +0900, Tejun Heo wrote:
> dp_04_module_param_ranged.diff
>
> This is the 4th patch of 16 patches for devparam.
This patch adds min and max fields of long type to struct
kernel_param and adds range checking for integer, charp and array
parameters. Also, struct kparam_string isn't necessary anymore as we
can use max field instead. I think this simple range-checking can
remove a lot of dulicated codes from drivers.
--
tejun
On Sat, 2004-10-23 at 13:25 +0900, Tejun Heo wrote:
> dp_04_module_param_ranged.diff
>
> This is the 4th patch of 16 patches for devparam.
>
> This patch implements module_param_flag() and module_param_invflag().
> They appear as boolean parameter to the outside, and bitwise OR the
> specified flag to flags when the specified boolean value is 1 and 0
> respectively.
Comment is wrong, of course: this patch adds range to the kernel_param
structure. But I'm not convinced that it's a great idea. It could be
added using the same method (kp->arg) used to extend the others instead.
(Same logic applied to spinlocking param variants).
It comes down to usage: currently there are few range-needing users, but
maybe that's because MODULE_PARM didn't support it. So I would drop
this or implement it as a wrapper, and later we can add it when it takes
over the world. Although I'd probably just add args to
module_param_call: I like having it as the "base".
The other thing is the min=1 max=0 case: I'd prefer INT_MAX, INT_MIN
etc. instead I think so there's no special cases.
Thanks!
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman
On Mon, Oct 25, 2004 at 02:45:20PM +1000, Rusty Russell wrote:
> On Sat, 2004-10-23 at 13:25 +0900, Tejun Heo wrote:
> > dp_04_module_param_ranged.diff
> >
> > This is the 4th patch of 16 patches for devparam.
> >
> > This patch implements module_param_flag() and module_param_invflag().
> > They appear as boolean parameter to the outside, and bitwise OR the
> > specified flag to flags when the specified boolean value is 1 and 0
> > respectively.
>
> Comment is wrong, of course:
Yeah, definitely. Sorry. :-P
> this patch adds range to the kernel_param
> structure. But I'm not convinced that it's a great idea. It could be
> added using the same method (kp->arg) used to extend the others instead.
>
> (Same logic applied to spinlocking param variants).
>
> It comes down to usage: currently there are few range-needing users, but
> maybe that's because MODULE_PARM didn't support it. So I would drop
> this or implement it as a wrapper, and later we can add it when it takes
> over the world. Although I'd probably just add args to
> module_param_call: I like having it as the "base".
And yes again. I also thought it would look cleaner to implement
range checking using a wrapping argument. But my rationales to include
them directly were
1. Most driver parameters need range-checking. I haven't seen many
driver parameters which don't require range-checking. The range
values are also useful for non-integer parameters including strings
and arrays.
2. If we use separate range structure. We would either
- have two different sets (vanilla and ranged) of standard parameter
set/get functions
- or, end up using the wrapped version for both. (turning off range
checking with some special value.)
And both didn't look very attractive to me. #1 is just not good
and #2 seems a bit pointless as we'll always be using the wrapping
argument for all types of paramters (integer, charp, string, array...).
So, I folded range into the kernel_param structure.
> The other thing is the min=1 max=0 case: I'd prefer INT_MAX, INT_MIN
> etc. instead I think so there's no special cases.
The thing is that the min, max values are used for all types
including signed and unsigned types. min=MIN_MAX, max=INT_MIN will be
interpreted as a valid [INT_MAX, (unsigned)INT_MAX + 1] range for
unsigned int parameters. So, I had to choose some value which,
regardless of type, maintains their relative relation, so the 1 and 0.
However, I do agree that plain 1, 0 doesn't look very clear. How
about using something like the following?
#define PARAM_UNRANGED_MIN 1
#define PARAM_UNRANGED_MAX 0
or
#define PARAM_UNRANGED 1, 0
I think it's better to be a bit feature-rich in module/device
paramter handling because by doing so we can avoid duplicated codes in
many places.
--
tejun