Re: [PATCH v2 2/6] btrfs-progs: check/common: Introduce a function to find imode using INODE_REF

[Nikolay Borisov <nborisov@suse.com>]

On 9.09.19 г. 17:24 ч., Qu Wenruo wrote:
> 
> 
> On 2019/9/9 下午9:25, Nikolay Borisov wrote:
>>
>>
>> On 5.09.19 г. 10:57 ч., Qu Wenruo wrote:
>>> Introduce a function, find_file_type(), to find filetype using
>>> INODE_REF.
>>>
>>> This function will:
>>> - Search DIR_INDEX first
>>>   DIR_INDEX is easier since there is only one item in it.
>>>
>>> - Valid the DIR_INDEX item
>>>   If the DIR_INDEX is valid, use the filetype and call it a day.
>>>
>>> - Search DIR_ITEM then
>>>
>>> - Valide the DIR_ITEM
>>>   If valid, call it a day. Or return -ENOENT;
>>>
>>> This would be used as the primary method to determine the imode in later
>>> imode repair code.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  check/mode-common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 99 insertions(+)
>>>
>>> diff --git a/check/mode-common.c b/check/mode-common.c
>>> index 195b6efaa7aa..c0ddc50a1dd0 100644
>>> --- a/check/mode-common.c
>>> +++ b/check/mode-common.c
>>> @@ -16,6 +16,7 @@
>>>
>>>  #include <time.h>
>>>  #include "ctree.h"
>>> +#include "hash.h"
>>>  #include "common/internal.h"
>>>  #include "common/messages.h"
>>>  #include "transaction.h"
>>> @@ -836,6 +837,104 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>  	return ret;
>>>  }
>>>
>>> +static int find_file_type(struct btrfs_root *root, u64 ino, u64 dirid,
>>> +			  u64 index, const char *name, u32 name_len,
>>> +			  u32 *imode_ret)
>>> +{
>>> +	struct btrfs_path path;
>>> +	struct btrfs_key location;
>>> +	struct btrfs_key key;
>>> +	struct btrfs_dir_item *di;
>>> +	char namebuf[BTRFS_NAME_LEN] = {0};
>>> +	unsigned long cur;
>>> +	unsigned long end;
>>> +	bool found = false;
>>> +	u8 filetype;
>>> +	u32 len;
>>> +	int ret;
>>> +
>>> +	btrfs_init_path(&path);
>>> +
>>> +	/* Search DIR_INDEX first */
>>> +	key.objectid = dirid;
>>> +	key.offset = index;
>>> +	key.type = BTRFS_DIR_INDEX_KEY;
>>> +
>>> +	ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0);
>>> +	if (ret > 0)
>>> +		ret = -ENOENT;
>>
>> Even if it returns 1 meaning there is no DIR_INDEX item perhaps it still
>> makes sense to go to dir_item: label to search for DIR_ITEM, what if the
>> corruption has affected just the DIR_INDEX item?
> 
> I didn't get the point.
> 
> The next line is just going to do dir_item search, just as you mentioned.

You are right, however, this is somewhat subtle and the fact I missed it
just proves the point. The following will be much better (and explicit):

if (ret)
   goto dir_item

In fact setting the -ENOENT on ret > 0 is only needed because of the
awkward way the return value of btrfs_search_slot is handled. So yeah,
I'm even more convinced that a simple if (ret) goto dir_item (or call a
function) is the way to go here.

>>
>>> +	if (ret < 0)
>>> +		goto dir_item;
>> nit: Use elseif to make it more explicit it is a single construct.
> 
> Not sure such usage is recommened, but I see a lot of usage like:
> 	ret = btrfs_search_slot();
> 	if (ret > 0)
> 		ret = -ENOENT;
> 	if (ret < 0)
> 		goto error;
> 
> So I just followed this practice.
> 

I guess this is debatable. You are right that most of the retval
handling is as you say but I think the more "purist" (so to say) way
should be an if {} else if {}. Because ultimately you are handling one
thing - the return value of btrfs_search_slot.

 David, what is your take on that ?

<snip>