Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
> Which is based on v5.0-rc7
> 
> There are 5 extent buffer alloc functions in btrfs:
> __alloc_extent_buffer();
> alloc_extent_buffer();
> __alloc_dummy_extent_buffer();
> alloc_dummy_extent_buffer();
> alloc_test_extent_buffer();
> 
> However their return value is not unified for failure mode:
> __alloc_extent_buffer()		Never fail
> alloc_extent_buffer()		PTR_ERR()
> __alloc_dummy_extent_buffer()	NULL

This function can never return NULL, if __alloc_extent_buffer cannot
fail then the only error this function returns is ERR_PTR(ENOMEM);

> alloc_dummy_extent_buffer()	NULL
Same thing applies to this function

> alloc_test_extent_buffer()	NULL

Same thing for this function, if we return exists then we must have
found it by find_extent_buffer hence it cannot be null. Otherwise we
return eb as allocated from alloc_dummy_extent_buffer. So how can null
be returned?

To me it really seems none of the function could return a NULL value, no?

> 
> This causes some wrapper function to have 2 failure modes, like
> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
> its failure.
> 
> This inconsistent behavior is making static checker and reader crazy.
> 
> This patchset will unify the failure more of above 5 functions to
> PTR_ERR().
> 
> Qu Wenruo (5):
>   btrfs: extent_io: Add comment about the return value of
>     alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>     with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of
>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>     with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of
>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
> 
>  fs/btrfs/backref.c                     |  8 ++--
>  fs/btrfs/ctree.c                       | 16 ++++----
>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>  fs/btrfs/qgroup.c                      |  5 ++-
>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>  9 files changed, 68 insertions(+), 39 deletions(-)
>