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

[Dave Chinner <david@fromorbit.com>]

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