Code duplication between VIRTIO drivers
- Dominant language
- Rust
- Stars
- 1.5k
- Forks
- 132
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 39
Description
Basic functionality absolutely required for VIRTIO driver initialisation has been duplicated between VIRTIO net, fs, and vsock drivers.
Concrete examples of this are
* the `negotiate_features` helper function that given a fixed set of wanted features, checks feature set spec conformance (or is likely supposed to do so, see #1690) and if the device supports a superset, applies these features.
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/vsock/mod.rs#L297-L321
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_fs.rs#L62-L85
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/net/virtio/mod.rs#L628-L652
* the extraction/mapping of PCI device configs from PCI capabilities and conversion of these to their respective Rust structs
* here the code snippets are not exactly identical, but functionally the same, with only the resulting driver type and device config type differing between instances of this code
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/vsock/pci.rs#L20-L97
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_pci.rs#L13-L84
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/net/virtio/pci.rs#L21-L123
It should be possible and desirable to factor out this common code, given that errors (albeit only doc/logging errors) have already snuck in:
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_fs.rs#L217
* https://github.com/hermit-os/kernel/blob/70ad8efdbb0245bf0c4d41ec387b2a0e1a00f637/src/drivers/fs/virtio_pci.rs#L62
For the config mapping from PCI caps task, a shared
```rust
pub struct VirtioDriverConfig {
dev_cfg: DeviceConfig,
com_cfg: ComCfg,
notif_cfg: NotiCfg,
isr_status: IsrStatus,
irq: u8,
}
```
that implements this mapping generically might work and could then be used as a field to the drivers instead of the drivers having all of the above fields each.
For the `negotiate_features` function perhaps a function generic over the feature type might work, with a new error type that can be used by the drivers in their error types, instead of duplicating variants.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.