rust-vmm / vm-virtio

virtio implementation
Apache License 2.0
364 stars 87 forks source link

virtio-bindings: don't leak irrelevant constants #215

Closed alyssais closed 1 year ago

alyssais commented 1 year ago

Summary of the PR

By default, bindgen will generate bindings for everything it can on the header it's run on, but also all headers included by that header. This is why we end up re-exporting a bunch of Glibc constants (and therefore why reproducing the bindings depends on having a specific Glibc version). It also means that the virtio_config bindings are duplicated in every other binding module.

To fix this, we can pass --allowlist-file to tell bindgen to only include bindings for constants declared in the specific header we're operating on, as well as any types that header depends on. This removes almost all of the bindings that were duplicated between modules, and removes the Glibc constants.

As a result of this change, users will have to include virtio_config constants from the virtio_config module, rather than relying on them being included in e.g. virtio_net. rustc is smart enough to suggest exactly the change that needs to be made. I tested all the crates on crates.io that depend on virtio-bindings as well as cloud-hypervisor, and of those only virtiofsd and cloud-hypervisor will need to make any change at all*. It would be possible to deprecate the leaked constants instead of immediately removing them, but I think it's probably not worth it, as for virtio_config constants, the fix is trivial and suggested by Cargo, and I think it's very unlikely that anybody is relying on virtio-bindings providing any of the other contants (e.g. Glibc exports). I'm open to discussion on this, though.

* I wasn't able to test dbs-virtio-devices due to it not including a Cargo.lock, and not building with the current versions of its dependencies as resolved by cargo.

~I haven't included a changelog to avoid conflicting with https://github.com/rust-vmm/vm-virtio/pull/213. Once either this PR or that one is merged, I'll update the one that's still open with the changelog entry for this change.~ Changelog entry now added.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

stsquad commented 1 year ago
use virtio_bindings::bindings::virtio_net::{VIRTIO_F_NOTIFY_ON_EMPTY,VIRTIO_F_VERSION_1};

Ahh I missed they are still in virtio_config. (sorry hit wrong button)