Re: [PATCH] Update the exportfs documentation and comments and fix some exportfs issues

[David Howells <dhowells@redhat.com>]

Christoph Hellwig <hch@infradead.org> wrote:

> Once you change the prototype please also pass the struct fid * instead
> of the current __u32 array, like the fh_to_dentry / fh_to_parent
> methods.

I can do that, but why not pass a u32* or void* instead in all cases?  The
operations are under no obligation to use struct fid.

> >  (*) The exportfs_encode_fh() function has it's acceptable() function pointer
> >      typedef'd.
> 
> I don't really see the point for that, but I don't care too much.

It's passed around between a number of functions and it makes the function
declarations more readable.

> These two should probably be broken out into a smaller patch, before all
> the cosmetic bits.

Okay.

> > +A filehandle fragment consists of an array of 1 or more 32-bit words.  The
> > +contents and the layout of the data in the array are entirely at the whim of
> > +the filesystem that generated it.
> 
> Not really true anymore, as we have a real struct for it now.

Which isn't used by all exportable filesystems...  Should those filesystems
have specific entries added to the union in struct fid?  Should they be
required to use the raw member of struct fid's union?

In fact, why struct fid at all: why not union fid?

It also seems odd to declare a struct that only has passing acquaintance with
reality.  struct fid is rarely used to its full extent, and may not even be
big enough - something that's decided on a per-use basis.

Furthermore, when the decode routines are called, less data may be presented
than is required to fill out a struct fid, so overlaying a struct fid on it
may include uninitialised data.

As I see it, struct fid doesn't gain anything - except perhaps documentary
purposes for people looking to build packet sniffers.  Even then it's not
really necessary, and nor is it complete.

The i32 branch of struct fid could be hidden inside fs/expfs.c, and the udf
branch inside fs/udf/.  The layouts could then be documented inside the
Documentation directory.

> But all the length calculation is still done in number of 4 byte words.
> Messy..

Yes.  The docs and comments either didn't specify the units, or stipulated
that they were in bytes:-(

This is one reason why I think passing a u32* might be better than struct fid
- that way the length is more obviously the size of the array in u32 units.

> Why the strange indentation?  Two tabs indent for spillover prototypes
> is pretty much standard..

So is aligning the spillover horizontally with the start of the scope (if
that's the right term).  It makes more sense too because relative indentation
gives a visual clue to relative significance.

> > +#define FAT_FILEID	((enum fid_type) 3)
> 
> That cast is completely pointless in C.

True, I suppose.

David