xfs
[Top] [All Lists]

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

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH] xfs: fix variable set but not used warnings
From: David Sterba <dave@xxxxxxxx>
Date: Thu, 7 Apr 2011 15:45:33 +0200
In-reply-to: <20110405060558.GA31057@dastard>
Mail-followup-to: xfs@xxxxxxxxxxx
References: <20110404125544.GA726@xxxxxxxxxxxxx> <1301941280.2630.61.camel@doink> <20110404235226.GC31675@xxxxxxxxxxxxx> <20110405060558.GA31057@dastard>
Reply-to: dave@xxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
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

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