On Sat, Mar 18, 2017 at 02:24:08AM -0400, Oleg Drokin wrote:
> Ever since sysfs migration, class_process_proc_param stopped working
> correctly as all the useful params were no longer present as lvars.
> Replace all the nasty fake proc writes with hopefully less nasty
> kobject attribute search and then update the attributes as needed.
> 
> Signed-off-by: Oleg Drokin <gr...@linuxhacker.ru>
> Reported-by: Al Viro <v...@zeniv.linux.org.uk>
> ---
> Al has quite rightfully complained in the past that class_process_proc_param
> is a terrible piece of code and needs to go.
> This patch is an attempt at improving it somewhat and in process drop
> all the user/kernel address space games we needed to play to make it work
> in the past (and which I suspect attracted Al's attention in the first place).
> 
> Now I wonder if iterating kobject attributes like that would be ok with
> you Greg, or do you think there is a better way?
> class_find_write_attr could be turned into something generic since it's
> certainly convenient to reuse same table of name-write_method pairs,
> but I did some cursory research and nobody else seems to need anything
> of the sort in-tree.
> 
> I know ll_process_config is still awful and I will likely just
> replace the current hack with kset_find_obj, but I just wanted to make
> sure this new approach would be ok before spending too much time on it.
> 
> Thanks!
> 
>  drivers/staging/lustre/lustre/include/obd_class.h  |  4 +-
>  drivers/staging/lustre/lustre/llite/llite_lib.c    | 10 +--
>  drivers/staging/lustre/lustre/lov/lov_obd.c        |  3 +-
>  drivers/staging/lustre/lustre/mdc/mdc_request.c    |  3 +-
>  .../staging/lustre/lustre/obdclass/obd_config.c    | 78 
> ++++++++++------------
>  drivers/staging/lustre/lustre/osc/osc_request.c    |  3 +-
>  6 files changed, 44 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/include/obd_class.h 
> b/drivers/staging/lustre/lustre/include/obd_class.h
> index 083a6ff..badafb8 100644
> --- a/drivers/staging/lustre/lustre/include/obd_class.h
> +++ b/drivers/staging/lustre/lustre/include/obd_class.h
> @@ -114,8 +114,8 @@ typedef int (*llog_cb_t)(const struct lu_env *, struct 
> llog_handle *,
>                        struct llog_rec_hdr *, void *);
>  /* obd_config.c */
>  int class_process_config(struct lustre_cfg *lcfg);
> -int class_process_proc_param(char *prefix, struct lprocfs_vars *lvars,
> -                          struct lustre_cfg *lcfg, void *data);
> +int class_process_attr_param(char *prefix, struct kobject *kobj,
> +                          struct lustre_cfg *lcfg);

As you are exporting these functions, they will need to end up with a
lustre_* prefix eventually :)

