Re: [PATCH 6/8] xfs: journal geometry is not properly bounds checked

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Tue, Jun 27, 2023 at 11:08:46PM -0700, Christoph Hellwig wrote:
> > +	if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) {
> > +		xfs_notice(mp,
> > +		"Log size 0x%x blocks too large, maximum size is 0x%llx blocks",
> > +			 sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS);
> 
> Just a little nitpick, didn't we traditionally align the overly
> long format strings with a single tab and not to the same line as
> the xfs_notice/warn/etc?

I don't think we have a particluarly formal definition of that.
 
I mean, just look at all the different variants in
xfs_sb_validate_common:

	if (xfs_sb_is_v5(sbp)) {
                if (sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
                        xfs_notice(mp,
"Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)",
                                sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE);
                        return -EFSCORRUPTED;
                }

                /* V5 has a separate project quota inode */
                if (sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
                        xfs_notice(mp,
                           "Version 5 of Super block has XFS_OQUOTA bits.");
                        return -EFSCORRUPTED;
                }

                /*
                 * Full inode chunks must be aligned to inode chunk size when
                 * sparse inodes are enabled to support the sparse chunk
                 * allocation algorithm and prevent overlapping inode records.
                 */
                if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_SPINODES) {
                        uint32_t        align;

                        align = XFS_INODES_PER_CHUNK * sbp->sb_inodesize
                                        >> sbp->sb_blocklog;
                        if (sbp->sb_inoalignmt != align) {
                                xfs_warn(mp,
"Inode block alignment (%u) must match chunk size (%u) for sparse inodes.",
                                         sbp->sb_inoalignmt, align);
                                return -EINVAL;
                        }
                }
        } else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
                                XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
                        xfs_notice(mp,
"Superblock earlier than Version 5 has XFS_{P|G}QUOTA_{ENFD|CHKD} bits.");
                        return -EFSCORRUPTED;
        }

        if (unlikely(
            sbp->sb_logstart == 0 && mp->m_logdev_targp == mp->m_ddev_targp)) {
                xfs_warn(mp,
                "filesystem is marked as having an external log; "
                "specify logdev on the mount command line.");
                return -EINVAL;
        }

        if (unlikely(
            sbp->sb_logstart != 0 && mp->m_logdev_targp != mp->m_ddev_targp)) {
                xfs_warn(mp,
                "filesystem is marked as having an internal log; "
                "do not specify logdev on the mount command line.");
                return -EINVAL;
        }

There's 4-5 different variations in indenting in 6-7 warning
messages. And there are more variations in that function, too.

There's no consistent rule to follow, so I just set the indenting
such that the format string didn't run over 80 columns and didn't
think any further..

I think that if we really care, a separate cleanup patch to make all
the long format strings use consistent zero-indenting and the
variables one tab like this one:

                        xfs_notice(mp,
"Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)",
                                sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE);

Could be done. If I'm ever lacking for something to do...

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>

Thanks!

_dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx