virtio-bindings: regenerate #194
Conversation
alyssais
requested review from
alexandruag,
andreeaflorescu,
jiangliu,
slp and
stsquad
as code owners
| #[derive(Copy, Clone)] | ||
| pub union virtio_net_hdr_v1__bindgen_ty_1 { | ||
| pub __bindgen_anon_1: virtio_net_hdr_v1__bindgen_ty_1__bindgen_ty_1, | ||
| pub csum: virtio_net_hdr_v1__bindgen_ty_1__bindgen_ty_2, | ||
| pub rsc: virtio_net_hdr_v1__bindgen_ty_1__bindgen_ty_3, | ||
| } |
There was a problem hiding this comment.
@alyssais I think it's not acceptable to lose the traits implementations (Debug, Default and PartialEq) since this might introduce some regressions for consumers of this crate. The only alternative I can think of is by manually implementing these traits for virtio_net_hdr_v1__bindgen_ty_1.
There was a problem hiding this comment.
What about the structs with added members? That could also cause regressions for consumers of the crate if they're constructing those structs with struct literals.
We can manually implement traits, but there's nothing we can do about that.
So I only see two ways forward here:
-
Revert virtio-bindings: drop bindings for older kernels #192, and accept that we'll have to support multiple kernel versions as features because the kernel headers are not actually backwards compatible in the Rust sense. We'd also maybe have to pin Glibc versions, in case crate users are depending on any of the values that are re-exported basically by accident, and possibly also bindgen versions?
-
Only generate bindings for a single kernel version at a time, and accept that the major version is bumped whenever the kernel version is updated.
I think it would be latter would be better. The backwards incompatibilities are unlikely to be a problem in practice, so it should be very easy for users of the crate to update anyway. We're not saving users from having to manually update by avoiding major version bumps, because they'd still need to manually change which feature they're using to get a new kernel version (or we'd have to bump the major version anyway if we changed the default feature). And it would stop this crate from continuing to grow in size as more kernel versions are added.
The only reason I see to prefer the features approach is that it would make it slightly easier to do releases that fix the bindings in ways that aren't breaking changes. But it wouldn't exactly be much harder to do releases for multiple kernel versions, and I don't expect that this would come up very often.
There was a problem hiding this comment.
Well the point about added members in structures doesn't really apply given we already agreed to follow what the kernel does, meaning it's acceptable to have new fields in a structure, and things like JSON encoding/decoding should be able to handle such situation from the consumer's code.
But in general I agree let's not make things too complex to maintain, meaning the second approach using minor (or major) version to introduce breaking changes (from a Rust perspective) would be the way to go IMO.
There was a problem hiding this comment.
/cc @andreeaflorescu @jiangliu @slp wdyt?
There was a problem hiding this comment.
If I remember correctly PartialEq, Debug and Default were not added to all structures by bindgen because of an issue in how they're generating the bindings. We added them manually for older versions because otherwise the structures are pretty hard to use. I think we need to find a way to still have these traits implemented. Let me do some digging and figure out how we handle this in the past.
There was a problem hiding this comment.
If you have some more time to invest in this, we can also find a better way of generating them while still keeping the derives that we need. In any case, I don't think it's acceptable to not have them.
There was a problem hiding this comment.
@alyssais did you give this more thought? Are you still interested in regenerating the bindings?
There was a problem hiding this comment.
Hi! I'm still interested, but likely won't get a chance to look at it until I'm back from vacation in mid-October.
There was a problem hiding this comment.
Ok, no problem, thanks for letting me know!
There was a problem hiding this comment.
Quick update: after I came back from my vacation I immediately got a bad flu, so I'm just catching up now. I'll look at this soon. :)
|
Updated to:
|
alyssais
force-pushed
the
regen
branch
2 times, most recently
from
36e0015
to
c6d0586
Compare
|
Should be all sorted and ready for review. :) |
|
AFAICT, CI failed because it timed out updating the crates.io index? And also for the same reason as #206, i.e. bheisler/criterion.rs#629. |
Yes, we need to ignore it as we already did here: rust-vmm/linux-loader@2580d45. |
|
And we need to fix and merge the rust-vmm-ci bump, the timeout problem you mentioned is (temporarily) fixed here. I will try to fix this asap, but don't quite have bandwidth today. |
I did the fix, you can just rebase on top of the main branch and there should be no more failures. |
Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
The linked issue was fixed in bindgen 0.60.0. Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
In future, it would be good to exclude a lot of these Glibc re-exports. They're probably not required for all the virtio bindings we want to expose. Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
bindgen can no longer generate the Debug and PartialEq implementations for virtio_net_hdr_v1, so I have added manual implementations of these traits. Signed-off-by: Alyssa Ross <alyssa.ross@unikie.com>
Summary of the PR
Here I've regenerated the bindings with modern versions of bindgen, Glibc, and Linux. I put each tool update into its own commit to make it easy to see which upgrade caused which changes.
Something that worries me a little bit is that as part of the update to the kernel headers, some structs have gained members, and virtio_net_hdr_v1 has lost its Debug, Default, and PartialEq implementations due to the addition of a union member. Both of these are technically breaking changes in the Rust sense, even if they're not considered so by the kernel. But they're also minor, so I'm not sure if it makes sense to worry about them? cc @sboeuf
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
unsafecode is properly documented.