Hiroyuki Katsura
2019-Jul-30 05:51 UTC
[Libguestfs] [PATCH] Rust bindings: Implement Event features
This patch includes: - Event callback handlers - Tests related to events(410-430) --- generator/rust.ml | 38 ++++++- rust/src/base.rs | 24 +++-- rust/src/error.rs | 8 +- rust/src/event.rs | 158 ++++++++++++++++++++++++++++ rust/src/lib.rs | 2 + rust/tests/040_create_multiple.rs | 2 +- rust/tests/410_close_event.rs | 38 +++++++ rust/tests/420_log_messages.rs | 60 +++++++++++ rust/tests/430_progress_messages.rs | 61 +++++++++++ 9 files changed, 381 insertions(+), 10 deletions(-) create mode 100644 rust/src/event.rs create mode 100644 rust/tests/410_close_event.rs create mode 100644 rust/tests/420_log_messages.rs create mode 100644 rust/tests/430_progress_messages.rs diff --git a/generator/rust.ml b/generator/rust.ml index a1735602c..1f5cefa62 100644 --- a/generator/rust.ml +++ b/generator/rust.ml @@ -72,6 +72,42 @@ extern \"C\" { } "; + (* event enum *) + pr "\n"; + pr "pub enum Event {\n"; + List.iter ( + fun (name, _) -> + pr " %s,\n" (snake2caml name) + ) events; + pr "}\n\n"; + + pr "impl Event {\n"; + pr " pub fn to_u64(&self) -> u64 {\n"; + pr " match self {\n"; + List.iter ( + fun (name, i) -> + pr " Event::%s => %d,\n" (snake2caml name) i + ) events; + pr " }\n"; + pr " }\n"; + pr " pub fn from_bitmask(bitmask: u64) -> Option<Event> {\n"; + pr " match bitmask {\n"; + List.iter ( + fun (name, i) -> + pr " %d => Some(Event::%s),\n" i (snake2caml name) + ) events; + pr " _ => None,\n"; + pr " }\n"; + pr " }\n"; + pr "}\n\n"; + + pr "pub const EVENT_ALL: [Event; %d] = [\n" (List.length events); + List.iter ( + fun (name, _) -> + pr " Event::%s,\n" (snake2caml name) + ) events; + pr "];\n"; + List.iter ( fun { s_camel_name = name; s_name = c_name; s_cols = cols } -> pr "\n"; @@ -356,7 +392,7 @@ extern \"C\" { pr "}\n"; - pr "impl Handle {\n"; + pr "impl<'a> Handle<'a> {\n"; List.iter ( fun ({ name = name; shortdesc = shortdesc; longdesc = longdesc; style = (ret, args, optargs) } as f) -> diff --git a/rust/src/base.rs b/rust/src/base.rs index 02ad33535..0c6a3bdba 100644 --- a/rust/src/base.rs +++ b/rust/src/base.rs @@ -17,6 +17,10 @@ */ use crate::error; +use crate::event; +use crate::guestfs; +use std::collections; +use std::sync; #[allow(non_camel_case_types)] #[repr(C)] @@ -34,31 +38,37 @@ extern "C" { const GUESTFS_CREATE_NO_ENVIRONMENT: i64 = 1; const GUESTFS_CREATE_NO_CLOSE_ON_EXIT: i64 = 2; -pub struct Handle { +pub struct Handle<'a> { pub(crate) g: *mut guestfs_h, + pub(crate) callbacks: collections::HashMap< + event::EventHandle, + sync::Arc<Fn(guestfs::Event, event::EventHandle, &[u8], &[u64]) + 'a>, + >, } -impl Handle { - pub fn create() -> Result<Handle, error::Error> { +impl<'a> Handle<'a> { + pub fn create() -> Result<Handle<'a>, error::Error> { let g = unsafe { guestfs_create() }; if g.is_null() { Err(error::Error::Create) } else { - Ok(Handle { g }) + let callbacks = collections::HashMap::new(); + Ok(Handle { g, callbacks }) } } - pub fn create_flags(flags: CreateFlags) -> Result<Handle, error::Error> { + pub fn create_flags(flags: CreateFlags) -> Result<Handle<'a>, error::Error> { let g = unsafe { guestfs_create_flags(flags.to_libc_int()) }; if g.is_null() { Err(error::Error::Create) } else { - Ok(Handle { g }) + let callbacks = collections::HashMap::new(); + Ok(Handle { g, callbacks }) } } } -impl Drop for Handle { +impl<'a> Drop for Handle<'a> { fn drop(&mut self) { unsafe { guestfs_close(self.g) } } diff --git a/rust/src/error.rs b/rust/src/error.rs index 705ee1735..ce444e199 100644 --- a/rust/src/error.rs +++ b/rust/src/error.rs @@ -20,6 +20,7 @@ use crate::base; use crate::utils; use std::convert; use std::ffi; +use std::io; use std::os::raw::{c_char, c_int}; use std::str; @@ -41,6 +42,7 @@ pub enum Error { API(APIError), IllegalString(ffi::NulError), Utf8Error(str::Utf8Error), + UnixError(io::Error, &'static str), Create, } @@ -56,7 +58,11 @@ impl convert::From<str::Utf8Error> for Error { } } -impl base::Handle { +pub(crate) fn unix_error(operation: &'static str) -> Error { + Error::UnixError(io::Error::last_os_error(), operation) +} + +impl<'a> base::Handle<'a> { pub(crate) fn get_error_from_handle(&self, operation: &'static str) -> Error { let c_msg = unsafe { guestfs_last_error(self.g) }; let message = unsafe { utils::char_ptr_to_string(c_msg).unwrap() }; diff --git a/rust/src/event.rs b/rust/src/event.rs new file mode 100644 index 000000000..942feec69 --- /dev/null +++ b/rust/src/event.rs @@ -0,0 +1,158 @@ +/* libguestfs Rust bindings + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> + * + * 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 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, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +use crate::base; +use crate::error; +use crate::guestfs; +use std::ffi; +use std::os::raw::{c_char, c_void}; +use std::slice; +use std::sync; + +type GuestfsEventCallback = extern "C" fn( + *const base::guestfs_h, + *const c_void, + u64, + i32, + i32, + *const i8, + usize, + *const u64, + usize, +); + +#[link(name = "guestfs")] +extern "C" { + fn guestfs_set_event_callback( + g: *const base::guestfs_h, + cb: GuestfsEventCallback, + event_bitmask: u64, + flags: i32, + opaque: *const c_void, + ) -> i32; + fn guestfs_delete_event_callback(g: *const base::guestfs_h, eh: i32); + fn guestfs_event_to_string(bitmask: u64) -> *const c_char; + fn free(buf: *const c_void); +} + +#[derive(Hash, PartialEq, Eq)] +pub struct EventHandle { + eh: i32, +} + +pub type Callback = FnMut(guestfs::Event, EventHandle, &[i8], &[u64]); + +fn events_to_bitmask(v: &[guestfs::Event]) -> u64 { + let mut r = 0u64; + for x in v.iter() { + r |= x.to_u64(); + } + r +} + +pub fn event_to_string(events: &[guestfs::Event]) -> Result<String, error::Error> { + let bitmask = events_to_bitmask(events); + + let r = unsafe { guestfs_event_to_string(bitmask) }; + if r.is_null() { + Err(error::unix_error("event_to_string")) + } else { + let s = unsafe { ffi::CStr::from_ptr(r) }; + let s = s.to_str()?.to_string(); + unsafe { free(r as *const c_void) }; + Ok(s) + } +} + +/* -- Why Not Box<Callback> but Arc<Callback> (in struct base::Handle)? -- + * Assume that there are more than threads. While callback is running, + * if a thread frees the handle, automatically the buffer is freed if Box<Callback> + * is used. Therefore Arc<Callback> is used. + */ + +impl<'a> base::Handle<'a> { + pub fn set_event_callback<C: 'a>( + &mut self, + callback: C, + events: &[guestfs::Event], + ) -> Result<EventHandle, error::Error> + where + C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), + { + extern "C" fn trampoline<C>( + _g: *const base::guestfs_h, + opaque: *const c_void, + event: u64, + event_handle: i32, + _flags: i32, + buf: *const c_char, + buf_len: usize, + array: *const u64, + array_len: usize, + ) where + C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), + { + // trampoline function + // c.f. https://s3.amazonaws.com/temp.michaelfbryan.com/callbacks/index.html + + let event = match guestfs::Event::from_bitmask(event) { + Some(x) => x, + None => panic!("Failed to parse bitmask: {}", event), + }; + let eh = EventHandle { eh: event_handle }; + let buf = unsafe { slice::from_raw_parts(buf as *const u8, buf_len) }; + let array = unsafe { slice::from_raw_parts(array, array_len) }; + + let callback_ptr = unsafe { &*(opaque as *const sync::Arc<C>) }; + let callback = sync::Arc::clone(&callback_ptr); + callback(event, eh, buf, array) + } + let callback = sync::Arc::<C>::new(callback); + let event_bitmask = events_to_bitmask(events); + + let eh = { + // Weak::into_raw is nightly. + // In order to make sure that callback is freed when handle is freed, + // lifetime is explicitly declared. + let ptr: &'a sync::Arc<C> = Box::leak(Box::new(callback.clone())); + unsafe { + guestfs_set_event_callback( + self.g, + trampoline::<C>, + event_bitmask, + 0, + ptr as *const sync::Arc<C> as *const c_void, + ) + } + }; + if eh == -1 { + return Err(self.get_error_from_handle("set_event_callback")); + } + self.callbacks.insert(EventHandle { eh }, callback); + + Ok(EventHandle { eh }) + } + + pub fn delete_event_callback(&mut self, eh: EventHandle) -> Result<(), error::Error> { + unsafe { + guestfs_delete_event_callback(self.g, eh.eh); + } + self.callbacks.remove(&eh); + Ok(()) + } +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index cc41a99f8..81adef2a3 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -18,9 +18,11 @@ mod base; mod error; +mod event; mod guestfs; mod utils; pub use crate::base::*; pub use crate::error::*; +pub use crate::event::*; pub use crate::guestfs::*; diff --git a/rust/tests/040_create_multiple.rs b/rust/tests/040_create_multiple.rs index cc93554a3..970c988af 100644 --- a/rust/tests/040_create_multiple.rs +++ b/rust/tests/040_create_multiple.rs @@ -18,7 +18,7 @@ extern crate guestfs; -fn create() -> guestfs::Handle { +fn create<'a>() -> guestfs::Handle<'a> { match guestfs::Handle::create() { Ok(g) => g, Err(e) => panic!("fail: {:?}", e), diff --git a/rust/tests/410_close_event.rs b/rust/tests/410_close_event.rs new file mode 100644 index 000000000..308471098 --- /dev/null +++ b/rust/tests/410_close_event.rs @@ -0,0 +1,38 @@ +/* libguestfs Rust bindings + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +extern crate guestfs; + +use std::sync::{Arc, Mutex}; + +#[test] +fn close_event() { + let close_invoked = Arc::new(Mutex::new(0)); + { + let mut g = guestfs::Handle::create().expect("create"); + g.set_event_callback( + |_, _, _, _| { + let mut data = (&close_invoked).lock().unwrap(); + *data += 1; + }, + &[guestfs::Event::Close], + ) + .unwrap(); + } + assert_eq!(*((&close_invoked).lock().unwrap()), 1); +} diff --git a/rust/tests/420_log_messages.rs b/rust/tests/420_log_messages.rs new file mode 100644 index 000000000..1e9627ca7 --- /dev/null +++ b/rust/tests/420_log_messages.rs @@ -0,0 +1,60 @@ +/* libguestfs Rust bindings + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +extern crate guestfs; + +use std::str; +use std::sync::{Arc, Mutex}; + +#[test] +fn log_messages() { + let close_invoked = Arc::new(Mutex::new(0)); + { + let mut g = guestfs::Handle::create().expect("create"); + g.set_event_callback( + |ev, _, buf, array| { + let mut data = (&close_invoked).lock().unwrap(); + *data += 1; + + let event = guestfs::event_to_string(&[ev]).unwrap(); + + let buf = str::from_utf8(buf).unwrap(); + let array = array + .into_iter() + .map(|x| format!("{}", x)) + .collect::<Vec<String>>() + .join(","); + + eprintln!("event logged: event={} buf={} array={}", event, buf, array) + }, + &[ + guestfs::Event::Appliance, + guestfs::Event::Library, + guestfs::Event::Warning, + guestfs::Event::Trace, + ], + ) + .unwrap(); + + g.set_trace(true).unwrap(); + g.set_verbose(true).unwrap(); + g.add_drive_ro("/dev/null").unwrap(); + g.set_autosync(true).unwrap(); + } + assert!(*((&close_invoked).lock().unwrap()) > 0); +} diff --git a/rust/tests/430_progress_messages.rs b/rust/tests/430_progress_messages.rs new file mode 100644 index 000000000..a1d33aff7 --- /dev/null +++ b/rust/tests/430_progress_messages.rs @@ -0,0 +1,61 @@ +/* libguestfs Rust bindings + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +extern crate guestfs; + +use std::default::Default; +use std::sync::{Arc, Mutex}; + +#[test] +fn progress_messages() { + let callback_invoked = Arc::new(Mutex::new(0)); + { + let mut g = guestfs::Handle::create().expect("create"); + g.add_drive("/dev/null", Default::default()).unwrap(); + g.launch().unwrap(); + + let eh = g + .set_event_callback( + |_, _, _, _| { + let mut data = (&callback_invoked).lock().unwrap(); + *data += 1; + }, + &[guestfs::Event::Progress], + ) + .unwrap(); + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); + assert!(*(&callback_invoked).lock().unwrap() > 0); + + *(&callback_invoked).lock().unwrap() = 0; + g.delete_event_callback(eh).unwrap(); + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); + assert_eq!(*(&callback_invoked).lock().unwrap(), 0); + + g.set_event_callback( + |_, _, _, _| { + let mut data = (&callback_invoked).lock().unwrap(); + *data += 1; + }, + &[guestfs::Event::Progress], + ) + .unwrap(); + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); + assert!(*(&callback_invoked).lock().unwrap() > 0); + } + assert!(*(&callback_invoked).lock().unwrap() > 0); +} -- 2.20.1 (Apple Git-117)
Martin Kletzander
2019-Jul-30 08:52 UTC
Re: [Libguestfs] [PATCH] Rust bindings: Implement Event features
On Tue, Jul 30, 2019 at 02:51:37PM +0900, Hiroyuki Katsura wrote:>This patch includes: > >- Event callback handlers >- Tests related to events(410-430) >--- > generator/rust.ml | 38 ++++++- > rust/src/base.rs | 24 +++-- > rust/src/error.rs | 8 +- > rust/src/event.rs | 158 ++++++++++++++++++++++++++++ > rust/src/lib.rs | 2 + > rust/tests/040_create_multiple.rs | 2 +- > rust/tests/410_close_event.rs | 38 +++++++ > rust/tests/420_log_messages.rs | 60 +++++++++++ > rust/tests/430_progress_messages.rs | 61 +++++++++++ > 9 files changed, 381 insertions(+), 10 deletions(-) > create mode 100644 rust/src/event.rs > create mode 100644 rust/tests/410_close_event.rs > create mode 100644 rust/tests/420_log_messages.rs > create mode 100644 rust/tests/430_progress_messages.rs >[...]>diff --git a/rust/src/event.rs b/rust/src/event.rs >new file mode 100644 >index 000000000..942feec69 >--- /dev/null >+++ b/rust/src/event.rs >@@ -0,0 +1,158 @@ >+/* libguestfs Rust bindings >+ * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> >+ * >+ * 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 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, write to the Free Software >+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >+ */ >+ >+use crate::base; >+use crate::error; >+use crate::guestfs; >+use std::ffi; >+use std::os::raw::{c_char, c_void}; >+use std::slice; >+use std::sync; >+ >+type GuestfsEventCallback = extern "C" fn( >+ *const base::guestfs_h, >+ *const c_void, >+ u64, >+ i32, >+ i32, >+ *const i8, >+ usize, >+ *const u64, >+ usize, >+); >+ >+#[link(name = "guestfs")] >+extern "C" { >+ fn guestfs_set_event_callback( >+ g: *const base::guestfs_h, >+ cb: GuestfsEventCallback, >+ event_bitmask: u64, >+ flags: i32, >+ opaque: *const c_void, >+ ) -> i32; >+ fn guestfs_delete_event_callback(g: *const base::guestfs_h, eh: i32); >+ fn guestfs_event_to_string(bitmask: u64) -> *const c_char; >+ fn free(buf: *const c_void); >+} >+ >+#[derive(Hash, PartialEq, Eq)] >+pub struct EventHandle { >+ eh: i32, >+} >+ >+pub type Callback = FnMut(guestfs::Event, EventHandle, &[i8], &[u64]); >+ >+fn events_to_bitmask(v: &[guestfs::Event]) -> u64 { >+ let mut r = 0u64; >+ for x in v.iter() { >+ r |= x.to_u64(); >+ } >+ r >+} >+ >+pub fn event_to_string(events: &[guestfs::Event]) -> Result<String, error::Error> { >+ let bitmask = events_to_bitmask(events); >+ >+ let r = unsafe { guestfs_event_to_string(bitmask) }; >+ if r.is_null() { >+ Err(error::unix_error("event_to_string")) >+ } else { >+ let s = unsafe { ffi::CStr::from_ptr(r) }; >+ let s = s.to_str()?.to_string(); >+ unsafe { free(r as *const c_void) }; >+ Ok(s) >+ } >+} >+ >+/* -- Why Not Box<Callback> but Arc<Callback> (in struct base::Handle)? -- >+ * Assume that there are more than threads. While callback is running, >+ * if a thread frees the handle, automatically the buffer is freed if Box<Callback> >+ * is used. Therefore Arc<Callback> is used. >+ */ >+ >+impl<'a> base::Handle<'a> { >+ pub fn set_event_callback<C: 'a>( >+ &mut self, >+ callback: C, >+ events: &[guestfs::Event], >+ ) -> Result<EventHandle, error::Error> >+ where >+ C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), >+ { >+ extern "C" fn trampoline<C>( >+ _g: *const base::guestfs_h, >+ opaque: *const c_void, >+ event: u64, >+ event_handle: i32, >+ _flags: i32, >+ buf: *const c_char, >+ buf_len: usize, >+ array: *const u64, >+ array_len: usize, >+ ) where >+ C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), >+ { >+ // trampoline function >+ // c.f. https://s3.amazonaws.com/temp.michaelfbryan.com/callbacks/index.html >+ >+ let event = match guestfs::Event::from_bitmask(event) { >+ Some(x) => x, >+ None => panic!("Failed to parse bitmask: {}", event), >+ }; >+ let eh = EventHandle { eh: event_handle }; >+ let buf = unsafe { slice::from_raw_parts(buf as *const u8, buf_len) }; >+ let array = unsafe { slice::from_raw_parts(array, array_len) }; >+ >+ let callback_ptr = unsafe { &*(opaque as *const sync::Arc<C>) }; >+ let callback = sync::Arc::clone(&callback_ptr); >+ callback(event, eh, buf, array) >+ } >+ let callback = sync::Arc::<C>::new(callback); >+ let event_bitmask = events_to_bitmask(events); >+ >+ let eh = { >+ // Weak::into_raw is nightly. >+ // In order to make sure that callback is freed when handle is freed, >+ // lifetime is explicitly declared. >+ let ptr: &'a sync::Arc<C> = Box::leak(Box::new(callback.clone()));So this has a lifetime same as the whole handle? Why do you then need to keep it in the list of callbacks then? Also you explicitly leak it here, so even without a lifetime it will not get freed. Are you sure you wanted to do both? Also if you do Box::into_raw instead of Box::leak, you get a *mut right away that you do not need to cast twice, I believe. Otherwise it should be the same, I think.
Hiroyuki Katsura
2019-Jul-30 12:43 UTC
Re: [Libguestfs] [PATCH] Rust bindings: Implement Event features
> So this has a lifetime same as the whole handle?Yes.> Why do you then need to keep > it in the list of callbacks then?Because I want to make sure that callbacks must exist while they are running.> Also you explicitly leak it here, so even > without a lifetime it will not get freed.If you do not explicly write lifetime here, the pointer will have static lifetime. https://doc.rust-lang.org/std/boxed/struct.Box.html#method.leak I think It is too long. But I think there is no more 'correct' lifetime than 'a. Actually, I wanted to use Weak::into_raw there. However, it is nightly-feature now.> if you do Box::into_raw instead of Box::leak, you get a *mut right away > that you do not need to cast twice, I believe. Otherwise it should bethe same,> I think.If you use Box::into_raw, you must make sure that they are freed by yourself. However, if the box pointer is passed to the guestfs API, the pointer will continue to live without being freed after the guestfs handle which contains the passed pointer has closed, I think. On the other hand, by using Box::leak, you can assure that they live until the handle closes. But the buffer will be freed when the guestfs is dropped. So, passing this pointer will not cause memory leak. 2019年7月30日(火) 17:52 Martin Kletzander <mkletzan@redhat.com>:> On Tue, Jul 30, 2019 at 02:51:37PM +0900, Hiroyuki Katsura wrote: > >This patch includes: > > > >- Event callback handlers > >- Tests related to events(410-430) > >--- > > generator/rust.ml | 38 ++++++- > > rust/src/base.rs | 24 +++-- > > rust/src/error.rs | 8 +- > > rust/src/event.rs | 158 ++++++++++++++++++++++++++++ > > rust/src/lib.rs | 2 + > > rust/tests/040_create_multiple.rs | 2 +- > > rust/tests/410_close_event.rs | 38 +++++++ > > rust/tests/420_log_messages.rs | 60 +++++++++++ > > rust/tests/430_progress_messages.rs | 61 +++++++++++ > > 9 files changed, 381 insertions(+), 10 deletions(-) > > create mode 100644 rust/src/event.rs > > create mode 100644 rust/tests/410_close_event.rs > > create mode 100644 rust/tests/420_log_messages.rs > > create mode 100644 rust/tests/430_progress_messages.rs > > > > [...] > > >diff --git a/rust/src/event.rs b/rust/src/event.rs > >new file mode 100644 > >index 000000000..942feec69 > >--- /dev/null > >+++ b/rust/src/event.rs > >@@ -0,0 +1,158 @@ > >+/* libguestfs Rust bindings > >+ * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> > >+ * > >+ * 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 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, write to the Free Software > >+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA > >+ */ > >+ > >+use crate::base; > >+use crate::error; > >+use crate::guestfs; > >+use std::ffi; > >+use std::os::raw::{c_char, c_void}; > >+use std::slice; > >+use std::sync; > >+ > >+type GuestfsEventCallback = extern "C" fn( > >+ *const base::guestfs_h, > >+ *const c_void, > >+ u64, > >+ i32, > >+ i32, > >+ *const i8, > >+ usize, > >+ *const u64, > >+ usize, > >+); > >+ > >+#[link(name = "guestfs")] > >+extern "C" { > >+ fn guestfs_set_event_callback( > >+ g: *const base::guestfs_h, > >+ cb: GuestfsEventCallback, > >+ event_bitmask: u64, > >+ flags: i32, > >+ opaque: *const c_void, > >+ ) -> i32; > >+ fn guestfs_delete_event_callback(g: *const base::guestfs_h, eh: i32); > >+ fn guestfs_event_to_string(bitmask: u64) -> *const c_char; > >+ fn free(buf: *const c_void); > >+} > >+ > >+#[derive(Hash, PartialEq, Eq)] > >+pub struct EventHandle { > >+ eh: i32, > >+} > >+ > >+pub type Callback = FnMut(guestfs::Event, EventHandle, &[i8], &[u64]); > >+ > >+fn events_to_bitmask(v: &[guestfs::Event]) -> u64 { > >+ let mut r = 0u64; > >+ for x in v.iter() { > >+ r |= x.to_u64(); > >+ } > >+ r > >+} > >+ > >+pub fn event_to_string(events: &[guestfs::Event]) -> Result<String, > error::Error> { > >+ let bitmask = events_to_bitmask(events); > >+ > >+ let r = unsafe { guestfs_event_to_string(bitmask) }; > >+ if r.is_null() { > >+ Err(error::unix_error("event_to_string")) > >+ } else { > >+ let s = unsafe { ffi::CStr::from_ptr(r) }; > >+ let s = s.to_str()?.to_string(); > >+ unsafe { free(r as *const c_void) }; > >+ Ok(s) > >+ } > >+} > >+ > >+/* -- Why Not Box<Callback> but Arc<Callback> (in struct base::Handle)? > -- > >+ * Assume that there are more than threads. While callback is running, > >+ * if a thread frees the handle, automatically the buffer is freed if > Box<Callback> > >+ * is used. Therefore Arc<Callback> is used. > >+ */ > >+ > >+impl<'a> base::Handle<'a> { > >+ pub fn set_event_callback<C: 'a>( > >+ &mut self, > >+ callback: C, > >+ events: &[guestfs::Event], > >+ ) -> Result<EventHandle, error::Error> > >+ where > >+ C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), > >+ { > >+ extern "C" fn trampoline<C>( > >+ _g: *const base::guestfs_h, > >+ opaque: *const c_void, > >+ event: u64, > >+ event_handle: i32, > >+ _flags: i32, > >+ buf: *const c_char, > >+ buf_len: usize, > >+ array: *const u64, > >+ array_len: usize, > >+ ) where > >+ C: Fn(guestfs::Event, EventHandle, &[u8], &[u64]), > >+ { > >+ // trampoline function > >+ // c.f. > https://s3.amazonaws.com/temp.michaelfbryan.com/callbacks/index.html > >+ > >+ let event = match guestfs::Event::from_bitmask(event) { > >+ Some(x) => x, > >+ None => panic!("Failed to parse bitmask: {}", event), > >+ }; > >+ let eh = EventHandle { eh: event_handle }; > >+ let buf = unsafe { slice::from_raw_parts(buf as *const u8, > buf_len) }; > >+ let array = unsafe { slice::from_raw_parts(array, array_len) > }; > >+ > >+ let callback_ptr = unsafe { &*(opaque as *const > sync::Arc<C>) }; > >+ let callback = sync::Arc::clone(&callback_ptr); > >+ callback(event, eh, buf, array) > >+ } > >+ let callback = sync::Arc::<C>::new(callback); > >+ let event_bitmask = events_to_bitmask(events); > >+ > >+ let eh = { > >+ // Weak::into_raw is nightly. > >+ // In order to make sure that callback is freed when handle > is freed, > >+ // lifetime is explicitly declared. > >+ let ptr: &'a sync::Arc<C> > Box::leak(Box::new(callback.clone())); > > So this has a lifetime same as the whole handle? Why do you then need to > keep > it in the list of callbacks then? Also you explicitly leak it here, so > even > without a lifetime it will not get freed. Are you sure you wanted to do > both? > > Also if you do Box::into_raw instead of Box::leak, you get a *mut right > away > that you do not need to cast twice, I believe. Otherwise it should be the > same, > I think. >
Pino Toscano
2019-Jul-31 15:00 UTC
Re: [Libguestfs] [PATCH] Rust bindings: Implement Event features
Hi Hiroyuki, On Tuesday, 30 July 2019 07:51:37 CEST Hiroyuki Katsura wrote:> This patch includes: > > - Event callback handlers > - Tests related to events(410-430) > ---Would it be possible to split the Handle -> Handle<'a> change in an own small patch? This way it can be documented why it was changed.> +pub fn event_to_string(events: &[guestfs::Event]) -> Result<String, error::Error> { > + let bitmask = events_to_bitmask(events); > + > + let r = unsafe { guestfs_event_to_string(bitmask) }; > + if r.is_null() { > + Err(error::unix_error("event_to_string")) > + } else { > + let s = unsafe { ffi::CStr::from_ptr(r) }; > + let s = s.to_str()?.to_string();These two look like utils::char_ptr_to_string().> diff --git a/rust/tests/410_close_event.rs b/rust/tests/410_close_event.rs > new file mode 100644 > index 000000000..308471098 > --- /dev/null > +++ b/rust/tests/410_close_event.rs > @@ -0,0 +1,38 @@ > +/* libguestfs Rust bindings > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program 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 General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > + > +extern crate guestfs; > + > +use std::sync::{Arc, Mutex}; > + > +#[test] > +fn close_event() { > + let close_invoked = Arc::new(Mutex::new(0));Maybe a thread-safe Arc is not needed for this test -- it's just a single-threaded test with just one handle.> + { > + let mut g = guestfs::Handle::create().expect("create"); > + g.set_event_callback( > + |_, _, _, _| { > + let mut data = (&close_invoked).lock().unwrap(); > + *data += 1; > + }, > + &[guestfs::Event::Close], > + ) > + .unwrap();Check that the close_invoked count is still 0 before the block ending.> diff --git a/rust/tests/420_log_messages.rs b/rust/tests/420_log_messages.rs > new file mode 100644 > index 000000000..1e9627ca7 > --- /dev/null > +++ b/rust/tests/420_log_messages.rs > @@ -0,0 +1,60 @@ > +/* libguestfs Rust bindings > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program 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 General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > + > +extern crate guestfs; > + > +use std::str; > +use std::sync::{Arc, Mutex}; > + > +#[test] > +fn log_messages() { > + let close_invoked = Arc::new(Mutex::new(0));Ditto as in 410_close_event.rs.> diff --git a/rust/tests/430_progress_messages.rs b/rust/tests/430_progress_messages.rs > new file mode 100644 > index 000000000..a1d33aff7 > --- /dev/null > +++ b/rust/tests/430_progress_messages.rs > @@ -0,0 +1,61 @@ > +/* libguestfs Rust bindings > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program 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 General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > + > +extern crate guestfs; > + > +use std::default::Default; > +use std::sync::{Arc, Mutex}; > + > +#[test] > +fn progress_messages() { > + let callback_invoked = Arc::new(Mutex::new(0));Ditto as in 410_close_event.rs.> + { > + let mut g = guestfs::Handle::create().expect("create"); > + g.add_drive("/dev/null", Default::default()).unwrap(); > + g.launch().unwrap(); > + > + let eh = g > + .set_event_callback( > + |_, _, _, _| { > + let mut data = (&callback_invoked).lock().unwrap(); > + *data += 1; > + }, > + &[guestfs::Event::Progress], > + ) > + .unwrap(); > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > + assert!(*(&callback_invoked).lock().unwrap() > 0); > + > + *(&callback_invoked).lock().unwrap() = 0; > + g.delete_event_callback(eh).unwrap(); > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > + assert_eq!(*(&callback_invoked).lock().unwrap(), 0); > + > + g.set_event_callback( > + |_, _, _, _| { > + let mut data = (&callback_invoked).lock().unwrap(); > + *data += 1; > + }, > + &[guestfs::Event::Progress], > + ) > + .unwrap(); > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > + assert!(*(&callback_invoked).lock().unwrap() > 0); > + } > + assert!(*(&callback_invoked).lock().unwrap() > 0);This assert is not needed, and most probably the whole scope here can be removed. -- Pino Toscano
Hiroyuki Katsura
2019-Aug-05 06:57 UTC
Re: [Libguestfs] [PATCH] Rust bindings: Implement Event features
I fixed based on comments. I'll send these two patches to this mailing list. - Fix Handle -> Handle<'a> - Add events Regards, Hiroyuki 2019年8月1日(木) 0:01 Pino Toscano <ptoscano@redhat.com>:> Hi Hiroyuki, > > On Tuesday, 30 July 2019 07:51:37 CEST Hiroyuki Katsura wrote: > > This patch includes: > > > > - Event callback handlers > > - Tests related to events(410-430) > > --- > > Would it be possible to split the Handle -> Handle<'a> change in an own > small patch? This way it can be documented why it was changed. > > > +pub fn event_to_string(events: &[guestfs::Event]) -> Result<String, > error::Error> { > > + let bitmask = events_to_bitmask(events); > > + > > + let r = unsafe { guestfs_event_to_string(bitmask) }; > > + if r.is_null() { > > + Err(error::unix_error("event_to_string")) > > + } else { > > + let s = unsafe { ffi::CStr::from_ptr(r) }; > > + let s = s.to_str()?.to_string(); > > These two look like utils::char_ptr_to_string(). > > > diff --git a/rust/tests/410_close_event.rs b/rust/tests/ > 410_close_event.rs > > new file mode 100644 > > index 000000000..308471098 > > --- /dev/null > > +++ b/rust/tests/410_close_event.rs > > @@ -0,0 +1,38 @@ > > +/* libguestfs Rust bindings > > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com > > > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program 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 General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA. > > + */ > > + > > +extern crate guestfs; > > + > > +use std::sync::{Arc, Mutex}; > > + > > +#[test] > > +fn close_event() { > > + let close_invoked = Arc::new(Mutex::new(0)); > > Maybe a thread-safe Arc is not needed for this test -- it's just a > single-threaded test with just one handle. > > > + { > > + let mut g = guestfs::Handle::create().expect("create"); > > + g.set_event_callback( > > + |_, _, _, _| { > > + let mut data = (&close_invoked).lock().unwrap(); > > + *data += 1; > > + }, > > + &[guestfs::Event::Close], > > + ) > > + .unwrap(); > > Check that the close_invoked count is still 0 before the block ending. > > > diff --git a/rust/tests/420_log_messages.rs b/rust/tests/ > 420_log_messages.rs > > new file mode 100644 > > index 000000000..1e9627ca7 > > --- /dev/null > > +++ b/rust/tests/420_log_messages.rs > > @@ -0,0 +1,60 @@ > > +/* libguestfs Rust bindings > > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com > > > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program 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 General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA. > > + */ > > + > > +extern crate guestfs; > > + > > +use std::str; > > +use std::sync::{Arc, Mutex}; > > + > > +#[test] > > +fn log_messages() { > > + let close_invoked = Arc::new(Mutex::new(0)); > > Ditto as in 410_close_event.rs. > > > diff --git a/rust/tests/430_progress_messages.rs b/rust/tests/ > 430_progress_messages.rs > > new file mode 100644 > > index 000000000..a1d33aff7 > > --- /dev/null > > +++ b/rust/tests/430_progress_messages.rs > > @@ -0,0 +1,61 @@ > > +/* libguestfs Rust bindings > > + * Copyright (C) 2019 Hiroyuki Katsura <hiroyuki.katsura.0513@gmail.com > > > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program 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 General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA. > > + */ > > + > > +extern crate guestfs; > > + > > +use std::default::Default; > > +use std::sync::{Arc, Mutex}; > > + > > +#[test] > > +fn progress_messages() { > > + let callback_invoked = Arc::new(Mutex::new(0)); > > Ditto as in 410_close_event.rs. > > > + { > > + let mut g = guestfs::Handle::create().expect("create"); > > + g.add_drive("/dev/null", Default::default()).unwrap(); > > + g.launch().unwrap(); > > + > > + let eh = g > > + .set_event_callback( > > + |_, _, _, _| { > > + let mut data = (&callback_invoked).lock().unwrap(); > > + *data += 1; > > + }, > > + &[guestfs::Event::Progress], > > + ) > > + .unwrap(); > > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > > + assert!(*(&callback_invoked).lock().unwrap() > 0); > > + > > + *(&callback_invoked).lock().unwrap() = 0; > > + g.delete_event_callback(eh).unwrap(); > > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > > + assert_eq!(*(&callback_invoked).lock().unwrap(), 0); > > + > > + g.set_event_callback( > > + |_, _, _, _| { > > + let mut data = (&callback_invoked).lock().unwrap(); > > + *data += 1; > > + }, > > + &[guestfs::Event::Progress], > > + ) > > + .unwrap(); > > + assert_eq!("ok", g.debug("progress", &["5"]).unwrap()); > > + assert!(*(&callback_invoked).lock().unwrap() > 0); > > + } > > + assert!(*(&callback_invoked).lock().unwrap() > 0); > > This assert is not needed, and most probably the whole scope here can > be removed. > > -- > Pino Toscano