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 <w...@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>
