| Message ID | 20190522081910.7689-7-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for other checksums | expand |
On Wed, May 22, 2019 at 10:19:03AM +0200, Johannes Thumshirn wrote: > Add a small helper for btrfs_print_data_csum_error() which formats the > checksum according to it's type for pretty printing. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/btrfs_inode.h | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index d5b438706b77..f0a757eb5744 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -337,22 +337,42 @@ static inline void btrfs_inode_resume_unlocked_dio(struct btrfs_inode *inode) > clear_bit(BTRFS_INODE_READDIO_NEED_LOCK, &inode->runtime_flags); > } > > +static inline void btrfs_csum_format(struct btrfs_super_block *sb, > + u32 csum, u8 *cbuf) > +{ > + size_t size = btrfs_super_csum_size(sb) * 8; > + > + switch (btrfs_super_csum_type(sb)) { > + case BTRFS_CSUM_TYPE_CRC32: > + snprintf(cbuf, size, "0x%08x", csum); > + break; > + default: /* can't happen - csum type is validated at mount time */ > + break; > + } > +} > + > static inline void btrfs_print_data_csum_error(struct btrfs_inode *inode, > u64 logical_start, u32 csum, u32 csum_expected, int mirror_num) > { > struct btrfs_root *root = inode->root; > + struct btrfs_super_block *sb = root->fs_info->super_copy; > + u8 cbuf[BTRFS_CSUM_SIZE]; > + u8 ecbuf[BTRFS_CSUM_SIZE]; > + > + btrfs_csum_format(sb, csum, cbuf); > + btrfs_csum_format(sb, csum_expected, ecbuf); cbuf is 32 bytes (BTRFS_CSUM_SIZE) for the binary representation, while you want reserve the space for the string representation, which is 2x the size in bytes + "0x" prefix but that can be moved to the printk format string. So, the above is fine for crc32c the string will fit there, but with sha256 this will cause buffer overflow, by 32 * 2 + 2 - 32 = 30 bytes. Gotcha. I think the helper is not needed at all, the format "%*phN" can be used for crc32c too without the intermediate buffers. For better readability, some macros can be added like "this is wrong csum " CSUM_FORMAT " end of string", CSUM_FORMAT_VALUE(csum_size, csum_bytes) with CSUM_FORMAT "0x%*phN" and CSUM_FORMAT_VALUE(size, bytes) size, bytes ie. just for the explict requirement of the variable length required by "*".
On Mon, May 27, 2019 at 06:57:19PM +0200, David Sterba wrote: > I think the helper is not needed at all, the format "%*phN" can be used > for crc32c too without the intermediate buffers. For better readability, > some macros can be added like > > "this is wrong csum " CSUM_FORMAT " end of string", > CSUM_FORMAT_VALUE(csum_size, csum_bytes) > > with CSUM_FORMAT "0x%*phN" and > CSUM_FORMAT_VALUE(size, bytes) size, bytes > > ie. just for the explict requirement of the variable length required by > "*". Good idea, will be updating the patch.
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index d5b438706b77..f0a757eb5744 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -337,22 +337,42 @@ static inline void btrfs_inode_resume_unlocked_dio(struct btrfs_inode *inode) clear_bit(BTRFS_INODE_READDIO_NEED_LOCK, &inode->runtime_flags); } +static inline void btrfs_csum_format(struct btrfs_super_block *sb, + u32 csum, u8 *cbuf) +{ + size_t size = btrfs_super_csum_size(sb) * 8; + + switch (btrfs_super_csum_type(sb)) { + case BTRFS_CSUM_TYPE_CRC32: + snprintf(cbuf, size, "0x%08x", csum); + break; + default: /* can't happen - csum type is validated at mount time */ + break; + } +} + static inline void btrfs_print_data_csum_error(struct btrfs_inode *inode, u64 logical_start, u32 csum, u32 csum_expected, int mirror_num) { struct btrfs_root *root = inode->root; + struct btrfs_super_block *sb = root->fs_info->super_copy; + u8 cbuf[BTRFS_CSUM_SIZE]; + u8 ecbuf[BTRFS_CSUM_SIZE]; + + btrfs_csum_format(sb, csum, cbuf); + btrfs_csum_format(sb, csum_expected, ecbuf); /* Output minus objectid, which is more meaningful */ if (root->root_key.objectid >= BTRFS_LAST_FREE_OBJECTID) btrfs_warn_rl(root->fs_info, - "csum failed root %lld ino %lld off %llu csum 0x%08x expected csum 0x%08x mirror %d", + "csum failed root %lld ino %lld off %llu csum %s expected csum %s mirror %d", root->root_key.objectid, btrfs_ino(inode), - logical_start, csum, csum_expected, mirror_num); + logical_start, cbuf, ecbuf, mirror_num); else btrfs_warn_rl(root->fs_info, - "csum failed root %llu ino %llu off %llu csum 0x%08x expected csum 0x%08x mirror %d", + "csum failed root %llu ino %llu off %llu csum %s expected csum %s mirror %d", root->root_key.objectid, btrfs_ino(inode), - logical_start, csum, csum_expected, mirror_num); + logical_start, cbuf, ecbuf, mirror_num); } #endif