On Tue, Jan 21, 2020 at 11:44:40AM -0800, Darrick J. Wong wrote:
> Clean up? This whole XATTR_/ATTR_/XFS_ATTR_/XFS_IOC_ATTR_ quadrality is
> /still/ confusing to me.
> > struct xfs_mount *mp = dp->i_mount;
> > struct xfs_trans_res tres;
> > - int rsvd = (args->flags & ATTR_ROOT) != 0;
> > + int rsvd = (args->attr_namespace & XFS_ATTR_ROOT);
>
> bool?
Ok.
> > @@ -87,7 +67,7 @@ struct xfs_attr_list_context {
> > int dupcnt; /* count dup hashvals seen */
> > int bufsize; /* total buffer size */
> > int firstu; /* first used byte in buffer */
> > - int flags; /* from VOP call */
> > + unsigned int attr_namespace;
>
> What flags go with this attr_namespace field? XFS_ATTR_{ROOT,SECURE}?
Yes.. I'll add a comment.
> > +/*
> > + * Flags for the attr ioctl interface.
> > + * NOTE: Must match the values declared in libattr without the XFS_IOC_ prefix.
> > + */
> > +#define XFS_IOC_ATTR_ROOT 0x0002 /* use attrs in root namespace */
> > +#define XFS_IOC_ATTR_SECURE 0x0008 /* use attrs in security namespace */
> > +#define XFS_IOC_ATTR_CREATE 0x0010 /* fail if attr already exists */
> > +#define XFS_IOC_ATTR_REPLACE 0x0020 /* fail if attr does not exist */
>
> I think it's worth nothing that these flags are supposed to be passed in
> via am_flags in the attrmulti structure.
Done.
> > +++ b/fs/xfs/libxfs/xfs_types.h
> > @@ -194,7 +194,8 @@ typedef struct xfs_da_args {
> > uint8_t filetype; /* filetype of inode for directories */
> > void *value; /* set of bytes (maybe contain NULLs) */
> > int valuelen; /* length of value */
> > - int flags; /* argument flags (eg: ATTR_NOCREATE) */
> > + unsigned int attr_namespace;
> > + unsigned int attr_flags;
>
> Please add comments to both of these fields explaining which flags go
> with which field.
Ok.
> AFAICT, xfs_da_args.attr_namespace holds the two ondisk namespace flags?
Yes.
> Which are XFS_ATTR_{ROOT,SECURE}?
Yes.
> And ... I think the next patch makes
> it so that people can pass XFS_ATTR_INCOMPLETE for lookups, too?
Yes. Internal lookups at least.
>
> vs. xfs_da_args.attr_flags, which contains the XATTR_{CREATE,REPLACE}
> flags, which are the attr operation flags that we got from userspace?
Yes.
> And what goes in xfs_attr_list_context.attr_namespace? Same values as
> xfs_da_args.attr_namespace?
Yes.
> > +static unsigned int
> > +xfs_attr_namespace(
> > + u32 ioc_flags)
> > +{
> > + unsigned int namespace = 0;
> > +
> > + if (ioc_flags & XFS_IOC_ATTR_ROOT)
> > + namespace |= XFS_ATTR_ROOT;
> > + if (ioc_flags & XFS_IOC_ATTR_SECURE)
> > + namespace |= XFS_ATTR_SECURE;
>
> Seeing as these are mutually exclusive options, I'm a little surprised
> there isn't more checking that both of these flags aren't set at the
> same time.
>
> (Or I've been reading this series too long and missed that it does...)
XFS never rejected the combination. It just won't find anything in that
case. Let me see if I could throw in another patch to add more checks
there.
