fs/btrfs/dir-item.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Return NULL instead of passing to ERR_PTR while ret is zero, this fix
smatch warnings:
fs/btrfs/dir-item.c:353
btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR'
Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item")
Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
---
fs/btrfs/dir-item.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 001c0c2f872c..cdb30ec7366a 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path,
if (ret > 0)
ret = 0;
- return ERR_PTR(ret);
+ return ret ? ERR_PTR(ret) : NULL;
}
struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
--
2.34.1On 19.10.24 11:07, Yue Haibing wrote:
> Return NULL instead of passing to ERR_PTR while ret is zero, this fix
> smatch warnings:
>
> fs/btrfs/dir-item.c:353
> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR'
>
> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item")
> Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
> ---
> fs/btrfs/dir-item.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index 001c0c2f872c..cdb30ec7366a 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path,
> if (ret > 0)
> ret = 0;
>
> - return ERR_PTR(ret);
> + return ret ? ERR_PTR(ret) : NULL;
> }
>
> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
The only caller to this is in btrfs_unlink_subvol(), which does the
following:
di = btrfs_search_dir_index_item(root, path, dir_ino,
&fname.disk_name);
if (IS_ERR_OR_NULL(di)) {
if (!di)
ret = -ENOENT;
else
ret = PTR_ERR(di);
btrfs_abort_transaction(trans, ret);
goto out;
}
to do:
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index d3093eba54a5..e755228d909a 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root
*root, struct btrfs_path *path,
return di;
}
/* Adjust return code if the key was not found in the next leaf. */
- if (ret > 0)
- ret = 0;
-
- return ERR_PTR(ret);
+ return ERR_PTR(-ENOENT);
}
struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle
*trans,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 35f89d14c110..00602634db3a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct
btrfs_trans_handle *trans,
*/
if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) {
di = btrfs_search_dir_index_item(root, path, dir_ino,
&fname.disk_name);
- if (IS_ERR_OR_NULL(di)) {
- if (!di)
- ret = -ENOENT;
- else
- ret = PTR_ERR(di);
+ if (IS_ERR(di)) {
+ ret = PTR_ERR(di);
btrfs_abort_transaction(trans, ret);
goto out;
}
This is completely untested though and needs to be re-checked if it's
even correct.
On 2024/10/21 16:25, Johannes Thumshirn wrote:
> On 19.10.24 11:07, Yue Haibing wrote:
>> Return NULL instead of passing to ERR_PTR while ret is zero, this fix
>> smatch warnings:
>>
>> fs/btrfs/dir-item.c:353
>> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR'
>>
>> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item")
>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
>> ---
>> fs/btrfs/dir-item.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
>> index 001c0c2f872c..cdb30ec7366a 100644
>> --- a/fs/btrfs/dir-item.c
>> +++ b/fs/btrfs/dir-item.c
>> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path,
>> if (ret > 0)
>> ret = 0;
>>
>> - return ERR_PTR(ret);
>> + return ret ? ERR_PTR(ret) : NULL;
>> }
>>
>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
>
> The only caller to this is in btrfs_unlink_subvol(), which does the
> following:
>
>
> di = btrfs_search_dir_index_item(root, path, dir_ino,
> &fname.disk_name);
> if (IS_ERR_OR_NULL(di)) {
> if (!di)
> ret = -ENOENT;
> else
> ret = PTR_ERR(di);
> btrfs_abort_transaction(trans, ret);
> goto out;
> }
>
> to do:
>
> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index d3093eba54a5..e755228d909a 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c
> @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root
> *root, struct btrfs_path *path,
> return di;
> }
> /* Adjust return code if the key was not found in the next leaf. */
ret is output variable of btrfs_for_each_slot, that return value can be 0, if a
valid slot was found, 1 if there were no more leaves, and < 0 if there was an
error.
> - if (ret > 0)
> - ret = 0;
> -
> - return ERR_PTR(ret);
> + return ERR_PTR(-ENOENT);
This overwrite other ret code, which expecting return to upstream caller
> }
>
> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle
> *trans,
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 35f89d14c110..00602634db3a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct
> btrfs_trans_handle *trans,
> */
> if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) {
> di = btrfs_search_dir_index_item(root, path, dir_ino,
> &fname.disk_name);
> - if (IS_ERR_OR_NULL(di)) {
> - if (!di)
> - ret = -ENOENT;
> - else
> - ret = PTR_ERR(di);
> + if (IS_ERR(di)) {
> + ret = PTR_ERR(di);
> btrfs_abort_transaction(trans, ret);
> goto out;
> }
> This is completely untested though and needs to be re-checked if it's
> even correct.
On 22.10.24 05:22, Yue Haibing wrote:
> On 2024/10/21 16:25, Johannes Thumshirn wrote:
>> On 19.10.24 11:07, Yue Haibing wrote:
>>> Return NULL instead of passing to ERR_PTR while ret is zero, this fix
>>> smatch warnings:
>>>
>>> fs/btrfs/dir-item.c:353
>>> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR'
>>>
>>> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item")
>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
>>> ---
>>> fs/btrfs/dir-item.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
>>> index 001c0c2f872c..cdb30ec7366a 100644
>>> --- a/fs/btrfs/dir-item.c
>>> +++ b/fs/btrfs/dir-item.c
>>> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path,
>>> if (ret > 0)
>>> ret = 0;
>>>
>>> - return ERR_PTR(ret);
>>> + return ret ? ERR_PTR(ret) : NULL;
>>> }
>>>
>>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
>>
>> The only caller to this is in btrfs_unlink_subvol(), which does the
>> following:
>>
>>
>> di = btrfs_search_dir_index_item(root, path, dir_ino,
>> &fname.disk_name);
>> if (IS_ERR_OR_NULL(di)) {
>> if (!di)
>> ret = -ENOENT;
>> else
>> ret = PTR_ERR(di);
>> btrfs_abort_transaction(trans, ret);
>> goto out;
>> }
>>
>> to do:
>>
>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
>> index d3093eba54a5..e755228d909a 100644
>> --- a/fs/btrfs/dir-item.c
>> +++ b/fs/btrfs/dir-item.c
>> @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root
>> *root, struct btrfs_path *path,
>> return di;
>> }
>> /* Adjust return code if the key was not found in the next leaf. */
>
>
> ret is output variable of btrfs_for_each_slot, that return value can be 0, if a
> valid slot was found, 1 if there were no more leaves, and < 0 if there was an
> error.
>
Yes.
>> - if (ret > 0)
>> - ret = 0;
>> -
>> - return ERR_PTR(ret);
>> + return ERR_PTR(-ENOENT);
>
> This overwrite other ret code, which expecting return to upstream caller
Right for ret < 0, but for ret >= 0 we set it to 0 and then do return
(void*)0 a.k.a. return NULL.
>
>> }
>>
>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle
>> *trans,
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 35f89d14c110..00602634db3a 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct
>> btrfs_trans_handle *trans,
>> */
>> if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) {
>> di = btrfs_search_dir_index_item(root, path, dir_ino,
>> &fname.disk_name);
>> - if (IS_ERR_OR_NULL(di)) {
>> - if (!di)
>> - ret = -ENOENT;
and then set it to ENOENT if it is NULL. So it should be
if (ret >= 0)
ret = -ENOENT;
return ERR_PTR(-ENOENT);
and in the caller
if (IS_ERR(di)) {
ret = PTR_ERR(di);
btrfs_abort_transaction(...);
break;
}
On 2024/10/22 14:37, Johannes Thumshirn wrote:
> On 22.10.24 05:22, Yue Haibing wrote:
>> On 2024/10/21 16:25, Johannes Thumshirn wrote:
>>> On 19.10.24 11:07, Yue Haibing wrote:
>>>> Return NULL instead of passing to ERR_PTR while ret is zero, this fix
>>>> smatch warnings:
>>>>
>>>> fs/btrfs/dir-item.c:353
>>>> btrfs_search_dir_index_item() warn: passing zero to 'ERR_PTR'
>>>>
>>>> Fixes: 9dcbe16fccbb ("btrfs: use btrfs_for_each_slot in btrfs_search_dir_index_item")
>>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
>>>> ---
>>>> fs/btrfs/dir-item.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
>>>> index 001c0c2f872c..cdb30ec7366a 100644
>>>> --- a/fs/btrfs/dir-item.c
>>>> +++ b/fs/btrfs/dir-item.c
>>>> @@ -350,7 +350,7 @@ btrfs_search_dir_index_item(struct btrfs_root *root, struct btrfs_path *path,
>>>> if (ret > 0)
>>>> ret = 0;
>>>>
>>>> - return ERR_PTR(ret);
>>>> + return ret ? ERR_PTR(ret) : NULL;
>>>> }
>>>>
>>>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
>>>
>>> The only caller to this is in btrfs_unlink_subvol(), which does the
>>> following:
>>>
>>>
>>> di = btrfs_search_dir_index_item(root, path, dir_ino,
>>> &fname.disk_name);
>>> if (IS_ERR_OR_NULL(di)) {
>>> if (!di)
>>> ret = -ENOENT;
>>> else
>>> ret = PTR_ERR(di);
>>> btrfs_abort_transaction(trans, ret);
>>> goto out;
>>> }
>>>
>>> to do:
>>>
>>> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
>>> index d3093eba54a5..e755228d909a 100644
>>> --- a/fs/btrfs/dir-item.c
>>> +++ b/fs/btrfs/dir-item.c
>>> @@ -345,10 +345,7 @@ btrfs_search_dir_index_item(struct btrfs_root
>>> *root, struct btrfs_path *path,
>>> return di;
>>> }
>>> /* Adjust return code if the key was not found in the next leaf. */
>>
>>
>> ret is output variable of btrfs_for_each_slot, that return value can be 0, if a
>> valid slot was found, 1 if there were no more leaves, and < 0 if there was an
>> error.
>>
>
> Yes.
>
>>> - if (ret > 0)
>>> - ret = 0;
>>> -
>>> - return ERR_PTR(ret);
>>> + return ERR_PTR(-ENOENT);
>>
>> This overwrite other ret code, which expecting return to upstream caller
>
> Right for ret < 0, but for ret >= 0 we set it to 0 and then do return
> (void*)0 a.k.a. return NULL.
>
>>
>>> }
>>>
>>> struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle
>>> *trans,
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 35f89d14c110..00602634db3a 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -4337,11 +4337,8 @@ static int btrfs_unlink_subvol(struct
>>> btrfs_trans_handle *trans,
>>> */
>>> if (btrfs_ino(inode) == BTRFS_EMPTY_SUBVOL_DIR_OBJECTID) {
>>> di = btrfs_search_dir_index_item(root, path, dir_ino,
>>> &fname.disk_name);
>>> - if (IS_ERR_OR_NULL(di)) {
>>> - if (!di)
>>> - ret = -ENOENT;
>
>
> and then set it to ENOENT if it is NULL. So it should be
>
> if (ret >= 0)
> ret = -ENOENT;
> return ERR_PTR(-ENOENT);
Here should be
return ERR_PTR(ret);
Will rework and send v2, thanks!
>
> and in the caller
>
> if (IS_ERR(di)) {
> ret = PTR_ERR(di);
> btrfs_abort_transaction(...);
> break;
> }
>
>
On 22.10.24 08:53, Yue Haibing wrote: >> and then set it to ENOENT if it is NULL. So it should be >> >> if (ret >= 0) >> ret = -ENOENT; >> return ERR_PTR(-ENOENT); > > Here should be > return ERR_PTR(ret); Yep of cause.
© 2016 - 2024 Red Hat, Inc.