Re: [PATCH 3/3] btrfs-progs: Create uuid tree with proper contents

[Qu Wenruo <wqu@suse.de>]

On 2019/1/2 下午6:07, Nikolay Borisov wrote:
> 
> 
> On 2.01.19 г. 12:00 ч., Qu Wenruo wrote:
>>
> 
> <snip>
> 
>>>> +}
>>>>  
>>>> +/*
>>>> + * Create a tree containing an root inode
>>>> + *
>>>> + * Caller must ensure at the time of calling, fs tree only contains 2 items
>>>> + * (one for INODE_ITEM and one for INODE_REF)
>>>> + */
>>>> +int create_inode_tree(struct btrfs_trans_handle *trans, u64 objectid)
>>>
>>> This function is really misnamed, since it's creating the
>>> DATA_RELOC_TREE, yet it's called create_inode_tree. Why not simply
>>> create_data_reloc_tree or are you planning on using this function more
>>> than once?
>>
>> In fact this could be used for fs, data reloc, and all subvolume trees.
>>
>> That's my primary reason not naming it create_data_reloc_tree().
>> Although currently it's only used by data reloc tree.
>>
>>>
>>>> +{
>>>> +	struct btrfs_root *fs_root = trans->fs_info->fs_root;
>>>> +
>>>> +	ASSERT(btrfs_header_level(fs_root->node) == 0 &&
>>>> +	       btrfs_header_nritems(fs_root->node) == 2);
>>>> +	return __create_tree(trans, fs_root, objectid);
>>>> +}
>>>> +
>>>> +int create_uuid_tree(struct btrfs_trans_handle *trans)
>>>> +{
>>>> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>>>> +	struct btrfs_root *uuid_root = fs_info->uuid_root;
>>>> +	struct btrfs_key key;
>>>> +	int ret;
>>>> +
>>>> +	if (!uuid_root) {
>>>
>>> Isn't this always true,  so the conditional here is redundant?
>> Just a much better solution than ASSERT()/BUG_ON().
>>
>> It won't hurt and will handle wrong calling order.
> 
> What will the wrong calling order be? The only error that's possible is
> if someone calls this function twice or creates the tree outside of this
> api. This could only occur if someone is deliberately touching modifying
> the code in which case a trigger of an ASSERT should suffice to force
> them to think about what they are doing.

OK, I'll change it to ASSERT().

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
> 
> <snip>
>