On Tue, Feb 18, 2020 at 09:28:09AM +1100, Dave Chinner wrote:
> > +int
> > +xfs_ioc_attrmulti_one(
> > + struct file *parfilp,
>
> Weird name for the file pointer. It's just a file pointer in this
> context, similar to...
>
> > + struct inode *inode,
>
> ... it just being an inode pointer in this context.
The naming is taken from the existing code. I think it stands for
parent which isn't quite true, but I think it tries to to document the
point that the file pointer is not for the inode we are operating on,
but some random open file on the file system that the handle operation
execures on.
>
> > + uint32_t opcode,
> > + void __user *uname,
> > + void __user *value,
> > + uint32_t *len,
> > + uint32_t flags)
> > +{
> > + unsigned char *name;
> > + int error;
> > +
> > + if ((flags & ATTR_ROOT) && (flags & ATTR_SECURE))
> > + return -EINVAL;
> > + flags &= ~ATTR_KERNEL_FLAGS;
>
> Ok, so this is a user ABI visible change - the old code would return
> to userspace with these flags cleared from ops[i].am_flags. Now that
> doesn't happen. I don't see this as a problem, but it needs to be
> documented in the commit message.
Well, the clearing was just added the current merge window, before that
userspace could pass and them and cause havoc..
> > + /*FALLTHRU*/
>
> All the recent code we've added uses:
>
> /* fall through */
>
> for this annotation - it's the most widely used variant in the
> XFS codebase, so it would be good to be consistent here...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
---end quoted text---
