[PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[Sidong Yang]

btrfs_extent_same() cannot be called with zero length. This patch add
code that initialize ret as -EINVAL to make it safe.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
v2:
 - Removed assert and added initializing ret
---
 fs/btrfs/reflink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
index 9b0814318e72..864f42198c5c 100644
--- a/fs/btrfs/reflink.c
+++ b/fs/btrfs/reflink.c
@@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	u64 i, tail_len, chunk_count;
 	struct btrfs_root *root_dst = BTRFS_I(dst)->root;
 
+	ret = -EINVAL;
 	spin_lock(&root_dst->root_item_lock);
 	if (root_dst->send_in_progress) {
 		btrfs_warn_rl(root_dst->fs_info,
-- 
2.25.1

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[Nikolay Borisov]

On 20.08.21 г. 3:41, Sidong Yang wrote:
> btrfs_extent_same() cannot be called with zero length. This patch add
> code that initialize ret as -EINVAL to make it safe.
> 
> Signed-off-by: Sidong Yang <realwakka@gmail.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

One minor nit: In this case the variable could have been initialized
during definition to save 1 extra line of code.

> ---
> v2:
>  - Removed assert and added initializing ret
> ---
>  fs/btrfs/reflink.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> index 9b0814318e72..864f42198c5c 100644
> --- a/fs/btrfs/reflink.c
> +++ b/fs/btrfs/reflink.c
> @@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
>  	u64 i, tail_len, chunk_count;
>  	struct btrfs_root *root_dst = BTRFS_I(dst)->root;
>  
> +	ret = -EINVAL;
>  	spin_lock(&root_dst->root_item_lock);
>  	if (root_dst->send_in_progress) {
>  		btrfs_warn_rl(root_dst->fs_info,
> 

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[David Sterba]

On Fri, Aug 20, 2021 at 09:08:26AM +0300, Nikolay Borisov wrote:
> 
> 
> On 20.08.21 г. 3:41, Sidong Yang wrote:
> > btrfs_extent_same() cannot be called with zero length. This patch add
> > code that initialize ret as -EINVAL to make it safe.
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> One minor nit: In this case the variable could have been initialized
> during definition to save 1 extra line of code.

Yeah the function is short enough to have the declaration initialization
in sight, I've changed that.

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[Sidong Yang]

On Fri, Aug 20, 2021 at 01:17:49PM +0200, David Sterba wrote:
> On Fri, Aug 20, 2021 at 09:08:26AM +0300, Nikolay Borisov wrote:
> > 
> > 
> > On 20.08.21 г. 3:41, Sidong Yang wrote:
> > > btrfs_extent_same() cannot be called with zero length. This patch add
> > > code that initialize ret as -EINVAL to make it safe.
> > > 
> > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > 
> > Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> > 
> > One minor nit: In this case the variable could have been initialized
> > during definition to save 1 extra line of code.
> 
> Yeah the function is short enough to have the declaration initialization
> in sight, I've changed that.

Thanks, I'll keep in mind. Any nit would be welcome.

Thanks,
Sidong

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[Filipe Manana]

On Fri, Aug 20, 2021 at 1:42 AM Sidong Yang <realwakka@gmail.com> wrote:
>
> btrfs_extent_same() cannot be called with zero length. This patch add
> code that initialize ret as -EINVAL to make it safe.

I suppose the motivation of the patch is to fix a warning from smatch,
or other similar tools, about 'ret' not being initialized when olen is
0.
Initializing 'ret' to some value surely makes the warning go away,
even though it's not possible for olen to be 0 at btrfs_extent_same(),
as that
is filtered up in the call chain.

However setting to -EINVAL by default is confusing and counter
intuitive because dedupe operations are supposed to return 0 (success)
for a 0 length range.
So 'ret' should be initialized to 0 to avoid any confusion.

Thanks.

>
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
> v2:
>  - Removed assert and added initializing ret
> ---
>  fs/btrfs/reflink.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> index 9b0814318e72..864f42198c5c 100644
> --- a/fs/btrfs/reflink.c
> +++ b/fs/btrfs/reflink.c
> @@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
>         u64 i, tail_len, chunk_count;
>         struct btrfs_root *root_dst = BTRFS_I(dst)->root;
>
> +       ret = -EINVAL;
>         spin_lock(&root_dst->root_item_lock);
>         if (root_dst->send_in_progress) {
>                 btrfs_warn_rl(root_dst->fs_info,
> --
> 2.25.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[Sidong Yang]

On Mon, Aug 23, 2021 at 10:44:08AM +0100, Filipe Manana wrote:
> On Fri, Aug 20, 2021 at 1:42 AM Sidong Yang <realwakka@gmail.com> wrote:
> >
> > btrfs_extent_same() cannot be called with zero length. This patch add
> > code that initialize ret as -EINVAL to make it safe.
> 
> I suppose the motivation of the patch is to fix a warning from smatch,
> or other similar tools, about 'ret' not being initialized when olen is
> 0.

Yes, Actually I used smatch you said.

> Initializing 'ret' to some value surely makes the warning go away,
> even though it's not possible for olen to be 0 at btrfs_extent_same(),
> as that
> is filtered up in the call chain.
> 
> However setting to -EINVAL by default is confusing and counter
> intuitive because dedupe operations are supposed to return 0 (success)
> for a 0 length range.

Yeah, I think it depends on btrfs_extent_same()'s concept. It does
nothing when 0 length. It's okay if we consider it's normal operation
and it seems natural.

>
> So 'ret' should be initialized to 0 to avoid any confusion.

Agree. I want to know other people's thoughts.

Thanks,
Sidong
 
> Thanks.
> 
> >
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> > v2:
> >  - Removed assert and added initializing ret
> > ---
> >  fs/btrfs/reflink.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c
> > index 9b0814318e72..864f42198c5c 100644
> > --- a/fs/btrfs/reflink.c
> > +++ b/fs/btrfs/reflink.c
> > @@ -653,6 +653,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> >         u64 i, tail_len, chunk_count;
> >         struct btrfs_root *root_dst = BTRFS_I(dst)->root;
> >
> > +       ret = -EINVAL;
> >         spin_lock(&root_dst->root_item_lock);
> >         if (root_dst->send_in_progress) {
> >                 btrfs_warn_rl(root_dst->fs_info,
> > --
> > 2.25.1
> >
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same()

[David Sterba]

On Mon, Aug 23, 2021 at 11:51:34PM +0000, Sidong Yang wrote:
> On Mon, Aug 23, 2021 at 10:44:08AM +0100, Filipe Manana wrote:
> > On Fri, Aug 20, 2021 at 1:42 AM Sidong Yang <realwakka@gmail.com> wrote:
> > >
> > > btrfs_extent_same() cannot be called with zero length. This patch add
> > > code that initialize ret as -EINVAL to make it safe.
> > 
> > I suppose the motivation of the patch is to fix a warning from smatch,
> > or other similar tools, about 'ret' not being initialized when olen is
> > 0.
> 
> Yes, Actually I used smatch you said.

It's good to mention that it's a warning reported by some tool, not all
such warnings are valid or have to be fixed.

> > Initializing 'ret' to some value surely makes the warning go away,
> > even though it's not possible for olen to be 0 at btrfs_extent_same(),
> > as that
> > is filtered up in the call chain.
> > 
> > However setting to -EINVAL by default is confusing and counter
> > intuitive because dedupe operations are supposed to return 0 (success)
> > for a 0 length range.
> 
> Yeah, I think it depends on btrfs_extent_same()'s concept. It does
> nothing when 0 length. It's okay if we consider it's normal operation
> and it seems natural.
> 
> >
> > So 'ret' should be initialized to 0 to avoid any confusion.
> 
> Agree. I want to know other people's thoughts.

I agree with Filipe, please resend the patch with ret = 0 and
explanation why want it. Thanks.