On 20.03.2018 12:11, Nikolay Borisov wrote:
>
>
> On 20.03.2018 11:53, Anand Jain wrote:
>> %csum_type is being checked to check the number of csum types we support,
>> which is 1 as of now. Instead just check if the type matches with the
>> only type we support, that is BTRFS_CSUM_TYPE_CRC32. And further adds
>> cleanup.
>>
>> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
>> ---
>> fs/btrfs/disk-io.c | 41 +++++++++++++++++++----------------------
>> 1 file changed, 19 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 1657d6aa4fa6..582ed6af3c50 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -399,32 +399,29 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>> struct btrfs_super_block *disk_sb =
>> (struct btrfs_super_block *)raw_disk_sb;
>> u16 csum_type = btrfs_super_csum_type(disk_sb);
>> - int ret = 0;
>> -
>> - if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
>> - u32 crc = ~(u32)0;
>> - char result[sizeof(crc)];
>> -
>> - /*
>> - * The super_block structure does not span the whole
>> - * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
>> - * is filled with zeros and is included in the checksum.
>> - */
>> - crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
>> - crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> - btrfs_csum_final(crc, result);
>> -
>> - if (memcmp(raw_disk_sb, result, sizeof(result)))
>> - ret = 1;
>> - }
>> + u32 crc = ~(u32)0;
>> + char result[sizeof(crc)];
>>
>> - if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
>> + /* We support csum type crc32 only as of now */
>> + if (csum_type != BTRFS_CSUM_TYPE_CRC32) {
>> btrfs_err(fs_info, "unsupported checksum algorithm %u",
>> - csum_type);
>> - ret = 1;
>> + csum_type);
>> + return 1;
>
> I think it would make sense to return EINVAL here. Then the sole caller
> of this function (open_ctree) can use the code a bit more idiomatically:
>
> ret = btrfs_check_super_csum()
>
> if (ret < 0) {
> handle failure
> }
>
> Right now we return 1 on both when the function fails due to invalid CRC
> algorithm and if the checksum doesn't match. And we are going to print 2
> things: unsupported checksum algorithm and the
> btrfs_err(fs_info, "superblock checksum mismatch"); from open_ctree.
>
> Let's implemented proper error returning strategy for this function.
>
Disregard, you have already implemented something like that in 2/3. I
will comment there how it can be improved.
>
>
>> }
>>
>> - return ret;
>> + /*
>> + * The super_block structure does not span the whole
>> + * BTRFS_SUPER_INFO_SIZE range, we expect that the unused space
>> + * is filled with zeros and is included in the checksum.
>> + */
>> + crc = btrfs_csum_data(raw_disk_sb + BTRFS_CSUM_SIZE,
>> + crc, BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE);
>> + btrfs_csum_final(crc, result);
>> +
>> + if (memcmp(raw_disk_sb, result, sizeof(result)))
>> + return 1;
>
> Perhaps return EUCLEAN i.e something is corrupted hence the checksum
> didn't pan out as we expected.
>
>> +
>> + return 0;
>> }
>>
>> /*
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
