On Wed, Jul 12, 2023 at 06:09:31AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 11, 2023 at 10:37:38PM -0700, Darrick J. Wong wrote:
> > Here's my version, where I go for a straight a[1] -> a[] conversion and
> > let downstream attrlist ioctl users eat their lumps. What do you think
> > of the excess-documentation approach?
>
> I think this is roughtly the right thing, with one big caveat:
>
> > + /* In Linux 6.5 this flex array was changed from list[1] to list[]. */
>
> For all the format headers there's no need for these comments. We've
> done this for various other structures that had the old one size arrays
> and never bothered.
<nod> Dave looked at an earlier version and wanted the comments for
xfs_da_format.h to steer people at the entsize helpers. I more or less
agree that it's overkill everywhere else though.
> > diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> > index 9c60ebb3..8927ecb2 100644
> > --- a/libxfs/xfs_fs.h
> > +++ b/libxfs/xfs_fs.h
> > @@ -588,16 +588,19 @@ typedef struct xfs_attrlist_cursor {
> > *
> > * NOTE: struct xfs_attrlist must match struct attrlist defined in libattr, and
> > * struct xfs_attrlist_ent must match struct attrlist_ent defined in libattr.
> > + *
> > + * WARNING: In Linux 6.5, al_offset and a_name were changed from array[1] to
> > + * array[]. Anyone using sizeof is (already) broken!
> > */
> > struct xfs_attrlist {
> > __s32 al_count; /* number of entries in attrlist */
> > __s32 al_more; /* T/F: more attrs (do call again) */
> > - __s32 al_offset[1]; /* byte offsets of attrs [var-sized] */
> > + __s32 al_offset[]; /* byte offsets of attrs [var-sized] */
> > };
> >
> > struct xfs_attrlist_ent { /* data from attr_list() */
> > __u32 a_valuelen; /* number bytes in value of attr */
> > - char a_name[1]; /* attr name (NULL terminated) */
> > + char a_name[]; /* attr name (NULL terminated) */
> > };
>
> Now these are currently exposed in a xfsprogs headeer and IFF someone
> is using them it would break them in nasty ways. That being said,
> xfsprogs itself doesn't use them as it relies on identically layed
> out but differntly named structures from libattr.
>
> So I think we should just move these two out of xfs_fs.h, because
> while they document a UAPI, they aren't actually used by userspace.
>
> With that the need for any caution goes away and we can just fix the
> definition without any caveats.
So I looked at the libattr headers for Ubuntu 22.04 and saw this:
/*
* List the names and sizes of the values of all the attributes of an object.
* "Cursor" must be allocated and zeroed before the first call, it is used
* to maintain context between system calls if all the attribute names won't
* fit into the buffer on the first system call.
* The return value is -1 on error (w/errno set appropriately), 0 on success.
*/
extern int attr_list(const char *__path, char *__buffer, const int __buffersize,
int __flags, attrlist_cursor_t *__cursor)
__attribute__ ((deprecated ("Use listxattr or llistxattr instead")));
extern int attr_listf(int __fd, char *__buffer, const int __buffersize,
int __flags, attrlist_cursor_t *__cursor)
__attribute__ ((deprecated ("Use flistxattr instead")));
I take that as a sign that they could drop all these xfs-specific APIs
one day, which means we ought to keep them in xfs_fs.h.
--D
