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@xxxxxxx +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
