[SLOF] [PATCH v2 07/19] virtio-{net, blk, scsi, 9p}: use status variable

[Thomas Huth]

Could you maybe add a short patch description that explains _why_ you
are doing this change? (I guess it's for byte-swapping later, but I
haven't read the other patches yet, so it would be nice if you could
mention it here already)

On 20.01.2016 13:10, Nikunj A Dadhania wrote:
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
>  lib/libvirtio/virtio-9p.c   | 22 +++++++++++++---------
>  lib/libvirtio/virtio-blk.c  | 22 +++++++++++++---------
>  lib/libvirtio/virtio-net.c  | 17 +++++++++++------
>  lib/libvirtio/virtio-scsi.c | 10 ++++++----
>  4 files changed, 43 insertions(+), 28 deletions(-)
...
> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
> index 8ae61bc..98c1a80 100644
> --- a/lib/libvirtio/virtio-blk.c
> +++ b/lib/libvirtio/virtio-blk.c
> @@ -31,32 +31,31 @@ virtioblk_init(struct virtio_device *dev)
>  	struct vring_avail *vq_avail;
>  	int blk_size = DEFAULT_SECTOR_SIZE;
>  	int features;
> +	uint32_t status = VIRTIO_STAT_ACKNOWLEDGE;
>  
>  	/* Reset device */
>  	virtio_reset_device(dev);
>  
>  	/* Acknowledge device. */
> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE);
> +	virtio_set_status(dev, status);
>  
>  	/* Tell HV that we know how to drive the device. */
> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER);
> +	status |= VIRTIO_STAT_DRIVER;
> +	virtio_set_status(dev, status);
>  
>  	/* Device specific setup - we support F_BLK_SIZE */
>  	virtio_set_guest_features(dev,  VIRTIO_BLK_F_BLK_SIZE);
>  
> -	if(!virtio_queue_init_vq(dev, &vq, 0)) {
> -		virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
> -				  |VIRTIO_STAT_FAILED);
> -		return 0;
> -	}
> +	if(!virtio_queue_init_vq(dev, &vq, 0))
> +		goto dev_error;
>  
>  	vq_avail = virtio_get_vring_avail(dev, 0);
>  	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>  	vq_avail->idx = 0;
>  
>  	/* Tell HV that setup succeeded */
> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
> -				|VIRTIO_STAT_DRIVER_OK);
> +	status |= VIRTIO_STAT_DRIVER_OK;
> +	virtio_set_status(dev, status);
>  
>  	virtio_get_host_features(dev, &features);
>  	if (features & VIRTIO_BLK_F_BLK_SIZE) {
> @@ -66,6 +65,11 @@ virtioblk_init(struct virtio_device *dev)
>  	}
>  
>  	return blk_size;
> +dev_error:
> +	fprintf(stderr, "Device Error \n");

You don't print anything for the other devices in case of errors... and
there was no fprintf in the old code, so maybe you should also omit this
fprintf?

> +	status |= VIRTIO_STAT_FAILED;
> +	virtio_set_status(dev, status);
> +	return 0;
>  }
...

 Thomas