Re: [PATCH 04/20] sysfs: Handle the general case of removing of directorieswith subdirectories

[Tejun Heo]

Eric W. Biederman wrote:
> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> 
> Modify sysfs to properly remove directories containing attributes and
> subdirectories.  The code is relatively simple and means we don't have
> to worry about what might use this logic.
> 
> In a quick survey I have only found /sys/dev/char and /sys/dev/block that are
> removing non-enmpty directories today (and they are exclusively filled with symlinks).
> So only removing empty directories does not appear to be an option.
> 
> I don't hold sysfs_mutex across the entire operation as that is unneeded
> for coherence at the sysfs level and some level of coordination is expected
> at the upper layers.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>
...
> -void sysfs_remove_subdir(struct sysfs_dirent *sd)
> -{
> -	remove_dir(sd);
> +	struct sysfs_dirent *sd = dir_sd;
> +	mutex_lock(&sysfs_mutex);
> +	while ((sysfs_type(sd) == SYSFS_DIR) && sd->s_dir.children)
> +		sd = sd->s_dir.children;
> +	if (sd != dir_sd)
> +		sysfs_get(sd);
> +	else
> +		sd = NULL;
> +	mutex_unlock(&sysfs_mutex);
> +	return sd;
>  }

Some blank lines wouldn't hurt, especially after local variable
declaration.

> -static void __sysfs_remove_dir(struct sysfs_dirent *dir_sd)
> +static void remove_dir(struct sysfs_dirent *dir_sd)
>  {
>  	struct sysfs_addrm_cxt acxt;
> -	struct sysfs_dirent **pos;
> -
> -	if (!dir_sd)
> -		return;
> +	struct sysfs_dirent *sd;
>  
>  	pr_debug("sysfs %s: removing dir\n", dir_sd->s_name);
> -	sysfs_addrm_start(&acxt, dir_sd);
> -	pos = &dir_sd->s_dir.children;
> -	while (*pos) {
> -		struct sysfs_dirent *sd = *pos;
>  
> -		if (sysfs_type(sd) != SYSFS_DIR)
> -			sysfs_remove_one(&acxt, sd);
> -		else
> -			pos = &(*pos)->s_sibling;
> +	while ((sd = sysfs_get_one(dir_sd))) {
> +		sysfs_addrm_start(&acxt, sd->s_parent);
> +		sysfs_remove_one(&acxt, sd);
> +		sysfs_addrm_finish(&acxt);
> +		sysfs_put(sd);
>  	}
> +	sysfs_addrm_start(&acxt, dir_sd->s_parent);
> +	sysfs_remove_one(&acxt, dir_sd);
>  	sysfs_addrm_finish(&acxt);
> +}

I agree we should be heading this way but what happens to attributes
or directories living below the subdirectories?  If it's gonna handle
recursive case, I think it better do it properly.  I had patches of
similar effect.

 http://article.gmane.org/gmane.linux.kernel/582151
 http://article.gmane.org/gmane.linux.kernel/582155

The patchset didn't really go anywhere but the recursive atomic
removal should be usable.

Thanks.

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