Satyam Sharma wrote: >> sysfs_find_dirent() -- to check for -EEXIST -- should be called >> *before* we create the new dentry for the to-be-created symlink >> in the first place. [ It's weird to grab a reference on the target >> for ourselves (and in fact even allocate the new dirent for the >> to-be-created symlink) and /then/ check for erroneous usage, >> and then go about undoing all that we should never have done >> at all. ] So this test could, and should, be made earlier, IMHO. > > A trivial nit: > > The cleanup ignores the return of sysfs_addrm_finish() -- functions > such as those could and should be void-returning. It doesn't even > need to return an int for success / failure ... I went over it's code, > and it's obvious that the function just never fails! > > Returning the count of objects actually added / removed is quite > redundant too, because we return "actx->cnt" unconditionally > from inside it, and the caller can know that anyway, without > even calling it. Also, note that nowhere in the present code is > the return of that function ever being used in that sense (i.e. as > a "count") anyway ... > > So: best to just make it void-returning. That's what it is.
Oh well, the function was made that way because that made the conversion easier when add/rm paths were consolidated using sysfs_addrm_cxt and friends. So, if you see the detail as a problem, please submit a patch. I dunno whether I would agree with the patch or not without seeing one. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
