Re: [PATCH]QNX6 filesystem (RO) driver

[Al Viro <viro@ZenIV.linux.org.uk>]

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.

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...

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...

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...

sb_set_blocksize() may fail; check return value...

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?

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().

_Never_ do strlen() on unsanitized on-disk data; you can't guarantee a
terminating '\0' in there.

+       /* "" 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.

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).