Re: [PATCH v8 06/10] io_uring/rw: add support to send metadata along with read/write

[Christoph Hellwig <hch@lst.de>]

On Thu, Nov 07, 2024 at 12:56:03PM +0530, Anuj gupta wrote:
> > > +/* extended capability flags */
> > > +#define EXT_CAP_PI   (1U << EXT_CAP_PI_BIT)
> >
> > This is getting into nitpicking, but is the a good reason to have that
> > enum, which is never used as a type and the values or only defined to
> > actually define the bit positions below?  That's a bit confusing to
> > me.
> 
> The enum is added to keep a check on the number of flags that can
> be added, and make sure that we don't overflow.

Umm, it is pretty clear you overflow when you do a

#define EXT_CAP_FOO   (1U << 16)

and assign it u16.  Just about every static checker will tell you
even if you don't instantly see it.  Basic testing will also show
you it won't work..

> > Also please document the ABI for EXT_CAP_PI, right now this is again
> > entirely undocumented.
> >
> 
> We are planning to document this in man/io_uring_enter.2 in the liburing
> repo, right after this series goes in. Or should it go somewhere else?

Well, it needs to go into the code actually explaining what the flag
does.  Throwing an undocumented flag into a uapi is just asking for
trouble.

> The attempt here is that if two extended capabilities are not known to
> co-exist then they can be kept in the same place. Since each extended
> capability is now a flag, we can check what combinations are valid and
> throw an error in case of incompatibility. Do you see this differently?

You only know they can't co-exist when you add them, and at that point
you can add a union.

> 
> >
> > struct io_uring_sqe_ext {
> >         /*
> >          * Reservered for please tell me what and why it is in the beginning
> >          * and not the end:
> >          */
> >         __u64   rsvd0[4];
> 
> This space is reserved for extended capabilities that might be added down
> the line. It was at the end in the earlier versions, but it is moved
> to the beginning
> now to maintain contiguity with the free space (18b) available in the first SQE,
> based on previous discussions [1].

I can't follow the argument.  But if you reserve space at the beginning
of the structure instead of the usual end you'd better add a comment
explaining it.