read_exact_from and write_all_to from GuestMemory don't have the correct semantics

[sboeuf]

When using the GuestMemory implementation of read_exact_from and write_all_to, I was expecting they would handle the retry, but they simply return an Error saying they couldn't read or write the entire buffer. That's the wrong behavior IMO, especially because of the naming that suggest they should behave exactly as the Rust traits Read and Write.

The proper solution would be to call underlying functions read_exact_from and write_all_to from the GuestRegionMmap implementation after we found the regions affected by the range, leading to a proper retry when not all data has been read or written.

/cc @andreeaflorescu @alexandruag @jiangliu

[bonzini]

I agree, they should retry as long as read_from/write_to return Ok.

However, there's the problem of returning the number of bytes processed before an IOError. It would be best to change IOError to { error: io::Error, processed: usize } and implementing From<io::Error> for guest_memory::Error. That change is an API breakage, but somewhat unlikely to cause problems.

[andreeaflorescu]

Is this issue related to #171?

[sboeuf]

@andreeaflorescu

Is this issue related to #171?

I don't think so.

[alindima]

That's the wrong behavior IMO, especially because of the naming that suggest they should behave exactly as the Rust traits Read and Write.

The rust trait documentation says that ErrorKind::Interrupted errors are handled with a retry. Any other IOError is not and results in returning an Error from the function. Moreover, it says that the contents of the buffer are undefined in this case.
See: https://doc.rust-lang.org/std/io/trait.Read.html#errors-3
So I'm not sure that this would be the right semantic for read_exact_from and write_all_to.
I don't think we should be hiding unrecoverable errors, because this call could block indefinitely

I agree, they should retry as long as read_from/write_to return Ok.

I agree as well, but this is already true. read_exact_from calls read_from only once, but read_from calls read in a try_access, which performs it in a loop as long as there isn't an error or until we're done reading. This is also the root-cause of this issue: #171

[bonzini]

I don't think we should be hiding unrecoverable errors, because this call could block indefinitely

Indeed, absolutely not. So I'm not sure what can/should be changed, except possibly adjusting the error type.

[bonzini]

Looking at the cloud-hypervisor commit, that seems like cloud-hypervisor is setting sockets to non-blocking mode? If so, that is certainly something that read_exact cannot handle, and neither should read_exact_from. cloud-hypervisor instead must handle WouldBlock correctly.