[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-btrfs
Subject:    Re: [PATCH 5/8] btrfs: inode: use btrfs_for_each_slot in btrfs_read_readdir
From:       Nikolay Borisov <nborisov () suse ! com>
Date:       2021-08-31 11:27:07
Message-ID: b61685b1-db19-872f-8c9e-74dfe6c80ca7 () suse ! com
[Download RAW message or body]



On 31.08.21 г. 14:10, David Sterba wrote:
> On Mon, Aug 30, 2021 at 04:05:28PM +0300, Nikolay Borisov wrote:
>>> @@ -6137,35 +6136,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
>>>  	key.offset = ctx->pos;
>>>  	key.objectid = btrfs_ino(BTRFS_I(inode));
>>>  
>>> -	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
>>> -	if (ret < 0)
>>> -		goto err;
>>> -
>>> -	while (1) {
>>> +	btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
>>
>> I don't think it's necessary to use iter_ret, instead you can use ret
>> directly. Because if either btrfs_search_slot return an error or
>> btrfs_valid_slot then ret would be set to the respective return value
>> and the body of the loop won't be executed at all, no?
> 
> Yeah thre's no reason to add another variable in this case. As long as
> the loop body does not use ret internally, then reusing ret is fine.
> 
> The point of having an explicit return value for the iterator is to be
> able to read the reason of failure after the iterator scope ends, so it
> can't be defined inside. We'd need to be careful to make sure that the
> iterator 'ret' is never used inside the body so that could be also
> useful to put to the documentation. I think a coccinelle script can be
> also useful to catch such things.
> 

Actually even if 'ret' is used inside the loop body it's still fine.
That's because as soon as we call the btrfs_valid_item (aka iter) and it
returns an error we break from the loop.
[prev in list] [next in list] [prev in thread] [next in thread]