| Message ID | 1526895134-1714-1-git-send-email-gujx@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.05.2018 12:32, Gu Jinxiang wrote: > Since add_excluded_extent always returns 0, > no need to judge ret. This patch is conceptually wrong because: a). Currently exclude_super_stripes is in fact buggy since it calls set_extent_bits which *may* fail, yet the return value is completely ignored. I think the best fix is to actually handle the failures from set_extent_bit inside add_excluded_extent and actually propagate them to callers. b) If it's deemed appropriate (which I think it's not) to ignore the return value then add_excluded_extent should be turned into a function which returns void. > > Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com> > --- > fs/btrfs/extent-tree.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 75cfb80d2551..4d876b32e136 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -249,8 +249,6 @@ static int exclude_super_stripes(struct btrfs_fs_info *fs_info, > cache->bytes_super += stripe_len; > ret = add_excluded_extent(fs_info, cache->key.objectid, > stripe_len); > - if (ret) > - return ret; > } > > for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) { > @@ -282,10 +280,6 @@ static int exclude_super_stripes(struct btrfs_fs_info *fs_info, > > cache->bytes_super += len; > ret = add_excluded_extent(fs_info, start, len); > - if (ret) { > - kfree(logical); > - return ret; > - } > } > > kfree(logical); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 21, 2018 at 05:32:14PM +0800, Gu Jinxiang wrote: > Since add_excluded_extent always returns 0, > no need to judge ret. The whole calltree starting in this function needs to be examined, if there's a function that can retrun value (like set_extent_bits) or if there's a BUG_ON that needs to be turned to proper error handling and pushed upwards eventually. When dealing with deep call sequences, it's fine and actually recommended to do one step at a time. See eg. 57599c7e7722daf5 . Do all necessary error handling in the function (undo changes, free resources, etc) and return a value. The caller that does not handle the return value at all will get the BUG_ON and will be converted the same way in next patch. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 75cfb80d2551..4d876b32e136 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -249,8 +249,6 @@ static int exclude_super_stripes(struct btrfs_fs_info *fs_info, cache->bytes_super += stripe_len; ret = add_excluded_extent(fs_info, cache->key.objectid, stripe_len); - if (ret) - return ret; } for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) { @@ -282,10 +280,6 @@ static int exclude_super_stripes(struct btrfs_fs_info *fs_info, cache->bytes_super += len; ret = add_excluded_extent(fs_info, start, len); - if (ret) { - kfree(logical); - return ret; - } } kfree(logical);
Since add_excluded_extent always returns 0, no need to judge ret. Signed-off-by: Gu Jinxiang <gujx@cn.fujitsu.com> --- fs/btrfs/extent-tree.c | 6 ------ 1 file changed, 6 deletions(-)