xfs
[Top] [All Lists]

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

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH v9 1/6] xfs: Move code around and remove typedefs
From: Chandra Seetharaman <sekharan@xxxxxxxxxx>
Date: Mon, 24 Jun 2013 18:23:18 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130624213656.GA20932@xxxxxxx>
Organization: IBM
References: <1372042107-27332-1-git-send-email-sekharan@xxxxxxxxxx> <1372042107-27332-2-git-send-email-sekharan@xxxxxxxxxx> <20130624213656.GA20932@xxxxxxx>
Reply-to: sekharan@xxxxxxxxxx
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
> 
> 


<Prev in Thread] Current Thread [Next in Thread>