On Fri, Feb 16, 2018 at 12:38 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Thu, Feb 15, 2018 at 6:16 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>> On Thu, Feb 15, 2018 at 6:07 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>>> On Tue, Feb 6, 2018 at 1:21 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
>>>> +static int ovl_dentry_connectable_layer(struct dentry *dentry)
>>>> +{
>>>> + struct ovl_entry *oe = OVL_E(dentry);
>>>> +
>>>> + /* We can get overlay root from root of any layer */
>>>> + if (dentry == dentry->d_sb->s_root)
>>>> + return oe->numlower;
>>>> +
>>>> + /* We cannot get upper/overlay path from non-indexed lower dentry */
>>>> + if (ovl_dentry_upper(dentry) &&
>>>
>>> Code is contradicting comment. I guess code is wrong?
>>>
>>
>>
>> This is patch v1, but already posted patch v2 with a bug fix.
>
> I was actually reviewing the git commit (which should be v2), but
> apparently found the wrong mail to reply to.
>
>> patch v2 has a comment above this function:
>> /* Return 0 for upper file handle, > 0 for lower file handle or < 0 on error. */
>>
>> N-connectable means you can get from layer <= N to overlay path.
>> Returning 0 here mean 0-connected, meaning only upper file handle
>> can be decoded.
>>
>> What do you find contradicting in code/comment?
>
> The comment says "non-indexed lower dentry", the code says
> "non-indexed upper dentry".
>
> But I think I see what you meant: "non-indexed merge dir", right?
Correct.
Actually, maybe comment should have said:
"We cannot get upper/overlay path from non-indexed lower file handle"
because this function is checking the future implications of encoding
a lower or upper file handle.
But its hard enough to understand it now, so changing comment to
"non-indexed merge dir" is fine by me.
>
>>
>>>> + !ovl_test_flag(OVL_INDEX, d_inode(dentry)))
>>>> + return 0;
>>>> +
>>>> + /* Pure upper cannot be ancestor of non pure upper */
>
> Not true. We can have a redirected dir inside a non-merge dir (but we
> don't call that pure, and it's not purity you are testing here, but
> non-mergeness).
>
> What cannot happen is that a lower dentry has a non-merge parent.
Yeh, I guess comment should have said "pure non-upper".
>
>>>> + if (WARN_ON(!oe->numlower))
>>>> + return 0;
>
> But we've already returned zero for the non-merge upper case, without
> any warning. So in this form, the warning makes no sense (probably
> should just be removed).
>
I agree.
> Other than this, I think the code is fine, only the comments are confusing.
>
You should have seen the earlier drafts... ;-)
Trying to explain the situation was the hard part of this patch.
I assume you will be making the modifications you suggested on commit.
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
