From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH] ASFS filesystem driver, kernel 2.4.xx and 2.6.x Date: Mon, 7 Jun 2004 10:56:25 +0100 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <20040607095625.GA18710@infradead.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, Sven Luther , Christoph Hellwig Return-path: Received: from [213.146.154.40] ([213.146.154.40]:47821 "EHLO pentafluge.infradead.org") by vger.kernel.org with ESMTP id S264379AbUFGJ40 (ORCPT ); Mon, 7 Jun 2004 05:56:26 -0400 To: Marek Szyprowski Content-Disposition: inline In-Reply-To: List-Id: linux-fsdevel.vger.kernel.org - asfs_fs.h, asfs_fs_i.h and asfs_fs_sb.h don't belong to include/linux but fs/asfs (and get saner names while you're at it) - there's a bunch of files completely under ifdef CONFIG_ASFS_RW, just make them depend on that in the makefile instead using asfs-y += dir.o ... asfs-$(CONFIG_ASFS_RW) += adminspace.o bitfuncs.o ... - names like setfreeblocks are a little too generic, please give them asfs_ prefixes - you probably want to replace the routines in bitfuncs.c with usage of linux-native routines from bitops.h like find_first_bit(), etc wherever possible - asfs_parse_options should use lib/parser.c helpers - in asfs_fill_super you're leaking if d_alloc_root fails - you're using lock_super for a lot of internal synchronization, but you should implement your own locking instead and document what each lock protects. - the filp->f_count checks in ->open and ->close are totally bogus - asfs_file_release looks very fishy. Why do you set ->datemodified on close, not on actual modification? - the whole handling of ->modified and reading the object from disk again when you want to write it back looks bogus. Why don't you - ->create, ->mkdir and ->mknod seem to share most of them code, shouldn't you use a common subroutine? Mayube dito for ->unlink and ->rmdir - please call asfs_delete asfs_unlink to match the method name. - this whole rececycled bin stuff makes me sick. Where is that handling encoded in the ondisk format? Can't you just do normal unix-style unlinks?