On Thu, 8 Sep 2011 18:02:46 -0400, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: > On Thu, Sep 08, 2011 at 04:07:54PM -0400, J. Bruce Fields wrote: > > On Thu, Sep 08, 2011 at 03:00:58PM +0530, Aneesh Kumar K.V wrote: > > > On Wed, 7 Sep 2011 16:39:16 -0400, "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote: > > > > On Mon, Sep 05, 2011 at 10:55:31PM +0530, Aneesh Kumar K.V wrote: > > > > > +static int may_delete(struct inode *dir, struct dentry *victim, > > > > > + int isdir, int replace) > > > > > { > > > > > - int error; > > > > > + int mask, error, is_sticky; > > > > > + struct inode *inode = victim->d_inode; > > > > > > > > > > - if (!victim->d_inode) > > > > > + if (!inode) > > > > > return -ENOENT; > > > > > > > > > > BUG_ON(victim->d_parent->d_inode != dir); > > > > > audit_inode_child(victim, dir); > > > > > > > > > > - error = inode_permission(dir, MAY_WRITE | MAY_EXEC); > > > > > + mask = MAY_WRITE | MAY_EXEC | MAY_DELETE_CHILD; > > > > > + if (replace) > > > > > + mask |= S_ISDIR(inode->i_mode) ? > > > > > + MAY_CREATE_DIR : MAY_CREATE_FILE; > > > > > > > > I'm having trouble understanding this next bit: > > > > > > > > > + is_sticky = check_sticky(dir, inode); > > > > > + error = inode_permission(dir, mask); > > > > > + if ((error || is_sticky) && IS_RICHACL(inode) && > > > > > + !inode_permission(dir, mask & ~(MAY_WRITE | MAY_DELETE_CHILD)) && > > > > > + !inode_permission(inode, MAY_DELETE_SELF)) > > > > > + error = 0; > > > > > > > > OK, so we can ignore the lack of write or delete permissions on the > > > > parent if we have delete_self permissions on the child. I guess that's > > > > right. > > > > > > > > Why the "|| is_sticky" above? > > > > > > > > Is there some less complicated why to write this? > > > > > > we removed the ns_capable check out of check_sticky, because we don't > > > want to do capability check when richacl allows access. We also want to > > > make sure that even if mode bits allow access (inode_permission(dir, mask)) > > > if sticky bit is set we do additional check. > > > > Why are the two inode_permissions ANDed? The windows semantics are that > > you can delete if you have MAY_DELETE_CHILD *or* MAY_DELETE_SELF. > > Either way, those conditions are just really hard to follow. Could you > simplify the logic, add comments, maybe move the richacl stuff into a > little helper function? > > Also, a nit: > > > > > > + !inode_permission(dir, mask & ~(MAY_WRITE | MAY_DELETE_CHILD)) && > > The way you calculated mask above it always includes MAY_WRITE and > MAY_DELETE_CHILD, so the above is equivalent to just > > !inode_permission(dir, MAY_WRITE | MAY_DELETE_CHILD) && > > isn't it? > I guess i can simplify it as !inode_permission(dir, MAY_EXEC | replace_mask) -aneesh -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html