[SLOF] [PATCH v2 15/19] virtio: 1.0 guest features negotiation

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 20.01.2016 13:10, Nikunj A Dadhania wrote:
>  /**
>>   * Set device status bits
>>   */
>> -void virtio_set_status(struct virtio_device *dev, int status)
>> +void virtio_set_status(struct virtio_device *dev, uint32_t status)
>
> Why do you force an uint32_t here? That sounds like status would be a
> 32-bit type ... but in the PCI space, this is only a single byte...
> so using "uint8_t" or keeping "int" would be better, I think.

Will maintain int

>>  
>> +int virtio_negotiate_guest_features(struct virtio_device *dev, uint64_t features)
>> +{
>> +	uint64_t host_features = 0;
>> +	uint32_t status;
>> +
>> +	/* Negotiate features */
>> +	virtio_get_host_features_long(dev, &host_features);
>> +	if (!(host_features & VIRTIO_F_VERSION_1)) {
>> +		fprintf(stderr, "Device does not support virtio 1.0 %llx\n", host_features);
>> +		return false;
>> +	}
>> +
>> +	virtio_set_guest_features_long(dev,  features);
>> +	virtio_get_host_features_long(dev, &host_features);
>> +	if ((host_features & features) != features) {
>> +		fprintf(stderr, "Features error %llx\n", features);
>> +		return false;
>> +	}
>> +
>> +	virtio_get_status(dev, &status);
>> +	status |= VIRTIO_STAT_FEATURES_OK;
>> +	virtio_set_status(dev, status);
>> +
>> +	/* Read back to verify the FEATURES_OK bit */
>> +	virtio_get_status(dev, &status);
>> +	if ((status & VIRTIO_STAT_FEATURES_OK) != VIRTIO_STAT_FEATURES_OK)
>> +		return false;
>> +
>> +	return true;
>> +}
>
> May I suggest to either change the return type into "bool" instead of
> "int", or to return 0 for success and a negative error code for
> failures?

Will continue as 0/-1 as int.

Regards
Nikunj