Libguestfs - Aug 2019 - Re: [nbdkit PATCH 2/2] rust: Add support for dynamic .thread_model

Similar to what I just did for OCaml (this IS an API break, requiring
recompilation of any existing Rust plugin), and done because I want to
add fast_zero support to both languages as part of my upcoming fast
zero series.

Figuring out how to get extents working was hard enough that I punted
that, still.

Eric Blake (2):
  rust: Implement can_cache
  rust: Add support for dynamic .thread_model

 plugins/rust/nbdkit-rust-plugin.pod | 29 ++++++++++++++++++++++++-----
 plugins/rust/examples/ramdisk.rs    |  8 ++++++--
 plugins/rust/src/lib.rs             | 23 ++++++++++++++++++++---
 3 files changed, 50 insertions(+), 10 deletions(-)

-- 
2.20.1

Implementing extents requires some coordination for the Rust code to
call back into libnbdkit; I'm not familiar with Rust enough to do
that. But with placeholders for those slots, implementing
can_cache/cache is trivial.  This improves the situation mentioned in
commit 031fae85.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/rust/nbdkit-rust-plugin.pod | 10 ++++++++++
 plugins/rust/src/lib.rs             | 13 +++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/plugins/rust/nbdkit-rust-plugin.pod
b/plugins/rust/nbdkit-rust-plugin.pod
index db2a55ee..930829cc 100644
--- a/plugins/rust/nbdkit-rust-plugin.pod
+++ b/plugins/rust/nbdkit-rust-plugin.pod
@@ -90,6 +90,16 @@ thread models, see L<nbdkit-plugin(3)/THREADS>.

 =back

+=head2 Missing callbacks
+
+=over 4
+
+=item Missing: C<can_extents> and C<extents>
+
+These are not yet supported.
+
+=back
+
 =head1 SEE ALSO

 L<nbdkit(1)>,
diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
index a3dbf43e..25af2fe6 100644
--- a/plugins/rust/src/lib.rs
+++ b/plugins/rust/src/lib.rs
@@ -93,6 +93,15 @@ pub struct Plugin {
     pub magic_config_key: *const c_char,

     pub can_multi_conn: Option<extern fn (h: *mut c_void) -> c_int>,
+
+    // Slots for extents functions, which needs more integration.
+    _can_extents: Option<extern fn ()>,
+    _extents: Option<extern fn ()>,
+
+    pub can_cache: Option<extern fn (h: *mut c_void) -> c_int>,
+    pub cache: Option<extern fn (h: *mut c_void,
+                                 count: u32, offset: u64,
+                                 flags: u32) -> c_int>,
 }

 pub enum ThreadModel {
@@ -146,6 +155,10 @@ impl Plugin {
             zero: None,
             magic_config_key: std::ptr::null(),
             can_multi_conn: None,
+            _can_extents: None,
+            _extents: None,
+            can_cache: None,
+            cache: None,
         }
     }
 }
-- 
2.20.1

We do not promise API stability for non-C languages; this is an API
break as follows: instead of calling plugin_init with a static model,
you can now populate .thread_model in the Plugin struct, with a
default to Parallel.  As in C, the model is still chosen at .load time
(at most, making it a function allows you to alter it based on
configuration), and not something that can change per-connection.

Since all existing Rust plugins will have already thought about thread
models, they can convert their existing model into the new
.thread_model field (and thus, I don't feel too bad making Parallel
the default, even if it is not always the safest).

And something I learned the hard way: enum ThreadModel _must_ be
#[repr(C)], otherwise Rust will compile a return type of this enum
into a 1-byte return, even though C expects sign-extension into a
4-byte return; the garbage left in the other three bytes (at least on
x86 architecture) would generally render .thread_model broken
otherwise.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

 plugins/rust/nbdkit-rust-plugin.pod | 19 ++++++++++++++-----
 plugins/rust/examples/ramdisk.rs    |  8 ++++++--
 plugins/rust/src/lib.rs             | 10 +++++++---
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/plugins/rust/nbdkit-rust-plugin.pod
b/plugins/rust/nbdkit-rust-plugin.pod
index 930829cc..61c2e0ba 100644
--- a/plugins/rust/nbdkit-rust-plugin.pod
+++ b/plugins/rust/nbdkit-rust-plugin.pod
@@ -37,14 +37,19 @@ compatible with the C struct used by C plugins.
  use nbdkit::ThreadModel::*;
  
  #[no_mangle]
