[libvirt] [PATCH 1/2] libvirt-rust: Fix bugs in Stream::recv

[Martin Kletzander]

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 at 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 at 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 at redhat.com
>> > https://www.redhat.com/mailman/listinfo/libvir-list
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190923/19303fda/attachment-0001.sig>