move checks from Queue::is_valid to setters

[lauralt]

There are a couple of things we are validating in Queue::is_valid() that are requirements according to the virtio spec. Calling is_valid() is optional, but we should always try to validate the MUST things from the spec, and not let the user be able to configure something that is not conforming to the spec.
We can move the mandatory checks from is_valid to the corresponding Queue setters, for example the following requirement The driver MUST ensure that the physical address of the first byte of each virtqueue part is a multiple of the specified alignment value in the above table. can be validated when setting the address of these parts. At this point these validations are done as part of is_valid. There are also requirements for the queue size, e.g. Queue Size value is always a power of 2. which can be done in the set_size() method that is added in this PR.

[germag]

There are a couple of things we are validating in Queue::is_valid() that are requirements according to the virtio spec. Calling is_valid() is optional, but we should always try to validate the MUST things from the spec, and not let the user be able to configure something that is not conforming to the spec. We can move the mandatory checks from is_valid to the corresponding Queue setters, for example the following requirement The driver MUST ensure that the physical address of the first byte of each virtqueue part is a multiple of the specified alignment value in the above table. can be validated when setting the address of these parts. At this point these validations are done as part of is_valid. There are also requirements for the queue size, e.g. Queue Size value is always a power of 2. which can be done in the set_size() method that is added in this PR.

How do you think errors should be handled? For instance, if the size checks are moved to set_size() like:

fn set_size(&mut size, size: u16) {
    if self.size > self.max_size ... {
        error!("virtio queue with...");
    } else {
        self.size = size
    }
}

If the size is invalid, since the error! macro only logs the error, the queue will be in an invalid state, is it the case or I'm missing something?, Also to update the test_queue_and_iterator() that currently depends on is_valid()
Should the issue #105 be resolved first?

[lauralt]

Yes it should, or maybe together. I was thinking we can return an error for now, and leave the events/metrics support for later.

[germag]

Yes it should, or maybe together. I was thinking we can return an error for now, and leave the events/metrics support for later.

sorry I'm not familiar (yet) with the code, so is it ok to change,

fn set_ready(&mut self, ready: bool); 

to something like this?:

fn set_ready(&mut self, ready: bool) -> Result<(), Error>; 

Since I need to change many setters, also for QueueStateSync, will it breaks something outside this crate?

[lauralt]

Yes it should, or maybe together. I was thinking we can return an error for now, and leave the events/metrics support for later.

sorry I'm not familiar (yet) with the code, so is it ok to change,

fn set_ready(&mut self, ready: bool); 

to something like this?:

fn set_ready(&mut self, ready: bool) -> Result<(), Error>; 

Since I need to change many setters, also for QueueStateSync, will it breaks something outside this crate?

Since the crate is not yet published, updating the interface should be ok. @jiangliu @slp @alexandruag @bonzini do you see any problem in returning a Result for set_size(), set_desc_table_address(), set_avail_ring_address() and set_used_ring_address()?

Btw, for your particular example, set_ready() should not be changed :-?, I don't remember any constraints regarding setting the ready bit. We can leave it as it is and double-check in is_valid() that the queue is indeed ready for processing.

[germag]

Yes it should, or maybe together. I was thinking we can return an error for now, and leave the events/metrics support for later.

sorry I'm not familiar (yet) with the code, so is it ok to change,

fn set_ready(&mut self, ready: bool); 

to something like this?:

fn set_ready(&mut self, ready: bool) -> Result<(), Error>; 

Since I need to change many setters, also for QueueStateSync, will it breaks something outside this crate?

Since the crate is not yet published, updating the interface should be ok. @jiangliu @slp @alexandruag @bonzini do you see any problem in returning a Result for set_size(), set_desc_table_address(), set_avail_ring_address() and set_used_ring_address()?

Btw, for your particular example, set_ready() should not be changed :-?, I don't remember any constraints regarding setting the ready bit. We can leave it as it is and double-check in is_valid() that the queue is indeed ready for processing.

Yes you are right, I meant:

fn set_size(&mut self, size: u16) -> Result<(), Error>; 

[germag]

In the virtio-device crate, VirtioMmioDevice::write calls these setters, what should I do here, if an invalid value is tried to be assigned?, should I set the device status to FAILED?

Also, in AvailIter::next() should I need to change something else or just the doc?

[lauralt]

In the virtio-device crate, VirtioMmioDevice::write calls these setters, what should I do here, if an invalid value is tried to be assigned?, should I set the device status to FAILED?

That's what I had in mind, but I have to think a bit more to see if it actually makes sense.

Also, in AvailIter::next() should I need to change something else or just the doc?

I don't think we should update something in the doc there (if you are referring to this comment), we are still checking in is_valid() that the memory range for each queue part is valid.