On Mon, 2013-06-24 at 16:36 -0500, Ben Myers wrote:
> Hi Chandra,
>
> On Sun, Jun 23, 2013 at 09:48:22PM -0500, Chandra Seetharaman wrote:
> > Removed some typedefs, defined new functions, made some code clean up all in
> > preparation of the series.
> >
> > No functional changes.
> >
> > Signed-off-by: Chandra Seetharaman <sekharan@xxxxxxxxxx>
>
> As Dave mentioned, there are a few categories of changes that would probably be
> better as separate patches. I count 6:
>
> 1) the addition of xfs_is_quota_inode
>
> 2) conversion of XFS_DQUOT_TREE macro to xfs_dquot_tree inlined function
>
> 3) addition of xfs_dq_to_quota_inode
>
> 4) cleanups in xfs_dquot.c (xfs_qm_quotacheck,xfs_qm_init_quotainos,
> xfs_qm_vop_dqalloc,xfs_qm_vop_chown_reserve) and xfs_trans_dquot.c
> (xfs_trans_reserve_quota_bydquots)
>
> 5) changes to struct xfs_quotainfo. I don't have an aversion to the comments
> in the structure. I'm guessing that you updated from xfs_inode_t to
> struct xfs_inode and then ran into an 80 column issue later in the
> structure. It'd be nice to have the entries all line up, but I think
> your compromise is fine. Might have been better off touching only
> the quota inodes and leaving the rest, but... style.
>
> 6) add dqtypes enum, make an array in xfs_dquot_acct, update
> xfs_trans_dup_dqinfo. Now we have a two dimensional array in there, and
> I wonder if it would be better if it were more strongly typed. e.g.
>
> tp->t_dqinfo->dq_type[j]->dqt_ents[i]
>
> (or something)
I will repost based on these suggestions.
<snip>
> > @@ -1697,14 +1695,14 @@ xfs_qm_vop_dqalloc(
> > * holding ilock.
> > */
> > xfs_iunlock(ip, lockflags);
> > - if ((error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t) uid,
> > + error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t) uid,
> > XFS_DQ_USER,
> > XFS_QMOPT_DQALLOC |
> > XFS_QMOPT_DOWARN,
> > - &uq))) {
> > - ASSERT(error != ENOENT);
> > + &uq);
> > + ASSERT(error != ENOENT);
>
> You have a series of these asserts which used to be in the error case taken out
> of the error case. Clearly the assertion is still true when error == 0... but
> I tend to prefer that it still be in the error curlies. A silly style thing
> which I welcome you to reject with prejudice.
This way I avoided a set of curly-braces for the error case. thats all.
>
<snip>
> > @@ -113,7 +111,9 @@ xfs_trans_dup_dqinfo(
> > if(otp->t_flags & XFS_TRANS_DQ_DIRTY)
> > ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
> >
> > - for (j = 0; j < 2; j++) {
> > + for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> > + oqa = otp->t_dqinfo->dqs[j];
> > + nqa = ntp->t_dqinfo->dqs[j];
> > for (i = 0; i < XFS_QM_TRANS_MAXDQS; i++) {
> > if (oqa[i].qt_dquot == NULL)
> > break;
> > @@ -138,8 +138,6 @@ xfs_trans_dup_dqinfo(
> > oq->qt_ino_res = oq->qt_ino_res_used;
> >
>
> Could clean up this extra line...
will do
>
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
