uefi: serial: improve read() #1852
Conversation
phip1611
force-pushed
the
serial-read
branch
from
19e0b16 to
b70296c
Compare
|
Please review again, @nicholasbishop! I changed the PR since your last review quite significantly. PS: I'm working on a small fun project with the serial device in UEFI. |
phip1611
force-pushed
the
serial-read
branch
2 times, most recently
from
bba5e4f to
9218d96
Compare
uefi/src/proto/console/serial.rs
Outdated
| // UEFI was either not able to fill the whole buffer in the specified | ||
| // timeout, but nevertheless read some data, or there was too much | ||
| // data. | ||
| Status::TIMEOUT => Ok(buffer_size), |
There was a problem hiding this comment.
I'm not sure we should swallow the Timeout error like this, end users might have good reason to handle timeout differently.
It's a bit ugly, but what if we instead added an output arg like num_bytes_read: &mut usize, and document that it will be set correctly in both the success and error cases?
There was a problem hiding this comment.
end users might have good reason to handle timeout differently.
Fair concern, there might be use-cases for that. Let me think about it again.
(I was working on a fun project over the holidays and I adapted it to mainly work for my use-case)
UPDATE: New solution up.
| #[cfg(feature = "alloc")] | ||
| pub fn read_to_end(&mut self) -> Result<Vec<u8>> { | ||
| let mut vec = Vec::new(); | ||
| let mut buf = [0; 2048]; |
There was a problem hiding this comment.
Rather than reading into this temporary buffer and copying it to the vec, could we write directly to the vec?
There was a problem hiding this comment.
good question! no we could not unfortunately, because then we would lose the ability to grow the vector dynamically.
There was a problem hiding this comment.
I'm not clear why. Couldn't we write the vec, resize if needed, and then write at an offset for the next iteration, and so on?
| pub fn write(&mut self, data: &[u8]) -> Result<(), usize> { | ||
| let mut buffer_size = data.len(); | ||
| unsafe { (self.0.write)(&mut self.0, &mut buffer_size, data.as_ptr()) }.to_result_with( | ||
| || debug_assert_eq!(buffer_size, data.len()), |
There was a problem hiding this comment.
Is removing this debug_assert intentional? I don't see anything wrong with it.
There was a problem hiding this comment.
I decided to remove that intentionally as the implementation may decide to only write some bytes and one must call the function again afterwards. https://uefi.org/specs/UEFI/2.10/12_Protocols_Console_Support.html#efi-serial-io-protocol-write
There was a problem hiding this comment.
That would seem to conflict with this function's docstring: "This operation will block until the data has been fully written or an error occurs." If the docstring is correct, than the assert can stay, otherwise we should fix the docstring as well. I find the UEFI spec a little ambiguous in its language here. "EFI_SUCCESS -- The data was written" could be taken to mean all the data was written, but I'm not sure. Did you observe that calling write multiple times is needed in some real-world test?
phip1611
force-pushed
the
serial-read
branch
3 times, most recently
from
10c84c2 to
b54e7fe
Compare
We didn't handle partial reads correctly. The serial protocol will return Status::TIMEOUT in case it could not read as many bytes as the caller asked for. This is however still the good case and just a normal partial read. Therefore, I made the abstraction more convenient and natural to use by handling this properly.
This helps to read everything that is currently available on the device.
|
I refactored the PR and hope that I now improve the situation in a more flexible way. |
We are logically modifying state, so this should be mut.
The old way is not just very inflexible but also incorrect and error prone.
Checklist