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(a)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