Re: [libvirt] [rust PATCH v2 1/5] libvirt-rust: stream: add more functions in stream

On Tue, Dec 24, 2019 at 12:12:51AM -0700, Zixing Liu wrote: > * added new functions: virStreamNew virStreamEventUpdateCallback > virStreamEventRemoveCallback > * added new constants: VIR_STREAM_NONBLOCK > * added new types/aliases: StreamFlags > * changed the previous `new` associate function to `from_ptr` to avoid > naming conflicts > * added basic tests to test the creation of Stream struct (can not test > the correctness of virStreamEventRemoveCallback and > virStreamEventUpdateCallback due to "unsupported by the connection > driver") > > Signed-off-by: Zixing Liu <liushuyu(a)aosc.io&gt; > --- > src/stream.rs | 42 +++++++++++++++++++++++++++++++++++++++++- > tests/stream.rs | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+), 1 deletion(-) > create mode 100644 tests/stream.rs Reviewed-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui(a)canonical.com&gt; > diff --git a/src/stream.rs b/src/stream.rs > index 8bcdf4b..de272ab 100644 > --- a/src/stream.rs > +++ b/src/stream.rs > @@ -18,6 +18,8 @@ > > extern crate libc; > > +use connect::Connect; > +use connect::sys::virConnectPtr; > use std::convert::TryFrom; > > use error::Error; > @@ -31,6 +33,9 @@ pub mod sys { > > #[link(name = "virt")] > extern "C" { > + fn virStreamNew(c: virConnectPtr, > + flags: libc::c_uint) > + -> sys::virStreamPtr; > fn virStreamSend(c: sys::virStreamPtr, > data: *const libc::c_char, > nbytes: libc::size_t) > @@ -42,6 +47,9 @@ extern "C" { > fn virStreamFree(c: sys::virStreamPtr) -> libc::c_int; > fn virStreamAbort(c: sys::virStreamPtr) -> libc::c_int; > fn virStreamFinish(c: sys::virStreamPtr) -> libc::c_int; > + fn virStreamEventUpdateCallback(c: sys::virStreamPtr, > + events: libc::c_int) -> libc::c_int; > + fn virStreamEventRemoveCallback(c: sys::virStreamPtr) -> libc::c_int; > } > > pub type StreamEventType = self::libc::c_uint; > @@ -50,6 +58,9 @@ pub const VIR_STREAM_EVENT_WRITABLE: StreamEventType = (1 << 1); > pub const VIR_STREAM_EVENT_ERROR: StreamEventType = (1 << 2); > pub const VIR_STREAM_EVENT_HANGUP: StreamEventType = (1 << 3); > > +pub type StreamFlags = self::libc::c_uint; > +pub const VIR_STREAM_NONBLOCK: StreamFlags = (1 << 0); > + > #[derive(Debug)] > pub struct Stream { > ptr: Option<sys::virStreamPtr>, > @@ -68,7 +79,17 @@ impl Drop for Stream { > } > > impl Stream { > - pub fn new(ptr: sys::virStreamPtr) -> Stream { > + pub fn new(conn: &Connect, flags: StreamFlags) -> Result<Stream, Error> { > + unsafe { > + let ptr = virStreamNew(conn.as_ptr(), flags as libc::c_uint); > + if ptr.is_null() { > + return Err(Error::new()); > + } Is this block can be outside of the unsafe ? +1 if you add comment explaining why it is safe to use conn.as_ptr().

> + return Ok(Stream::from_ptr(ptr)); > + } > + } > + > + pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { > Stream { ptr: Some(ptr) } > } > > @@ -125,4 +146,23 @@ impl Stream { > }; > usize::try_from(ret).map_err(|_| Error::new()) > } > + > + pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { > + let ret = unsafe { > + virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) nit: Could be great to have a comment explaining why self.as_ptr() is safe to use. > + }; > + if ret == -1 { > + return Err(Error::new()); > + } > + return Ok(()); > + } > + > + pub fn event_remove_callback(&self) -> Result<(), Error> { > + unsafe { > + if virStreamEventRemoveCallback(self.as_ptr()) == -1 { Same comment here. > + return Err(Error::new()); > + } > + return Ok(()); > + } > + } > } > diff --git a/tests/stream.rs b/tests/stream.rs > new file mode 100644 > index 0000000..16531b3 > --- /dev/null > +++ b/tests/stream.rs > @@ -0,0 +1,40 @@ > +/* > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + * Sahid Orentino Ferdjaoui <sahid.ferdjaoui(a)redhat.com&gt; > + */ > + > +extern crate virt; > + > +mod common; > + > +use virt::stream::Stream; > +use virt::stream::VIR_STREAM_NONBLOCK; > + > +#[test] > +fn test_create_blocking() { > + let c = common::conn(); > + let s = Stream::new(&c, 0).unwrap(); > + drop(s); > + common::close(c); > +} > + > +#[test] > +fn test_create_non_blocking() { > + let c = common::conn(); > + let s = Stream::new(&c, VIR_STREAM_NONBLOCK).unwrap(); > + drop(s); > + common::close(c); > +} > -- > 2.24.1