On Fri, 2023-01-06 at 07:18 -0500, Brian Masney wrote:
> On Mon, Nov 28, 2022 at 12:17:12PM +0100, Alexander Larsson wrote:
> > This is the basic inode and filesystem implementation.
> >
> > Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
> > Signed-off-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
>
> Note: I'm looking at this from the VFS viewpoint since I haven't done
> anything in this subsystem. Just looking for some generic
> suggestions.
>
> >
> > +
> > +struct cfs_inode {
> > + /* must be first for clear in cfs_alloc_inode to work */
> > + struct inode vfs_inode;
>
> [ snip ]
>
> > +static struct inode *cfs_alloc_inode(struct super_block *sb)
> > +{
> > + struct cfs_inode *cino =
> > + alloc_inode_sb(sb, cfs_inode_cachep, GFP_KERNEL);
> > +
> > + if (!cino)
> > + return NULL;
> > +
> > + memset((u8 *)cino + sizeof(struct inode), 0,
> > + sizeof(struct cfs_inode) - sizeof(struct inode));
>
> Why not use container_of() to look up the vfs_inode and then you can
> get
> rid of the restriction of this being first. This may also break with
> structure randomization turned on.
This is not clearing vfs_inode though, it is clearing everything after
vfs_inode, because we're using an allocation cache for the vfs_inode
part. I don't see how container_of can help us here?
Also, surely structure randomization won't change the offset of the
first element of the struct? That will break all sorts of "derived
object" usecases.
>
> > +static inline struct cfs_inode *CFS_I(struct inode *inode)
>
> CFS_I in upper case doesn't match naming conventions in the rest of
> the kernel.
>
It is similar to VFS_I, XFS_I, BTRFS_I, EXT4_I, etc
> > +static unsigned int cfs_split_basedirs(char *str)
> > +{
> > + unsigned int ctr = 1;
> > + char *s, *d;
> > +
> > + for (s = d = str;; s++, d++) {
> > + if (*s == '\\') {
> > + s++;
> > + } else if (*s == ':') {
> > + *d = '\0';
> > + ctr++;
> > + continue;
> > + }
> > + *d = *s;
> > + if (!*s)
> > + break;
> > + }
> > + return ctr;
> > +}
>
> To expand on the comment, this is ovl_split_lowerdirs in
> fs/overlayfs/super.c. It'd be nice if there was a common place where
> this could go.
>
It would be nice, but its not like a huge amount of code duplication,
as it is rather specialized.
>
>
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a superhumanly strong neurotic barbarian with no name. She's a
blind
cigar-chomping bounty hunter from a different time and place. They
fight
crime!
