[libvirt] [PATCH 1/2] libvirt-rust: Fix bugs in Stream::recv
* 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@faern.net> --- 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()) } } -- 2.21.0
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@faern.net> --- 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. Other than that it's good.
} } -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday, September 20, 2019 9:58 AM, Martin Kletzander <mkletzan@redhat.com> 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@faern.net
------------------------------------------------
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. IMO, the best would be to make Error into a more expressive type with something similar to std::io::ErrorKind.
Other than that it's good.
} }
----
2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> 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@faern.net
------------------------------------------------
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Linus Färnstrand -
Martin Kletzander