On Sun, Sep 22, 2019 at 04:38:26PM +0000, Linus Färnstrand wrote:
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, September 20, 2019 9:58 AM, Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:On Thu, Sep 19, 2019 at 05:48:37AM +0000, Linus Färnstrand wrote: > - pass same size to virStreamRecv as the buffer has allocated > - Handle -2 error case > - Fix FFI declaration to take size_t instead of c_uint > - Allow user to pass in buffer. To allow user to decide where to > allocate it. And to be able to re-use the same buffer > > - Don't try to treat binary data as a string > > Signed-off-by: Linus Färnstrand faern@xxxxxxxxx > > ------------------------------------------------ > > src/stream.rs | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > diff --git a/src/stream.rs b/src/stream.rs > index 8333ee5..af6c8ec 100644 > --- a/src/stream.rs > +++ b/src/stream.rs > @@ -18,6 +18,7 @@ > extern crate libc; > +use std::convert::TryFrom; > use std::str; > use error::Error; > @@ -37,7 +38,7 @@ extern "C" { > -> libc::c_int; > fn virStreamRecv(c: sys::virStreamPtr, > data: *mut libc::c_char, > > - nbytes: libc::c_uint) > > > > - nbytes: libc::size_t) > -> libc::c_int; > > > fn virStreamFree(c: sys::virStreamPtr) -> libc::c_int; > fn virStreamAbort(c: sys::virStreamPtr) -> libc::c_int; > @@ -116,14 +117,14 @@ impl Stream { > } > } > > > - pub fn recv(&self, size: u32) -> Result<String, Error> { > - unsafe { > > > - let mut data: [libc::c_char; 2048] = ['\\0' as i8; 2048]; > > > - let ret = virStreamRecv(self.as_ptr(), data.as_mut_ptr(), size as libc::c_uint); > > > - if ret == -1 { > > > - return Err(Error::new()); > > > - } > > > - return Ok(c_chars_to_string!(data.as_ptr())); > > > - } > > > > - pub fn recv(&self, buf: &mut [u8]) -> Result<usize, Error> { > - let ret = unsafe { > > > - virStreamRecv( > > > - self.as_ptr(), > > > - buf.as_mut_ptr() as *mut libc::c_char, > > > - buf.len(), > > > - ) > > > - }; > > > - usize::try_from(ret).map_err(|_| Error::new()) > > This way it is impossible to distinguish -1 and -2. We could get a bit closer to std::io::Read if -2 gets transformed to 0, as we haven't read anything.I thought the libvirt error would be set to something appropriate in the -2 case. Maybe I'm wrong. The problem is that the error type currently is very opaque. I wanted to also make the error type better, but that felt out of scope for this patch. I don't think just transforming -2 into 0 is very good, since that also does not allow distinguish that error from nothing read.
That error is basically "nothing read". Some drivers even check if 0 bytes were read just so they can return -2. There is no error that would be set so getting one from libvirt is like calling strerror(0).
IMO, the best would be to make Error into a more expressive type with something similar to std::io::ErrorKind.
Maybe, as WouldBlock would then clearly say what's happening.
Other than that it's good. > } > } > > ---- > > 2.21.0 > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