+ extern fn myplugin_thread_model () -> ThreadModel {
+    Serialize_AllRequests
+ }
+ 
+ //... more functions
+ 
  pub extern fn plugin_init () -> *const Plugin {
     // Plugin name.
     let name = "myplugin\0"
       as *const str as *const [c_char] as *const c_char;

-    // Create a mutable plugin, setting the 5 required fields.
+    // Create a mutable plugin, setting the 4 required fields.
     let mut plugin = Plugin::new (
-        Serialize_All_Requests,
         name,
         myplugin_open,
         myplugin_get_size,
@@ -53,6 +58,7 @@ compatible with the C struct used by C plugins.
     // Update any other fields as required.
     plugin.close = Some (myplugin_close);
     plugin.pwrite = Some (myplugin_pwrite);
+    plugin.thread_model = Some (myplugin_thread_model);

     // Return the pointer.
     let plugin = Box::new(plugin);
@@ -74,9 +80,12 @@ L<nbdkit-plugin(3)>):

 =head2 Threads

-The first parameter of C<Plugin::new> is the thread model, which can
-be one of the values in the table below.  For more information on
-thread models, see L<nbdkit-plugin(3)/THREADS>.
+One of the members in C<Plugin> returned by C<plugin_init> is
+C<thread_model>, which must return one of the values in the table
+below. For more information on thread models, see
+L<nbdkit-plugin(3)/THREADS>.  If this optional function is not
+provided, the thread model defaults to
+C<nbdkit::ThreadModel::Parallel>.

 =over 4

diff --git a/plugins/rust/examples/ramdisk.rs b/plugins/rust/examples/ramdisk.rs
index 9d0af0ee..e9462fca 100644
--- a/plugins/rust/examples/ramdisk.rs
+++ b/plugins/rust/examples/ramdisk.rs
@@ -53,6 +53,10 @@ struct Handle {
     _not_used: i32,
 }

+extern fn ramdisk_thread_model () -> ThreadModel {
+    Parallel
+}
+
 extern fn ramdisk_open (_readonly: c_int) -> *mut c_void {
     let h = Handle {_not_used: 0};
     let h = Box::new(h);
@@ -98,9 +102,8 @@ pub extern fn plugin_init () -> *const Plugin {
     // https://github.com/rust-lang/rfcs/issues/400
     let name = "ramdisk\0" as *const str as *const [c_char] as *const
c_char;

-    // Create a mutable plugin, setting the 5 required fields.
+    // Create a mutable plugin, setting the 4 required fields.
     let mut plugin = Plugin::new (
-        Parallel,
         name,
         ramdisk_open,
         ramdisk_get_size,
@@ -109,6 +112,7 @@ pub extern fn plugin_init () -> *const Plugin {
     // Update any other fields as required.
     plugin.close = Some (ramdisk_close);
     plugin.pwrite = Some (ramdisk_pwrite);
+    plugin.thread_model = Some (ramdisk_thread_model);

     // Return the pointer.
     let plugin = Box::new(plugin);
diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
index 25af2fe6..e369ecbe 100644
--- a/plugins/rust/src/lib.rs
+++ b/plugins/rust/src/lib.rs
@@ -36,6 +36,7 @@ use std::os::raw::{c_char, c_int, c_void};
 // function must return.
 #[repr(C)]
 pub struct Plugin {
+    // Do not modify these three fields directly.
     _struct_size: u64,
     _api_version: c_int,
     _thread_model: c_int,
@@ -102,8 +103,11 @@ pub struct Plugin {
     pub cache: Option<extern fn (h: *mut c_void,
                                  count: u32, offset: u64,
                                  flags: u32) -> c_int>,
+
+    pub thread_model: Option<extern fn () -> ThreadModel>,
 }

+#[repr(C)]
 pub enum ThreadModel {
     SerializeConnections = 0,
     SerializeAllRequests = 1,
@@ -112,8 +116,7 @@ pub enum ThreadModel {
 }

 impl Plugin {
-    pub fn new (thread_model: ThreadModel,
-                name: *const c_char,
+    pub fn new (name: *const c_char,
                 open: extern fn (c_int) -> *mut c_void,
                 get_size: extern fn (*mut c_void) -> i64,
                 pread: extern fn (h: *mut c_void, buf: *mut c_char,
@@ -122,7 +125,7 @@ impl Plugin {
         Plugin {
             _struct_size: mem::size_of::<Plugin>() as u64,
             _api_version: 2,
-            _thread_model: thread_model as c_int,
+            _thread_model: ThreadModel::Parallel as c_int,
             name: name,
             longname: std::ptr::null(),
             version: std::ptr::null(),
@@ -159,6 +162,7 @@ impl Plugin {
             _extents: None,
             can_cache: None,
             cache: None,
+	    thread_model: None,
         }
     }
 }
-- 
2.20.1

On Fri, Aug 16, 2019 at 12:08:09PM -0500, Eric Blake
wrote:
> Similar to what I just did for OCaml (this IS an API break, requiring
> recompilation of any existing Rust plugin), and done because I want to
> add fast_zero support to both languages as part of my upcoming fast
> zero series.
Note that it's an API break, but not an ABI break.  OCaml and Rust
nbdkit plugins use the C ABI.

Adding Martin K in CC as he's better at this than me.  The other
patches are:
https://www.redhat.com/archives/libguestfs/2019-August/msg00286.html

Rich.
> Figuring out how to get extents working was hard enough that I punted
> that, still.
> 
> Eric Blake (2):
>   rust: Implement can_cache
>   rust: Add support for dynamic .thread_model
> 
>  plugins/rust/nbdkit-rust-plugin.pod | 29 ++++++++++++++++++++++++-----
>  plugins/rust/examples/ramdisk.rs    |  8 ++++++--
>  plugins/rust/src/lib.rs             | 23 ++++++++++++++++++++---
>  3 files changed, 50 insertions(+), 10 deletions(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Fri, Aug 16, 2019 at 12:08:10PM -0500, Eric Blake
wrote:
> Implementing extents requires some coordination for the Rust code to
> call back into libnbdkit; I'm not familiar with Rust enough to do
> that. But with placeholders for those slots, implementing
> can_cache/cache is trivial.  This improves the situation mentioned in
> commit 031fae85.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/rust/nbdkit-rust-plugin.pod | 10 ++++++++++
>  plugins/rust/src/lib.rs             | 13 +++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/plugins/rust/nbdkit-rust-plugin.pod
b/plugins/rust/nbdkit-rust-plugin.pod
> index db2a55ee..930829cc 100644
> --- a/plugins/rust/nbdkit-rust-plugin.pod
> +++ b/plugins/rust/nbdkit-rust-plugin.pod
> @@ -90,6 +90,16 @@ thread models, see L<nbdkit-plugin(3)/THREADS>.
> 
>  =back
> 
> +=head2 Missing callbacks
> +
> +=over 4
> +
> +=item Missing: C<can_extents> and C<extents>
> +
> +These are not yet supported.
> +
> +=back
> +
>  =head1 SEE ALSO
> 
>  L<nbdkit(1)>,
> diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
> index a3dbf43e..25af2fe6 100644
> --- a/plugins/rust/src/lib.rs
> +++ b/plugins/rust/src/lib.rs
> @@ -93,6 +93,15 @@ pub struct Plugin {
>      pub magic_config_key: *const c_char,
> 
>      pub can_multi_conn: Option<extern fn (h: *mut c_void) ->
c_int>,
> +
> +    // Slots for extents functions, which needs more integration.
> +    _can_extents: Option<extern fn ()>,
> +    _extents: Option<extern fn ()>,
> +
> +    pub can_cache: Option<extern fn (h: *mut c_void) -> c_int>,
> +    pub cache: Option<extern fn (h: *mut c_void,
> +                                 count: u32, offset: u64,
> +                                 flags: u32) -> c_int>,
>  }
> 
>  pub enum ThreadModel {
> @@ -146,6 +155,10 @@ impl Plugin {
>              zero: None,
>              magic_config_key: std::ptr::null(),
>              can_multi_conn: None,
> +            _can_extents: None,
> +            _extents: None,
> +            can_cache: None,
> +            cache: None,
>          }
>      }
>  }
This one seems straightforward, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Fri, Aug 16, 2019 at 12:08:11PM -0500, Eric Blake
wrote:
> We do not promise API stability for non-C languages; this is an API
> break as follows: instead of calling plugin_init with a static model,
> you can now populate .thread_model in the Plugin struct, with a
> default to Parallel.  As in C, the model is still chosen at .load time
> (at most, making it a function allows you to alter it based on
> configuration), and not something that can change per-connection.
> 
> Since all existing Rust plugins will have already thought about thread
> models, they can convert their existing model into the new
> .thread_model field (and thus, I don't feel too bad making Parallel
> the default, even if it is not always the safest).
> 
> And something I learned the hard way: enum ThreadModel _must_ be
> #[repr(C)], otherwise Rust will compile a return type of this enum
> into a 1-byte return, even though C expects sign-extension into a
> 4-byte return; the garbage left in the other three bytes (at least on
> x86 architecture) would generally render .thread_model broken
> otherwise.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
>  plugins/rust/nbdkit-rust-plugin.pod | 19 ++++++++++++++-----
>  plugins/rust/examples/ramdisk.rs    |  8 ++++++--
>  plugins/rust/src/lib.rs             | 10 +++++++---
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/plugins/rust/nbdkit-rust-plugin.pod
b/plugins/rust/nbdkit-rust-plugin.pod
> index 930829cc..61c2e0ba 100644
> --- a/plugins/rust/nbdkit-rust-plugin.pod
> +++ b/plugins/rust/nbdkit-rust-plugin.pod
> @@ -37,14 +37,19 @@ compatible with the C struct used by C plugins.
>   use nbdkit::ThreadModel::*;
>   
>   #[no_mangle]
> + extern fn myplugin_thread_model () -> ThreadModel {
> +    Serialize_AllRequests
> + }
> + 
> + //... more functions
> + 
>   pub extern fn plugin_init () -> *const Plugin {
>      // Plugin name.
>      let name = "myplugin\0"
>        as *const str as *const [c_char] as *const c_char;
> 
> -    // Create a mutable plugin, setting the 5 required fields.
> +    // Create a mutable plugin, setting the 4 required fields.
>      let mut plugin = Plugin::new (
> -        Serialize_All_Requests,
>          name,
>          myplugin_open,
>          myplugin_get_size,
> @@ -53,6 +58,7 @@ compatible with the C struct used by C plugins.
>      // Update any other fields as required.
>      plugin.close = Some (myplugin_close);
>      plugin.pwrite = Some (myplugin_pwrite);
> +    plugin.thread_model = Some (myplugin_thread_model);
> 
>      // Return the pointer.
>      let plugin = Box::new(plugin);
> @@ -74,9 +80,12 @@ L<nbdkit-plugin(3)>):
> 
>  =head2 Threads
> 
> -The first parameter of C<Plugin::new> is the thread model, which can
> -be one of the values in the table below.  For more information on
> -thread models, see L<nbdkit-plugin(3)/THREADS>.
> +One of the members in C<Plugin> returned by C<plugin_init> is
> +C<thread_model>, which must return one of the values in the table
> +below. For more information on thread models, see
> +L<nbdkit-plugin(3)/THREADS>.  If this optional function is not
> +provided, the thread model defaults to
> +C<nbdkit::ThreadModel::Parallel>.
> 
>  =over 4
> 
> diff --git a/plugins/rust/examples/ramdisk.rs
b/plugins/rust/examples/ramdisk.rs
> index 9d0af0ee..e9462fca 100644
> --- a/plugins/rust/examples/ramdisk.rs
> +++ b/plugins/rust/examples/ramdisk.rs
> @@ -53,6 +53,10 @@ struct Handle {
>      _not_used: i32,
>  }
> 
> +extern fn ramdisk_thread_model () -> ThreadModel {
> +    Parallel
> +}
> +
>  extern fn ramdisk_open (_readonly: c_int) -> *mut c_void {
>      let h = Handle {_not_used: 0};
>      let h = Box::new(h);
> @@ -98,9 +102,8 @@ pub extern fn plugin_init () -> *const Plugin {
>      // https://github.com/rust-lang/rfcs/issues/400
>      let name = "ramdisk\0" as *const str as *const [c_char] as
*const c_char;
> 
> -    // Create a mutable plugin, setting the 5 required fields.
> +    // Create a mutable plugin, setting the 4 required fields.
>      let mut plugin = Plugin::new (
> -        Parallel,
>          name,
>          ramdisk_open,
>          ramdisk_get_size,
> @@ -109,6 +112,7 @@ pub extern fn plugin_init () -> *const Plugin {
>      // Update any other fields as required.
>      plugin.close = Some (ramdisk_close);
>      plugin.pwrite = Some (ramdisk_pwrite);
> +    plugin.thread_model = Some (ramdisk_thread_model);
> 
>      // Return the pointer.
>      let plugin = Box::new(plugin);
> diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
> index 25af2fe6..e369ecbe 100644
> --- a/plugins/rust/src/lib.rs
> +++ b/plugins/rust/src/lib.rs
> @@ -36,6 +36,7 @@ use std::os::raw::{c_char, c_int, c_void};
>  // function must return.
>  #[repr(C)]
>  pub struct Plugin {
> +    // Do not modify these three fields directly.
>      _struct_size: u64,
>      _api_version: c_int,
>      _thread_model: c_int,
> @@ -102,8 +103,11 @@ pub struct Plugin {
>      pub cache: Option<extern fn (h: *mut c_void,
>                                   count: u32, offset: u64,
>                                   flags: u32) -> c_int>,
> +
> +    pub thread_model: Option<extern fn () -> ThreadModel>,
>  }
> 
> +#[repr(C)]
>  pub enum ThreadModel {
>      SerializeConnections = 0,
>      SerializeAllRequests = 1,
> @@ -112,8 +116,7 @@ pub enum ThreadModel {
>  }
> 
>  impl Plugin {
> -    pub fn new (thread_model: ThreadModel,
> -                name: *const c_char,
> +    pub fn new (name: *const c_char,
>                  open: extern fn (c_int) -> *mut c_void,
>                  get_size: extern fn (*mut c_void) -> i64,
>                  pread: extern fn (h: *mut c_void, buf: *mut c_char,
> @@ -122,7 +125,7 @@ impl Plugin {
>          Plugin {
>              _struct_size: mem::size_of::<Plugin>() as u64,
>              _api_version: 2,
> -            _thread_model: thread_model as c_int,
> +            _thread_model: ThreadModel::Parallel as c_int,
>              name: name,
>              longname: std::ptr::null(),
>              version: std::ptr::null(),
> @@ -159,6 +162,7 @@ impl Plugin {
>              _extents: None,
>              can_cache: None,
>              cache: None,
> +	    thread_model: None,
Indentation?
>          }
>      }
>  }
Anyway this looks fine to me.  If it compiles and runs there's
not much that can go wrong, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 8/16/19 12:08 PM, Eric Blake wrote:
> We do not promise API stability for non-C languages; this is an API
> break as follows: instead of calling plugin_init with a static model,
> you can now populate .thread_model in the Plugin struct, with a
> default to Parallel.  As in C, the model is still chosen at .load time
> (at most, making it a function allows you to alter it based on
> configuration), and not something that can change per-connection.
> 
> Since all existing Rust plugins will have already thought about thread
> models, they can convert their existing model into the new
> .thread_model field (and thus, I don't feel too bad making Parallel
> the default, even if it is not always the safest).
> 
> +++ b/plugins/rust/nbdkit-rust-plugin.pod
> @@ -37,14 +37,19 @@ compatible with the C struct used by C plugins.
>   use nbdkit::ThreadModel::*;
>   
>   #[no_mangle]
> + extern fn myplugin_thread_model () -> ThreadModel {
> +    Serialize_AllRequests
This is the wrong spelling.  But in fixing it, it turns out we had
pre-existing typos (our .pod used _, but lib.rs did not).  So I squashed
in that fix locally.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 8/16/19 12:08 PM, Eric Blake wrote:
> Implementing extents requires some coordination for the Rust code to
> call back into libnbdkit; I'm not familiar with Rust enough to do
> that. But with placeholders for those slots, implementing
> can_cache/cache is trivial.  This improves the situation mentioned in
> commit 031fae85.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> +++ b/plugins/rust/src/lib.rs
> @@ -93,6 +93,15 @@ pub struct Plugin {
>      pub magic_config_key: *const c_char,
> 
>      pub can_multi_conn: Option<extern fn (h: *mut c_void) ->
c_int>,
> +
> +    // Slots for extents functions, which needs more integration.
> +    _can_extents: Option<extern fn ()>,
> +    _extents: Option<extern fn ()>,
Here, I was just copying what we had already done for the v1 fields that
should remain unused (see above, in _pread_old for example).  But is
there a saner way to write a Rust struct that reserves the space
required by the C ABI, but where the field MUST be left as NULL and not
populated with Some(...) by the end user?  Perhaps by marking the field
const, while the rest of the struct is used as a mutable?  (Doesn't
change this patch, so much as a cleanup we could apply on top to prevent
all of our _named fields from being overwritten after the initial
construction with default values).

And of course, if you want to actually implement extents, and figure out
how to expose C-based nbdkit functions to be called by Rust code, be my
guest (for .zero, we need nbdkit_set_error(), for extents, we need
nbdkit_add_extent(), and then we have several other utility functions
like nbdkit_realpath() that could be useful).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org