xfs
[Top] [All Lists]

Re: [PATCH 1/5] xfs: make superblock version checks reflect reality

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/5] xfs: make superblock version checks reflect reality
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 6 May 2014 18:39:11 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140506082948.GA18865@xxxxxxxxxxxxx>
References: <1399348559-19889-1-git-send-email-david@xxxxxxxxxxxxx> <1399348559-19889-2-git-send-email-david@xxxxxxxxxxxxx> <20140506082948.GA18865@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, May 06, 2014 at 01:29:48AM -0700, Christoph Hellwig wrote:
> > +/*
> > + * The first XFS version we support is a v4 superblock with V2 directories.
> > + */
> > +static inline bool xfs_sb_good_v4_features(struct xfs_sb *sbp)
> >  {
> > +   if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
> > +           return false;
> >  
> > +   /* check for unknown features in the fs */
> > +   if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> 
> Given that sb_versionnum is a __uint16_t and XFS_SB_VERSION_OKBITS is
> 0xffff this will never evaluate to false and a sane compiler should warn
> about it.  How about remove this check and XFS_SB_VERSION_OKBITS?

It gets changed later to this:

#define XFS_SB_VERSION_OKBITS           \
        ((XFS_SB_VERSION_NUMBITS | XFS_SB_VERSION_ALLFBITS) & \
                ~XFS_SB_VERSION_SHAREDBIT)

So we don't ever consider the shared bit valid. IOWs, it's not
0xffff ;)

> The various has_ macros are a bit confusing to me, as some explicitly
> check for 5 superblocks, and some assume the caller handles them in
> some way, but I think this is something we can leave for later cleanups.
> 
> >   * For example, for a bit defined as XFS_SB_VERSION2_FUNBIT, has a macro:
> >   *
> > - * SB_VERSION_HASFUNBIT(xfs_sb_t *sbp)
> > + * SB_VERSION_HASFUNBIT(struct xfs_sb *sbp)
> 
> >   * ((xfs_sb_version_hasmorebits(sbp) &&
> >   *  ((sbp)->sb_features2 & XFS_SB_VERSION2_FUNBIT)
> >   */
> 
> This should be updated to the lowe case convention inlines we've used
> for a long time.  Or just removed as new features should go into v5
> superblocks..

I'll update it.

> Modulo these minor bits:
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>

Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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