Re: [PATCH v2 10/11] btrfs-progs: add xxhash64 as checksum algorithm

[Johannes Thumshirn <jthumshirn@suse.de>]

On 27/08/2019 16:16, Nikolay Borisov wrote:
[...]

>>  static void dump_superblock(struct btrfs_super_block *sb, int full)
>>  {
>>  	int i;
>> @@ -326,15 +337,11 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>  	csum_type = btrfs_super_csum_type(sb);
>>  	csum_size = BTRFS_CSUM_SIZE;
>>  	printf("csum_type\t\t%hu (", csum_type);
>> -	if (csum_type >= ARRAY_SIZE(btrfs_csum_sizes)) {
>> +	if (csum_type >= ARRAY_SIZE(btrfs_csums)) {
> Why not is_valid_csum_type ?

Fixed.

>>  		printf("INVALID");
>>  	} else {
>> -		if (csum_type == BTRFS_CSUM_TYPE_CRC32) {
>> -			printf("crc32c");
>> -			csum_size = btrfs_csum_sizes[csum_type];
>> -		} else {
>> -			printf("unknown");
>> -		}
>> +		printf("%s", btrfs_csums[csum_type].name);
>> +		csum_size = btrfs_csums[csum_type].size;
>>  	}
>>  	printf(")\n");
>>  	printf("csum_size\t\t%llu\n", (unsigned long long)csum_size);
>> @@ -342,8 +349,8 @@ static void dump_superblock(struct btrfs_super_block *sb, int full)
>>  	printf("csum\t\t\t0x");
>>  	for (i = 0, p = sb->csum; i < csum_size; i++)
>>  		printf("%02x", p[i]);
>> -	if (csum_type != BTRFS_CSUM_TYPE_CRC32 ||
>> -	    csum_size != btrfs_csum_sizes[BTRFS_CSUM_TYPE_CRC32])
>> +	if (!is_valid_csum_type(csum_type) ||
>> +	    csum_size != btrfs_csums[csum_type].size)
> 
> That second check - can it ever trigger? If the csum_type >= ARRAY_SIZE
> goes into the else branch then csum_size == btrfs_csums[csum_type].size
> so this check is guaranteed to never fail. OTOH, if we print invalid
> above then csum_type is guaranteed to be above ARRAY_SIZE(btrfs_csums)
> and I thin this guarantees that !is_valid_csum_type(csum_type) is going
> to be true e.g. we will print UNKNOWN CSUM type. So I guess a simple
> 
> 'if (!is_valid_csum_type(csum_type)' will suffice here?

Right, fixed as well.

[...]

>> -#include "xxh3.h"
>> +/* #include "xxh3.h" */
> 
> Does that mean progs compilation is broken by the previous patch since
> it includes a file which cannot be found?

It broke bisectability, fix it now.


[...]
>>  	if (strcasecmp(s, "crc32c") == 0) {
>>  		return BTRFS_CSUM_TYPE_CRC32;
>> +	} else if (strcasecmp(s, "xxhash64") == 0 ||
>> +		   strcasecmp(s, "xxhash") == 0) {
> 
> Don't we want to be very explicit about only supporting xxhash64, and
> not aliasing xxhash to mean xxhash64? I.e remove the xxhash comparison
> and consider it invalid.

I'll keep that alias as per Dave's comment.

Thanks,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 247165, AG München)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850