| Message ID | 20230310234525.3986352-7-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | s390: simplify sysctl registration | expand |
On Fri, Mar 10, 2023 at 03:45:25PM -0800, Luis Chamberlain wrote: > The routine appldata_register_ops() allocates a sysctl table > with 4 entries. The firsts one, ops->ctl_table[0] is the parent directory > with an empty entry following it, ops->ctl_table[1]. The next entry is > for the the ops->name and that is ops->ctl_table[2]. It needs an empty for the ops->name > entry following that, and that is ops->ctl_table[3]. And so hence the > kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL). > > We can simplify this considerably since sysctl_register("foo", table) > can create the parent directory for us if it does not exist. So we > can just remove the first two entries and move back the ops->name to > the first entry, and just use kcalloc(2, ...). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > arch/s390/appldata/appldata_base.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c > index c593f2228083..a60c1e093039 100644 > --- a/arch/s390/appldata/appldata_base.c > +++ b/arch/s390/appldata/appldata_base.c > @@ -351,7 +351,8 @@ int appldata_register_ops(struct appldata_ops *ops) > if (ops->size > APPLDATA_MAX_REC_SIZE) > return -EINVAL; > > - ops->ctl_table = kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL); > + /* The last entry must be an empty one */ > + ops->ctl_table = kcalloc(2, sizeof(struct ctl_table), GFP_KERNEL); > if (!ops->ctl_table) > return -ENOMEM; > > @@ -359,17 +360,12 @@ int appldata_register_ops(struct appldata_ops *ops) > list_add(&ops->list, &appldata_ops_list); > mutex_unlock(&appldata_ops_mutex); > > - ops->ctl_table[0].procname = appldata_proc_name; > - ops->ctl_table[0].maxlen = 0; > - ops->ctl_table[0].mode = S_IRUGO | S_IXUGO; > - ops->ctl_table[0].child = &ops->ctl_table[2]; > + ops->ctl_table[0].procname = ops->name; > + ops->ctl_table[0].mode = S_IRUGO | S_IWUSR; > + ops->ctl_table[0].proc_handler = appldata_generic_handler; > + ops->ctl_table[0].data = ops; > > - ops->ctl_table[2].procname = ops->name; > - ops->ctl_table[2].mode = S_IRUGO | S_IWUSR; > - ops->ctl_table[2].proc_handler = appldata_generic_handler; > - ops->ctl_table[2].data = ops; > - > - ops->sysctl_header = register_sysctl_table(ops->ctl_table); > + ops->sysctl_header = register_sysctl(appldata_proc_name, ops->ctl_table); > if (!ops->sysctl_header) > goto out; > return 0; > -- > 2.39.1 I'll take it with the commit message change mentioned above and the following fixup, which addresses the obvious problem found during testing: --- diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c index a60c1e093039..f462d60679d7 100644 --- a/arch/s390/appldata/appldata_base.c +++ b/arch/s390/appldata/appldata_base.c @@ -281,7 +281,7 @@ appldata_generic_handler(struct ctl_table *ctl, int write, mutex_lock(&appldata_ops_mutex); list_for_each(lh, &appldata_ops_list) { tmp_ops = list_entry(lh, struct appldata_ops, list); - if (&tmp_ops->ctl_table[2] == ctl) { + if (&tmp_ops->ctl_table[0] == ctl) { found = 1; } } --
diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c index c593f2228083..a60c1e093039 100644 --- a/arch/s390/appldata/appldata_base.c +++ b/arch/s390/appldata/appldata_base.c @@ -351,7 +351,8 @@ int appldata_register_ops(struct appldata_ops *ops) if (ops->size > APPLDATA_MAX_REC_SIZE) return -EINVAL; - ops->ctl_table = kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL); + /* The last entry must be an empty one */ + ops->ctl_table = kcalloc(2, sizeof(struct ctl_table), GFP_KERNEL); if (!ops->ctl_table) return -ENOMEM; @@ -359,17 +360,12 @@ int appldata_register_ops(struct appldata_ops *ops) list_add(&ops->list, &appldata_ops_list); mutex_unlock(&appldata_ops_mutex); - ops->ctl_table[0].procname = appldata_proc_name; - ops->ctl_table[0].maxlen = 0; - ops->ctl_table[0].mode = S_IRUGO | S_IXUGO; - ops->ctl_table[0].child = &ops->ctl_table[2]; + ops->ctl_table[0].procname = ops->name; + ops->ctl_table[0].mode = S_IRUGO | S_IWUSR; + ops->ctl_table[0].proc_handler = appldata_generic_handler; + ops->ctl_table[0].data = ops; - ops->ctl_table[2].procname = ops->name; - ops->ctl_table[2].mode = S_IRUGO | S_IWUSR; - ops->ctl_table[2].proc_handler = appldata_generic_handler; - ops->ctl_table[2].data = ops; - - ops->sysctl_header = register_sysctl_table(ops->ctl_table); + ops->sysctl_header = register_sysctl(appldata_proc_name, ops->ctl_table); if (!ops->sysctl_header) goto out; return 0;
The routine appldata_register_ops() allocates a sysctl table with 4 entries. The firsts one, ops->ctl_table[0] is the parent directory with an empty entry following it, ops->ctl_table[1]. The next entry is for the the ops->name and that is ops->ctl_table[2]. It needs an empty entry following that, and that is ops->ctl_table[3]. And so hence the kcalloc(4, sizeof(struct ctl_table), GFP_KERNEL). We can simplify this considerably since sysctl_register("foo", table) can create the parent directory for us if it does not exist. So we can just remove the first two entries and move back the ops->name to the first entry, and just use kcalloc(2, ...). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- arch/s390/appldata/appldata_base.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)