Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 6 May 2022 19:59:42 +0530
From:   Jagdish Gediya <jvgediya@linux.ibm.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        rafael@kernel.org, akpm@linux-foundation.org,
        keescook@chromium.org, andriy.shevchenko@linux.intel.com,
        geert@linux-m68k.org, linux@roeck-us.net, adobriyan@gmail.com
Subject: Re: [PATCH] kobject: Refactor kobject_set_name_vargs()
Message-ID: <YnUw1pRzusU8j0A7@li-6e1fa1cc-351b-11b2-a85c-b897023bb5f3.ibm.com>
References: <20220506133309.36794-1-jvgediya@linux.ibm.com>
 <YnUqdBuxy7MofDoR@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YnUqdBuxy7MofDoR@kroah.com>
Precedence: bulk

On Fri, May 06, 2022 at 04:02:28PM +0200, Greg KH wrote:
> 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@linux.ibm.com>
> > ---
> >  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?

I think it can be moved to lib/, Will correct it in next version.

> And who else will be needing to use this?  Why the churn for no
> actual users?

I am working on a sepatare series for memory tiers where this kind
of functionality is needed, Based on numa topology of the system,
We can build the memory tiers nodemasks based on numa distances,
such masks need to be viewed/stored from sysfs using interfaces
/sys/*/memory_tier0, /sys/*/memory_tier1 etc. there can be upto
MAX_TIERS of memory tiers in the system, so we can define struct
device_attribute array statically but their names need to be set
as per tier number where this function is useful.

Also I think this requirement is generic although there are no
current users, It would be useful to not just restrict it to
kobjects.

> > +{
> > +	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.

Will remove in next version.

> 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
>