On Mon, Dec 07, 2020 at 02:49:41PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote:
> > /*
> > + * Initialise a newly allocated inode and return the in-core inode to the
> > + * caller locked exclusively.
> > */
> > -static int
> > -xfs_ialloc(
> > - xfs_trans_t *tp,
> > - xfs_inode_t *pip,
> > - umode_t mode,
> > - xfs_nlink_t nlink,
> > - dev_t rdev,
> > - prid_t prid,
> > - xfs_buf_t **ialloc_context,
> > - xfs_inode_t **ipp)
> > +static struct xfs_inode *
> > +xfs_dir_ialloc_init(
>
> This is boderline bikeshedding, but I would just call this
> xfs_init_new_inode.
>
> > int
> > xfs_dir_ialloc(
> > @@ -954,83 +908,59 @@ xfs_dir_ialloc(
> > xfs_inode_t **ipp) /* pointer to inode; it will be
> > locked. */
> > {
> > xfs_inode_t *ip;
> > xfs_buf_t *ialloc_context = NULL;
> > + xfs_ino_t pino = dp ? dp->i_ino : 0;
>
> Maybe spell out parent_inode? pino reminds of some of the weird Windows
> code that start all variable names for pointers with a "p".
>
> > + /* Initialise the newly allocated inode. */
> > + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid);
> > + if (IS_ERR(ip))
> > + return PTR_ERR(ip);
> > + *ipp = ip;
> > return 0;
>
> I wonder if we should just return the inode by reference from
> xfs_dir_ialloc_init as well, as that nicely fits the calling convention
> in the caller, i.e. this could become
>
> return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp);
>
> Note with the right naming we don't really need the comment either,
> as the function name should explain everything.
/me notes that some day he will get around to a formal posting of his
giant inode allocation cleanup series that will wrap these parameters
into a struct so we don't have to pass 8 arguments around, and fix the
inode flag inheritance inconsistencies between userspace and kernel...
--D
