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?
>
>>> + !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.
>>> + 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).
Other than this, I think the code is fine, only the comments are confusing.
Thanks,
Miklos
--
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
