Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vhost_user: Remove commented code #205

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

germag
Copy link
Collaborator

@germag germag commented Nov 14, 2023

Summary of the PR

Let's remove commented vhost-user message definitions, some of the message definition are not supported and the other is duplicated (i.e., VhostUserLog is already defined).

Note to reviewers

I think we also need to remove the following back-end message types:

pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 6,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 7,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 8,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 9,
    ...
}

These are not in the vhost-user standard, which in itself is confusing, but even conflict with values that are defined in the standard (see Back-end message types), such as: VHOST_USER_BACKEND_SHARED_OBJECT_ADD, VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE and VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP.

I have not removed them (yet) because there is not only the definition but also part of the API implemented in the FrontendReqHandler and in Backend.

@stefano-garzarella
Copy link
Member

I think we also need to remove the following back-edn message types:

pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 6,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 7,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 8,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 9,
    ...
}

These are not in the vhost-user standard, which in itself is confusing, but even conflict with values that are defined in the standard (see Back-end message types), such as: VHOST_USER_BACKEND_SHARED_OBJECT_ADD, VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE and VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP.

I have not removed them (yet) because there is not only the definition but also part of the API implemented in the FrontendReqHandler and in Backend.

If they are not into the spec, IMHO we should remove them, or at least put under some features (e.g. unstable), to make clear that their values are not stables.

@germag @jiangliu @sboeuf @slp Do you know if someone is using that values?

@sboeuf
Copy link
Collaborator

sboeuf commented Nov 14, 2023

I think we also need to remove the following back-edn message types:

pub enum BackendReq {
    ...
    /// Virtio-fs draft: map file content into the window.
    FS_MAP = 6,
    /// Virtio-fs draft: unmap file content from the window.
    FS_UNMAP = 7,
    /// Virtio-fs draft: sync file content.
    FS_SYNC = 8,
    /// Virtio-fs draft: perform a read/write from an fd directly to GPA.
    FS_IO = 9,
    ...
}

These are not in the vhost-user standard, which in itself is confusing, but even conflict with values that are defined in the standard (see Back-end message types), such as: VHOST_USER_BACKEND_SHARED_OBJECT_ADD, VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE and VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP.
I have not removed them (yet) because there is not only the definition but also part of the API implemented in the FrontendReqHandler and in Backend.

If they are not into the spec, IMHO we should remove them, or at least put under some features (e.g. unstable), to make clear that their values are not stables.

@germag @jiangliu @sboeuf @slp Do you know if someone is using that values?

I'm not aware of someone using these values, I think they had been removed from Cloud Hypervisor (/cc @chenb). Deprecating them by moving them under a specific feature is a good idea. If someone using them moves to the latest vhost version, they might reach out because of the new feature gate.

@germag
Copy link
Collaborator Author

germag commented Nov 14, 2023

But even behind a feature, their values should be bumped at least by 1, since those are conflicting with the SHARED_OBJECT messages and that will be a runtime error.

Also, if someone is using those, they could reach out or just add the feature, I think it should be a compilation error if the feature is enabled if we do not want to remove them directly

@stefano-garzarella
Copy link
Member

But even behind a feature, their values should be bumped at least by 1, since those are conflicting with the SHARED_OBJECT messages and that will be a runtime error.

Yep, this for sure. The feature is more about not removing code that's already there. Then, if one enables it, it's his own risk if we explain that it's some unstable thing for us.

Also, if someone is using those, they could reach out or just add the feature, I think it should be a compilation error if the feature is enabled if we do not want to remove them directly

yeah, that could be an option, just to have the code there, wait a couple of releases, then remove it.

@stefano-garzarella
Copy link
Member

Looking at the code it looks like a lot and also hard to maintain since it's not in the spec.
At this point I'm really tempted to remove it completely and only accept it when we have stable specifications.

@germag
Copy link
Collaborator Author

germag commented Nov 15, 2023

I'm not aware of someone using these values, I think they had been removed from Cloud Hypervisor (/cc @chenb).

it was my understanding that CH has its own internal version of vhost-user...

@alyssais
Copy link
Contributor

Cloud Hypervisor uses the upstream version of the vhost crate, currently 0.8.1. Perhaps you're mixing it up with crosvm?

@Ablu
Copy link
Collaborator

Ablu commented Nov 15, 2023

it was my understanding that CH has its own internal version of vhost-user...

Not sure if that is what was meant here, but: There are discussions on importing cloud-hypervisor's vhost-user-frontend code into the rust-vmm umberella.

@germag
Copy link
Collaborator Author

germag commented Nov 15, 2023

Cloud Hypervisor uses the upstream version of the vhost crate, currently 0.8.1. Perhaps you're mixing it up with crosvm?

Ok (I was confused) thanks Alyssa

@stefano-garzarella
Copy link
Member

@germag please open an issue to track your suggestion of removing FS_ types

Let's remove commented vhost-user message definitions, some of the
message definition are not supported and the other is duplicated
(i.e., VhostUserLog is already defined).

Signed-off-by: German Maglione <gmaglione@redhat.com>
@stefano-garzarella stefano-garzarella merged commit 826a194 into rust-vmm:main Nov 23, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants