Messages in this thread | | | Date | Tue, 3 Aug 2004 13:43:31 +0100 | From | viro@parcelfa ... | Subject | Re: [PATCH 1/5] Add sysfs_dirent to sysfs dentry |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |