[libvirt] [rust PATCH v2 0/5] 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". Also the changes have run though the rustfmt to ensure the format conforms to Rust officially recommended code style. Version 2: Addressed comments Zixing Liu (5): libvirt-rust: stream: add more functions in stream libvirt-rust: stream: add more functions in stream libvirt-rust: stream: automated lint libvirt-rust: use reference instead of moving libvirt-rust: stream: addressed comments src/domain.rs | 2 +- src/stream.rs | 129 +++++++++++++++++++++++++++++++++++++++++------- tests/stream.rs | 40 +++++++++++++++ 3 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 tests/stream.rs -- 2.24.1
* 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 | 42 +++++++++++++++++++++++++++++++++++++++++- tests/stream.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 tests/stream.rs 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()); + } + 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) + }; + 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.24.1
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@aosc.io> --- 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@canonical.com>
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@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.24.1
On 2020-01-10 04:36, Sahid Orentino Ferdjaoui wrote:
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@aosc.io> --- 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@canonical.com>
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().
Yes, and the change was included in this series of patches, patch number 05. For the comment, should I argue about the pointer in question is mostly safe since it's coming from the underlying `libvirt` library?
+ 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@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.24.1
* 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 de272ab..1ffd186 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -47,6 +47,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; @@ -61,9 +67,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 { @@ -75,6 +98,13 @@ impl Drop for Stream { e.message) } } + 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) + } + } } } @@ -90,7 +120,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 { @@ -147,6 +177,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.24.1
On Tue, Dec 24, 2019 at 12:12:52AM -0700, Zixing Liu wrote:
* 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(-)
Reviewed-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com>
diff --git a/src/stream.rs b/src/stream.rs index de272ab..1ffd186 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -47,6 +47,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; @@ -61,9 +67,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)]
This line can be removed.
pub struct Stream { ptr: Option<sys::virStreamPtr>, + callback: Option<Box<dyn FnMut(&Stream, StreamEventType)>>, }
impl Drop for Stream { @@ -75,6 +98,13 @@ impl Drop for Stream { e.message) } } + 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) + } + } } }
@@ -90,7 +120,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 { @@ -147,6 +177,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.24.1
* used rustfmt to clean up the code Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@ extern crate libc; -use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom; use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; } @@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { } pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } } pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { } pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
On Tue, Dec 24, 2019 at 12:12:53AM -0700, Zixing Liu wrote:
* used rustfmt to clean up the code
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-)
NAK, we don't have specific rule for that. Even if I'm OK with you to use rustfmt we should consider having CI to also check that.
diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@
extern crate libc;
-use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom;
use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; }
@@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { }
pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } }
pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { }
pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
On 2020-01-10 04:48, Sahid Orentino Ferdjaoui wrote:
On Tue, Dec 24, 2019 at 12:12:53AM -0700, Zixing Liu wrote:
* used rustfmt to clean up the code
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-) NAK, we don't have specific rule for that. Even if I'm OK with you to use rustfmt we should consider having CI to also check that.
Well, it's also one part of my habit since formatted code is more readable and looks neat. Speaking of CI, I think you could integrate the format check or even commit format changes in CI using `cargo-fmt` which comes with every Rust installation.
diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@
extern crate libc;
-use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom;
use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; }
@@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { }
pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } }
pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { }
pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
On Fri, Jan 10, 2020 at 11:35:32AM -0700, Zixing Liu wrote:
On 2020-01-10 04:48, Sahid Orentino Ferdjaoui wrote:
On Tue, Dec 24, 2019 at 12:12:53AM -0700, Zixing Liu wrote:
* used rustfmt to clean up the code
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-) NAK, we don't have specific rule for that. Even if I'm OK with you to use rustfmt we should consider having CI to also check that.
Well, it's also one part of my habit since formatted code is more readable and looks neat. Speaking of CI, I think you could integrate the format check or even commit format changes in CI using `cargo-fmt` which comes with every Rust installation.
Do you think you can add with this patch a change in CI so that format of src/stream.rs will be verified? So basically we will avoid regression and we will be able to clean the code incrementally.
diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@
extern crate libc;
-use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom;
use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; }
@@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { }
pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } }
pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { }
pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
On 1/13/20 3:53 AM, Sahid Orentino Ferdjaoui wrote:
On Fri, Jan 10, 2020 at 11:35:32AM -0700, Zixing Liu wrote:
On 2020-01-10 04:48, Sahid Orentino Ferdjaoui wrote:
On Tue, Dec 24, 2019 at 12:12:53AM -0700, Zixing Liu wrote:
* used rustfmt to clean up the code
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-) NAK, we don't have specific rule for that. Even if I'm OK with you to use rustfmt we should consider having CI to also check that. Well, it's also one part of my habit since formatted code is more readable and looks neat. Speaking of CI, I think you could integrate the format check or even commit format changes in CI using `cargo-fmt` which comes with every Rust installation. Do you think you can add with this patch a change in CI so that format of src/stream.rs will be verified? So basically we will avoid regression and we will be able to clean the code incrementally.
Well, I guess I can add the patch to check the format in the changed files from the last commit in the CI system. However, I am not quite sure after merging a multi-commit patch, what changes would the `HEAD^...HEAD` include. Also, in this patch series, I changed more than one file. The formatter will check the entire file in which the changes had occurred instead of the diff since the format check is contextual in Rust. And are there any other problems with this patch-set that I haven't addressed?
diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@
extern crate libc;
-use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom;
use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; }
@@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { }
pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } }
pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { }
pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
On Fri, Jan 17, 2020 at 12:23:46AM -0700, Zixing Liu wrote:
On 1/13/20 3:53 AM, Sahid Orentino Ferdjaoui wrote:
On Fri, Jan 10, 2020 at 11:35:32AM -0700, Zixing Liu wrote:
On 2020-01-10 04:48, Sahid Orentino Ferdjaoui wrote:
On Tue, Dec 24, 2019 at 12:12:53AM -0700, Zixing Liu wrote:
* used rustfmt to clean up the code
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 97 +++++++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 41 deletions(-) NAK, we don't have specific rule for that. Even if I'm OK with you to use rustfmt we should consider having CI to also check that. Well, it's also one part of my habit since formatted code is more readable and looks neat. Speaking of CI, I think you could integrate the format check or even commit format changes in CI using `cargo-fmt` which comes with every Rust installation. Do you think you can add with this patch a change in CI so that format of src/stream.rs will be verified? So basically we will avoid regression and we will be able to clean the code incrementally.
Well, I guess I can add the patch to check the format in the changed files from the last commit in the CI system. However, I am not quite sure after merging a multi-commit patch, what changes would the `HEAD^...HEAD` include.
Also, in this patch series, I changed more than one file. The formatter will check the entire file in which the changes had occurred instead of the diff since the format check is contextual in Rust.
And are there any other problems with this patch-set that I haven't addressed?
I suggest you to rebase your change and re-order the patches so the "3/5 libvirt-rust: stream: automated lint" become the last one + you add to that patch the CI update so that will ensure no regression regarding the formatting of src/stream.rs. About the other patches I commented to some of your patches, they are just nits. if you can address them that would be great. Thank you Zixing Liu for your work I will wait for your change to be merged before to release a new version. s.
diff --git a/src/stream.rs b/src/stream.rs index 1ffd186..0d84fd7 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -18,8 +18,8 @@
extern crate libc;
-use connect::Connect; use connect::sys::virConnectPtr; +use connect::Connect; use std::convert::TryFrom;
use error::Error; @@ -33,28 +33,28 @@ 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) - -> libc::c_int; - fn virStreamRecv(c: sys::virStreamPtr, - data: *mut libc::c_char, - nbytes: libc::size_t) - -> libc::c_int; + fn virStreamNew(c: virConnectPtr, flags: libc::c_uint) -> sys::virStreamPtr; + fn virStreamSend( + c: sys::virStreamPtr, + data: *const libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; + fn virStreamRecv( + c: sys::virStreamPtr, + data: *mut libc::c_char, + nbytes: libc::size_t, + ) -> libc::c_int; 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 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; }
@@ -71,14 +71,16 @@ pub type StreamEventCallback = extern "C" fn(sys::virStreamPtr, libc::c_int, *co 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_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) {} @@ -93,16 +95,18 @@ impl Drop for Stream { fn drop(&mut self) { if self.ptr.is_some() { if let Err(e) = self.free() { - panic!("Unable to drop memory for Stream, code {}, message: {}", - e.code, - e.message) + panic!( + "Unable to drop memory for Stream, code {}, message: {}", + e.code, e.message + ) } } 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) + panic!( + "Unable to remove event callback for Stream, code {}, message: {}", + e.code, e.message + ) } } } @@ -120,7 +124,10 @@ impl Stream { }
pub fn from_ptr(ptr: sys::virStreamPtr) -> Stream { - Stream { ptr: Some(ptr), callback: None } + Stream { + ptr: Some(ptr), + callback: None, + } }
pub fn as_ptr(&self) -> sys::virStreamPtr { @@ -160,7 +167,7 @@ impl Stream { virStreamSend( self.as_ptr(), data.as_ptr() as *mut libc::c_char, - data.len() + data.len(), ) }; usize::try_from(ret).map_err(|_| Error::new()) @@ -177,10 +184,20 @@ 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> { + 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) + virStreamEventAddCallback( + self.as_ptr(), + events as libc::c_int, + event_callback, + ptr, + event_free, + ) }; if ret == -1 { return Err(Error::new()); @@ -190,9 +207,7 @@ impl Stream { }
pub fn event_update_callback(&self, events: StreamEventType) -> Result<(), Error> { - let ret = unsafe { - virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) - }; + let ret = unsafe { virStreamEventUpdateCallback(self.as_ptr(), events as libc::c_int) }; if ret == -1 { return Err(Error::new()); } -- 2.24.1
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 40a18d8..61ca411 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -1546,7 +1546,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(), string_to_c_chars!(name), -- 2.24.1
On Tue, Dec 24, 2019 at 12:12:54AM -0700, Zixing Liu wrote:
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/domain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@canonical.com>
diff --git a/src/domain.rs b/src/domain.rs index 40a18d8..61ca411 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -1546,7 +1546,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(), string_to_c_chars!(name), -- 2.24.1
* minimized unsafe scope * removed pub from `from_ptr` function Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/stream.rs b/src/stream.rs index 0d84fd7..ea623f6 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -114,16 +114,14 @@ 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 { + fn from_ptr(ptr: sys::virStreamPtr) -> Stream { Stream { ptr: Some(ptr), callback: None, @@ -139,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.24.1
On Tue, Dec 24, 2019 at 12:12:55AM -0700, Zixing Liu wrote:
* minimized unsafe scope * removed pub from `from_ptr` function
Signed-off-by: Zixing Liu <liushuyu@aosc.io> --- src/stream.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
Reviewed-by: Sahid Orentino Ferdjaoui <sahud.ferdjaoui@canonical.com>
diff --git a/src/stream.rs b/src/stream.rs index 0d84fd7..ea623f6 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -114,16 +114,14 @@ 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 { + fn from_ptr(ptr: sys::virStreamPtr) -> Stream { Stream { ptr: Some(ptr), callback: None, @@ -139,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.24.1
participants (2)
-
Sahid Orentino Ferdjaoui -
Zixing Liu