Re: [PATCH v4 2/6] sysfs: wrap __compat_only_sysfs_link_entry_to_kobj function to change the symlink name

[Sourabh Jain]

On 12/6/19 6:16 PM, Greg KH wrote:
> On Fri, Dec 06, 2019 at 05:54:30PM +0530, Sourabh Jain wrote:
>> The __compat_only_sysfs_link_entry_to_kobj function creates a symlink to a
>> kobject but doesn't provide an option to change the symlink file name.
>>
>> This patch adds a wrapper function create_sysfs_symlink_entry_to_kobj that
>> extends the __compat_only_sysfs_link_entry_to_kobj functionality which
>> allows function caller to customize the symlink name.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
>> ---
>>  fs/sysfs/group.c      | 28 +++++++++++++++++++++++++---
>>  include/linux/sysfs.h | 12 ++++++++++++
>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>> index d41c21fef138..5eb38145b957 100644
>> --- a/fs/sysfs/group.c
>> +++ b/fs/sysfs/group.c
>> @@ -424,6 +424,25 @@ EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  				      struct kobject *target_kobj,
>>  				      const char *target_name)
>> +{
>> +	return create_sysfs_symlink_entry_to_kobj(kobj, target_kobj,
>> +						target_name, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>> +
>> +/**
>> + * create_sysfs_symlink_entry_to_kobj - add a symlink to a kobject pointing
>> + * to a group or an attribute
>> + * @kobj:		The kobject containing the group.
>> + * @target_kobj:	The target kobject.
>> + * @target_name:	The name of the target group or attribute.
>> + * @symlink_name:	The name of the symlink file (target_name will be
>> + *			considered if symlink_name is NULL).
>> + */
>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>> +				       struct kobject *target_kobj,
>> +				       const char *target_name,
>> +				       const char *symlink_name)
>>  {
>>  	struct kernfs_node *target;
>>  	struct kernfs_node *entry;
>> @@ -448,12 +467,15 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  		return -ENOENT;
>>  	}
>>  
>> -	link = kernfs_create_link(kobj->sd, target_name, entry);
>> +	if (!symlink_name)
>> +		symlink_name = target_name;
>> +
>> +	link = kernfs_create_link(kobj->sd, symlink_name, entry);
>>  	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
>> -		sysfs_warn_dup(kobj->sd, target_name);
>> +		sysfs_warn_dup(kobj->sd, symlink_name);
>>  
>>  	kernfs_put(entry);
>>  	kernfs_put(target);
>>  	return PTR_ERR_OR_ZERO(link);
>>  }
>> -EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
>> +EXPORT_SYMBOL_GPL(create_sysfs_symlink_entry_to_kobj);
>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>> index 5420817ed317..123c6f10333a 100644
>> --- a/include/linux/sysfs.h
>> +++ b/include/linux/sysfs.h
>> @@ -300,6 +300,10 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
>>  int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>>  				      struct kobject *target_kobj,
>>  				      const char *target_name);
>> +int create_sysfs_symlink_entry_to_kobj(struct kobject *kobj,
>> +				       struct kobject *target_kobj,
>> +				       const char *target_name,
>> +				       const char *symlink_name);
> 
> sysfs_create_symlink_entry_to_kobj()?
> 
> I can't remember why we put __compat_only there, perhaps because we do
> not want people to really use this unless you really really have to?

We don't have much option here. I tried replicating the sysfs files
in older patch series but creating symlink at old location is much
better approach.

The __compat_only_sysfs_link_entry_to_kobj function is pretty generic,
unable to understand the reason behind restricting its usage.

> 
> So then keep compat_only here as well?

Sure, I will rename the wrapper function.

But how about changing the function signature instead of creating
a wrapper function?

Considering the fact that there are only two places this function
has called.

> 
> What breaks if you remove those undocumented sysfs files?  What
> userspace tool do you have that will even notice?

The scripts used in kdump service need those sysfs files to control
the dump collection. So we can't just move the sysfs files to the
new location.

Thanks,
Sourabh Jain