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: Tue, 5 Apr 2011 01:52:27 +0200
In-reply-to: <1301941280.2630.61.camel@doink>
Mail-followup-to: xfs@xxxxxxxxxxx
References: <20110404125544.GA726@xxxxxxxxxxxxx> <1301941280.2630.61.camel@doink>
Reply-to: dave@xxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
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

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