On Fri, May 06, 2022 at 07:03:09PM +0530, Jagdish Gediya wrote:
> Setting name as per the format is not only useful for kobjects.
> It can also be used to set name for other things for e.g. setting
> the name of the struct attribute when multiple same kind of attributes
> need to be created with some identifier in name, instead of managing
> memory for names at such places case by case, it would be good if
> something like current kobject_set_name_vargs() can be utilized.
>
> Refactor kobject_set_name_vargs(), Create a new generic function
> set_name_vargs() which can be used for kobjects as well as at
> other places.
>
> This patch doesn't introduce any functionality change.
>
> Signed-off-by: Jagdish Gediya <jvgediya@xxxxxxxxxxxxx>
> ---
> include/linux/string.h | 1 +
> lib/kobject.c | 30 +-----------------------------
> mm/util.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b6572aeca2f5..f329962e5ae9 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -9,6 +9,7 @@
> #include <linux/stdarg.h>
> #include <uapi/linux/string.h>
>
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs);
> extern char *strndup_user(const char __user *, long);
> extern void *memdup_user(const void __user *, size_t);
> extern void *vmemdup_user(const void __user *, size_t);
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 5f0e71ab292c..870d05971e3a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -249,35 +249,7 @@ static int kobject_add_internal(struct kobject *kobj)
> int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> va_list vargs)
> {
> - const char *s;
> -
> - if (kobj->name && !fmt)
> - return 0;
> -
> - s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> - if (!s)
> - return -ENOMEM;
> -
> - /*
> - * ewww... some of these buggers have '/' in the name ... If
> - * that's the case, we need to make sure we have an actual
> - * allocated copy to modify, since kvasprintf_const may have
> - * returned something from .rodata.
> - */
> - if (strchr(s, '/')) {
> - char *t;
> -
> - t = kstrdup(s, GFP_KERNEL);
> - kfree_const(s);
> - if (!t)
> - return -ENOMEM;
> - strreplace(t, '/', '!');
> - s = t;
> - }
> - kfree_const(kobj->name);
> - kobj->name = s;
> -
> - return 0;
> + return set_name_vargs(&kobj->name, fmt, vargs);
> }
>
> /**
> diff --git a/mm/util.c b/mm/util.c
> index 54e5e761a9a9..808d29b17ea7 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -112,6 +112,46 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp)
> }
> EXPORT_SYMBOL(kstrndup);
>
> +/**
> + * set_name_vargs() - Set the name as per format
> + * @name: pointer to point to the name as per format
> + * @fmt: format string used to build the name
> + * @vargs: vargs to format the string.
> + */
> +int set_name_vargs(const char **name, const char *fmt, va_list vargs)
Why is this a mm/ thing and not a lib/ thing?
And who else will be needing to use this? Why the churn for no
actual users?
> +{
> + const char *s;
> +
> + if (*name && !fmt)
> + return 0;
> +
> + s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
> + if (!s)
> + return -ENOMEM;
> +
> + /*
> + * ewww... some of these buggers have '/' in the name ... If
> + * that's the case, we need to make sure we have an actual
> + * allocated copy to modify, since kvasprintf_const may have
> + * returned something from .rodata.
> + */
> + if (strchr(s, '/')) {
> + char *t;
> +
> + t = kstrdup(s, GFP_KERNEL);
> + kfree_const(s);
> + if (!t)
> + return -ENOMEM;
> + strreplace(t, '/', '!');
> + s = t;
> + }
> + kfree_const(*name);
> + *name = s;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(set_name_vargs);
No need to export this as there are no users in modules.
And if there was, shouldn't it be EXPORT_SYMBOL_GPL() as that's what the
kobject functions are exported as (most of them at least.)
But again, why is this needed at all?
thanks,
greg k-h
