* [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() @ 2021-08-20 0:41 Sidong Yang 2021-08-20 6:08 ` Nikolay Borisov 2021-08-23 9:44 ` Filipe Manana 0 siblings, 2 replies; 7+ messages in thread From: Sidong Yang @ 2021-08-20 0:41 UTC (permalink / raw) To: linux-btrfs, David Sterba, Nikolay Borisov; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-20 0:41 [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() Sidong Yang @ 2021-08-20 6:08 ` Nikolay Borisov 2021-08-20 11:17 ` David Sterba 2021-08-23 9:44 ` Filipe Manana 1 sibling, 1 reply; 7+ messages in thread From: Nikolay Borisov @ 2021-08-20 6:08 UTC (permalink / raw) To: Sidong Yang, linux-btrfs, David Sterba 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, > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-20 6:08 ` Nikolay Borisov @ 2021-08-20 11:17 ` David Sterba 2021-08-20 11:50 ` Sidong Yang 0 siblings, 1 reply; 7+ messages in thread From: David Sterba @ 2021-08-20 11:17 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Sidong Yang, linux-btrfs, 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-20 11:17 ` David Sterba @ 2021-08-20 11:50 ` Sidong Yang 0 siblings, 0 replies; 7+ messages in thread From: Sidong Yang @ 2021-08-20 11:50 UTC (permalink / raw) To: dsterba, Nikolay Borisov, linux-btrfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-20 0:41 [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() Sidong Yang 2021-08-20 6:08 ` Nikolay Borisov @ 2021-08-23 9:44 ` Filipe Manana 2021-08-23 23:51 ` Sidong Yang 1 sibling, 1 reply; 7+ messages in thread From: Filipe Manana @ 2021-08-23 9:44 UTC (permalink / raw) To: Sidong Yang; +Cc: linux-btrfs, David Sterba, Nikolay Borisov 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.” ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-23 9:44 ` Filipe Manana @ 2021-08-23 23:51 ` Sidong Yang 2021-08-26 14:08 ` David Sterba 0 siblings, 1 reply; 7+ messages in thread From: Sidong Yang @ 2021-08-23 23:51 UTC (permalink / raw) To: Filipe Manana; +Cc: linux-btrfs, David Sterba, Nikolay Borisov 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.” ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() 2021-08-23 23:51 ` Sidong Yang @ 2021-08-26 14:08 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2021-08-26 14:08 UTC (permalink / raw) To: Sidong Yang; +Cc: Filipe Manana, linux-btrfs, David Sterba, Nikolay Borisov 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-26 14:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-20 0:41 [PATCH v2] btrfs: reflink: Initialize return value in btrfs_extent_same() Sidong Yang 2021-08-20 6:08 ` Nikolay Borisov 2021-08-20 11:17 ` David Sterba 2021-08-20 11:50 ` Sidong Yang 2021-08-23 9:44 ` Filipe Manana 2021-08-23 23:51 ` Sidong Yang 2021-08-26 14:08 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).