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

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> Could you maybe add a short patch description that explains _why_ you
> are doing this change?

The virtio_set_status lines were getting too long because of OR'ing the
status on the same line of the call. Moreover, going forward we need to
add FEATURES_OK status as well. The state progress is quite straight
forward, so used a variable instead. Code looks cleaner and can easily
make out what change I am making in the state.


> (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?

virtio-net does have an error. virtio-{blk,9p} does not have it. Thought of
having it here to indicate device failure. May be I can add in 9p code
as well. I will come to virtio-scsi later, as we havent started using
virtio_queue_init_vq there.

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