libkrun: Refactor VirtioFsTable to VirtioFsLookup trait

[dorindabassey]

Introduce a trait-based abstraction for VirtioFS file descriptor lookup. The trait design enables future extensions like inode-based lookups for O_PATH file descriptors (needed for D-Bus over virtgpu with file transfer support). Provides DefaultVirtioFsLookup for backward compatibility.

This supersedes #43

[dorindabassey]

Hi @valpackett wdyt?

[valpackett]

Oh wow, I didn't even realize the previous thing was merged and released- oh, on a temporary libkrun branch with its own releases, I see! Seems a bit early to commit to backwards compat and provide deprecated annotations but fine :)

Actual substance: LGTM 👍

[mtjhrc]

I would remove the default impl, otherwise this LGTM.

[gurchetansingh]

How does everyone feel about modifying the RutabagaHandler to be more generic, and even renaming it to RutabagaCallback (it actually compiles with crosvm with no source modifications outside of rutabaga, since RutabagaCallback isn't used directly):

#[derive(Clone)]
pub struct RutabagaCallback<S, T> {
    closure: Arc<dyn Fn(S) -> T + Send + Sync>,
}

impl<S, T> RutabagaCallback<S, T>
where
    S: Send + Sync + Clone + 'static,
    T: Send + Sync + Clone + 'static,
{
    /// Creates a new handler from a closure.
    pub fn new(closure: impl Fn(S) -> T + Send + Sync + 'static) -> Self {
        RutabagaCallback {
            closure: Arc::new(closure),
        }
    }

    /// Executes the stored closure with the provided data.
    pub fn call(&self, data: S) -> T {
        (self.closure)(data)
    }
}

pub type RutabagaFenceHandler = RutabagaCallback<RutabagaFence, ()>;
pub type RutabagaDebugHandler = RutabagaCallback<RutabagaDebug, ()>;
pub type RutabagaFsLookup = RutabagaCallback<u64, RutabagaDescriptor>;

[dorindabassey]

How does everyone feel about modifying the RutabagaHandler to be more generic, and even renaming it to RutabagaCallback (it actually compiles with crosvm with no source modifications outside of rutabaga, since RutabagaCallback isn't used directly):

yeah, I think it makes sense consolidating to a generic RutabagaCallback - it's cleaner. One small issue though: VirtioFsLookup takes two u64 parameters (fs_id and handle), so it should probably be:

  pub type RutabagaFsLookup = RutabagaCallback<(u64, u64), RutabagaResult<RutabagaDescriptor>>;

[gurchetansingh]

One small issue though: VirtioFsLookup takes two u64 parameters (fs_id and handle), so it should probably be:

Ack, you can also do:

struct RutabagaFsKey {
    fs_id: u64,
   handle: u64
}

leave it up to you if you prefer the pair or struct

[valpackett]

But now we're back to just a single callback type that can't be easily extended.. Well I can extend everything "not-easily", sure :)

Why do we need the double Arc btw? (i.e. virtiofs_lookup: Option<Arc<RutabagaFsLookup>> where RutabagaFsLookup = RutabagaCallback<…> which is struct RutabagaCallback<S, T> { closure: Arc<dyn Fn(S) -> T + Send + Sync> }?

[mtjhrc]

Hmm, thinking about it, I think the trait was better, we could extend it like Val wants by adding methods with default impl quite easily.

[dorindabassey]

I could bring back the VirtioFsLookup trait and still keep the RutabagaCallback for simple handlers.

[mtjhrc]

IMHO just the simple specific-purpose trait without any of the RutabagaCallback/RutabagaHandler is the best (especially because we already expect it to have to be extended).

[gurchetansingh]

Sure if the separate trait is better, no problem supporting that. We can probably optimize later anyways,

[dorindabassey]

Fixed, Thanks all!

[gurchetansingh]

https://crates.io/crates/rutabaga_gfx/0.1.76-libkrun.1 <-- cut release with this commit