Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin Callback Issues in Rust Library #21

Closed
dcullen12 opened this issue Apr 20, 2023 · 6 comments
Closed

Plugin Callback Issues in Rust Library #21

dcullen12 opened this issue Apr 20, 2023 · 6 comments

Comments

@dcullen12
Copy link

dcullen12 commented Apr 20, 2023

For reference using nbdkit = "0.2.0" (appears to be latest version on crates.io)

Noticed a couple gaps in the rust library for writing plugins:

  1. open() does not return Result<Box<dyn Server>>, instead it simply returns Box<dyn Server>

    • because of this I have to create a struct full of OnceCells and actually initialize things in get_size(). I would rather do the initialization in open() and potentially return an error. This would remove the need for OnceCell (not a huge performance difference, and get_size will fail the connection all the same. But the code would be cleaner this way)
  2. there is no after_fork() callback. I ran into some bugs where a lazy_static tokio runtime was being accessed (and therefor initialized) too early (before fork). this resulted in failures when the runtime was used after fork() (presumably due to the loss of the runtime's internal threads). it would be nice to have after_fork() to verify at startup time that the runtime is in a good state (and panic accordingly if not). in general, it appears inconvenient to spin up background threads for Rust plugins. right now these checks can only be done retrospectively when a connection is opened, which is undesirable (server should fail to start up rather than mislead user into thinking it is good-to-go until an actual connections happens.)

@rwmjones
Copy link
Member

Note the actual repo is https://gitlab.com/nbdkit/nbdkit This is a mirror. And out of date because I notice the mirroring stopped working ...

@asomers ^

@rwmjones
Copy link
Member

after_fork is definitely needed by any plugin that uses threads. We originally introduced it after noticing strange problems in a 3rd party library, which turned out to be because the library was creating threads at start-up which were then killed by forking.

@asomers
Copy link
Collaborator

asomers commented Apr 20, 2023

Yes, I think both of those features could be added. Would you care to submit a PR?

@rwmjones
Copy link
Member

A posted a simple implementation of after_fork here: https://listman.redhat.com/archives/libguestfs/2023-April/031353.html

@rwmjones
Copy link
Member

after_fork support is present in commit d62f268

@rwmjones
Copy link
Member

For the other change see https://gitlab.com/nbdkit/nbdkit/-/merge_requests/23

asomers pushed a commit to asomers/nbdkit that referenced this issue Jun 15, 2023
See: libguestfs#21

Tested by applying the following patch to the example plugin and
running it with verbose enabled:

    --- a/plugins/rust/examples/ramdisk.rs
    +++ b/plugins/rust/examples/ramdisk.rs
    @@ -43,6 +43,12 @@ struct RamDisk {
     }

     impl Server for RamDisk {
    +    fn after_fork() -> Result<()> {
    +        // A place to start background threads.
    +        eprintln!("forked");
    +        Ok(())
    +    }
    +
         fn get_size(&self) -> Result<i64> {
             Ok(DISK.lock().unwrap().len() as i64)
         }
    @@ -76,4 +82,4 @@ impl Server for RamDisk {
         }
     }

    -plugin!(RamDisk {thread_model, write_at});
    +plugin!(RamDisk {thread_model, write_at, after_fork});
asomers pushed a commit to asomers/nbdkit that referenced this issue Jun 15, 2023
The underlying plugin API allows the open method to return an error
(by returning NULL).  However this was not mirrored in the rust
bindings because you could only return a boxed object (equivalent to
returning a non-NULL object in C).

Modify the rust API so Result<> is required.  The API changes as
follows, requiring all plugins to change:

  - fn open(readonly: bool) -> Box<dyn Server> where Self: Sized;
  + fn open(readonly: bool) -> Result<Box<dyn Server>> where Self: Sized;

Trivially this can be done by surrounding the return value of the
existing open method with Ok(...)

Tested by applying the following patch to example.rs:

  --- a/plugins/rust/examples/ramdisk.rs
  +++ b/plugins/rust/examples/ramdisk.rs
  @@ -52,7 +52,8 @@ impl Server for RamDisk {
       }

       fn open(_readonly: bool) -> Result<Box<dyn Server>> {
  -        Ok(Box::<RamDisk>::default())
  +    //  Ok(Box::<RamDisk>::default())
  +        Err(Error::new(libc::EINVAL, "oops"))
       }

       fn read_at(&self, buf: &mut [u8], offset: u64) -> Result<()> {

and observing the behaviour when connecting to the plugin:

  nbdkit: ramdisk[1]: debug: ramdisk: open readonly=0 exportname="" tls=0
  nbdkit: ramdisk[1]: debug: ramdisk: default_export readonly=0 tls=0
  nbdkit: ramdisk[1]: error: oops

See: libguestfs#21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants