[libvirt-rust PATCH v3 0/4] Map more functions in stream module
This set of patches will add more functions to the Rust bindings. Newly mapped functions from C library: virStreamNew virStreamEventUpdateCallback virStreamEventRemoveCallback virStreamEventAddCallback. virStreamEventAddCallback can accept normal fn functions or closures (can capture variables outside) The changes are not very thoroughly tested since event module is not implemented at all so the virStreamEventAddCallback will always return "unsupported by the connection driver". Version 2: Addressed comments Version 3: Undo format changes and rebased against latest branch Zixing Liu (4): libvirt-rust: stream: add more functions in stream libvirt-rust: stream: add more functions in stream libvirt-rust: use reference instead of moving libvirt-rust: stream: addressed comments src/domain.rs | 2 +- src/stream.rs | 94 ++++++++++++++++++++++++++++++++++++++++++++++--- tests/stream.rs | 40 +++++++++++++++++++++ 3 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 tests/stream.rs -- 2.25.0
* 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@aosc.io> --- src/stream.rs | 43 ++++++++++++++++++++++++++++++++++++++++++- tests/stream.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 tests/stream.rs diff --git a/src/stream.rs b/src/stream.rs index a110695..96e37cb 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,10 @@ 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, @@ -44,6 +50,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; @@ -52,6 +61,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>, @@ -71,7 +83,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()); + } + return Ok(Stream::from_ptr(ptr)); + } + } + + pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { Stream { ptr: Some(ptr) } } @@ -128,4 +150,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) + }; + if ret == -1 { + return Err(Error::new()); + } + return Ok(()); + } + + pub fn event_remove_callback(&self) -> Result<(), Error> { + unsafe { + if virStreamEventRemoveCallback(self.as_ptr()) == -1 { + 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@redhat.com> + */ + +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.25.0
* added virStreamEventAddCallback function * added new types: StreamEventCallback and FreeCallback * added new field: callback for storing event callback * drop: will drop the Box<callback> if any * added wrapper event_callback for easier callback authoring for the user (so that closures with Fn or FnMut references could be used) * added padding function event_free to just makes it compile (Rust should not need this, because Rust sticks to RAII) Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 96e37cb..3a83b34 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -50,6 +50,12 @@ 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 virStreamEventAddCallback(c: sys::virStreamPtr, + event: libc::c_int, + callback: StreamEventCallback, + opaque: *const libc::c_void, + ff: FreeCallback) + -> libc::c_int; fn virStreamEventUpdateCallback(c: sys::virStreamPtr, events: libc::c_int) -> libc::c_int; fn virStreamEventRemoveCallback(c: sys::virStreamPtr) -> libc::c_int; @@ -64,9 +70,26 @@ 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 type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *const libc::c_void); +pub type FreeCallback = extern "C" fn(*mut libc::c_void); + +// wrapper for callbacks +extern "C" fn event_callback(c: sys::virStreamPtr, flags: libc::c_int, opaque: *const libc::c_void) { + let flags = flags as StreamFlags; + let shadow_self = unsafe { + &mut*(opaque as *mut Stream) + }; + if let Some(callback) = &mut shadow_self.callback { + callback(&Stream::from_ptr(c), flags); + } +} + +extern "C" fn event_free(_opaque: *mut libc::c_void) {} + +// #[derive(Debug)] pub struct Stream { ptr: Option<sys::virStreamPtr>, + callback: Option<Box<dyn FnMut(&Stream, StreamEventType)>>, } impl Drop for Stream { @@ -79,6 +102,13 @@ impl Drop for Stream { ) } } + if self.callback.is_some() { + if let Err(e) = self.event_remove_callback() { + panic!("Unable to remove event callback for Stream, code {}, message: {}", + e.code, + e.message) + } + } } } @@ -94,7 +124,7 @@ impl Stream { } pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr) } + Stream { ptr: Some(ptr), callback: None } } pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -151,6 +181,18 @@ impl Stream { usize::try_from(ret).map_err(|_| Error::new()) } + pub fn event_add_callback<F: 'static + FnMut(&Stream, StreamEventType)>(&mut self, events: StreamEventType, cb: F) -> Result<(), Error> { + let ret = unsafe { + let ptr = &*self as *const _ as *const _; + virStreamEventAddCallback(self.as_ptr(), events as libc::c_int, event_callback, ptr, event_free) + }; + if ret == -1 { + return Err(Error::new()); + } + self.callback = Some(Box::new(cb)); + return Ok(()); + } + pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) -- 2.25.0
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/domain.rs b/src/domain.rs index 121d0f9..5de66c6 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -1644,7 +1644,7 @@ impl Domain { } } - pub fn open_console(&self, name: &str, stream: Stream, flags: u32) -> Result<u32, Error> { + pub fn open_console(&self, name: &str, stream: &Stream, flags: u32) -> Result<u32, Error> { unsafe { let ret = virDomainOpenConsole( self.as_ptr(), -- 2.25.0
* minimized unsafe scope * removed pub from `from_ptr` function Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 3a83b34..10145eb 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -114,17 +114,18 @@ impl Drop for Stream { impl 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()); - } - return Ok(Stream::from_ptr(ptr)); + let ptr = unsafe { virStreamNew(conn.as_ptr(), flags as libc::c_uint) }; + if ptr.is_null() { + return Err(Error::new()); } + return Ok(Stream::from_ptr(ptr)); } - pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + fn from_ptr(ptr: sys::virStreamPtr) -> Stream { + Stream { + ptr: Some(ptr), + callback: None, + } } pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -136,9 +137,9 @@ impl Stream { if virStreamFree(self.as_ptr()) == -1 { return Err(Error::new()); } - self.ptr = None; - return Ok(()); } + self.ptr = None; + return Ok(()); } pub fn finish(self) -> Result<(), Error> { -- 2.25.0
On Wed, Jan 29, 2020 at 08:24:10PM -0700, Zixing Liu wrote:
This set of patches will add more functions to the Rust bindings. Newly mapped functions from C library: virStreamNew virStreamEventUpdateCallback virStreamEventRemoveCallback virStreamEventAddCallback.
virStreamEventAddCallback can accept normal fn functions or closures (can capture variables outside)
The changes are not very thoroughly tested since event module is not implemented at all so the virStreamEventAddCallback will always return "unsupported by the connection driver".
Version 2: Addressed comments Version 3: Undo format changes and rebased against latest branch
The version 3 of your patches don't pass CI. Please consider to use `cargo fmt -v -- --check` to validate the coding style before to submit any patches. Also you can use `cargo fmt` to help you for the coding style.
Zixing Liu (4): libvirt-rust: stream: add more functions in stream libvirt-rust: stream: add more functions in stream
I would imagine both of these patches can be merged together, `git rebase -i master` and usage of `squash` is really helpful of that. For the title you should find something a bit more explicit like: stream: add better coverage for the stream API
libvirt-rust: use reference instead of moving libvirt-rust: stream: addressed comments
The best is to have the comments addressed in the patch itself. This can easily be achieved using `git rebase -i master` and by editing the right patch. Besides that your patches are OK. If you don't mind I could take care of addressing my comments before to merge them. You don't have to be worry, you still keep the credits for them. Sounds good for you?
src/domain.rs | 2 +- src/stream.rs | 94 ++++++++++++++++++++++++++++++++++++++++++++++--- tests/stream.rs | 40 +++++++++++++++++++++ 3 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 tests/stream.rs
-- 2.25.0
participants (2)
-
Sahid Orentino Ferdjaoui -
Zixing Liu