On Fri, Jan 02, 2009 at 08:05:50PM +0100, Andi Kleen wrote: > Some items I remember from my last look at the code that should > be cleaned up before mainline merge (that wasn't a full in depth review): > > - locking.c needs a lot of cleanup. > If combination spinlocks/mutexes are really a win they should be > in the generic mutex framework. And I'm still dubious on the hardcoded > numbers. I don't think this needs to be cleaned up before merge. I've spent an hour or two looking at it, and while we can do a somewhat better job as part of the generic mutex framework, it's quite tricky (due to the different <asm/mutex.h> implementations). It has the potential to introduce some hard-to-hit bugs in the generic mutexes, and there's some API discussions to have. It's no worse than XFS (which still has its own implementation of 'synchronisation variables', a (very thin) wrapper around mutexes, a (thin) wrapper around rwsems, and wrappers around kmalloc and kmem_cache. > - compat.h needs to go Later. It's still there for XFS. > - there's various copy'n'pasted code from the VFS (like may_create) > that needs to be cleaned up. No urgency here. > - there should be manpages for all the ioctls and other interfaces. I wonder if Michael Kerrisk has time to help with that. Cc'd. > - ioctl.c was not explicitely root protected. security issues? This does need auditing. > - some code was severly undercommented. > e.g. each file should at least have a one liner > describing what it does (ideally at least a paragraph). Bad examples > are export.c or free-space-cache.c, but also others. Nice to have, but generally not required. > - ENOMEM checks are still missing all over (e.g. with most of the > btrfs_alloc_path callers). If you keep it that way you would need > at least XFS style "loop for ever" alloc wrappers, but better just > fix all the callers. Also there used to be a lot of BUG_ON()s on > memory allocation failure even. > - In general BUG_ONs need review I think. Lots of externally triggerable > ones. Agreed on these two. > - various checkpath.pl level problems I think (e.g. printk levels) Can be fixed up later. > - the printks should all include which file system they refer to Ditto. -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
