rust-vmm / vhost

Apache License 2.0
126 stars 64 forks source link

Add shared object #241

Closed aesteve-rh closed 3 weeks ago

aesteve-rh commented 1 month ago

Summary of the PR

Add SHARED_OBJECT_* vhost-user message IDs to align with standard at https://qemu-project.gitlab.io/qemu/interop/vhost-user.html

Move FS_* type IDs to avoid current and future collisions.

Requirements

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

aesteve-rh commented 1 month ago

v3 -> v4: (I think) Separate the frontend_req_handler test fix (joining the frontend handler threads) to its own commit.

stefano-garzarella commented 1 month ago

Thanks, LGTM!

Last doubt: whether we should merge commit 3 and 4 together to improve bisectability.

aesteve-rh commented 1 month ago

Last doubt: whether we should merge commit 3 and 4 together to improve bisectability.

I do not have a strong opinion either way. Commit 4 updated BackendInternal and vhost-user-backend, and did not change any parts of the shared object logic, except for the check itself. So it felt a contained feature that would be clearer to review in a separate commit. On the other hand, it is still part of the shared object support, so I see your point.

If in doubt, I can squash them as they are already reviewed anyway.

stefano-garzarella commented 1 month ago

Last doubt: whether we should merge commit 3 and 4 together to improve bisectability.

I do not have a strong opinion either way. Commit 4 updated BackendInternal and vhost-user-backend, and did not change any parts of the shared object logic, except for the check itself. So it felt a contained feature that would be clearer to review in a separate commit. On the other hand, it is still part of the shared object support, so I see your point.

If in doubt, I can squash them as they are already reviewed anyway.

I don't have a strong opinion either. Theoretically, patch 3 without patch 4 is incorrect, but only if the backend misbehaves. Also, the frontend should handle this eventually, so I think it's fine unless others have different opinions.

aesteve-rh commented 1 month ago

Also, the frontend should handle this eventually

I was so convinced that QEMU did this already… But I checked and, no, verifies it only on the backend side. You are right, commit 3 is incorrect by itself.

stefano-garzarella commented 3 weeks ago

My approval is still up :-) any other reviews? @Ablu @eryugey @germag @jiangliu @sboeuf @slp