>  struct obd_device *class_incref(struct obd_device *obd,
>                               const char *scope, const void *source);
>  void class_decref(struct obd_device *obd,
> diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c 
> b/drivers/staging/lustre/lustre/llite/llite_lib.c
> index 7b80040..192b877 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_lib.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
> @@ -2259,7 +2259,7 @@ int ll_obd_statfs(struct inode *inode, void __user *arg)
>  int ll_process_config(struct lustre_cfg *lcfg)
>  {
>       char *ptr;
> -     void *sb;
> +     struct super_block *sb;
>       struct lprocfs_static_vars lvars;
>       unsigned long x;
>       int rc = 0;
> @@ -2273,15 +2273,15 @@ int ll_process_config(struct lustre_cfg *lcfg)
>       rc = kstrtoul(ptr, 16, &x);
>       if (rc != 0)
>               return -EINVAL;
> -     sb = (void *)x;
> +     sb = (struct super_block *)x;
>       /* This better be a real Lustre superblock! */
> -     LASSERT(s2lsi((struct super_block *)sb)->lsi_lmd->lmd_magic == 
> LMD_MAGIC);
> +     LASSERT(s2lsi(sb)->lsi_lmd->lmd_magic == LMD_MAGIC);
>  
>       /* Note we have not called client_common_fill_super yet, so
>        * proc fns must be able to handle that!
>        */
> -     rc = class_process_proc_param(PARAM_LLITE, lvars.obd_vars,
> -                                   lcfg, sb);
> +     rc = class_process_attr_param(PARAM_LLITE, &ll_s2sbi(sb)->ll_kobj,
> +                                   lcfg);
>       if (rc > 0)
>               rc = 0;
>       return rc;
> diff --git a/drivers/staging/lustre/lustre/lov/lov_obd.c 
> b/drivers/staging/lustre/lustre/lov/lov_obd.c
> index b3161fb..c33a327 100644
> --- a/drivers/staging/lustre/lustre/lov/lov_obd.c
> +++ b/drivers/staging/lustre/lustre/lov/lov_obd.c
> @@ -926,8 +926,7 @@ int lov_process_config_base(struct obd_device *obd, 
> struct lustre_cfg *lcfg,
>  
>               lprocfs_lov_init_vars(&lvars);
>  
> -             rc = class_process_proc_param(PARAM_LOV, lvars.obd_vars,
> -                                           lcfg, obd);
> +             rc = class_process_attr_param(PARAM_LOV, &obd->obd_kobj, lcfg);
>               if (rc > 0)
>                       rc = 0;
>               goto out;
> diff --git a/drivers/staging/lustre/lustre/mdc/mdc_request.c 
> b/drivers/staging/lustre/lustre/mdc/mdc_request.c
> index 6bc2fb8..00387b8 100644
> --- a/drivers/staging/lustre/lustre/mdc/mdc_request.c
> +++ b/drivers/staging/lustre/lustre/mdc/mdc_request.c
> @@ -2670,8 +2670,7 @@ static int mdc_process_config(struct obd_device *obd, 
> u32 len, void *buf)
>       lprocfs_mdc_init_vars(&lvars);
>       switch (lcfg->lcfg_command) {
>       default:
> -             rc = class_process_proc_param(PARAM_MDC, lvars.obd_vars,
> -                                           lcfg, obd);
> +             rc = class_process_attr_param(PARAM_MDC, &obd->obd_kobj, lcfg);
>               if (rc > 0)
>                       rc = 0;
>               break;
> diff --git a/drivers/staging/lustre/lustre/obdclass/obd_config.c 
> b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> index 8fce88f..08fd126 100644
> --- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
> +++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
> @@ -995,26 +995,42 @@ int class_process_config(struct lustre_cfg *lcfg)
>  }
>  EXPORT_SYMBOL(class_process_config);
>  
> -int class_process_proc_param(char *prefix, struct lprocfs_vars *lvars,
> -                          struct lustre_cfg *lcfg, void *data)
> +static int class_find_write_attr(struct kobject *kobj, char *name, int 
> namelen,
> +                              char *val, int vallen)
> +{
> +     struct attribute *attr;
> +     struct kobj_type *kt = get_ktype(kobj);
> +     int i;
> +
> +     /* Empty object? */
> +     if (!kt || !kt->default_attrs)
> +             return -ENOENT;
> +
> +     for (i = 0; (attr = kt->default_attrs[i]) != NULL; i++) {
> +             if (!strncmp(attr->name, name, namelen) &&
> +                 namelen == strlen(attr->name)) {

Why do you care about namelen?  Why can't you just do a "normal"
strcmp()?  Is this "untrusted" user data?

> +                     if (kt->sysfs_ops && kt->sysfs_ops->store)
> +                             return kt->sysfs_ops->store(kobj, attr, val,
> +                                                         vallen);
> +                     return -EINVAL;
> +             }
> +     }
> +
> +     return -ENOENT;
> +}
> +
> +int class_process_attr_param(char *prefix, struct kobject *kobj,
> +                          struct lustre_cfg *lcfg)
>  {
> -     struct lprocfs_vars *var;
> -     struct file fakefile;
> -     struct seq_file fake_seqfile;
>       char *key, *sval;
>       int i, keylen, vallen;
> -     int matched = 0, j = 0;
>       int rc = 0;
> -     int skip = 0;
>  
>       if (lcfg->lcfg_command != LCFG_PARAM) {
>               CERROR("Unknown command: %d\n", lcfg->lcfg_command);
>               return -EINVAL;
>       }
>  
> -     /* fake a seq file so that var->fops->write can work... */
> -     fakefile.private_data = &fake_seqfile;
> -     fake_seqfile.private = data;
>       /* e.g. tunefs.lustre --param mdt.group_upcall=foo /r/tmp/lustre-mdt
>        * or   lctl conf_param lustre-MDT0000.mdt.group_upcall=bar
>        * or   lctl conf_param lustre-OST0000.osc.max_dirty_mb=36
> @@ -1038,39 +1054,16 @@ int class_process_proc_param(char *prefix, struct 
> lprocfs_vars *lvars,
>               keylen = sval - key;
>               sval++;
>               vallen = strlen(sval);
> -             matched = 0;
> -             j = 0;
> -             /* Search proc entries */
> -             while (lvars[j].name) {
> -                     var = &lvars[j];
> -                     if (!class_match_param(key, var->name, NULL) &&
> -                         keylen == strlen(var->name)) {
> -                             matched++;
> -                             rc = -EROFS;
> -                             if (var->fops && var->fops->write) {
> -                                     mm_segment_t oldfs;
> -
> -                                     oldfs = get_fs();
> -                                     set_fs(KERNEL_DS);
> -                                     rc = var->fops->write(&fakefile,
> -                                             (const char __user *)sval,
> -                                                             vallen, NULL);
> -                                     set_fs(oldfs);
> -                             }
> -                             break;
> -                     }
> -                     j++;
> -             }
> -             if (!matched) {
> +             rc = class_find_write_attr(kobj, key, keylen, sval, vallen);
> +
> +             if (rc == -ENOENT) {
>                       CERROR("%.*s: %s unknown param %s\n",
>                              (int)strlen(prefix) - 1, prefix,
>                              (char *)lustre_cfg_string(lcfg, 0), key);
>                       /* rc = -EINVAL;        continue parsing other params */
> -                     skip++;
>               } else if (rc < 0) {
> -                     CERROR("%s: error writing proc entry '%s': rc = %d\n",
> -                            prefix, var->name, rc);
> -                     rc = 0;
> +                     CERROR("%s: error writing proc entry '%.*s': rc = %d\n",
> +                            prefix, keylen, key, rc);

It's not a "proc" entry anymore :)

Other than that minor issue, and the question about namelen, this looks
semi-sane to me.  Want to resend this as a non-rfc patch?

thanks,

greg k-h
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to