Re: [PATCH 1/5] Add sysfs_dirent to sysfs dentry

[viro]

On Thu, Jul 29, 2004 at 03:38:21PM -0500, Maneesh Soni wrote:
> +			} else {
> +				/* error, release the ref taken in
> +				 * sysfs_create()
> +				 */
> +				dput(*d);  

Umm...  Shouldn't we unhash here?  sysfs_create() did hash it, so...
Same goes for other places where we fail to create sysfs_dirent; as
the matter of fact, how about making
	sysfs_make_dirent(dentry, mode, data, type)
that would either allocate sysfs_dirent and bind it to dentry, or
unhash and dput() dentry and return error?  AFAICS, that would make
life easier.

>  int sysfs_create_file(struct kobject * kobj, const struct attribute * attr)
>  {
> -	if (kobj && attr)
> -		return sysfs_add_file(kobj->dentry,attr);
> +	if (kobj && kobj->dentry && attr)
> +		return sysfs_add_file(kobj->dentry, attr, SYSFS_KOBJ_ATTR);

Can we legitimately get NULL kobj or kobj->dentry here?  If not, that'd
better be BUG_ON()...

> @@ -65,15 +98,28 @@ int sysfs_create_link(struct kobject * k
>  	struct dentry * d;
>  	int error = 0;
>  
> +	if (!name)
> +		return -EINVAL;

Again, can that happen legitimately?

>  	down(&dentry->d_inode->i_sem);
>  	d = sysfs_get_dentry(dentry,name);
>  	if (!IS_ERR(d)) {
>  		error = sysfs_create(d, S_IFLNK|S_IRWXUGO, init_symlink);
> -		if (!error)
> -			/* 
> -			 * associate the link dentry with the target kobject 
> -			 */
> -			d->d_fsdata = kobject_get(target);
> +		if (!error) {
> +			struct sysfs_dirent * sd;
> +			sd = sysfs_add_link(dentry->d_fsdata, name, target);
> +			if (!IS_ERR(sd)) {
> +				/* 
> +			 	* associate the link dentry with the target  
> +			 	* through the corresponding sysfs_dirent.
> +			 	*/
> +				d->d_fsdata = sd;
> +				sd->s_dentry = dentry;
> +			} else {
> +				dput(d);
> +				error = PTR_ERR(sd);
> +			}

	I'd pull that inside sysfs_add_link() (and then further into
sysfs_new_dirent()).
-
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/