Christoph Hellwig <hch@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
