Messages in this thread | | | Date | Sun, 12 Mar 2000 21:26:45 -0500 (EST) | From | Alexander Viro <> | Subject | Re: (reiserfs) Re: patch: reiserfs for 2.3.49 |
| |
> Ok, Is it the 64 bits of information needed to find a an object in > reiserfs, or is there something else? We are more than open to criticism > and direction, privately or to the list.
Well, this ->read_inode2 thing is just plain ugly, but that is the last of my problems with the code (IOW, I know how to get rid of it). What makes me very uneasy is (between other reasons) the fact that none of you guys cared to RTFfs/namei.c. I mean, WTF? You got checks that are done in VFS all over the friggin place. Got them wrong, BTW. And excuse me, but I don't believe in paranoia theory - it just means that nobody cared to read the damn thing. I _hate_ code duplication in such places and I've spent a lot of time removing it from the tree. Because in case of any change this code tends to stay buggy. And be copied into other filesystems. Example: + if (new_inode && S_ISDIR(new_inode->i_mode)) { + /* new name exists and points to directory */ + retval = -EISDIR; + if (!S_ISDIR(old_inode->i_mode)) + goto end_rename; + retval = -EINVAL; + if (is_subdir (new_dentry, old_dentry)) + goto end_rename; + retval = -ENOTEMPTY; + if (!reiserfs_empty_dir (new_inode)) + goto end_rename; + retval = -EBUSY; + if (new_inode->i_count > 1) + goto end_rename; + }
WTF is _that_? Earth to reiserfs team - where had you been for, ugh, last 12 months? And as for the last check here - excuse me, but in this case s/12/30/. Code is full of crap like that and IMO removing this 'just-in-case' humus is prerequisite to any talks about inclusion into the tree. It was _never_ audited against VFS. Oh, lovely - in the same patch, same function: + /* Update the dcache */ + d_move(old_dentry, new_dentry); + retval = 0;
Wonderful. So you didn't even test it well enough on 2.2. Furrfu... BTW, what's up with this try_again: in the same function? Could somebody in your team actually _read_ {do,vfs}_rename()? You know, the caller of this animal... It's not like this stuff was undocumented or something... BTW, is anyone of you subscribed to fsdevel?
Please, start with removing the crud from your code. It's the same picture all over the place. Excuse me, it's kinda hard to consider the patch seriously when it's bloody obvious that nobody cared to check what happened with VFS during the last two years unless the changes broke compile. I _know_ that not all changes of rules were detectable that way. And I wouldn't bet a dime that you got them right (especially since one example of the contrary can be found two paragraphs above). It's not like that was a 2.3-only thing - your code is incorrect for 2.2 since the February/March 1999...
When this sort of crap will be cleaned up and code really audited and _understood_ by your team - fine, there will be a point in talking about this animal more seriously. Right now you are pushing the patch with large chunks of code you've copied years ago and had not maintained since then. Sorry, but it's not a good idea.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |