From mboxrd@z Thu Jan  1 00:00:00 1970
From: Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [PATCH]QNX6 filesystem (RO) driver
Date: Sun, 12 Feb 2012 22:43:58 +0000
Message-ID: <20120212224358.GW23916@ZenIV.linux.org.uk>
References: <4F29A37F.6070909@ontika.net>
 <20120212045629.GO23916@ZenIV.linux.org.uk>
 <4F3839CE.8040805@ontika.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: linux-fsdevel@vger.kernel.org
To: Kai Bankett <chaosman@ontika.net>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:32991 "EHLO
	ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754173Ab2BLWn7 (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Sun, 12 Feb 2012 17:43:59 -0500
Content-Disposition: inline
In-Reply-To: <4F3839CE.8040805@ontika.net>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Sun, Feb 12, 2012 at 11:14:38PM +0100, Kai Bankett wrote:

re junk removal from qnx4 - sure, just make is a separate patch.

> >qnx6_iget() - what, they really don't allow named pipes/sockets/device
> >nodes on the filesystem?  If not, you can just use init_special_inode()
> >instead of that printk+iget_failed().
> At least so far I could not find any additional extras.
> I followed your advise and used init_special_inode() to get rid of
> that printk+stuff.

Umm...  How does qnx encode device numbers for block/character devices?

> Yes, I have the name length in the directory entry, but only for
> short filenames.
> For long filenames the length field in the directory entry is always 0xff.
> I just moved the length check up by one if-clause.
> Personally I don't think that moving it around further makes that
> much sense, as the length for short filenames is also checked before
> copying the filename.
> The way it currently is it's at least symmetrical between short und
> long filename handling code.
> 
> I hope I got all the points and fixes right?

A few more things:
	* duplicating di_block_ptr[] array seems to be pointless
	* qnx6_get_inode_loc(), qnx6_dir_lfile_block() and qnx6_block_map()
seem to have a lot of duplicated code, at the very least.  Looks like
a missing helper function...  Incidentally, I'd switched qnx6_get_devblock()
and qnx6_check_blockptr() to __fs32 argument, along with making QNX6_PTR_UNSET
(~(__fs32)0)
	* what is actually stored in ->de_size for long entries in case
of big-endian fs?  4 bytes of de_inode - fine, as as for short ones, but
then what?  If it's really 32bit big-endian 0xff, it will have the first byte
equal to 0, which would confuse the living hell out of your code.  Or is
it actually __u8 + 3 bytes of padding?  Then it would be compatible with
short dir entries, but conversions would be the wrong thing to do there...
	* I seriously suspect that you want to cook yourself a couple of
unhashed in-core struct inode for Longfile and Inode ones; then get_inode_loc(),
dir_lfile_block() and block_map() would become identical *and* you just might
be able to use page cache instead of all that messing with buffer_heads in
readdir et.al.  Just do new_inode() and fill di_block_ptr/levels/i_mode/i_size
and ->a_ops.  iput() both in ->put_super() and that's it...  For directories
in pagecache see how e.g. ext2 is doing it - or minixfs, for that matter.
	* mmu_private is never used.  Just lose it...