lkml.org 
[lkml]   [2000]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: (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/

\
 
 \ /
  Last update: 2005-03-22 13:57    [W:0.242 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site