[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 be the 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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]