search

rust-vmm / vm-virtio

virtio implementation
Apache License 2.0
364 stars 87 forks source link

vsock: support header and data on a single descriptor #207

Closed stefano-garzarella closed 1 year ago

stefano-garzarella commented 1 year ago

Summary of the PR

Starting from Linux 6.2 the virtio-vsock driver can use a single descriptor for both header and data: https://lore.kernel.org/lkml/20221215043645.3545127-1-bobby.eshleman@bytedance.com/

So with this modification the virtio-vsock device can support header and data on a single descriptor or on two. This is just a Linux implementation detail though, for the spec it could be multiple descriptors as well.

For now let's add only the single descriptor support, since it doesn't involve an API change, but in the future we should change the API and support an array of VolatileSlices as suggested by Laura here: https://github.com/rust-vmm/vm-virtio/issues/204#issuecomment-1332511007

Let's make the tests work with this change (mainly by always using PKT_HEADER_SIZE for the header descriptor) and adding new tests to cover the single descriptor scenario.

Fixes: #204

Requirements

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

stefano-garzarella commented 1 year ago

About the CI failure on the commit message, should I remove the links? IMHO they are useful.

lauralt commented 1 year ago

About the CI failure on the commit message, should I remove the links?

No, we can merge the PR with admin rights when it'll be the time.

andreeaflorescu commented 1 year ago

@stefano-garzarella The tests that we are now using for generating the fuzz input are failing, can you take a look? https://buildkite.com/rust-vmm/vm-virtio-ci/builds/728#01852a4d-1e33-4d66-885b-006d851b7344

stefano-garzarella commented 1 year ago

@stefano-garzarella The tests that we are now using for generating the fuzz input are failing, can you take a look? https://buildkite.com/rust-vmm/vm-virtio-ci/builds/728#01852a4d-1e33-4d66-885b-006d851b7344

Ooops, I forgot to adjust the descriptors in the fuzz subfolder.

andreeaflorescu commented 1 year ago

@stefano-garzarella if you do a rebase on top of main the cargo audit test should now pass, and we shouldn't see any more failures. Besides the commit message test all the other tests should pass to merge this PR.

andreeaflorescu commented 1 year ago

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

stefano-garzarella commented 1 year ago

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow. Let me push it and IIUC the 1day pipeline will start, right?

stefano-garzarella commented 1 year ago

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow. Let me push it and IIUC the 1day pipeline will start, right?

ah no, I thought was on main. So I'll send the new version and then you can update it.

andreeaflorescu commented 1 year ago

@stefano-garzarella I created a longer running fuzzing pipeline for PRs that runs fuzzing for 1 day, but doesn't block the merge of PRs. Do you mind if I do an update branch on your PR to trigger the run?

It's fine, but the current branch doesn't include the change that I proposed to avoid the overflow. Let me push it and IIUC the 1day pipeline will start, right?

Yap, that's right! Once it is pushed once, then we can also retrigger it from github. But since the webhook was yet never called, GitHub doesn't know of its existence.

lauralt commented 1 year ago

This documentation needs to be updated.

lauralt commented 1 year ago

We should probably also add a CHANGELOG entry for this, even if it's not an API change.

stefano-garzarella commented 1 year ago

new version:

andreeaflorescu commented 1 year ago

Do we publish this as a patch release, in that case we should follow this guide which involves creating a new branch etc.

I don't think we need a patch release on a new branch unless we want to backport this fix to previous releases. We can just do patch releases from main as well as long as the history is linear, which is what I think is the case here.

stefano-garzarella commented 1 year ago

new version: