> > > > What could be useful for userspace is also ability to figure out which
> > > > FS_DOSATTRIB_* are supported by the filesystem. Because for example UDF
> > > > on-disk format supports only FS_DOSATTRIB_HIDDEN bit. And FAT only those
> > > > attributes which are in the lowest byte.
> > > >
> > >
> > > Exactly.
> > > statx has this solved with the stx_attributes_mask field.
> > >
> > > We could do the same for FS_IOC_FS[GS]ETXATTR, but because
> > > right now, this API does not verify that fsx_pad is zero, we will need to
> > > define a new set of ioctl consants FS_IOC_[GS]ETFSXATTR2
> > > with the exact same functionality but that userspace knows that they
> > > publish and respect the dosattrib mask:
> >
> > I understand and this is a problem.
> >
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -868,9 +868,11 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > > case FS_IOC_SETFLAGS:
> > > return ioctl_setflags(filp, argp);
> > >
> > > + case FS_IOC_GETFSXATTR2:
> > > case FS_IOC_FSGETXATTR:
> > > return ioctl_fsgetxattr(filp, argp);
> > >
> > > + case FS_IOC_SETFSXATTR2:
> > > case FS_IOC_FSSETXATTR:
> > > return ioctl_fssetxattr(filp, argp);
> > > --- a/include/uapi/linux/fs.h
> > > +++ b/include/uapi/linux/fs.h
> > > @@ -145,7 +145,8 @@ struct fsxattr {
> > > __u32 fsx_nextents; /* nextents field value (get) */
> > > __u32 fsx_projid; /* project identifier (get/set) */
> > > __u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > - unsigned char fsx_pad[8];
> > > + __u32 fsx_dosattrib; /* dosattrib field value (get/set) */
> > > + __u32 fsx_dosattrib_mask; /* dosattrib field validity mask */
> > > };
> > >
> > > /*
> > > @@ -238,6 +248,9 @@ struct fsxattr {
> > > #define FS_IOC32_SETFLAGS _IOW('f', 2, int)
> > > #define FS_IOC32_GETVERSION _IOR('v', 1, int)
> > > #define FS_IOC32_SETVERSION _IOW('v', 2, int)
> > > +#define FS_IOC_GETFSXATTR2 _IOR('x', 31, struct fsxattr)
> > > +#define FS_IOC_SETFSXATTR2 _IOW('x', 32, struct fsxattr)
> > > +/* Duplicate legacy ioctl numbers for backward compact */
> > > #define FS_IOC_FSGETXATTR _IOR('X', 31, struct fsxattr)
> > > #define FS_IOC_FSSETXATTR _IOW('X', 32, struct fsxattr)
> > > #define FS_IOC_GETFSLABEL _IOR(0x94, 49, char[FSLABEL_MAX])
> > >
> > > We could also use this opportunity to define a larger fsxattr2 struct
> > > that also includes an fsx_xflags_mask field, so that the xflags namespace
> > > could also be extended in a backward compat way going forward:
> > >
> > > @@ -145,7 +145,21 @@ struct fsxattr {
> > > __u32 fsx_nextents; /* nextents field value (get) */
> > > __u32 fsx_projid; /* project identifier (get/set) */
> > > __u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > unsigned char fsx_pad[8];
> > >
> > > };
> > > +
> > > +/*
> > > + * Structure for FS_IOC_[GS]ETFSXATTR2.
> > > + */
> > > +struct fsxattr2 {
> > > + __u32 fsx_xflags; /* xflags field value (get/set) */
> > > + __u32 fsx_extsize; /* extsize field value (get/set)*/
> > > + __u32 fsx_nextents; /* nextents field value (get) */
> > > + __u32 fsx_projid; /* project identifier (get/set) */
> > > + __u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > + __u32 fsx_xflags_mask; /* xflags field validity mask */
> > > + __u32 fsx_dosattrib; /* dosattrib field value (get/set) */
> > > + __u32 fsx_dosattrib_mask; /* dosattrib field validity mask */
> > > +};
> > >
> > > And you'd also need to flug those new mask and dosattrib
> > > via struct fileattr into filesystems - too much to explain.
> > > try to figure it out (unless someone objects) and if you can't figure
> > > it out let me know.
> >
> > Yea, I think that this is thing which I should be able to figure out
> > once I start changing it.
> >
> > Anyway, I have alternative idea to the problem with fsx_pad. What about
> > introducing new fsx_xflags flag which would say that fsx_pad=fsx_dosattrib
> > is present? E.g.
> >
> > #define FS_XFLAG_HASDOSATTRIB 0x40000000
> >
> > Then we would not need new FS_IOC_GETFSXATTR2/FS_IOC_SETFSXATTR2 ioctls.
> >
> > Also fsx_pad has 8 bytes, which can store both attrib and mask, so new
> > struct fsxattr2 would not be needed too.
>
> In case we decide to not do 1-to-1 mapping of Windows FILE_ATTRIBUTE_*
The 1-to-1 is definitely not a requirement of the API.
> constants to these new Linux DOSATTRIB_* constants then we do not need
> 32-bit variable, but just 16-bit variable (I counted that we need just
> 10 bits for now).
The "for now" part is what worries me in this sentence.
> fsx_pad has currently 64 bits and we could use it for:
> - 32 bits for fsx_xflags_mask
> - 16 bits for fsx_dosattrib
> - 16 bits for fsx_dosattrib_mask
This is possible.
> And therefore no need for FS_IOC_GETFSXATTR2/FS_IOC_SETFSXATTR2 iocl or
> struct fsxattr2. Just an idea how to simplify new API. Maybe together
no need for struct fsxattr2.
but regarding no new ioctl I am not so sure.
> with new fsx_xflags bit to indicate that fsx_xflags_mask field is present:
> #define FS_XFLAG_HASXFLAGSMASK 0x20000000
>
I don't think that this flag is needed because there is no filesystem
with empty xflags_mask, so any non zero value of xflags_mask
is equivalent to setting this flag.
This is for the get side. For the set side things are more complicated.
A proper backward compat API should reject (-EINVAL) unknown flags.
As far as I can tell ioctl_fssetxattr() does not at any time verify that
fsx_pad is zero and as far as I can tell xfs_fileattr_set() does not
check for unsupported fsx_xflags.
So unless I am missing something, FS_IOC_FSSETXATTR will silently
ignore dosattrib flags (and mask) when called on an old kernel.
This is the justification of FS_IOC_SETFSXATTR2 - it will fail on an
old kernel, so application can trust that if it works - it also respects
dosattib and the masks.
Yes, applications can call FS_IOC_FSGETXATTR, check non zero
mask and deduce that FS_IOC_FSSETXATTR will respect the mask
This will probably work, but it is not a very clean API IMO.
Thanks,
Amir.
