Re: [PATCH 2/3] fsstack: Generic get/set lower object functions

[Mark Williamson <mark.williamson@cl.cam.ac.uk>]

> > Why are you defining all these structs that are just wrapping unions?
>
> The reason for the union is simple...

I think the query was more about why you'd need a struct which contains only 
an anonymous union - why not just have a named union?  Or do you anticipate 
adding extra fields to the struct later?

I guess that having a union foo * rather than a struct foo * would be a bit 
unconventional in the kernel.  The named struct / anonymous union combo does 
hide the union as merely an implementation detail, which is nice.  Was this 
your motivation?

Cheers,
Mark

> 1) if you have a linear stackable filesystem (e.g., ecryptfs), your objects
> need to point to only one lower object (sb -> lower sb, etc.)
>
> 2) if you have a fanout stackable filesystem (e.g., unionfs), your objects
> need to point to n lower objects
>
> Since we don't want to hurt linear stacks by declaring arrays of pointers,
> I think the best way is to have the lower pointer (e.g., *sb) in a union
> with the lower double pointer (e.g., **sbs) - this works simply because
> linear stacks will always
>
> > > +/* get the fs dependent data */
> > > +static inline void * fsstack_inode_data(struct inode *inode)
> > > +{
> > > +	return &((struct __fsstack_inode_generic_info*) inode)->info;
> >
> > Please make this wrap container_of() instead of rolling your own.
>
> Will do.
>
> > Also note that the naming convention for such a wrapper in almost all
> > other filesystems would be FSSTACK_I()
>
> Very true, I'll change it to match.
>
> I suppose the function to get the dentry private data should be called
> FSSTACK_D() and for the superblock FSSTACK_SB() ?
>
> > > +static inline struct inode *
> > > +__fsstack_lower_inode(struct inode *inode, unsigned long branch_idx)
> > > +{
> > > +	struct fsstack_inode_info *info = fsstack_inode_data(inode);
> > > +
> > > +	return info->inodes[branch_idx];
> > > +}
> >
> > What is the value of "functions" like the above? They appear just to
> > obfuscate the code. Unless your aim is to hide the internals of the
> > struct __fsstack_inode_generic_info (sort of futile, since you are
> > asking users to include that structure in their private inode structs
>
> Another idea is to move the fsstack_foo_info structure elsewhere...
>
> For stackable filesystems it makes sense to have the pointers right in the
> inode, but we don't want to penalize the rest if the filesystems. One
> solution would be to have a special stackable_inode which contains the
> lower inode pointers. This would still hide the details from the user...
>
> > )
> > then it is much more obvious to see what is going on when you write
> >
> > 	inode = FSSTACK_I(inode)->inodes[branch];
> >
> > rather than
> >
> > 	inode = __fsstack_lower_inode(inode, branch);
>
> Point taken. You need to know what's going to anyway, so we might as well
> make it painfully obvious.
>
> Josef "Jeff" Sipek.

-- 
Dave: Just a question. What use is a unicyle with no seat?  And no pedals!
Mark: To answer a question with a question: What use is a skateboard?
Dave: Skateboards have wheels.
Mark: My wheel has a wheel!