Re: [PATCH v4 02/27] btrfs: Get zone information of zoned block devices

[Naohiro Aota <naohiro.aota@wdc.com>]

On Fri, Aug 23, 2019 at 01:57:33PM +0200, Johannes Thumshirn wrote:
>On 23/08/2019 12:10, Naohiro Aota wrote:
>[...]
>> +int btrfs_get_dev_zone_info(struct btrfs_device *device)
>> +{
>> +	struct btrfs_zoned_device_info *zone_info = NULL;
>> +	struct block_device *bdev = device->bdev;
>> +	sector_t nr_sectors = bdev->bd_part->nr_sects;
>> +	sector_t sector = 0;
>> +	struct blk_zone *zones = NULL;
>> +	unsigned int i, nreported = 0, nr_zones;
>> +	unsigned int zone_sectors;
>> +	int ret;
>> +
>> +	if (!bdev_is_zoned(bdev))
>> +		return 0;
>> +
>> +	zone_info = kzalloc(sizeof(*zone_info), GFP_KERNEL);
>> +	if (!zone_info)
>> +		return -ENOMEM;
>> +
>> +	zone_sectors = bdev_zone_sectors(bdev);
>> +	ASSERT(is_power_of_2(zone_sectors));
>> +	zone_info->zone_size = (u64)zone_sectors << SECTOR_SHIFT;
>> +	zone_info->zone_size_shift = ilog2(zone_info->zone_size);
>> +	zone_info->nr_zones = nr_sectors >> ilog2(bdev_zone_sectors(bdev));
>> +	if (nr_sectors & (bdev_zone_sectors(bdev) - 1))
>> +		zone_info->nr_zones++;
>> +
>> +	zone_info->seq_zones = kcalloc(BITS_TO_LONGS(zone_info->nr_zones),
>> +				       sizeof(*zone_info->seq_zones),
>> +				       GFP_KERNEL);
>> +	if (!zone_info->seq_zones) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	zone_info->empty_zones = kcalloc(BITS_TO_LONGS(zone_info->nr_zones),
>> +					 sizeof(*zone_info->empty_zones),
>> +					 GFP_KERNEL);
>> +	if (!zone_info->empty_zones) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +
>> +	zones = kcalloc(BTRFS_REPORT_NR_ZONES,
>> +			sizeof(struct blk_zone), GFP_KERNEL);
>> +	if (!zones)
>> +		return -ENOMEM;
>
>Won't this will leak zone_info, zone_info->seq_zones and
>zone_info->empty_zones.

Ah, yes. I'll fix that using the suggested style below.

Thanks

>
>[...]
>
>> +out:
>> +	kfree(zones);
>> +
>> +	if (ret) {
>> +		kfree(zone_info->seq_zones);
>> +		kfree(zone_info->empty_zones);
>> +		kfree(zone_info);
>> +	}
>
>Which is why I think it would be more clear to have:
>free_zones:
>	kfree(zones);
>free_zi_emp_zones:
>	kfree(zone_info->empty_zones);
>free_zi_seq_zones:
>	kfree(zone_info->seq_zones);
>free_zi:
>	kfree(zone_info);
>
>
>
>-- 
>Johannes Thumshirn                            SUSE Labs Filesystems
>jthumshirn@suse.de                                +49 911 74053 689
>SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
>GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
>HRB 21284 (AG Nürnberg)
>Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850