Re: [PATCH]QNX6 filesystem (RO) driver

[Kai Bankett <chaosman@ontika.net>]

Updated patch: http://a6.ontika.net/patches/patch-3.2.5-qnx6.gz
Signed-off-by: Kai Bankett <chaosman@ontika.net>

On 02/12/2012 05:56 AM, Al Viro wrote:
> On Wed, Feb 01, 2012 at 09:41:35PM +0100, Kai Bankett wrote:
>
>> - Endianness support
>> QNX6 offers to create filesystems either little- or big endian. That's
>> an advantage if the created filesystem later is used on an architecture
>> that got a different endianness.
>> I introduced some TO_CPUxx() macros and a superblock detection mechanism
>> for detecting the FS endianness.
>> Could not test it on a different architecture than x86 and therefore I'm
>> unsure if it works on a different endian linux system, but from (my)
>> theory it should do.
>> At least on x86 it's no problem to mount a different endian QNX6 FS and
>> read from it - that's what I've tested.
>> If the macros are the right approach ... well, happy to receive feedback ;)
> Nope, that's not the right way to do it.  Don't use __leNN for that; define
> your own types.  Take a look at e.g. fs/sysv/sysv.h - __fs{16,32} in there
> and functions for work with those.  And duplicate that.  What's more, you
> really want the direction of conversion spelled out; sure, it's an involution
> and you'll get the same code in both directions.  But it's much easier to
> verify that conversions are right that way.  sparse is useful for that, BTW.
> And don't use the same variable for host- and fs-endian values - it's not
> worth the PITA.

Thanks alot. That was exactly what I was looking for when implementing 
that TO_CPU stuff.
Looked around in some other fs drivers, but did not have a look in the sysv.
You are right, using the alignment functions asymetric makes 
understanding the code far easier as one can directly see in which 
direction things are moved.
At least sparse seems to be fine with my work ...

>
> Don't bother with struct qnx6_ptr - sparse _will_ complain about all places
> where you do arithmetics on __bitwise types.
>
> While we are at it, please use explicitly-sized type when you are dealing
> with disk block numbers.  And pick unsigned one...
I decided to use unsigned int. All adressing is using 32bit pointers. So 
this seems to make most sense to me.
>
> Note that unsigned long is target-dependent; on some it's 64bit, on some
> 32bit.  For some things that's exactly what we want, for some...
Fixed. I decided to stay with unsigned int. Just a couple of blocks (12 
blocks of 32bit).
So, in case someone is really creating a full 32bit * blocksize fs, 
there could be a problem.
I currently can not test how QNX would deal with that special situation 
- sorry.
I hope that's ok ...
>
> qnx6_find_bits(): what's wrong with ilog2()?  Or order_base_2(), if you
> care about the case when argument itself may be not a power of 2.  Both
> are in linux/log2.h...
Great. Exactly what I was looking for!
Fixed.
>
> sb_set_blocksize() may fail; check return value...
Fixed
> WTH are writepage and friends doing in read-only fs driver, especially
> seeing that you don't seem to have anything resembling a block allocator?
Fixed
Those were leftovers from the qnx4 fs driver.
Maybe it would be a good idea if I create a patch for the qnx4 driver 
for removing those?

> 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.
>
> _Never_ do strlen() on unsanitized on-disk data; you can't guarantee a
> terminating '\0' in there.
Fixed.
Full ack. One more leftover from the qnx4 driver.
However, it seems that the qnx4 driver cannot live without. (at least 
from a quick check, I'm not deep enough in the qnx4 fs)
> +       /* "" means "." --->  so paths like "/usr/lib//libc.a" work */
> +       if (!len&&  (lf->lf_fname[0] == '.')&&  (lf->lf_fname[1] == '\0'))
> +               return TO_CPU32(dir->i_sb, de->de_inode);
>
> Huh?  That thing is not going to get called with len == 0 and paths like
> that are handled in fs/namei.c just fine without forcing every fs driver
> to do that kind of contortions.  Moreover, . and .. are _also_ handled
> there - ->lookup() never has to deal with those.
Removed.
Leftover from qnx4 driver.
In my eyes that one could well be also removed from there.
> Incidentally, don't you have the name length right in the directory
> entry?  Or is it just an upper limit?  If it's exact, it would be faster
> to reject mismatches without looking at the name contents (and do memcmp()
> in case of match).
>
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?

Additionally I removed the old bitmap.c. Currently it was not used anyways.
I also cleaned up the previously (at least in my eyes) ugly 
qnx6_super_block and mmi superblock structure.
As the superblock root nodes (inode, bitmap and longfilename) all got 
the same structure, I've added a qnx6_root_inode structure for each root 
node in the superblock.

Thank you very much for the extremely helpful feedback!

Kai