Virtualization - Nov 2022 - [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

On Wed, Nov 23, 2022 at 10:22:26PM +0000, Chaitanya Kulkarni
wrote:
> 
> > +/* Get lifetime information from device */
> > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned
long arg)
> > +{
> > +	struct request_queue *q = vblk->disk->queue;
> > +	struct request *req = NULL;
> > +	struct virtblk_req *vbr;
> > +	struct virtio_blk_lifetime lifetime;
> > +	int ret;
> > +
> > +	/* The virtio_blk_lifetime struct fields follow virtio spec.
> > +	 * There is no check/decode on values received from the device.
> > +	 * The data is sent as is to the user.
> > +	 */
> > +
> > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > +	 * feature is negotiated.
> > +	 */
> > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > +		return -ENOTTY;
> > +
> > +	memset(&lifetime, 0, sizeof(lifetime));
> > +
> 
> you can remove memset 0 call here and declare initialize struct var
> something like totally untested :-
> 
> 	struct virtio_blk_lifetime lifetime = { };
> 
> -ck
Yes, that's a bit cleaner, but there should be no space between {}:
	struct virtio_blk_lifetime lifetime = {};


-- 
MST

Thanks for your comments.
> > +     /* Pass the data to the user */
> > +     if (copy_to_user((void __user *)arg, &lifetime,
sizeof(lifetime))) {
> > +             ret = -EFAULT;
> > +             goto out;
>
>
> there nothing here to "goto out" following is sufficient I guess
:-

You're right, but like Michael said, it will be pretty easy to forget
adding a goto statement if adding more code to the function.
> > +     memset(&lifetime, 0, sizeof(lifetime));
> > +
>
>
> you can remove memset 0 call here and declare initialize struct var
> something like totally untested :-
>
>
>         struct virtio_blk_lifetime lifetime = { };
>
> Yes, that's a bit cleaner, but there should be no space between {}:
>         struct virtio_blk_lifetime lifetime = {};

I will fix it in the next version.

Alvaro