On Mon, May 23, 2016 at 07:51:16AM -0700, Christoph Hellwig wrote:
> On Thu, May 12, 2016 at 03:38:16PM -0500, Bill O'Donnell wrote:
> > +verify_sb_blocksize(xfs_sb_t *sb)
> > +{
> > + __uint32_t bsize;
> > + int i;
> > +
> > + /* check to make sure blocksize is legal 2^N, 9 <= N <= 16 */
> > + if (sb->sb_blocksize == 0)
> > + return(XR_BAD_BLOCKSIZE);
> > +
> > + bsize = 1;
> > +
> > + for (i = 0; bsize < sb->sb_blocksize &&
> > + i < sizeof(sb->sb_blocksize) * NBBY; i++)
> > + bsize <<= 1;
> > +
> > + if (i < XFS_MIN_BLOCKSIZE_LOG || i > XFS_MAX_BLOCKSIZE_LOG)
> > + return(XR_BAD_BLOCKSIZE);
> > +
> > + /* check sb blocksize field against sb blocklog field */
> > + if (i != sb->sb_blocklog)
> > + return(XR_BAD_BLOCKLOG);
>
> Couldn't we do this much simpler?
>
> if (sb->sb_blocksize == 0)
> return XR_BAD_BLOCKSIZE;
> if (sb->sb_blocksize != (1 << sb->sb_blocklog))
> return XR_BAD_BLOCKLOG;
> if (sb->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> sb->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG)
> return XR_BAD_BLOCKLOG;
Makes sense, yes.
>
> > /*
> > + * Attempt to find secondary sb with a coarse approach,
> > + * first trying agblocks and blocksize read from sb, providing
> > + * they're sane.
> > */
> > + if (verify_sb_blocksize(rsb) == 0) {
> > + skip = rsb->sb_agblocks * rsb->sb_blocksize;
> > + if ((skip >= XFS_AG_MIN_BYTES) && (skip <= XFS_AG_MAX_BYTES))
>
> no need for the inner braces here.
Check.
Thanks for the review!
-Bill
|