Re: [PATCH V2 2/2] ublk_drv: make sure that correct flags(features) returned to userspace

[Ming Lei <ming.lei@redhat.com>]

On Thu, Jul 21, 2022 at 10:50:43PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 22, 2022 at 01:09:30PM +0800, Ming Lei wrote:
> > +	unsigned long *map = (unsigned long *)&ub->dev_info.flags[0];
> > +
> > +	/* We are not ready to support zero copy */
> > +	ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
> > +
> > +	/*
> > +	 * 128bit flags will be copied back to userspace as feature
> > +	 * negotiation result, so have to clear flags which driver
> > +	 * doesn't support yet, then userspace can get correct flags
> > +	 * (features) to handle.
> > +	 */
> > +	bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS);
> 
> Please don't do the cast and bitmap ops.  In fact I think the current
> ABI is rather nasty.  To make everyones life easier just use a single
> u64 flags, an mark the second one reserved so that we can extent into
> it with extra flags or something else.  That way normal C operator
> leve bitops just work.

OK.

> 
> > +enum ublk_flag_bits {
> > +	__UBLK_F_SUPPORT_ZERO_COPY,
> > +	__UBLK_F_URING_CMD_COMP_IN_TASK,
> > +	__UBLK_F_NR_BITS,
> > +};
> 
> Please make these #defines so that userspace can detect if they
> exist in a header using #ifdef.

userspace is supposed to only use UBLK_F_* instead of __UBLK_F_*, one
benefit of using enum is that UBLK_F_NR_BITS can be figured out
automatically, otherwise how can we figure out the max bits?


thanks,
Ming