[PATCH] xfs: fix variable set but not used warnings

[PATCH] xfs: fix variable set but not used warnings

[Christoph Hellwig]

GCC 4.6 now warnings about variables set but not used.  Fix the trivially
fixable warnings of this sort.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_buf.c	2011-04-03 06:47:59.406434458 -0700
+++ linux-2.6/fs/xfs/linux-2.6/xfs_buf.c	2011-04-03 06:48:16.276433605 -0700
@@ -293,7 +293,6 @@ xfs_buf_allocate_memory(
 	size_t			nbytes, offset;
 	gfp_t			gfp_mask = xb_to_gfp(flags);
 	unsigned short		page_count, i;
-	pgoff_t			first;
 	xfs_off_t		end;
 	int			error;
 
@@ -333,7 +332,6 @@ use_alloc_page:
 		return error;
 
 	offset = bp->b_offset;
-	first = bp->b_file_offset >> PAGE_SHIFT;
 	bp->b_flags |= _XBF_PAGES;
 
 	for (i = 0; i < bp->b_page_count; i++) {
Index: linux-2.6/fs/xfs/quota/xfs_qm.c
===================================================================
--- linux-2.6.orig/fs/xfs/quota/xfs_qm.c	2011-04-03 06:43:03.563116104 -0700
+++ linux-2.6/fs/xfs/quota/xfs_qm.c	2011-04-03 06:44:26.459778573 -0700
@@ -461,12 +461,10 @@ xfs_qm_dqflush_all(
 	struct xfs_quotainfo	*q = mp->m_quotainfo;
 	int			recl;
 	struct xfs_dquot	*dqp;
-	int			niters;
 	int			error;
 
 	if (!q)
 		return 0;
-	niters = 0;
 again:
 	mutex_lock(&q->qi_dqlist_lock);
 	list_for_each_entry(dqp, &q->qi_dqlist, q_mplist) {
@@ -1314,14 +1312,9 @@ xfs_qm_dqiter_bufs(
 {
 	xfs_buf_t	*bp;
 	int		error;
-	int		notcommitted;
-	int		incr;
 	int		type;
 
 	ASSERT(blkcnt > 0);
-	notcommitted = 0;
-	incr = (blkcnt > XFS_QM_MAX_DQCLUSTER_LOGSZ) ?
-		XFS_QM_MAX_DQCLUSTER_LOGSZ : blkcnt;
 	type = flags & XFS_QMOPT_UQUOTA ? XFS_DQ_USER :
 		(flags & XFS_QMOPT_PQUOTA ? XFS_DQ_PROJ : XFS_DQ_GROUP);
 	error = 0;
Index: linux-2.6/fs/xfs/quota/xfs_qm.h
===================================================================
--- linux-2.6.orig/fs/xfs/quota/xfs_qm.h	2011-04-03 06:43:34.569781201 -0700
+++ linux-2.6/fs/xfs/quota/xfs_qm.h	2011-04-03 06:43:41.396447521 -0700
@@ -65,11 +65,6 @@ extern kmem_zone_t	*qm_dqtrxzone;
  * block in the dquot/xqm code.
  */
 #define XFS_DQUOT_CLUSTER_SIZE_FSB	(xfs_filblks_t)1
-/*
- * When doing a quotacheck, we log dquot clusters of this many FSBs at most
- * in a single transaction. We don't want to ask for too huge a log reservation.
- */
-#define XFS_QM_MAX_DQCLUSTER_LOGSZ	3
 
 typedef xfs_dqhash_t	xfs_dqlist_t;
 
Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
===================================================================
--- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:40:45.399789765 -0700
+++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:43:00.219782939 -0700
@@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
 {
 	int		error;
 	uint		qf;
-	uint		accflags;
 	__int64_t	sbflags;
 
 	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
 	/*
 	 * Switching on quota accounting must be done at mount time.
 	 */
-	accflags = flags & XFS_ALL_QUOTA_ACCT;
 	flags &= ~(XFS_ALL_QUOTA_ACCT);
 
 	sbflags = 0;
Index: linux-2.6/fs/xfs/xfs_itable.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_itable.c	2011-04-03 06:47:24.483102893 -0700
+++ linux-2.6/fs/xfs/xfs_itable.c	2011-04-03 06:47:47.356435070 -0700
@@ -204,7 +204,6 @@ xfs_bulkstat(
 	xfs_agi_t		*agi;	/* agi header data */
 	xfs_agino_t		agino;	/* inode # in allocation group */
 	xfs_agnumber_t		agno;	/* allocation group number */
-	xfs_daddr_t		bno;	/* inode cluster start daddr */
 	int			chunkidx; /* current index into inode chunk */
 	int			clustidx; /* current index into inode cluster */
 	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
@@ -463,7 +462,6 @@ xfs_bulkstat(
 						 mp->m_sb.sb_inopblog);
 				}
 				ino = XFS_AGINO_TO_INO(mp, agno, agino);
-				bno = XFS_AGB_TO_DADDR(mp, agno, agbno);
 				/*
 				 * Skip if this inode is free.
 				 */

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[Alex Elder]

On Mon, 2011-04-04 at 08:55 -0400, Christoph Hellwig wrote:
> GCC 4.6 now warnings about variables set but not used.  Fix the trivially
> fixable warnings of this sort.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.  I have an unrelated question though.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:40:45.399789765 -0700
> +++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:43:00.219782939 -0700
> @@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
>  {
>  	int		error;
>  	uint		qf;
> -	uint		accflags;
>  	__int64_t	sbflags;
>  
>  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
>  	/*
>  	 * Switching on quota accounting must be done at mount time.
>  	 */
> -	accflags = flags & XFS_ALL_QUOTA_ACCT;
>  	flags &= ~(XFS_ALL_QUOTA_ACCT);

Unrelated, but isn't the effect of this line plus the one a few
lines up the same as this?

	flags &= XFS_ALL_QUOTA_ENFD;

>  
>  	sbflags = 0;

. . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[David Sterba]

Hi,

I've seen this one too and I think this may not be a trivialy removable one, at
least not without some analysis.

On Mon, Apr 04, 2011 at 01:21:20PM -0500, Alex Elder wrote:
> > Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
> > ===================================================================
> > --- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:40:45.399789765 -0700
> > +++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:43:00.219782939 -0700
> > @@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
> >  {
> >  	int		error;
> >  	uint		qf;
> > -	uint		accflags;
> >  	__int64_t	sbflags;
> >  
> >  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
> >  	/*
> >  	 * Switching on quota accounting must be done at mount time.
> >  	 */
> > -	accflags = flags & XFS_ALL_QUOTA_ACCT;
> >  	flags &= ~(XFS_ALL_QUOTA_ACCT);
> 
> Unrelated, but isn't the effect of this line plus the one a few
> lines up the same as this?
> 
> 	flags &= XFS_ALL_QUOTA_ENFD;

yes it its, but then why was the separate accflags there? That's why I'm not
sure it should go away so easily.

> 
> >  
> >  	sbflags = 0;

and not only this, a few lines below:

        if (((flags & XFS_UQUOTA_ACCT) == 0 &&
              ^^^^^^^^^^^^^^^^^^^^^^^

            (mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 &&
            (flags & XFS_UQUOTA_ENFD))
             ^^^^^^^^^^^^^^^^^^^^^^^

            ||
            ((flags & XFS_PQUOTA_ACCT) == 0 &&
              ^^^^^^^^^^^^^^^^^^^^^^^

            (mp->m_sb.sb_qflags & XFS_PQUOTA_ACCT) == 0 &&
            (flags & XFS_GQUOTA_ACCT) == 0 &&
             ^^^^^^^^^^^^^^^^^^^^^^^

            (mp->m_sb.sb_qflags & XFS_GQUOTA_ACCT) == 0 &&
            (flags & XFS_OQUOTA_ENFD))) {

                xfs_debug(mp,
                        "%s: Can't enforce without acct, flags=%x sbflags=%x\n",
                        __func__, flags, mp->m_sb.sb_qflags);
                return XFS_ERROR(EINVAL);
        }

(the GQUOTA/PQUOTA/UQUOTA are covered by XFS_ALL_QUOTA_ACCT, masked above)

all the underlined expressions will be always zero, leading to a true ==
condition and leaving only the superblock flags to be checked.

Callchain of xfs_qm_scall_quotaon() goes up to

quotactl
  do_quotactl
    quota_setxstate
      .set_xstate
        xfs_fs_set_xstate
          xfs_qm_scall_quotaon

where the final flags/accflags are passed from ioctl uflags and then
converted to XFS quota flags. Ie., the caller of the quotactl has to set
the ACCT flags, therefore I'd say the checked flags should state

      accflags & XFS_GQUOTA_ACCT        (...etc)

else, if the ACCT flags are not specified via superblock, the quotaon
sysctl will always fail with "Can't enforce without acct", even when
the accounting flags are specified via a mount option (speculated and
not tested myself).

The code is there since ages so I wonder whether anybody hit this bug or
if I misunderstood xfs/quota documentation.

Besides, there are more cleanups and fixups in this function:

line 331:
        /* No fs can turn on quotas with a delayed effect */
	ASSERT((flags & XFS_ALL_QUOTA_ACCT) == 0);

seems to be broken too:
* the acct flags are masked out -> always asserts true
* doing s/flags/accflags/ does not make sense

line 374:
        if ((error = xfs_qm_write_sb_changes(mp, sbflags)))
	                return (error);

convert to:

	error = xfs_qm_...
	if (error) ...

line 395:
        mp->m_qflags |= (flags & XFS_ALL_QUOTA_ENFD);

->

        mp->m_qflags |= flags;

I could send a patch, but I want to be clear on the accounting flags.


I'd like to ask maintainers to be cautious when accepting these
simply looking cleanups, they may actually hide bugs.


dave

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[David Sterba]

Hi,

besides the accflags commented in another mail, the rest is ok, but I'll
flush my finding anyway, JFYI.

dave

On Mon, Apr 04, 2011 at 08:55:44AM -0400, Christoph Hellwig wrote:
> GCC 4.6 now warnings about variables set but not used.  Fix the trivially
> fixable warnings of this sort.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_buf.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_buf.c	2011-04-03 06:47:59.406434458 -0700
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_buf.c	2011-04-03 06:48:16.276433605 -0700
> @@ -293,7 +293,6 @@ xfs_buf_allocate_memory(
>  	size_t			nbytes, offset;
>  	gfp_t			gfp_mask = xb_to_gfp(flags);
>  	unsigned short		page_count, i;
> -	pgoff_t			first;
>  	xfs_off_t		end;
>  	int			error;
>  
> @@ -333,7 +332,6 @@ use_alloc_page:
>  		return error;
>  
>  	offset = bp->b_offset;
> -	first = bp->b_file_offset >> PAGE_SHIFT;

leftover from

commit 0e6e847ffe37436e331c132639f9f872febce82e
Author: Dave Chinner <dchinner@redhat.com>
Date:   Sat Mar 26 09:16:45 2011 +1100
    xfs: stop using the page cache to back the buffer cache

originally used by find_or_create_page()

>  	bp->b_flags |= _XBF_PAGES;
>  
>  	for (i = 0; i < bp->b_page_count; i++) {
> Index: linux-2.6/fs/xfs/quota/xfs_qm.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/quota/xfs_qm.c	2011-04-03 06:43:03.563116104 -0700
> +++ linux-2.6/fs/xfs/quota/xfs_qm.c	2011-04-03 06:44:26.459778573 -0700
> @@ -461,12 +461,10 @@ xfs_qm_dqflush_all(
>  	struct xfs_quotainfo	*q = mp->m_quotainfo;
>  	int			recl;
>  	struct xfs_dquot	*dqp;
> -	int			niters;
>  	int			error;
>  
>  	if (!q)
>  		return 0;
> -	niters = 0;
>  again:
>  	mutex_lock(&q->qi_dqlist_lock);
>  	list_for_each_entry(dqp, &q->qi_dqlist, q_mplist) {
> @@ -1314,14 +1312,9 @@ xfs_qm_dqiter_bufs(
>  {
>  	xfs_buf_t	*bp;
>  	int		error;
> -	int		notcommitted;
> -	int		incr;
>  	int		type;
>  
>  	ASSERT(blkcnt > 0);
> -	notcommitted = 0;
> -	incr = (blkcnt > XFS_QM_MAX_DQCLUSTER_LOGSZ) ?
> -		XFS_QM_MAX_DQCLUSTER_LOGSZ : blkcnt;

dead variables since the original git commit

>  	type = flags & XFS_QMOPT_UQUOTA ? XFS_DQ_USER :
>  		(flags & XFS_QMOPT_PQUOTA ? XFS_DQ_PROJ : XFS_DQ_GROUP);
>  	error = 0;
> Index: linux-2.6/fs/xfs/quota/xfs_qm.h
> ===================================================================
> --- linux-2.6.orig/fs/xfs/quota/xfs_qm.h	2011-04-03 06:43:34.569781201 -0700
> +++ linux-2.6/fs/xfs/quota/xfs_qm.h	2011-04-03 06:43:41.396447521 -0700
> @@ -65,11 +65,6 @@ extern kmem_zone_t	*qm_dqtrxzone;
>   * block in the dquot/xqm code.
>   */
>  #define XFS_DQUOT_CLUSTER_SIZE_FSB	(xfs_filblks_t)1
> -/*
> - * When doing a quotacheck, we log dquot clusters of this many FSBs at most
> - * in a single transaction. We don't want to ask for too huge a log reservation.
> - */
> -#define XFS_QM_MAX_DQCLUSTER_LOGSZ	3
>  
>  typedef xfs_dqhash_t	xfs_dqlist_t;
>  
> Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:40:45.399789765 -0700
> +++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:43:00.219782939 -0700
> @@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
>  {
>  	int		error;
>  	uint		qf;
> -	uint		accflags;
>  	__int64_t	sbflags;
>  
>  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
>  	/*
>  	 * Switching on quota accounting must be done at mount time.
>  	 */
> -	accflags = flags & XFS_ALL_QUOTA_ACCT;

(commented in another mail)

>  	flags &= ~(XFS_ALL_QUOTA_ACCT);
>  
>  	sbflags = 0;
> Index: linux-2.6/fs/xfs/xfs_itable.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/xfs_itable.c	2011-04-03 06:47:24.483102893 -0700
> +++ linux-2.6/fs/xfs/xfs_itable.c	2011-04-03 06:47:47.356435070 -0700
> @@ -204,7 +204,6 @@ xfs_bulkstat(
>  	xfs_agi_t		*agi;	/* agi header data */
>  	xfs_agino_t		agino;	/* inode # in allocation group */
>  	xfs_agnumber_t		agno;	/* allocation group number */
> -	xfs_daddr_t		bno;	/* inode cluster start daddr */
>  	int			chunkidx; /* current index into inode chunk */
>  	int			clustidx; /* current index into inode cluster */
>  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
> @@ -463,7 +462,6 @@ xfs_bulkstat(
>  						 mp->m_sb.sb_inopblog);
>  				}
>  				ino = XFS_AGINO_TO_INO(mp, agno, agino);
> -				bno = XFS_AGB_TO_DADDR(mp, agno, agbno);

a leftover from

commit 7b6259e7a83647948fa33a736cc832310c8d85aa
Author: Dave Chinner <dchinner@redhat.com>
Date:   Thu Jun 24 11:35:17 2010 +1000
    xfs: remove block number from inode lookup code

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[Dave Chinner]

On Tue, Apr 05, 2011 at 01:52:27AM +0200, David Sterba wrote:
> Hi,
> 
> I've seen this one too and I think this may not be a trivialy removable one, at
> least not without some analysis.

Sure.

Background: the quotaon syscall on XFS can only change whether
quotas are enforced or not in XFS. It can't switch quotas on at
all because that has to be done at mount time. Switching quotas on
at mount time sets the appropriate flags in the superblock, so we
can always rely on a superblock flag check for whether accounting
is enabled or not.

> On Mon, Apr 04, 2011 at 01:21:20PM -0500, Alex Elder wrote:
> > > Index: linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c
> > > ===================================================================
> > > --- linux-2.6.orig/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:40:45.399789765 -0700
> > > +++ linux-2.6/fs/xfs/quota/xfs_qm_syscalls.c	2011-04-03 06:43:00.219782939 -0700
> > > @@ -313,14 +313,12 @@ xfs_qm_scall_quotaon(
> > >  {
> > >  	int		error;
> > >  	uint		qf;
> > > -	uint		accflags;
> > >  	__int64_t	sbflags;
> > >  
> > >  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
> > >  	/*
> > >  	 * Switching on quota accounting must be done at mount time.
> > >  	 */
> > > -	accflags = flags & XFS_ALL_QUOTA_ACCT;
> > >  	flags &= ~(XFS_ALL_QUOTA_ACCT);
> > 
> > Unrelated, but isn't the effect of this line plus the one a few
> > lines up the same as this?
> > 
> > 	flags &= XFS_ALL_QUOTA_ENFD;
> 
> yes it its, but then why was the separate accflags there? That's why I'm not
> sure it should go away so easily.

It's dead, Jim.

>From the historical XFS archives, the accflags were used when the
root filesystem was special and quota accounting for the root
filesystem was not done at mount time. That was removed in late
2002:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commit;h=3e01b37c1ba0a3839956660b78e09a17ac5ff67b
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=archive/xfs-import.git;a=commitdiff;h=3e01b37c1ba0a3839956660b78e09a17ac5ff67b

so accflags has been unused for almost 10 years. I've gone through
quite a bit more of the history, but thæt commit it the key one.

> and not only this, a few lines below:
> 
>         if (((flags & XFS_UQUOTA_ACCT) == 0 &&
>               ^^^^^^^^^^^^^^^^^^^^^^^

The above commit should have removed these checks are they were not
needed anymore.

> 
>             (mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 &&
>             (flags & XFS_UQUOTA_ENFD))
>              ^^^^^^^^^^^^^^^^^^^^^^^

And those are the checks that are meaningful.

> The code is there since ages so I wonder whether anybody hit this bug or
> if I misunderstood xfs/quota documentation.

Not a bug. Crufty, yes, but functioning correctly.

> Besides, there are more cleanups and fixups in this function:
> 
> line 331:
>         /* No fs can turn on quotas with a delayed effect */
> 	ASSERT((flags & XFS_ALL_QUOTA_ACCT) == 0);
> 
> seems to be broken too:
> * the acct flags are masked out -> always asserts true
> * doing s/flags/accflags/ does not make sense

No, that makes perfect sense given the commit above. We use ASSERT()
statements to document constraints and assumptions in the code, and
that is exactly what this does....

> I'd like to ask maintainers to be cautious when accepting these
> simply looking cleanups, they may actually hide bugs.

Well, yes. that's why we have a relatively strict review process -
the reviewer needs to check stuff like this, not just blindly trust
the person who wrote the code did it. That's the essence of the
meaning of the Reviewed-by tag we throw around all the time - it's
not just a rubber stamp.

Indeed, it's not uncommon for me to spend hours doing this sort of
analysis when reviewing complex changes, and all you might see on
the mailing list is (maybe) a short comment accompanying a
Reviewed-by tag....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[Christoph Hellwig]

On Mon, Apr 04, 2011 at 01:21:20PM -0500, Alex Elder wrote:
> >  	__int64_t	sbflags;
> >  
> >  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
> >  	/*
> >  	 * Switching on quota accounting must be done at mount time.
> >  	 */
> > -	accflags = flags & XFS_ALL_QUOTA_ACCT;
> >  	flags &= ~(XFS_ALL_QUOTA_ACCT);
> 
> Unrelated, but isn't the effect of this line plus the one a few
> lines up the same as this?
> 
> 	flags &= XFS_ALL_QUOTA_ENFD;

Yes, it is.  I have a patch in my queue to clean this and a few others
bits around it up, but unlike the warnings fixes it's really 2.6.40
material.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: fix variable set but not used warnings

[David Sterba]

Hi,

On Tue, Apr 05, 2011 at 04:05:58PM +1000, Dave Chinner wrote:
> Background: the quotaon syscall on XFS can only change whether
> quotas are enforced or not in XFS. It can't switch quotas on at
> all because that has to be done at mount time. Switching quotas on
> at mount time sets the appropriate flags in the superblock, so we
> can always rely on a superblock flag check for whether accounting
> is enabled or not.

Thanks for the explanation.

> > 
> >             (mp->m_sb.sb_qflags & XFS_UQUOTA_ACCT) == 0 &&
> >             (flags & XFS_UQUOTA_ENFD))
> >              ^^^^^^^^^^^^^^^^^^^^^^^
> 
> And those are the checks that are meaningful.

Should have been the flags & _ACCT check line above, my mistake.

> > Besides, there are more cleanups and fixups in this function:
> > 
> > line 331:
> >         /* No fs can turn on quotas with a delayed effect */
> > 	ASSERT((flags & XFS_ALL_QUOTA_ACCT) == 0);
> > 
> > seems to be broken too:
> > * the acct flags are masked out -> always asserts true
> > * doing s/flags/accflags/ does not make sense
> 
> No, that makes perfect sense given the commit above. We use ASSERT()
> statements to document constraints and assumptions in the code, and
> that is exactly what this does....

Yes, in context of the linked commit. From a technical POV, this assert
would only fail when the constants for XFS_ALL_QUOTA_ACCT and
XFS_ALL_QUOTA_ENFD overlap.

> Well, yes. that's why we have a relatively strict review process -
> the reviewer needs to check stuff like this, not just blindly trust
> the person who wrote the code did it. That's the essence of the
> meaning of the Reviewed-by tag we throw around all the time - it's
> not just a rubber stamp.
> 
> Indeed, it's not uncommon for me to spend hours doing this sort of
> analysis when reviewing complex changes, and all you might see on
> the mailing list is (maybe) a short comment accompanying a
> Reviewed-by tag....

Understood and no doubts here. I was missing something like
"removing just the variable and will clean the rest later", which
Christoph wrote in a later email.


dave

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs