Re: [PATCH 5/5] btrfs: ctree: Checking key orders before merged tree blocks

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 10, 2019 at 03:12:37PM +0300, Nikolay Borisov wrote:
> On 10.07.19 г. 15:02 ч., Qu Wenruo wrote:
> > On 2019/7/10 下午7:19, Nikolay Borisov wrote:
> >> nit: I wonder if it will make it a bit easier to reason about the code
> >> if that function is renamed to valid_cross_block_key_order and make it
> >> return true or false, then it's callers will do if
> >> (!valid_cross_block_key_ordered) {
> >> return -EUCLEAN
> >> }>
> > I'm always uncertain what's the correct schema for check function.
> > 
> > Sometimes we have bool check_whatever() sometimes we have bool
> > is_whatever(), and sometimes we have int check_whatever().
> > 
> > If we have a good guidance for btrfs, it will be a no-brain choice.
> 
> There is no such guidance in this case my logic is that this function
> really returns a boolean value - 0 or -EUCLEAN to make that explicit I'd
> define it as returning bool and rename it to valid_cross_block_key_order
> to really emphasize the fact it's a predicated-type of function. Thus if
> someone reads the they will be certain that this function return either
> true or false depending on whether the input parameters make sense.

Also what
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#function-return-values-and-names
says.

Int in place of bool is in the old code, we should use bool in new code,
and convert the old code eventually. The naming of predicates prefixed
wich check_ sounds understandable to me, like going through a check
list, if it's ok continue, otherwise exit.

	if (check_this_is_fine())
		goto out_it_is_not;

Using 'is_' could be possible in some cases, and eg. for is_fstree or
is_bad_inode is ok.

> Whereas right now I will have to go and look at the implementation to
> determine what are the return values because of the "int" return type.
> 
> Again, that's just me and if you deem it doesn't bring value then feel
> free to ignore it.

I think this becomes important in a large code base that btrfs is slowly
turning into, so it's not just you. As I have to read a lot of code I
can notice patterns that are easy to understand and where checking other
files is necessary, which takes time and is distracting. New code should
follow the best practices and old code should be updated when changed.