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@xxxxxxx>
> >>>> ---
> >>>> 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
