Hi Christoph,
On Wed, Dec 09, 2020 at 08:52:46AM +0100, Christoph Hellwig wrote:
> > + /* Initialise the newly allocated inode. */
> > + return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
>
> IMHO this comment is not overly helpful..
That was inherited from old version, I could get rid of in the next version....
>
> > + if (IS_ERR(ip)) {
> > + error = PTR_ERR(ip);
> > + ip = NULL;
> > goto out_trans_cancel;
> > + }
>
> And the calling convention with the ERR_PTR return does not seem to
> fit the call chain to well. But those are minor details, so:
Yeah, I also think so, since I found many error exit paths
rely on ip == NULL.
>
> > STATIC int
> > xfs_qm_qino_alloc(
> > - xfs_mount_t *mp,
> > - xfs_inode_t **ip,
> > - uint flags)
> > + struct xfs_mount *mp,
> > + struct xfs_inode **ipp,
> > + unsigned int flags)
> > {
> > xfs_trans_t *tp;
> > int error;
> > bool need_alloc = true;
>
> Why do you reindent and de-typdefify the arguments, but not the local
> variables?
since I renamed *ip to *ipp (it seems that should be *ipp here), so I need to
modify this line
- xfs_inode_t **ip,
so I fixed the typedef, but it introduced an intent issue, so I fixed the
whole input argument block for better coding style.
That is related to the argument only, so I didn't fix the local argument
though (since I didn't touch them).
>
> All the stuff below also seems to deal with the fact that the old return
> ip by reference calling convention seems to actually work better with
> the code base..
Yeah, so maybe I should revert back to the old code? not sure... Anyway,
I think codebase could be changed over time from a single change. Anyway,
I'm fine with either way. So I may hear your perference about this and send
out the next version (I think such cleanup can be fited in 5.11, so I can
base on this and do more work....)
Thanks,
Gao Xiang
>
