Re: [PATCH v3] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

Michael S. Tsirkin
2022-12-06T16:40:22Z

On Tue, Dec 06, 2022 at 11:31:44AM -0500, Stefan Hajnoczi wrote:
> On Mon, Dec 05, 2022 at 06:20:34PM +0200, Alvaro Karsz wrote:
>
> I don't like that the ioctl lifetime struct is passed through
> little-endian from the device to userspace. The point of this new ioctl
> is not to be a passthrough interface. The kernel should define a proper
> UABI struct for the ioctl and handle endianness conversion. But I think
> Michael is happy with this approach, so nevermind.
>
> > @@ -219,4 +247,8 @@ struct virtio_scsi_inhdr {
> > #define VIRTIO_BLK_S_OK 0
> > #define VIRTIO_BLK_S_IOERR 1
> > #define VIRTIO_BLK_S_UNSUPP 2
> > +
> > +/* Virtblk ioctl commands */
> > +#define VBLK_GET_LIFETIME _IOR('r', 0, struct virtio_blk_lifetime)
>
> Does something include <linux/ioctl.h> for _IOR()? Failure to include
> the necessary header file could break userspace applications that
> include <linux/virtio_blk.h>.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Good point. I think it's preferable to add a new header
for this stuff. virtio_blk_ioctl.h ? Have that pull in linux/ioctl.h
Also VIRTIO_BLK_IOCTL_GET_LIFETIME
might be a better name to avoid confusion and collisions.
And s/Virtblk/virtio-blk/

--
MST