vhost_user: Remove commented code

[germag]

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]

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]

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]

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]

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]

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]

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]

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

[Ablu]

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]

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]

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