Re: [PATCH 13/13] sysfs: Factor out sysfs_rename fromsysfs_rename_dir and sysfs_move_dir

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm@xxxxxxxxxxxx):
> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> 
> These two functions do 90% of the same work and it doesn't significantly
> obfuscate the function to allow both the parent dir and the name to change
> at the same time.  So merge them together to simplify maintenance, and
> increase testing.
> 
> Acked-by: Tejun Heo <tj@xxxxxxxxxx>
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>

Based on just this patchset it seems sysfs_rename() could be static,
but since it isn't static I assume your later patchset will use it
outside fs/sysfs/dir.c?

Acked-by: Serge Hallyn <serue@xxxxxxxxxx>


> ---
>  fs/sysfs/dir.c   |   62 +++++++++++++++++++++++++----------------------------
>  fs/sysfs/sysfs.h |    3 ++
>  2 files changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 0b60212..e1a86d1 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -760,30 +760,42 @@ void sysfs_remove_dir(struct kobject * kobj)
>  	__sysfs_remove_dir(sd);
>  }
> 
> -int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
> +int sysfs_rename(struct sysfs_dirent *sd,
> +	struct sysfs_dirent *new_parent_sd, const char *new_name)
>  {
> -	struct sysfs_dirent *sd = kobj->sd;
>  	const char *dup_name = NULL;
>  	int error;
> 
>  	mutex_lock(&sysfs_mutex);
> 
>  	error = 0;
> -	if (strcmp(sd->s_name, new_name) == 0)
> +	if ((sd->s_parent == new_parent_sd) &&
> +	    (strcmp(sd->s_name, new_name) == 0))
>  		goto out;	/* nothing to rename */
> 
>  	error = -EEXIST;
> -	if (sysfs_find_dirent(sd->s_parent, new_name))
> +	if (sysfs_find_dirent(new_parent_sd, new_name))
>  		goto out;
> 
>  	/* rename sysfs_dirent */
> -	error = -ENOMEM;
> -	new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
> -	if (!new_name)
> -		goto out;
> +	if (strcmp(sd->s_name, new_name) != 0) {
> +		error = -ENOMEM;
> +		new_name = dup_name = kstrdup(new_name, GFP_KERNEL);
> +		if (!new_name)
> +			goto out;
> +
> +		dup_name = sd->s_name;
> +		sd->s_name = new_name;
> +	}
> 
> -	dup_name = sd->s_name;
> -	sd->s_name = new_name;
> +	/* Remove from old parent's list and insert into new parent's list. */
> +	if (sd->s_parent != new_parent_sd) {
> +		sysfs_unlink_sibling(sd);
> +		sysfs_get(new_parent_sd);
> +		sysfs_put(sd->s_parent);
> +		sd->s_parent = new_parent_sd;
> +		sysfs_link_sibling(sd);
> +	}
> 
>  	error = 0;
>   out:
> @@ -792,37 +804,21 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
>  	return error;
>  }
> 
> +int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
> +{
> +	return sysfs_rename(kobj->sd, kobj->sd->s_parent, new_name);
> +}
> + 
>  int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
>  {
>  	struct sysfs_dirent *sd = kobj->sd;
>  	struct sysfs_dirent *new_parent_sd;
> -	int error;
> 
>  	BUG_ON(!sd->s_parent);
> -
> -	mutex_lock(&sysfs_mutex);
> -	new_parent_sd = (new_parent_kobj && new_parent_kobj->sd) ?
> +	new_parent_sd = new_parent_kobj && new_parent_kobj->sd ?
>  		new_parent_kobj->sd : &sysfs_root;
> 
> -	error = 0;
> -	if (sd->s_parent == new_parent_sd)
> -		goto out;	/* nothing to move */
> -
> -	error = -EEXIST;
> -	if (sysfs_find_dirent(new_parent_sd, sd->s_name))
> -		goto out;
> -
> -	/* Remove from old parent's list and insert into new parent's list. */
> -	sysfs_unlink_sibling(sd);
> -	sysfs_get(new_parent_sd);
> -	sysfs_put(sd->s_parent);
> -	sd->s_parent = new_parent_sd;
> -	sysfs_link_sibling(sd);
> -
> -	error = 0;
> -out:
> -	mutex_unlock(&sysfs_mutex);
> -	return error;
> +	return sysfs_rename(sd, new_parent_sd, sd->s_name);
>  }
> 
>  /* Relationship between s_mode and the DT_xxx types */
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index 98a15bf..ca52e7b 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -130,6 +130,9 @@ int sysfs_create_subdir(struct kobject *kobj, const char *name,
>  			struct sysfs_dirent **p_sd);
>  void sysfs_remove_subdir(struct sysfs_dirent *sd);
> 
> +int sysfs_rename(struct sysfs_dirent *sd,
> +	struct sysfs_dirent *new_parent_sd, const char *new_name);
> +
>  static inline struct sysfs_dirent *__sysfs_get(struct sysfs_dirent *sd)
>  {
>  	if (sd) {
> -- 
> 1.6.5.2.143.g8cc62
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/