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

[Nikolay Borisov <nborisov@suse.com>]

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.

> 
> Thanks,
> Qu
> 

<snip>