| Message ID | 20180326082742.9235-3-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26.03.2018 11:27, Anand Jain wrote: > Return the required -EINVAL and -EUCLEAN from the function > btrfs_check_super_csum(). And more the error log into the > parent function. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b9b435579527..8b4e602ed60a 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -392,9 +392,10 @@ static int verify_parent_transid(struct extent_io_tree *io_tree, > /* > * Return 0 if the superblock checksum type matches the checksum value of that > * algorithm. Pass the raw disk superblock data. > + * Otherwise: -EINVAL if csum type is not found > + * -EUCLEAN if csum does not match > */ > -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > - char *raw_disk_sb) > +static int btrfs_check_super_csum(char *raw_disk_sb) > { > struct btrfs_super_block *disk_sb = > (struct btrfs_super_block *)raw_disk_sb; > @@ -402,11 +403,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > u32 crc = ~(u32)0; > char result[sizeof(crc)]; > > - if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) { > - btrfs_err(fs_info, "unsupported checksum algorithm %u", > - csum_type); > - return 1; > - } > + if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) > + return -EINVAL; > > /* > * The super_block structure does not span the whole > @@ -418,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > btrfs_csum_final(crc, result); > > if (memcmp(raw_disk_sb, result, sizeof(result))) > - return 1; > + return -EUCLEAN; > > return 0; > } > @@ -2570,9 +2568,16 @@ int open_ctree(struct super_block *sb, > * We want to check superblock checksum, the type is stored inside. > * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k). > */ > - if (btrfs_check_super_csum(fs_info, bh->b_data)) { > - btrfs_err(fs_info, "superblock checksum mismatch"); > - err = -EINVAL; > + err = btrfs_check_super_csum(bh->b_data); > + if (err) { > + if (err == -EINVAL) > + pr_err("BTRFS error (device %pg): "\ > + "unsupported checksum algorithm", > + fs_devices->latest_bdev); I don't think strings should be idented. I.e the correct thing here should be to have only fs_devices->latest_bdev on a new line, even if the string goes above the char limit of 80. In any case the \ is not needed due to gcc's support for string literal concatenation: pr_err("BTRFS error (device %pg): " "unsupported checksum algorithm", fs_devices->latest_bdev) should work equally well. But as I said I don't think this is even needed. Let's wait and see what David says. > + else > + pr_err("BTRFS error (device %pg): "\ > + "superblock checksum mismatch", > + fs_devices->latest_bdev); > brelse(bh); > goto fail_alloc; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 27, 2018 at 11:05:53AM +0300, Nikolay Borisov wrote: > > + if (err) { > > + if (err == -EINVAL) > > + pr_err("BTRFS error (device %pg): "\ > > + "unsupported checksum algorithm", > > + fs_devices->latest_bdev); > > I don't think strings should be idented. I.e the correct thing here > should be to have only fs_devices->latest_bdev on a new line, even if > the string goes above the char limit of 80. In any case the \ is not > needed due to gcc's support for string literal concatenation: > > pr_err("BTRFS error (device %pg): " > "unsupported checksum algorithm", > fs_devices->latest_bdev) > > should work equally well. But as I said I don't think this is even > needed. Let's wait and see what David says. The strings should not be split, because we want to be able to grep the sources for the error messages. If the string does not fit 80 chars per line, then it can be moved to the next line and un-indented to the left. Plenty of examples. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/28/2018 03:16 AM, David Sterba wrote: > On Tue, Mar 27, 2018 at 11:05:53AM +0300, Nikolay Borisov wrote: >>> + if (err) { >>> + if (err == -EINVAL) >>> + pr_err("BTRFS error (device %pg): "\ >>> + "unsupported checksum algorithm", >>> + fs_devices->latest_bdev); >> >> I don't think strings should be idented. I.e the correct thing here >> should be to have only fs_devices->latest_bdev on a new line, even if >> the string goes above the char limit of 80. In any case the \ is not >> needed due to gcc's support for string literal concatenation: >> >> pr_err("BTRFS error (device %pg): " >> "unsupported checksum algorithm", >> fs_devices->latest_bdev) >> >> should work equally well. But as I said I don't think this is even >> needed. Let's wait and see what David says. > > The strings should not be split, because we want to be able to grep the > sources for the error messages. If the string does not fit 80 chars > per line, then it can be moved to the next line and un-indented to the > left. Plenty of examples. I understand reason behind string should not be split, but I thought here will be an exception, as if you want to search for the string you will still use "unsupported checksum algorithm" which is still in one line. About the grep not able to find the string which are split, I look at it as fixing the wrong end of the problem. That is Grep end problem being fixes at the c code end. Anyway as its kind of linux general guidlines, I shall fix my patch. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 28, 2018 at 04:43:02AM +0800, Anand Jain wrote: > I understand reason behind string should not be split, but I > thought here will be an exception, as if you want to search > for the string you will still use "unsupported checksum algorithm" > which is still in one line. > About the grep not able to find the string which are split, > I look at it as fixing the wrong end of the problem. That is Grep > end problem being fixes at the c code end. Anyway as its kind of > linux general guidlines, I shall fix my patch. It's not just grep, but any other similar line-oriented searching tool or internal searches in editors or IDEs. Searching for fragments of the string will likely lead to the result but splitting lines makes it unnecessarily more difficult. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index b9b435579527..8b4e602ed60a 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -392,9 +392,10 @@ static int verify_parent_transid(struct extent_io_tree *io_tree, /* * Return 0 if the superblock checksum type matches the checksum value of that * algorithm. Pass the raw disk superblock data. + * Otherwise: -EINVAL if csum type is not found + * -EUCLEAN if csum does not match */ -static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, - char *raw_disk_sb) +static int btrfs_check_super_csum(char *raw_disk_sb) { struct btrfs_super_block *disk_sb = (struct btrfs_super_block *)raw_disk_sb; @@ -402,11 +403,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, u32 crc = ~(u32)0; char result[sizeof(crc)]; - if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) { - btrfs_err(fs_info, "unsupported checksum algorithm %u", - csum_type); - return 1; - } + if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) + return -EINVAL; /* * The super_block structure does not span the whole @@ -418,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, btrfs_csum_final(crc, result); if (memcmp(raw_disk_sb, result, sizeof(result))) - return 1; + return -EUCLEAN; return 0; } @@ -2570,9 +2568,16 @@ int open_ctree(struct super_block *sb, * We want to check superblock checksum, the type is stored inside. * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k). */ - if (btrfs_check_super_csum(fs_info, bh->b_data)) { - btrfs_err(fs_info, "superblock checksum mismatch"); - err = -EINVAL; + err = btrfs_check_super_csum(bh->b_data); + if (err) { + if (err == -EINVAL) + pr_err("BTRFS error (device %pg): "\ + "unsupported checksum algorithm", + fs_devices->latest_bdev); + else + pr_err("BTRFS error (device %pg): "\ + "superblock checksum mismatch", + fs_devices->latest_bdev); brelse(bh); goto fail_alloc; }
Return the required -EINVAL and -EUCLEAN from the function btrfs_check_super_csum(). And more the error log into the parent function. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)