Re: [PATCH] xfs_repair: further improvement on secondary superblock search method

["Bill O'Donnell" <billodo@redhat.com>]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs