Eric Blake
2019-Aug-16 17:08 UTC
[Libguestfs] [nbdkit PATCH 0/2] rust: Implement some missing v2 callbacks
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
Eric Blake
2019-Aug-16 17:08 UTC
[Libguestfs] [nbdkit PATCH 1/2] rust: Implement can_cache
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
Eric Blake
2019-Aug-16 17:08 UTC
[Libguestfs] [nbdkit PATCH 2/2] rust: Add support for dynamic .thread_model
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
Richard W.M. Jones
2019-Aug-16 17:26 UTC
Re: [Libguestfs] [nbdkit PATCH 0/2] rust: Implement some missing v2 callbacks
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
Richard W.M. Jones
2019-Aug-16 17:26 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] rust: Implement can_cache
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
Richard W.M. Jones
2019-Aug-16 17:29 UTC
Re: [Libguestfs] [nbdkit PATCH 2/2] rust: Add support for dynamic .thread_model
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
Eric Blake
2019-Aug-16 17:52 UTC
Re: [Libguestfs] [nbdkit PATCH 2/2] rust: Add support for dynamic .thread_model
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_AllRequestsThis 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
Eric Blake
2019-Aug-16 18:01 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] rust: Implement can_cache
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
Apparently Analagous Threads
- [PATCH nbdkit] Add support for writing plugins in Rust.
- [nbdkit PATCH 0/2] Few rust plugin fixups/nitpicks
- [PATCH libnbd] Add bindings for Rust language
- Re: [nbdkit PATCH 2/2] rust: Add support for dynamic .thread_model
- Re: [PATCH] Rust bindings: Add Rust bindings