Re: [PATCH v9 1/6] xfs: Move code around and remove typedefs

[Chandra Seetharaman <sekharan@us.ibm.com>]

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@us.ibm.com>
> 
> 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs